[PATCH] D66564: [clang-tidy] new performance struct pack align check

[Aaron Ballman via Phabricator via llvm-commits]

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