This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] new altera single work item barrier check
ClosedPublic

Authored by ffrankies on Jan 5 2020, 7:31 PM.

Details

Summary

This lint check is a part of the FLOCL (FPGA Linters for OpenCL) project out of the Synergy Lab at Virginia Tech.

FLOCL is a set of lint checks aimed at FPGA developers who write code in OpenCL.

The altera single work item barrier check finds OpenCL kernel functions that call a barrier function but do not call an ID function. These kernel functions will be treated as single work-item kernels, which could be inefficient or lead to errors.

Based on the "Altera SDK for OpenCL: Best Practices Guide."

Depends on https://reviews.llvm.org/D66564 due to the altera module being introduced there.

Diff Detail

Event Timeline

ffrankies created this revision.Jan 5 2020, 7:31 PM

I think will be good idea to separate module code in own review or refer to previous one of previous reviews as dependency.

clang-tidy/altera/SingleWorkItemBarrierCheck.cpp
51 ↗(On Diff #236289)

const auto *.

69 ↗(On Diff #236289)

Just IsNDRange. See readability-simplify-boolean-expr.

docs/clang-tidy/checks/altera-single-work-item-barrier.rst
51 ↗(On Diff #236289)

Please highlight 1600 with single back-quotes.

mgehre removed a subscriber: mgehre.Jan 5 2020, 10:59 PM
ffrankies updated this revision to Diff 243717.Feb 10 2020, 7:38 PM
ffrankies marked 3 inline comments as done.

Implemented requested changes by @Eugene.Zelenko

  • Changed auto to const auto *
  • Changed if(IsNDRange == true) to if(IsNDRange)
  • Highlighted 1600 with single back-quotes
  • Added link to https://reviews.llvm.org/D66564 as a dependency in Summary (is there someplace else I can refer to that as a dependency?)
ffrankies updated this revision to Diff 254304.Apr 1 2020, 1:44 PM
Eugene.Zelenko edited reviewers, added: njames93; removed: Eugene.Zelenko.Apr 1 2020, 2:10 PM
Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
ffrankies updated this revision to Diff 295698.Oct 1 2020, 5:14 PM
ffrankies removed a project: Restricted Project.
  • Rebased code and fixed merge conflicts with D66564
  • Added SingleWorkItemBarrierCheck.cpp to BUILD.gn
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2020, 5:14 PM
aaron.ballman added inline comments.Nov 2 2020, 8:34 AM
clang-tools-extra/clang-tidy/altera/SingleWorkItemBarrierCheck.cpp
35

You can use hasAnyName() and pass both strings rather than two hasName() calls.

39

Same here.

50

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

62

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?

72

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

clang-tools-extra/docs/clang-tidy/checks/altera-single-work-item-barrier.rst
8

Maybe link to documentation describing what an ID function is (or describe it more explicitly here)?

llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
16 ↗(On Diff #295698)

FWIW, you don't need to maintain the gn files and can leave them out of your patch.

ffrankies updated this revision to Diff 309565.Dec 4 2020, 10:05 AM

Implemented changes requested by @aaron.ballman

  • using hasAnyName() instead of multiple hasName() calls in the matcher
  • switched to a combination of hasAttr<>() and getAttr<>() to remove need for casting and looping over all attributes (did not use specific_attrs<>() because there should only be one ReqdWorkGroupSizeAttr per function)
  • re-phrased diagnostic messages
  • added all 4 ID functions to the documentation
  • removed the update to BUILD.gn
This revision is now accepted and ready to land.Dec 9 2020, 12:38 PM
ffrankies marked 4 inline comments as done.Dec 11 2020, 11:40 AM

@aaron.ballman Thank you! If there are no further comments, could you please commit this on my behalf? My GitHub username is ffrankies.

clang-tools-extra/clang-tidy/altera/SingleWorkItemBarrierCheck.cpp
50

I went with hasAttr<> and then getAttr<> to avoid a loop over all attributes - there should only be one ReqdWorkGroupSizeAttr

72

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.

@aaron.ballman Thank you! If there are no further comments, could you please commit this on my behalf? My GitHub username is ffrankies.

I'd be happy to except the diff seems to be causing me problems that I'm struggling to figure out how to solve.

F:\llvm-project>git apply "F:\Aaron Ballman\Desktop\D72241.diff"
F:/Aaron Ballman/Desktop/D72241.diff:207: trailing whitespace.
Based on the `Altera SDK for OpenCL: Best Practices Guide
F:/Aaron Ballman/Desktop/D72241.diff:213: trailing whitespace.

F:/Aaron Ballman/Desktop/D72241.diff:230: trailing whitespace.

F:/Aaron Ballman/Desktop/D72241.diff:279: trailing whitespace.
  // CHECK-MESSAGES-OLDCLOLDAOC: :[[@LINE-1]]:3: note: barrier call is in a sing
le work-item and may error out
F:/Aaron Ballman/Desktop/D72241.diff:339: trailing whitespace.
  // CHECK-MESSAGES-NEWCLOLDAOC: :[[@LINE-1]]:3: note: barrier call is in a sing
le work-item and may error out
error: clang-tidy/altera/AlteraTidyModule.cpp: No such file or directory
error: clang-tidy/altera/CMakeLists.txt: No such file or directory
error: docs/ReleaseNotes.rst: No such file or directory
error: docs/clang-tidy/checks/list.rst: No such file or directory

What's so strange is -- those files all exist for me. I tried switching the directory separators to be Windows separators, but then I got errors about invalid file paths. Can you rebase your patch on ToT and maybe that'll apply more cleanly for me?

ffrankies updated this revision to Diff 312658.Dec 17 2020, 6:09 PM
ffrankies marked an inline comment as done.

@aaron.ballman hmm, that is strange. I've rebased the patch and updated the diff, let me know if this one doesn't work either or there's something else you'd like me to try. Thanks! For what it's worth it's building just fine and passing the clang-tools tests on my end.

aaron.ballman closed this revision.Dec 18 2020, 4:53 AM

@aaron.ballman hmm, that is strange. I've rebased the patch and updated the diff, let me know if this one doesn't work either or there's something else you'd like me to try. Thanks! For what it's worth it's building just fine and passing the clang-tools tests on my end.

This time everything applied cleanly and worked fine -- not certain what was up before. I've commit on your behalf in e69e551e0e5fddffb6479da6a2998457104ba9e6, thank you for the new check!

njames93 added a comment.EditedDec 18 2020, 4:53 AM

@aaron.ballman hmm, that is strange. I've rebased the patch and updated the diff, let me know if this one doesn't work either or there's something else you'd like me to try. Thanks! For what it's worth it's building just fine and passing the clang-tools tests on my end.

FWIW This version I was able to pull just fine and run the clang tidy tests without a hitch.

NVM, aaron has already sorted it

@aaron.ballman @njames93 thanks for testing this out, the feedback, and the commit!

Could you also take a look at D72235 unroll loops check ? It's been updated and is ready for review.