[PATCH] D72241: [clang-tidy] new altera single work item barrier check

[Frank Derry Wanye via Phabricator via llvm-commits]

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

@aaron.ballman Thank you! If there are no further comments, could you please commit this on my behalf? My GitHub username is ffrankies <https://github.com/ffrankies>.



================
Comment at: clang-tools-extra/clang-tidy/altera/SingleWorkItemBarrierCheck.cpp:49
+  bool IsNDRange = false;
+  for (const Attr *Attribute : MatchedDecl->getAttrs()) {
+    if (Attribute->getKind() == attr::Kind::ReqdWorkGroupSize) {
----------------
aaron.ballman wrote:
> Rather than getting all attributes, you can use `specific_attrs<>` to loop over just the specific attributes you care about on the declaration. That also fixes the non-idiomatic way to convert from an attribute to a specific derived attribute (which should use `dyn_cast<>` or friends).
I went with `hasAttr<>` and then `getAttr<>` to avoid a loop over all attributes - there should only be one `ReqdWorkGroupSizeAttr`


================
Comment at: clang-tools-extra/clang-tidy/altera/SingleWorkItemBarrierCheck.cpp:71
+    diag(MatchedDecl->getLocation(),
+         "Kernel function %0 does not call get_global_id or get_local_id may "
+         "be a viable single work-item kernel, but barrier call at %1 will "
----------------
aaron.ballman wrote:
> Same here as above.
> 
> Will users know what `NDRange execution` means? (I'm can't really understand what the diagnostic is telling me, but this is outside of my typical domain.)
I discussed this with other students in my lab and the consensus there is this should be clear for OpenCL/OpenCL-for-FPGA users. If really needed, we can try to re-phrase it, but then the diagnostics will most likely become more wordy.


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

https://reviews.llvm.org/D72241