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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/altera/SingleWorkItemBarrierCheck.cpp:34
+                                             hasName("barrier"),
+                                             hasName("work_group_barrier")))))
+                                    .bind("barrier")),
----------------
You can use `hasAnyName()` and pass both strings rather than two `hasName()` calls.


================
Comment at: clang-tools-extra/clang-tidy/altera/SingleWorkItemBarrierCheck.cpp:38
+              unless(hasDescendant(callExpr(callee(functionDecl(anyOf(
+                  hasName("get_global_id"), hasName("get_local_id")))))))))
+          .bind("function"),
----------------
Same here.


================
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) {
----------------
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).


================
Comment at: clang-tools-extra/clang-tidy/altera/SingleWorkItemBarrierCheck.cpp:61
+    diag(MatchedDecl->getLocation(),
+         "Kernel function %0 does not call get_global_id or get_local_id and "
+         "will be treated as single-work-item.\nBarrier call at %1 may error "
----------------
clang-tidy diagnostics shouldn't start with a capital letter or contain terminating punctuation, the function names should be surrounded in single quotes, and we don't typically use newlines or print out source locations like that in diagnostics. How about:

`kernel function %0 does not call 'get_global_id' or 'get_local_id' and is treated as a single work-item`

and issue a note diagnostic `barrier call may error out` at the barrier location?


================
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 "
----------------
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.)


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/altera-single-work-item-barrier.rst:7
+Finds OpenCL kernel functions that call a barrier function but do not call
+an ID function.
+
----------------
Maybe link to documentation describing what an ID function is (or describe it more explicitly here)?


================
Comment at: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn:16
     "AlteraTidyModule.cpp",
+    "SingleWorkItemBarrierCheck.cpp",
     "StructPackAlignCheck.cpp",
----------------
FWIW, you don't need to maintain the gn files and can leave them out of your patch.


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

https://reviews.llvm.org/D72241