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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
