ffrankies updated this revision to Diff 230828.
ffrankies edited the summary of this revision.
ffrankies added a comment.
Implemented changes requested by @aaron.ballman
- Moved the check from the "performance" module into the new "altera" module
- Split the diagnostic message into a warning and a note
- Readability/general code refactoring changes
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66564/new/
https://reviews.llvm.org/D66564
Files:
clang-tidy/CMakeLists.txt
clang-tidy/ClangTidyForceLinker.h
clang-tidy/altera/AlteraTidyModule.cpp
clang-tidy/altera/CMakeLists.txt
clang-tidy/altera/StructPackAlignCheck.cpp
clang-tidy/altera/StructPackAlignCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/altera-struct-pack-align.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/index.rst
test/clang-tidy/checkers/altera-struct-pack-align.cpp
Index: test/clang-tidy/checkers/altera-struct-pack-align.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/checkers/altera-struct-pack-align.cpp
@@ -0,0 +1,80 @@
+// RUN: %check_clang_tidy %s altera-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 [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: note: use "__attribute__((packed))" to reduce the amount of padding applied to struct 'error'
+// CHECK-MESSAGES: :[[@LINE-7]]:8: warning: accessing fields in struct 'error' is inefficient due to poor alignment; currently aligned to 8 bytes, but recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-8]]:8: note: use "__attribute__((aligned(16)))" to align struct 'error' to 16 bytes
+
+// 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 recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: note: use "__attribute__((aligned(16)))" to align struct 'error_packed' to 16 bytes
+
+// 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 recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-9]]:8: note: use "__attribute__((aligned(16)))" to align struct 'align_only' to 16 bytes
+
+// 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 recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: note: use "__attribute__((aligned(16)))" to align struct 'bad_align' to 16 bytes
+
+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 recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: note: use "__attribute__((aligned(16)))" to align struct 'bad_align2' to 16 bytes
+
+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 recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: note: use "__attribute__((aligned(16)))" to align struct 'bad_align3' to 16 bytes
+
+// 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/index.rst
===================================================================
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -59,6 +59,7 @@
====================== =========================================================
``abseil-`` Checks related to Abseil library.
``android-`` Checks related to Android.
+``altera-`` Checks related to OpenCL programming for FPGAs.
``boost-`` Checks related to Boost library.
``bugprone-`` Checks that target bugprone code constructs.
``cert-`` Checks related to CERT Secure Coding Guidelines.
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -22,6 +22,7 @@
abseil-time-comparison
abseil-time-subtraction
abseil-upgrade-duration-conversions
+ altera-struct-pack-align
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
Index: docs/clang-tidy/checks/altera-struct-pack-align.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/altera-struct-pack-align.rst
@@ -0,0 +1,51 @@
+.. title:: clang-tidy - altera-struct-pack-align
+
+altera-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/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -67,6 +67,18 @@
Improvements to clang-tidy
--------------------------
+- New :doc:`altera <clang-tidy/modules/altera>` module.
+
+ Includes checks related to OpenCL for FPGA coding guidelines, 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>`_.
+
+- New :doc:`altera-struct-pack-align
+ <clang-tidy/checks/altera-struct-pack-align>` check.
+
+ Finds structs that are inefficiently packed or aligned and recommends
+ packing and/or aligning of said structs as needed.
+
- New :doc:`bugprone-dynamic-static-initializers
<clang-tidy/checks/bugprone-dynamic-static-initializers>` check.
Index: clang-tidy/altera/StructPackAlignCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/altera/StructPackAlignCheck.h
@@ -0,0 +1,41 @@
+//===--- 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_ALTERA_STRUCTPACKALIGNCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ALTERA_STRUCTPACKALIGNCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace altera {
+
+/// 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/altera-struct-pack-align.html
+class StructPackAlignCheck : public ClangTidyCheck {
+public:
+ StructPackAlignCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ MaxConfiguredAlignment(Options.get("MaxConfiguredAlignment", 128)) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void storeOptions(ClangTidyOptions::OptionMap &Opts);
+
+private:
+ const unsigned MaxConfiguredAlignment;
+ CharUnits computeRecommendedAlignment(CharUnits MinByteSize);
+};
+
+} // namespace altera
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ALTERA_STRUCTPACKALIGNCHECK_H
Index: clang-tidy/altera/StructPackAlignCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/altera/StructPackAlignCheck.cpp
@@ -0,0 +1,119 @@
+//===--- 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 <limits.h>
+#include <math.h>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace altera {
+
+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() >= MaxConfiguredAlignment)
+ 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;
+ }
+
+ uint64_t CharSize = Result.Context->getCharWidth();
+ // 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((float)TotalBitSize / CHAR_BIT));
+ CharUnits::fromQuantity(ceil((float)TotalBitSize / CharSize));
+ 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() / CharSize) != NewAlign.getQuantity()) &&
+ // ((Struct->getMaxAlignment() / CHAR_BIT) != 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")
+ << Struct << (int)MinByteSize.getQuantity()
+ << (int)CurrSize.getQuantity();
+ diag(Struct->getLocation(),
+ "use \"__attribute__((packed))\" to reduce the amount of padding "
+ "applied to struct %0", DiagnosticIDs::Note)
+ << Struct;
+ }
+
+ // 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 recommended alignment is %2 bytes")
+ << Struct << (int)CurrAlign.getQuantity()
+ << (int)NewAlign.getQuantity();
+ diag(Struct->getLocation(),
+ "use \"__attribute__((aligned(%0)))\" to align struct %1 to %0 bytes",
+ DiagnosticIDs::Note)
+ << (int)NewAlign.getQuantity() << Struct;
+ }
+}
+
+void StructPackAlignCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "MaxConfiguredAlignment", MaxConfiguredAlignment);
+}
+
+} // namespace altera
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/altera/CMakeLists.txt
===================================================================
--- /dev/null
+++ clang-tidy/altera/CMakeLists.txt
@@ -0,0 +1,15 @@
+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyAlteraModule
+ AlteraTidyModule.cpp
+ StructPackAlignCheck.cpp
+
+ LINK_LIBS
+ clangAnalysis
+ clangAST
+ clangASTMatchers
+ clangBasic
+ clangLex
+ clangTidy
+ clangTidyUtils
+ )
Index: clang-tidy/altera/AlteraTidyModule.cpp
===================================================================
--- /dev/null
+++ clang-tidy/altera/AlteraTidyModule.cpp
@@ -0,0 +1,39 @@
+//===--- AlteraTidyModule.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 "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+#include "StructPackAlignCheck.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace altera {
+
+class AlteraModule : public ClangTidyModule {
+public:
+ void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+ CheckFactories.registerCheck<StructPackAlignCheck>(
+ "altera-struct-pack-align");
+ }
+};
+
+} // namespace altera
+
+// Register the AlteraTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<altera::AlteraModule>
+ X("altera-module", "Adds Altera FPGA OpenCL lint checks.");
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the AlteraModule.
+volatile int AlteraModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/ClangTidyForceLinker.h
===================================================================
--- clang-tidy/ClangTidyForceLinker.h
+++ clang-tidy/ClangTidyForceLinker.h
@@ -25,6 +25,11 @@
static int LLVM_ATTRIBUTE_UNUSED AbseilModuleAnchorDestination =
AbseilModuleAnchorSource;
+// This anchor is used to force the linker to link the AlteraModule.
+extern volatile int AlteraModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED AlteraModuleAnchorDestination =
+ AlteraModuleAnchorSource;
+
// This anchor is used to force the linker to link the BoostModule.
extern volatile int BoostModuleAnchorSource;
static int LLVM_ATTRIBUTE_UNUSED BoostModuleAnchorDestination =
Index: clang-tidy/CMakeLists.txt
===================================================================
--- clang-tidy/CMakeLists.txt
+++ clang-tidy/CMakeLists.txt
@@ -41,6 +41,7 @@
# If you add a check, also add it to ClangTidyForceLinker.h in this directory.
add_subdirectory(android)
add_subdirectory(abseil)
+add_subdirectory(altera)
add_subdirectory(boost)
add_subdirectory(bugprone)
add_subdirectory(cert)
@@ -65,6 +66,7 @@
set(ALL_CLANG_TIDY_CHECKS
clangTidyAndroidModule
clangTidyAbseilModule
+ clangTidyAlteraModule
clangTidyBoostModule
clangTidyBugproneModule
clangTidyCERTModule
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
