ffrankies updated this revision to Diff 219358.
ffrankies added a comment.
Herald added subscribers: kadircet, jkorous.
Sorry for the delay.
I was mostly having trouble building clang-tidy after pulling from master and
having it recognize that the struct-pack-align check exists. I finally realized
that the check had to be "registered" in more files, and those changes are a
part of the update.
I have also updated the ReleaseNotes to use the word "checks" instead of "lint
checks", and implemented the suggestions from @alexfh for adhering to the style
guide and being more concise (thanks for those comments! They'll be useful to
most if not all of the other checks we're planning to submit).
Regarding the comment by @riccibruno: Our current plan is to submit checks as
part of two modules: "OpenCL" and "FPGA", where the "OpenCL" checks are taken
from OpenCL specifications, and "FPGA" checks are taken from Altera best
practices and restrictions guides. That said, the struct-pack-align check is
not specific to FPGAs; it's useful whenever a struct is moved from host to
device, which could be something other than an FPGA. We are unaware of another
module where this check would be more appropriate, so we stuck it here, but
we're open to other suggestions, including moving it to the OpenCL module.
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/fpga/CMakeLists.txt
clang-tidy/fpga/FPGATidyModule.cpp
clang-tidy/fpga/StructPackAlignCheck.cpp
clang-tidy/fpga/StructPackAlignCheck.h
clang-tidy/plugin/CMakeLists.txt
clang-tidy/tool/CMakeLists.txt
clangd/CMakeLists.txt
clangd/ClangdUnit.cpp
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,73 @@
+// 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
@@ -211,6 +211,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
+<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 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:`bugprone-dynamic-static-initializers
<clang-tidy/checks/bugprone-dynamic-static-initializers>` check.
Index: clangd/ClangdUnit.cpp
===================================================================
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -756,6 +756,7 @@
LINK_TIDY_MODULE(Bugprone);
LINK_TIDY_MODULE(CERT);
LINK_TIDY_MODULE(CppCoreGuidelines);
+LINK_TIDY_MODULE(FPGA);
LINK_TIDY_MODULE(Fuchsia);
LINK_TIDY_MODULE(Google);
LINK_TIDY_MODULE(HICPP);
Index: clangd/CMakeLists.txt
===================================================================
--- clangd/CMakeLists.txt
+++ clangd/CMakeLists.txt
@@ -119,6 +119,7 @@
clangTidyBugproneModule
clangTidyCERTModule
clangTidyCppCoreGuidelinesModule
+ clangTidyFPGAModule
clangTidyFuchsiaModule
clangTidyGoogleModule
clangTidyHICPPModule
Index: clang-tidy/tool/CMakeLists.txt
===================================================================
--- clang-tidy/tool/CMakeLists.txt
+++ clang-tidy/tool/CMakeLists.txt
@@ -23,6 +23,7 @@
clangTidyBugproneModule
clangTidyCERTModule
clangTidyCppCoreGuidelinesModule
+ clangTidyFPGAModule
clangTidyFuchsiaModule
clangTidyGoogleModule
clangTidyHICPPModule
Index: clang-tidy/plugin/CMakeLists.txt
===================================================================
--- clang-tidy/plugin/CMakeLists.txt
+++ clang-tidy/plugin/CMakeLists.txt
@@ -14,6 +14,7 @@
clangTidyBugproneModule
clangTidyCERTModule
clangTidyCppCoreGuidelinesModule
+ clangTidyFPGAModule
clangTidyFuchsiaModule
clangTidyGoogleModule
clangTidyHICPPModule
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,105 @@
+//===--- 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_CONFIGURED_ALIGNMENT = 128; // 1 << 7;
+
+void StructPackAlignCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(recordDecl(isStruct(), isDefinition()).bind("struct"),
+ this);
+}
+
+void StructPackAlignCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *Struct = Result.Nodes.getNodeAs<RecordDecl>("struct");
+
+ // Get sizing info for the struct
+ std::vector<std::pair<unsigned int, unsigned int>> 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((TotalBitSize + 7) / 8);
+ CharUnits CurrAlign =
+ Result.Context->getASTRecordLayout(Struct).getAlignment();
+ 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;
+ }
+
+ // Check if struct has a "packed" attribute
+ bool IsPacked = false;
+ if (Struct->hasAttrs()) {
+ for (const Attr *StructAttribute : Struct->getAttrs()) {
+ if (StructAttribute->getKind() == attr::Packed) {
+ 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() / 8) != 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 @@
+//===--- FPGATidyModule.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
+ )
Index: clang-tidy/ClangTidyForceLinker.h
===================================================================
--- clang-tidy/ClangTidyForceLinker.h
+++ clang-tidy/ClangTidyForceLinker.h
@@ -50,6 +50,11 @@
static int LLVM_ATTRIBUTE_UNUSED CppCoreGuidelinesModuleAnchorDestination =
CppCoreGuidelinesModuleAnchorSource;
+// This anchor is used to force the linker to link the FPGAModule.
+extern volatile int FPGAModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED FPGAModuleAnchorDestination =
+ FPGAModuleAnchorSource;
+
// This anchor is used to force the linker to link the FuchsiaModule.
extern volatile int FuchsiaModuleAnchorSource;
static int LLVM_ATTRIBUTE_UNUSED FuchsiaModuleAnchorDestination =
Index: clang-tidy/CMakeLists.txt
===================================================================
--- clang-tidy/CMakeLists.txt
+++ clang-tidy/CMakeLists.txt
@@ -42,6 +42,7 @@
add_subdirectory(bugprone)
add_subdirectory(cert)
add_subdirectory(cppcoreguidelines)
+add_subdirectory(fpga)
add_subdirectory(fuchsia)
add_subdirectory(google)
add_subdirectory(hicpp)
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
