Page MenuHomePhabricator

[clang-tidy] new altera ID dependent backward branch check
ClosedPublic

Authored by ffrankies on Nov 11 2019, 10:36 AM.

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 ID dependent backward branch lint check finds ID dependent variables and fields used within loops, and warns of their usage. Using these variables in loops can lead to performance degradation.

Diff Detail

Event Timeline

ffrankies created this revision.Nov 11 2019, 10:36 AM
jdoerfert resigned from this revision.Nov 11 2019, 11:55 AM
Eugene.Zelenko added inline comments.
clang-tidy/altera/IdDependentBackwardBranchCheck.cpp
107

Please don't use auto when type is not explicitly spelled in same statement or iterator.

130

Please don't use auto when type is not explicitly spelled in same statement or iterator.

mgehre removed a subscriber: mgehre.Nov 11 2019, 10:55 PM

Implemented changes requested by @Eugene.Zelenko

Also changed for loop in hasIdDepVar and hasIdDepField to range-based for loops, and updated the license information in IdDependentBackwardBranchCheck.cpp

clang-tidy/altera/IdDependentBackwardBranchCheck.cpp
116

May be comments were shifted with code, but here const auto * could be used, because type is spelled in dyn_cast<>.

117

May be comments were shifted with code, but here const auto * could be used, because type is spelled in dyn_cast<>.

125

May be comments were shifted with code, but here const auto * could be used, because type is iterator.

126

May be comments were shifted with code, but here const auto * could be used, because type is spelled in dyn_cast<>.

166

const auto *. Same in other dyn_cast<>.

ffrankies updated this revision to Diff 232724.Dec 7 2019, 8:13 PM
ffrankies marked 2 inline comments as done.

Addressed @Eugene.Zelenko's comments on use of auto

ffrankies updated this revision to Diff 254305.Apr 1 2020, 1:45 PM
ffrankies marked 5 inline comments as done.
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 334979.Apr 2 2021, 10:05 AM
  • Rebased on top of latest changes in main branch
  • The diagnostic that identifies the code location where an ID-dependent variable/field is assigned has been changed from a warning to a note
  • Changes addressing code style
