Page MenuHomePhabricator

[clang-tidy] new altera unroll loops check
Needs ReviewPublic

Authored by ffrankies on Jan 5 2020, 4:09 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 unroll loops check finds inner loops that have not been unrolled, as well as fully-unrolled loops that should be partially unrolled due to unknown loop bounds or a large number of loop iterations.

Based on the Altera SDK for OpenCL: Best Practices Guide

Diff Detail

Event Timeline

ffrankies created this revision.Jan 5 2020, 4:09 PM
ffrankies updated this revision to Diff 236270.Jan 5 2020, 4:12 PM
ffrankies updated this revision to Diff 236271.Jan 5 2020, 4:15 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/UnrollLoopsCheck.cpp
31 ↗(On Diff #236271)

Could be const auto *, because type is spelled in same sentence. See modernize-use-auto.

69 ↗(On Diff #236271)

May be merge declaration with initialization and use const auto *?

96 ↗(On Diff #236271)

Could be const auto *, because type is spelled in same sentence.

113 ↗(On Diff #236271)

Could be const auto *, because type is spelled in same sentence.

117 ↗(On Diff #236271)

Could be const auto *, because type is spelled in same sentence.

121 ↗(On Diff #236271)

Could be const auto *, because type is spelled in same sentence.

134 ↗(On Diff #236271)

Could be const auto *, because type is spelled in same sentence.

docs/ReleaseNotes.rst
79 ↗(On Diff #236271)

Please synchronize with first statement in documentation.

docs/clang-tidy/checks/altera-unroll-loops.rst
68 ↗(On Diff #236271)

Please document default value.

Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.Jan 5 2020, 5:32 PM
Eugene.Zelenko removed a project: Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2020, 5:32 PM
ffrankies marked 9 inline comments as done.

Addressed comments by @Eugene.Zelenko

  • Used const auto * when type is mentioned in same sentence
  • Fixed formatting in documentation
  • Documented default value of MaxLoopIterations option

Please rebase from master.

ffrankies updated this revision to Diff 248322.Mar 4 2020, 2:49 PM

@Eugene.Zelenko It turns out we were using an old mirror of the clang-tools-extra repository, that no longer seems to be updated. I switched to the llvm-project repo, and updated the documentation accordingly (some of the formatting is different now). Let me know if I missed anything - if not, I will be switching the other checks over to the llvm-project repo as well over the coming weeks.

Eugene.Zelenko added inline comments.Mar 4 2020, 3:16 PM
clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp
42

Please elide braces.

67

Please elide braces.

94

Please elide braces.

131

Please elide braces.

139

Please elide braces.

142

Please elide braces.

153

Please elide braces.

157

Please elide braces.

clang-tools-extra/docs/ReleaseNotes.rst
99

Please synchronize with first statement in documentation.

ffrankies updated this revision to Diff 254299.Apr 1 2020, 1:41 PM

Addressed comments by @Eugene.Zelenko

  • Removed braces from one-lien if statements
  • Release notes on the altera unroll loops check now match the first line of documentation.
Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
Eugene.Zelenko removed a project: Restricted Project.
ffrankies updated this revision to Diff 312675.Dec 17 2020, 7:44 PM
ffrankies marked 9 inline comments as done.
  • Rebased with master branch
  • Diagnostics are re-worded so they're not complete sentences

Check ready for further review

ffrankies updated this revision to Diff 312676.Dec 17 2020, 7:47 PM
  • removed unnecessary comment
njames93 added inline comments.Dec 18 2020, 8:48 AM
clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp
23

Dont use all caps variable names, preferred style is CamelCase.

25–27
36

This loops like it should be a switch

69

Shouldn't this by dyn_cast too.

70

To reduce indentations can this be an early exit

80

Default statements are usually bad as the prevent compiler diagnostics if the enum is ever updated.

94–95
108–121
129

dyn_cast again

146

Elide inner parens

149–151
clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.h
32–34

Should probably define this out of line in the cpp file.

  • Addressed comments from @njames93
  • Rebased with latest master branch to make sure there are no merge issues with the latest committed altera check
aaron.ballman added inline comments.Jan 4 2021, 12:58 PM
clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp
40
43
51–52
58
85

This looks like dead code (it's still inside the switch statement).

111

Should we also handle CXXForRangeStmt? I'm thinking of cases like: for (int i : {1, 2, 3, 4}) {} or other cases where there's a known bounds.

145

If the check won't work too well if the value is decremented rather than incremented, or if it uses an induction variable that increases by something other than 1 or -1, we should probably document the limitations and consider whether we want to note that scenario in the code and bail out (rather than produce incorrect diagnostics, assuming we do).

clang-tools-extra/docs/clang-tidy/checks/altera-unroll-loops.rst
4
clang-tools-extra/test/clang-tidy/checkers/altera-unroll-loops.cpp
359

Can you also add some test cases where the loop is decrementing rather than incrementing, and some tests where the increment is by more than 1?