Page MenuHomePhabricator

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

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.Mon, Nov 2, 8:34 AM
clang-tools-extra/clang-tidy/altera/SingleWorkItemBarrierCheck.cpp
34

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

38

Same here.

49

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

61

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?

71

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
7

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

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