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

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies added a subscriber: alexandre.isoard.
ffrankies added a comment.
Herald added a subscriber: usaxena95.

@alexandre.isoard wrote:

> I'm not sure what is the advantage of this compared to -Wpadded?


This option only warns when padding exists. Our check does two things; it warns 
when there is //too much// padding applied to a struct, and when the alignment 
of the struct isn’t optimal.

In D66564#1670659 , @lebedev.ri wrote:

> I, too, don't believe this is FPGA specific; it should likely go into `misc-` 
> or even `performance-`.


The check can definitely be moved into another module (probably `performance-` 
is a better bet, since it deals with efficiency).

In D66564#1670659 , @lebedev.ri wrote:

> Forgot the most important question.
>  Right now this will fire on every single struct.
>  But it won't matter unless the alignment/size actually matters, and most 
> often that will happen when you have e.g. a vector of such structs.
>  What i'm asking is - should this be more picky, and complain only about the 
> cases where this matters?


I may need to give some context for this lint check (and the ones to follow, 
since we have a few others we’d like to upstream once we figure out the 
process/iron out some bugs).

Our checks are written from the perspective of programmers that write OpenCL 
code specifically for FPGAs, typically in a SIMD context. There is a compiler 
framework that performs the necessary compilation for that to work, but due to 
the nature of the FPGA hardware, it takes a long time for this compilation 
process to complete. Because of this, it is much preferable to use a static 
code analysis tool (clang-tidy) to catch errors/inefficiencies in the code 
before the expensive compilation process starts.

This specific check is based off of the documentation from Intel FPGA SDK for 
OpenCL Pro Edition: Best Practices Guide 
.
 The TLDR version is that if structs are not aligned and/or padded correctly, 
the resulting FPGA configuration becomes inefficient.

To answer the question; the size and alignment of the struct will matter when 
the struct is in an array, but also when the struct’s elements are accessed as 
part of an uncached loop.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Isoard via cfe-commits]

I'm not sure what is the advantage of this compared to -Wpadded?

On Sun, Sep 15, 2019 at 12:05 PM Roman Lebedev via Phabricator via
llvm-commits  wrote:

> lebedev.ri added a comment.
>
> Forgot the most important question.
> Right now this will fire on every single struct.
> But it won't matter unless the alignment/size actually matters, and most
> often that will happen when you have e.g. a vector of such structs.
> What i'm asking is - should this be more picky, and complain only about
> the cases where this matters?
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D66564/new/
>
> https://reviews.llvm.org/D66564
>
>
>
> ___
> llvm-commits mailing list
> llvm-comm...@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>


-- 
*Alexandre Isoard*
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Forgot the most important question.
Right now this will fire on every single struct.
But it won't matter unless the alignment/size actually matters, and most often 
that will happen when you have e.g. a vector of such structs.
What i'm asking is - should this be more picky, and complain only about the 
cases where this matters?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

I, too, don't believe this is FPGA specific; it should likely go into `misc-` 
or even `performance-`.
The wording of the diags seems weird to me, it would be good to 1. add more 
explanation to the docs and 2. reword the diags.




Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:31
+  // Get sizing info for the struct
+  std::vector> FieldSizes;
+  unsigned int TotalBitSize = 0;

`llvm::SmallVector`



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:50
+  CharUnits CurrSize = Result.Context->getASTRecordLayout(Struct).getSize();
+  CharUnits MinByteSize = CharUnits::fromQuantity((TotalBitSize + 7) / 8);
+  CharUnits CurrAlign =

`roundUpTo()`?



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:54-62
+  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)

I'm not sure what is going on here.
I think this should be a helper function with meaningful name.



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:67-76
+  // 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;

```
bool IsPacked = Struct->hasAttr();
```



Comment at: docs/clang-tidy/checks/fpga-struct-pack-align.rst:45
+
+  // Explicitly aligning a struct to a bad value will result in a warning
+  struct badly_aligned_example {

'bad'?



Comment at: test/clang-tidy/fpga-struct-pack-align.cpp:9
+};
+// 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]

'has inefficient access' reads weird to me. I'm not sure what that actually 
means.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies marked 22 inline comments as done.
ffrankies added a comment.

In D66564#1647779 , @BlackAngel35 
wrote:

> > Please mention new module and check in Release Notes.


The new module and check are now mentioned in Release Notes.

P.S. The above change was requested by a user that has since been disabled. Is 
there something I have to do about this?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Frank Derry Wanye via Phabricator via cfe-commits]

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;
+  

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

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

I do not understand why this check is specific to FPGAs.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Macide Celik via Phabricator via cfe-commits]

BlackAngel35 requested changes to this revision.
BlackAngel35 added a comment.
This revision now requires changes to proceed.

In D66564#1640584 , @Eugene.Zelenko 
wrote:

> Please mention new module and check in Release Notes.





CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Thanks for the contribution and welcome to the LLVM community! A few more 
comments from me. I hope, this will help to tune to the LLVM coding style and 
conventions. This doc may be useful to read, if you haven't done so already: 
http://llvm.org/docs/CodingStandards.html




Comment at: clang-tidy/fpga/FPGATidyModule.cpp:5
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.

Eugene.Zelenko wrote:
> License was changed this year. Same in other files.
Please mark the addressed comments "Done".



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:31
+  // If not a definition, do nothing
+  if (Struct != StructDef)
+return;

