This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] new altera unroll loops check
ClosedPublic

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
41

Please elide braces.

66

Please elide braces.

93

Please elide braces.

130

Please elide braces.

138

Please elide braces.

141

Please elide braces.

152

Please elide braces.

156

Please elide braces.

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

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
41
44
52–53
59
86

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

112

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.

146

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
5
clang-tools-extra/test/clang-tidy/checkers/altera-unroll-loops.cpp
360

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?

ffrankies updated this revision to Diff 328579.Mar 5 2021, 10:54 AM
ffrankies marked 12 inline comments as done.
  • Added support for CXXForRangeStmt loops
  • Added support for different for loop increments (++, --, +=, -=, *=, \=)
    • Depending on the exit condition, the calculations for the number of Iterations may be off by 1, but that should not be an issue performance-wise since the threshold for a "large" number of iterations is likely to be arbitrary.
    • If any other increment is used in the increment part of the for loop, we recommend partial unrolling.
  • Changed the way while and do..while loops are handled: because the loop variable is changed within the loop body and it is untrivial to determine what is happening to it, we now emit a Note diagnostic (instead of warning) recommending partial unrolling if a while or do..while loop is fully unrolled.
  • Added tests for the above
  • Documented the above caveats in clang-tools-extra/docs/clang-tidy/checks/altera-unroll-loops.rst
Eugene.Zelenko added inline comments.Mar 5 2021, 4:40 PM
clang-tools-extra/docs/clang-tidy/checks/altera-unroll-loops.rst
16

Please use double back-ticks for language constructs. Same in rest of note text.

ffrankies updated this revision to Diff 328933.Mar 7 2021, 9:40 PM
  • Surrounded language constructs with double back ticks in clang-tools-extra/docs/clang-tidy/checks/altera-unroll-loops.rst.
  • Removed trailing whitespace.
  • Added single quotes around #pragma unrolls in diagnostics.
  • Added full stops to the comments that didn't have them in clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp
ffrankies updated this revision to Diff 328935.Mar 7 2021, 10:02 PM
ffrankies marked 4 inline comments as done.
  • Removed unnecessary braces.
  • Simplified boolean return statements.
ffrankies updated this revision to Diff 329082.Mar 8 2021, 11:35 AM
ffrankies marked 6 inline comments as done.

Rebased with the latest main branch to fix patch application errors.

ffrankies updated this revision to Diff 329207.Mar 8 2021, 9:38 PM
  • Ran git-clang-format HEAD^
  • Addressed comments automatically added by the Lint: Pre-merge Checks
aaron.ballman accepted this revision.Mar 18 2021, 7:57 AM

Mostly just nits for the check, otherwise this LGTM.

clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp
66
84–85

Pretty sure you don't need to use a qualified name here.

117–121
124
177–178

The cast isn't necessary (IntergerLiteral is already an Expr). You should probably assert that CXXLoopBound isn't null here though.

181–182
197–198
This revision is now accepted and ready to land.Mar 18 2021, 7:57 AM
ffrankies updated this revision to Diff 332303.Mar 22 2021, 8:54 AM

Implemented changes requested by @aaron.ballman

ffrankies marked 7 inline comments as done.Mar 22 2021, 8:58 AM

@aaron.ballman If there are no more changes, can you please commit this on my behalf? My github username is ffrankies.

Thanks! I've commit on your behalf in 5a87f81fe9aee996dfe3a84dd833f0a48e093e7f

Thank you very much!