Eugene.Zelenko added inline comments.Apr 2 2021, 10:36 AM
clang-tools-extra/docs/clang-tidy/checks/altera-id-dependent-backward-branch.rst
9 ↗(On Diff #334979)

Usually link to external documentation is placed at the end. See other checks documentation as example.

ffrankies updated this revision to Diff 335195.Apr 4 2021, 10:09 PM
  • Addressed comments from the automated pre-merge checks
  • Moved link to external documentation to the end of clang-tools-extra/docs/clang-tidy/checks/altera-id-dependent-backward-branch.rst as requested by @Eugene.Zelenko
Eugene.Zelenko added inline comments.Apr 5 2021, 7:03 AM
clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.cpp
200 ↗(On Diff #335195)

Is loop ID is not enough? Why does comment with numerical code used? Same below.

clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.h
1 ↗(On Diff #335195)

Please add space after clang-tidy and language code. See other checks as example.

36 ↗(On Diff #335195)

Please use = default;

ffrankies updated this revision to Diff 336512.EditedApr 9 2021, 10:31 AM

Addressed comments by @Eugene.Zelenko and the automated build bot

  • Fixed header comments and include guard style
  • Removed unnecessary comments in getLoopType()
  • changed IDDependencyRecord() {} to IDDependencyRecord() = default;
Eugene.Zelenko added inline comments.Apr 9 2021, 10:38 AM
clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.cpp
1 ↗(On Diff #336512)

*- C++ -* is not necessary for .cpp files.

ffrankies updated this revision to Diff 338160.Apr 16 2021, 9:58 AM
ffrankies marked 3 inline comments as done.

Removed *- C++ -* from IdDependentBackwardBranchCheck.cpp file-level comment.

ffrankies marked an inline comment as done.Apr 29 2021, 11:47 AM

@Eugene.Zelenko @aaron.ballman Are there any more changes that need to be made to this check or comments that need to be addressed?

clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.cpp
200 ↗(On Diff #335195)

Removed the comment with numerical code (I simply added it to avoid having to check the header file to see their numerical value). The LoopType is used in the diagnostics to select and emit the correct loop type as part of the diagnostic message, e.g.:

diag(CondExpr->getBeginLoc(),
        "backward branch (%select{do|while|for}0 loop) is ID-dependent due "
        "to ID function call and may cause performance degradation")
           << Type;

I'm assuming the loop ID is a unique object identifier, so I don't think it'll serve the same purpose.

aaron.ballman added inline comments.Apr 30 2021, 12:11 PM
clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.cpp
28–31 ↗(On Diff #338160)

Can you use the isAssignmentOperator() AST matcher instead of spelling out the operator names manually?

54 ↗(On Diff #338160)
101–102 ↗(On Diff #338160)
121–122 ↗(On Diff #338160)
132–134 ↗(On Diff #338160)

You should use an llvm::Twine for this.

141–143 ↗(On Diff #338160)

Same here as above.

154 ↗(On Diff #338160)

I'd use llvm::raw_string_ostream here (basically, try to avoid STL iostream functionality as much as possible, it's really slow/bad).

199–204 ↗(On Diff #338160)

Rather than repeatedly use isa<>, I'd consider using a switch over Loop->getStmtClass(). This also addresses the "no else after a return" coding style concern.

clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.h
26 ↗(On Diff #338160)

The enumerators don't match the style guide -- can you rename them to UnknownLoop, DoLoop, etc so they look less like macros?

32 ↗(On Diff #338160)
35 ↗(On Diff #338160)
37–38 ↗(On Diff #338160)

To ensure they get initialized to something sensible from the default ctor.

ffrankies updated this revision to Diff 342278.May 2 2021, 2:29 PM

Addressed comments by @aaron.ballman

  • Changed capitalization in enum
  • Used std::move in IdDependencyRecord constructors
  • Initialized VariableDeclaration and FieldDeclaration to nullptr
  • Used isAssignmentOperator() instead of listing the assingment operators in the matchers
  • Simplified code around if statement expressions
  • Switched to llvm::Twine and llvm::raw_string_ostream when constructing warning and note messages
  • Changed getLoopType() body to a switch statement instead of a series of if-else statements
ffrankies updated this revision to Diff 342280.May 2 2021, 2:42 PM
ffrankies marked 12 inline comments as done.

Removed unused import statements from IdDependentBackwardBranch.cpp

aaron.ballman added inline comments.May 3 2021, 6:01 AM
clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.h
32 ↗(On Diff #338160)

Ah, it looks like some changes conflicted with my suggestion -- when the function was taking a std::string, the move was needed, but now with a Twine, the move is an issue. You should remove the std::move.

35 ↗(On Diff #338160)

Same here.

ffrankies updated this revision to Diff 342467.May 3 2021, 10:58 AM

Addressed comment by @aaron.ballman and the Pre-update checks linter

  • Removed calls to std::move for llvm::Twine::str() object in IdDependencyRecord constructors
aaron.ballman accepted this revision.May 5 2021, 9:47 AM

LGTM! Btw, given that you've had several of these approved, would you be interested in getting commit access (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access)?

This revision is now accepted and ready to land.May 5 2021, 9:47 AM
ffrankies marked 2 inline comments as done.May 6 2021, 12:39 PM

@aaron.ballman Thank you! As far as I'm aware, this is the last check that we are planning to submit, so if I do get commit access now it's likely to be unused. However, if that does change, then yes I would be interested in obtaining commit access. For now, can you please commit this on my behalf? My github username is ffrankies.

aaron.ballman closed this revision.May 6 2021, 2:03 PM

@aaron.ballman Thank you! As far as I'm aware, this is the last check that we are planning to submit, so if I do get commit access now it's likely to be unused. However, if that does change, then yes I would be interested in obtaining commit access. For now, can you please commit this on my behalf? My github username is ffrankies.

Ah, good to know! Thank you for the new check -- I've commit on your behalf in 83af66e18e3d3760d56ea7e3bdbff3428ae7730d