If you need to only process struct definitions, it's better to use the 
`isDefinition()` matcher to limit the matches to definitions 
(`recordDecl(isStruct(), isDefinition())`).



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:37
+  unsigned int TotalBitSize = 0;
+  for (auto StructField : Struct->fields()) {
+// For each StructField, record how big it is (in bits)

`const auto *` will make it clear that it's a pointer. Actually, I'd better 
specify the type explicitly.



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:45
+.Width;
+FieldSizes.push_back(std::pair(
+StructFieldWidth, StructField->getFieldIndex()));

`emplace_back(StructFieldWidth, StructField->getFieldIndex())` would be more 
succinct.



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:47
+StructFieldWidth, StructField->getFieldIndex()));
+// TODO: Recommend a reorganization of the struct (sort by StructField 
size,
+// largest to smallest)

It's more common to use `FIXME` than `TODO` in comments in LLVM code.



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:55
+  CharUnits CurrSize = Result.Context->getASTRecordLayout(Struct).getSize();
+  CharUnits MinByteSize = CharUnits::fromQuantity((TotalBitSize + 7) >> 3);
+  CharUnits CurrAlign =

` / 8` would be clearer than ` >> 3` (and any sane compiler would optimize this 
itself: https://gcc.godbolt.org/z/nXdGeU, even though here the optimization 
doesn't bring anything).



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:63
+  NewAlign = NewAlign.alignTo(
+  CharUnits::fromQuantity(((int)NewAlign.getQuantity()) << 1));
+  // Abort if the computed alignment meets the maximum configured alignment

Too many parentheses here. And the `<< 1` would be easier to read as `* 2`, if 
there's no intentional behavior difference here.



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:65
+  // Abort if the computed alignment meets the maximum configured alignment
+  if (NewAlign.getQuantity() >= (1 << MAX_ALIGN_POWER_OF_TWO))
+break;

A well named constant for `(1 << MAX_ALIGN_POWER_OF_TWO)` would make the code 
easier to understand, IMO.



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:75
+  if (Struct->hasAttrs()) {
+for (auto StructAttribute : Struct->getAttrs()) {
+  if (std::string(StructAttribute->getSpelling()).compare("packed") == 0) {

Same comment about `auto` as above: please specify the type explicitly or at 
least use `const auto *`.



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:76
+for (auto StructAttribute : Struct->getAttrs()) {
+  if (std::string(StructAttribute->getSpelling()).compare("packed") == 0) {
+IsPacked = true;

It's better to construct an llvm::StringRef than a std::string: the former 
doesn't copy the contents. Next thing is that `operator==` should be preferred 
here, while `.compare()` is needed when ordering is important.

But here this all is irrelevant: `StructAttribute->getKind() == attr::Packed` 
allows to check this condition without comparing strings.



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:100
+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 "

Diagnostic messages are not complete sentences. Thus, different punctuation is 
used: "alignment. Currently" -> "alignment; currently"


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:72
+
+  Contains lint checks related to OpenCL programming for FPGAs. 
+

Just checks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies updated this revision to Diff 217224.
ffrankies added a comment.

Added space after clang-tidy in header comments, updated check documentation to 
use link syntax.


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) 
Index: docs/clang-tidy/checks/fpga-struct-pack-ali

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/fpga/StructPackAlignCheck.h:1
+//===--- StructPackAlignCheck.h - clang-tidy-*- C++ 
-*-===//
+//

Please add space after clang-tidy. Same in source file.



Comment at: docs/clang-tidy/checks/fpga-struct-pack-align.rst:12
+
+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

You could use link syntax. See Fuchsia checks as example.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Frank Derry Wanye via Phabricator via cfe-commits]

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) 
Index: docs/clang-tidy/checks/fpga-struct-pack-align.rst
===

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/fpga/FPGATidyModule.cpp:15
+
+
+using namespace clang::ast_matchers;

Unnecessary empty line.



Comment at: clang-tidy/fpga/FPGATidyModule.cpp:32
+
+} // namespace flocl
+

Wrong namespace in comment. Try to run Clang-tidy llvm-namespace-comment.



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:10
+
+#ifndef MAX_ALIGN_POWER_OF_TWO
+#define MAX_ALIGN_POWER_OF_TWO 7

Should be  be constexpr.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention new module and check in Release Notes.




Comment at: clang-tidy/fpga/FPGATidyModule.cpp:5
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.

License was changed this year. Same in other files.



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:31
+  const auto *Struct = Result.Nodes.getNodeAs("struct");
+  const auto *StructDef = Struct->getDefinition();
+  

Please don't use auto when type is not spelled in same statement or not 
iterator.



Comment at: clang-tidy/fpga/StructPackAlignCheck.cpp:34
+  // If not a definition, do nothing
+  if (Struct != StructDef) return;
+  

Please run Clang-format.



Comment at: docs/clang-tidy/checks/fpga-struct-pack-align.rst:12
+
+Based on the "Altera SDK for OpenCL: Best Practices Guide".
+

Please add link to Altera documentation.



Comment at: docs/clang-tidy/index.rst:67
 ``clang-analyzer-``Clang Static Analyzer checks.
+``fpga-``  Checks related to OpenCL programming for FPGAs..
 ``fuchsia-``   Checks related to Fuchsia coding conventions.

Please fix double dot.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits