aaron.ballman added a comment.
I sort of understand why this was asked to be put into `performance`, but I'm
not convinced that's the right place to put it. Performance could be degraded
by packing structures on some architectures depending on how the objects are
accessed. I worry that people will start packing or aligning structures and
having degraded performance due to architecture-specific details or access
pattern issues.
I sort of wonder whether this is best put in its own module (opencl or altera,
depending on how generic the functionality is)?
================
Comment at: clang-tidy/performance/StructPackAlignCheck.cpp:21
+
+constexpr int MAX_CONFIGURED_ALIGNMENT = 128;
+constexpr float SIZEOF_BYTE = 8.0;
----------------
Is there a reason this isn't a user option?
================
Comment at: clang-tidy/performance/StructPackAlignCheck.cpp:22
+constexpr int MAX_CONFIGURED_ALIGNMENT = 128;
+constexpr float SIZEOF_BYTE = 8.0;
+
----------------
This shouldn't be needed; we already track the size of a byte. If we do need
it, why a float?
================
Comment at: clang-tidy/performance/StructPackAlignCheck.cpp:37
+ CharUnits::fromQuantity(((int)NewAlign.getQuantity()) * 2));
+ // Abort if the computed alignment meets the maximum configured alignment
+ if (NewAlign.getQuantity() >= MAX_CONFIGURED_ALIGNMENT)
----------------
You should add a full stop at the end of the comment (here and in the rest of
the files).
================
Comment at: clang-tidy/performance/StructPackAlignCheck.cpp:66-68
+ // FIXME: Express this as CharUnit rather than a hardcoded 8-bits (Rshift3)i
+ // After computing the minimum size in bits, check for an existing alignment
+ // flag
----------------
`Result.Context->getCharWidth()`?
================
Comment at: clang-tidy/performance/StructPackAlignCheck.cpp:83
+ if (MinByteSize < CurrSize &&
+ ((Struct->getMaxAlignment() / 8) != NewAlign.getQuantity()) &&
+ (CurrSize != NewAlign) && !IsPacked) {
----------------
Should this be dividing by the bit-width of a char, not hard-coded to 8?
================
Comment at: clang-tidy/performance/StructPackAlignCheck.cpp:87
+ "accessing fields in struct %0 is inefficient due to padding; only "
+ "needs %1 bytes but is using %2 bytes, use \"__attribute((packed))\"")
+ << Struct << (int)MinByteSize.getQuantity()
----------------
The diagnostic is a bit wordy, but I like the words. ;-) I think a better
approach may be to split this into two diagnostics. The first one is a warning
and the second is a note with a fixit to add the attribute to the structure
declaration in the correct place. Also, it probably should be `__attribute__`
instead of `__attribute`.
================
Comment at: clang-tidy/performance/StructPackAlignCheck.cpp:98
+ "currently aligned to %1 bytes, but size %3 bytes is large enough to "
+ "benefit from \"__attribute((aligned(%2)))\"")
+ << Struct << (int)CurrAlign.getQuantity() <<
(int)NewAlign.getQuantity()
----------------
Similar suggestion here to split into a warning and a note/fixit.
================
Comment at: docs/clang-tidy/checks/performance-struct-pack-align.rst:4
+performance-struct-pack-align
+======================
+
----------------
The underlining does not look correct here.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66564/new/
https://reviews.llvm.org/D66564
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
