ffrankies updated this revision to Diff 224802.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66564/new/
https://reviews.llvm.org/D66564
Files:
clang-tidy/performance/CMakeLists.txt
clang-tidy/performance/PerformanceTidyModule.cpp
clang-tidy/performance/StructPackAlignCheck.cpp
clang-tidy/performance/StructPackAlignCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/performance-struct-pack-align.rst
test/clang-tidy/performance-struct-pack-align.cpp
Index: test/clang-tidy/performance-struct-pack-align.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/performance-struct-pack-align.cpp
@@ -0,0 +1,73 @@
+// RUN: %check_clang_tidy %s performance-struct-pack-align %t -- -header-filter=.* "--" --include opencl-c.h -cl-std=CL1.2 -c
+
+// Struct needs both alignment and packing
+struct error {
+ char a;
+ double b;
+ char c;
+};
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: accessing fields in struct 'error' is inefficient due to padding; only needs 10 bytes but is using 24 bytes, use "__attribute((packed))" [performance-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: warning: accessing fields in struct 'error' is inefficient due to poor alignment; currently aligned to 8 bytes, but size 10 bytes is large enough to benefit from "__attribute((aligned(16)))" [performance-struct-pack-align]
+
+// Struct is explicitly packed, but needs alignment
+struct error_packed {
+ char a;
+ double b;
+ char c;
+} __attribute((packed));
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: accessing fields in struct 'error_packed' is inefficient due to poor alignment; currently aligned to 1 bytes, but size 10 bytes is large enough to benefit from "__attribute((aligned(16)))" [performance-struct-pack-align]
+
+// Struct is properly packed, but needs alignment
+struct align_only {
+ char a;
+ char b;
+ char c;
+ char d;
+ int e;
+ double f;
+};
+// CHECK-MESSAGES: :[[@LINE-8]]:8: warning: accessing fields in struct 'align_only' is inefficient due to poor alignment; currently aligned to 8 bytes, but size 16 bytes is large enough to benefit from "__attribute((aligned(16)))" [performance-struct-pack-align]
+
+// Struct is perfectly packed but wrongly aligned
+struct bad_align {
+ char a;
+ double b;
+ char c;
+} __attribute((packed)) __attribute((aligned(8)));
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: accessing fields in struct 'bad_align' is inefficient due to poor alignment; currently aligned to 8 bytes, but size 10 bytes is large enough to benefit from "__attribute((aligned(16)))" [performance-struct-pack-align]
+
+struct bad_align2 {
+ char a;
+ double b;
+ char c;
+} __attribute((packed)) __attribute((aligned(32)));
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: accessing fields in struct 'bad_align2' is inefficient due to poor alignment; currently aligned to 32 bytes, but size 10 bytes is large enough to benefit from "__attribute((aligned(16)))" [performance-struct-pack-align]
+
+struct bad_align3 {
+ char a;
+ double b;
+ char c;
+} __attribute((packed)) __attribute((aligned(4)));
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: accessing fields in struct 'bad_align3' is inefficient due to poor alignment; currently aligned to 4 bytes, but size 10 bytes is large enough to benefit from "__attribute((aligned(16)))" [performance-struct-pack-align]
+
+// Struct is both perfectly packed and aligned
+struct success {
+ char a;
+ double b;
+ char c;
+} __attribute((packed)) __attribute((aligned(16)));
+//Should take 10 bytes and be aligned to 16 bytes
+
+// Struct is properly packed, and explicitly aligned
+struct success2 {
+ int a;
+ int b;
+ int c;
+} __attribute((aligned(16)));
+
+// If struct is properly aligned, packing not needed
+struct error_aligned {
+ char a;
+ double b;
+ char c;
+} __attribute((aligned(16)));
Index: docs/clang-tidy/checks/performance-struct-pack-align.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/performance-struct-pack-align.rst
@@ -0,0 +1,51 @@
+.. title:: clang-tidy - performance-struct-pack-align
+
+performance-struct-pack-align
+======================
+
+Finds structs that are inefficiently packed or aligned, and recommends
+packing and/or aligning of said structs as needed.
+
+Structs that are not packed take up more space than they should, and accessing
+structs that are not well aligned is inefficient.
+
+Based on the `Altera SDK for OpenCL: Best Practices Guide
+<https://www.altera.com/en_US/pdfs/literature/hb/opencl-sdk/aocl_optimization_guide.pdf>`_.
+
+.. code-block:: c++
+
+ // The following struct is originally aligned to 4 bytes, and thus takes up
+ // 12 bytes of memory instead of 10. Packing the struct will make it use
+ // only 10 bytes of memory, and aligning it to 16 bytes will make it
+ // efficient to access.
+ struct example {
+ char a; // 1 byte
+ double b; // 8 bytes
+ char c; // 1 byte
+ };
+
+ // The following struct is arranged in such a way that packing is not needed.
+ // However, it is aligned to 4 bytes instead of 8, and thus needs to be
+ // explicitly aligned.
+ struct implicitly_packed_example {
+ char a; // 1 byte
+ char b; // 1 byte
+ char c; // 1 byte
+ char d; // 1 byte
+ int e; // 4 bytes
+ };
+
+ // The following struct is explicitly aligned and packed.
+ struct good_example {
+ char a; // 1 byte
+ double b; // 8 bytes
+ char c; // 1 byte
+ } __attribute((packed)) __attribute((aligned(16));
+
+ // Explicitly aligning a struct to the wrong value will result in a warning.
+ // The following example should be aligned to 16 bytes, not 32.
+ struct badly_aligned_example {
+ char a; // 1 byte
+ double b; // 8 bytes
+ char c; // 1 byte
+ } __attribute((packed)) __attribute((aligned(32)));
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -341,6 +341,7 @@
performance-move-const-arg
performance-move-constructor-init
performance-noexcept-move-constructor
+ performance-struct-pack-align
performance-type-promotion-in-math-fn
performance-unnecessary-copy-initialization
performance-unnecessary-value-param
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -109,6 +109,12 @@
Finds Objective-C implementations that implement ``-isEqual:`` without also
appropriately implementing ``-hash``.
+- New :doc:`performance-struct-pack-align
+ <clang-tidy/checks/performance-struct-pack-align>` check
+
+ Finds structs that are inefficiently packed or aligned and recommends
+ packing and/or aligning of said structs as needed.
+
- Improved :doc:`bugprone-posix-return
<clang-tidy/checks/bugprone-posix-return>` check.
Index: clang-tidy/performance/StructPackAlignCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/performance/StructPackAlignCheck.h
@@ -0,0 +1,38 @@
+//===--- StructPackAlignCheck.h - clang-tidy --------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_STRUCTPACKALIGNCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_STRUCTPACKALIGNCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+/// Finds structs that are inefficiently packed or aligned, and recommends
+/// packing and/or aligning of said structs as needed.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/performance-struct-pack-align.html
+class StructPackAlignCheck : public ClangTidyCheck {
+public:
+ StructPackAlignCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+ CharUnits computeRecommendedAlignment(CharUnits MinByteSize);
+};
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_STRUCTPACKALIGNCHECK_H
Index: clang-tidy/performance/StructPackAlignCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/performance/StructPackAlignCheck.cpp
@@ -0,0 +1,106 @@
+//===--- StructPackAlignCheck.cpp - clang-tidy ----------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "StructPackAlignCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <math.h>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+constexpr int MAX_CONFIGURED_ALIGNMENT = 128;
+constexpr float SIZEOF_BYTE = 8.0;
+
+void StructPackAlignCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(recordDecl(isStruct(), isDefinition()).bind("struct"),
+ this);
+}
+
+CharUnits
+StructPackAlignCheck::computeRecommendedAlignment(CharUnits MinByteSize) {
+ CharUnits NewAlign = CharUnits::fromQuantity(1);
+ if (!MinByteSize.isPowerOfTwo()) {
+ int MSB = (int)MinByteSize.getQuantity();
+ for (; MSB > 0; MSB /= 2) {
+ NewAlign = NewAlign.alignTo(
+ CharUnits::fromQuantity(((int)NewAlign.getQuantity()) * 2));
+ // Abort if the computed alignment meets the maximum configured alignment
+ if (NewAlign.getQuantity() >= MAX_CONFIGURED_ALIGNMENT)
+ break;
+ }
+ } else {
+ NewAlign = MinByteSize;
+ }
+ return NewAlign;
+}
+
+void StructPackAlignCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *Struct = Result.Nodes.getNodeAs<RecordDecl>("struct");
+
+ // Get sizing info for the struct
+ llvm::SmallVector<std::pair<unsigned int, unsigned int>, 10> FieldSizes;
+ unsigned int TotalBitSize = 0;
+ for (const FieldDecl *StructField : Struct->fields()) {
+ // For each StructField, record how big it is (in bits)
+ // Would be good to use a pair of <offset, size> to advise a better
+ // packing order
+ unsigned int StructFieldWidth =
+ (unsigned int)Result.Context
+ ->getTypeInfo(StructField->getType().getTypePtr())
+ .Width;
+ FieldSizes.emplace_back(StructFieldWidth, StructField->getFieldIndex());
+ // FIXME: Recommend a reorganization of the struct (sort by StructField
+ // size, largest to smallest)
+ TotalBitSize += StructFieldWidth;
+ }
+ // 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
+ CharUnits CurrSize = Result.Context->getASTRecordLayout(Struct).getSize();
+ CharUnits MinByteSize =
+ CharUnits::fromQuantity(ceil(TotalBitSize / SIZEOF_BYTE));
+ CharUnits CurrAlign =
+ Result.Context->getASTRecordLayout(Struct).getAlignment();
+ CharUnits NewAlign = computeRecommendedAlignment(MinByteSize);
+
+ // Check if struct has a "packed" attribute
+ bool IsPacked = Struct->hasAttr<PackedAttr>();
+
+ // If it's using much more space than it needs, suggest packing.
+ // (Do not suggest packing if it is currently explicitly aligned to what the
+ // minimum byte size would suggest as the new alignment)
+ if (MinByteSize < CurrSize &&
+ ((Struct->getMaxAlignment() / 8) != NewAlign.getQuantity()) &&
+ (CurrSize != NewAlign) && !IsPacked) {
+ diag(Struct->getLocation(),
+ "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()
+ << (int)CurrSize.getQuantity();
+ }
+
+ // And suggest the minimum power-of-two alignment for the struct as a whole
+ // (with and without packing)
+ if (CurrAlign.getQuantity() != NewAlign.getQuantity()) {
+ diag(Struct->getLocation(),
+ "accessing fields in struct %0 is inefficient due to poor alignment; "
+ "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()
+ << (int)MinByteSize.getQuantity();
+ }
+}
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/performance/PerformanceTidyModule.cpp
===================================================================
--- clang-tidy/performance/PerformanceTidyModule.cpp
+++ clang-tidy/performance/PerformanceTidyModule.cpp
@@ -18,6 +18,7 @@
#include "MoveConstArgCheck.h"
#include "MoveConstructorInitCheck.h"
#include "NoexceptMoveConstructorCheck.h"
+#include "StructPackAlignCheck.h"
#include "TypePromotionInMathFnCheck.h"
#include "UnnecessaryCopyInitialization.h"
#include "UnnecessaryValueParamCheck.h"
@@ -47,6 +48,8 @@
"performance-move-constructor-init");
CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
"performance-noexcept-move-constructor");
+ CheckFactories.registerCheck<StructPackAlignCheck>(
+ "performance-struct-pack-align");
CheckFactories.registerCheck<TypePromotionInMathFnCheck>(
"performance-type-promotion-in-math-fn");
CheckFactories.registerCheck<UnnecessaryCopyInitialization>(
Index: clang-tidy/performance/CMakeLists.txt
===================================================================
--- clang-tidy/performance/CMakeLists.txt
+++ clang-tidy/performance/CMakeLists.txt
@@ -11,6 +11,7 @@
MoveConstructorInitCheck.cpp
NoexceptMoveConstructorCheck.cpp
PerformanceTidyModule.cpp
+ StructPackAlignCheck.cpp
TypePromotionInMathFnCheck.cpp
UnnecessaryCopyInitialization.cpp
UnnecessaryValueParamCheck.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
