ffrankies updated this revision to Diff 216928.
ffrankies added a comment.
Implemented changes requested by Eugene.Zelenko
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66564/new/
https://reviews.llvm.org/D66564
Files:
clang-tidy/fpga/CMakeLists.txt
clang-tidy/fpga/FPGATidyModule.cpp
clang-tidy/fpga/StructPackAlignCheck.cpp
clang-tidy/fpga/StructPackAlignCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/fpga-struct-pack-align.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/index.rst
test/clang-tidy/fpga-struct-pack-align.cpp
Index: test/clang-tidy/fpga-struct-pack-align.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/fpga-struct-pack-align.cpp
@@ -0,0 +1,74 @@
+// RUN: %check_clang_tidy %s fpga-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: struct 'error' has inefficient access due to padding, only needs 10 bytes but is using 24 bytes, use "__attribute((packed))" [fpga-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: warning: struct 'error' has inefficient access due to poor alignment. Currently aligned to 8 bytes, but size 10 bytes is large enough to benefit from "__attribute((aligned(16)))" [fpga-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: struct 'error_packed' has inefficient access due to poor alignment. Currently aligned to 1 bytes, but size 10 bytes is large enough to benefit from "__attribute((aligned(16)))" [fpga-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: struct 'align_only' has inefficient access due to poor alignment. Currently aligned to 8 bytes, but size 16 bytes is large enough to benefit from "__attribute((aligned(16)))" [fpga-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: struct 'bad_align' has inefficient access due to poor alignment. Currently aligned to 8 bytes, but size 10 bytes is large enough to benefit from "__attribute((aligned(16)))" [fpga-struct-pack-align]
+
+struct bad_align2 {
+char a;
+double b;
+char c;
+} __attribute((packed)) __attribute((aligned(32)));
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: struct 'bad_align2' has inefficient access due to poor alignment. Currently aligned to 32 bytes, but size 10 bytes is large enough to benefit from "__attribute((aligned(16)))" [fpga-struct-pack-align]
+
+struct bad_align3 {
+char a;
+double b;
+char c;
+} __attribute((packed)) __attribute((aligned(4)));
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: struct 'bad_align3' has inefficient access due to poor alignment. Currently aligned to 4 bytes, but size 10 bytes is large enough to benefit from "__attribute((aligned(16)))" [fpga-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/index.rst
===================================================================
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -64,6 +64,7 @@
``cert-`` Checks related to CERT Secure Coding Guidelines.
``cppcoreguidelines-`` Checks related to C++ Core Guidelines.
``clang-analyzer-`` Clang Static Analyzer checks.
+``fpga-`` Checks related to OpenCL programming for FPGAs.
``fuchsia-`` Checks related to Fuchsia coding conventions.
``google-`` Checks related to Google coding conventions.
``hicpp-`` Checks related to High Integrity C++ Coding Standard.
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -210,6 +210,7 @@
cppcoreguidelines-pro-type-vararg
cppcoreguidelines-slicing
cppcoreguidelines-special-member-functions
+ fpga-struct-pack-align
fuchsia-default-arguments-calls
fuchsia-default-arguments-declarations
fuchsia-header-anon-namespaces (redirects to google-build-namespaces) <fuchsia-header-anon-namespaces>
Index: docs/clang-tidy/checks/fpga-struct-pack-align.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/fpga-struct-pack-align.rst
@@ -0,0 +1,50 @@
+.. title:: clang-tidy - fpga-struct-pack-align
+
+fpga-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" found here:
+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 a bad value will result in a warning
+ 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,16 @@
Improvements to clang-tidy
--------------------------
+- New :doc:`fpga <clang-tidy/index>` module
+
+ Contains lint checks related to OpenCL programming for FPGAs.
+
+- New :doc:`fpga-struct-pack-align
+ <clang-tidy/checks/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:`linuxkernel-must-use-errs
<clang-tidy/checks/linuxkernel-must-use-errs>` check.
Index: clang-tidy/fpga/StructPackAlignCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/fpga/StructPackAlignCheck.h
@@ -0,0 +1,35 @@
+//===--- 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_FPGA_STRUCTPACKALIGNCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FPGA_STRUCTPACKALIGNCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace FPGA {
+
+/// 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/FPGA-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;
+};
+
+} // namespace FPGA
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FPGA_STRUCTPACKALIGNCHECK_H
Index: clang-tidy/fpga/StructPackAlignCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/fpga/StructPackAlignCheck.cpp
@@ -0,0 +1,110 @@
+//===--- 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"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace FPGA {
+
+constexpr int MAX_ALIGN_POWER_OF_TWO = 7;
+
+void StructPackAlignCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(recordDecl(isStruct()).bind("struct"), this);
+}
+
+void StructPackAlignCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *Struct = Result.Nodes.getNodeAs<RecordDecl>("struct");
+ const RecordDecl *StructDef = Struct->getDefinition();
+
+ // If not a definition, do nothing
+ if (Struct != StructDef)
+ return;
+
+ // Get sizing info for the struct
+ std::vector<std::pair<unsigned int, unsigned int>> FieldSizes;
+ unsigned int TotalBitSize = 0;
+ for (auto 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.push_back(std::pair<unsigned int, unsigned int>(
+ StructFieldWidth, StructField->getFieldIndex()));
+ // TODO: Recommend a reorganization of the struct (sort by StructField size,
+ // largest to smallest)
+ TotalBitSize += StructFieldWidth;
+ }
+ // TODO: 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((TotalBitSize + 7) >> 3);
+ CharUnits CurrAlign =
+ Result.Context->getASTRecordLayout(Struct).getAlignment();
+ CharUnits NewAlign = CharUnits::fromQuantity(1);
+ if (!MinByteSize.isPowerOfTwo()) {
+ int MSB = (int)MinByteSize.getQuantity();
+ for (; MSB > 0; MSB >>= 1) {
+ NewAlign = NewAlign.alignTo(
+ CharUnits::fromQuantity(((int)NewAlign.getQuantity()) << 1));
+ // Abort if the computed alignment meets the maximum configured alignment
+ if (NewAlign.getQuantity() >= (1 << MAX_ALIGN_POWER_OF_TWO))
+ break;
+ }
+ } else {
+ NewAlign = MinByteSize;
+ }
+
+ // Check if struct has a "packed" attribute
+ bool IsPacked = false;
+ if (Struct->hasAttrs()) {
+ for (auto StructAttribute : Struct->getAttrs()) {
+ if (std::string(StructAttribute->getSpelling()).compare("packed") == 0) {
+ IsPacked = true;
+ break;
+ }
+ }
+ }
+
+ // 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() >> 3) != NewAlign.getQuantity()) &&
+ (CurrSize != NewAlign) && !IsPacked) {
+ diag(Struct->getLocation(),
+ "struct %0 has inefficient access 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(),
+ "struct %0 has inefficient access 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 FPGA
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/fpga/FPGATidyModule.cpp
===================================================================
--- /dev/null
+++ clang-tidy/fpga/FPGATidyModule.cpp
@@ -0,0 +1,38 @@
+//===--- OpenCLTidyModule.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 FPGA {
+
+class FPGAModule : public ClangTidyModule {
+public:
+ void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+ CheckFactories.registerCheck<StructPackAlignCheck>(
+ "fpga-struct-pack-align");
+ }
+};
+
+static ClangTidyModuleRegistry::Add<FPGAModule>
+ X("fpga-module", "Adds Altera FPGA OpenCL lint checks.");
+
+} // namespace FPGA
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the MyModule.
+volatile int FPGAModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/fpga/CMakeLists.txt
===================================================================
--- /dev/null
+++ clang-tidy/fpga/CMakeLists.txt
@@ -0,0 +1,14 @@
+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyFPGAModule
+ FPGATidyModule.cpp
+ StructPackAlignCheck.cpp
+
+ LINK_LIBS
+ clangAST
+ clangASTMatchers
+ clangBasic
+ clangLex
+ clangTidy
+ clangTidyUtils
+ )
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
