aaron.ballman added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:40
+ "kernel performance could be improved by unrolling this loop with a
"
+ "#pragma unroll directive");
+ break;
----------------
================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:43
+ case PartiallyUnrolled:
+ // Loop already partially unrolled, do nothing
+ break;
----------------
================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:51-52
+ "cannot be fully unrolled; to partially unroll this loop, use "
+ "the #pragma unroll <num> directive");
+ return;
+ }
----------------
================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:58
+ "full unrolling requested, but loop bounds are not known; to "
+ "partially unroll this loop, use the #pragma unroll <num> "
+ "directive");
----------------
================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:85
+ return NotUnrolled;
+ return NotUnrolled;
+ }
----------------
This looks like dead code (it's still inside the switch statement).
================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:111
+const Expr *UnrollLoopsCheck::getCondExpr(const Stmt *Statement) {
+ if (const auto *ForLoop = dyn_cast<ForStmt>(Statement))
+ return ForLoop->getCond();
----------------
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.
================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:145
+ // safe.
+ // The following assumes values go from 0 to Val in increments of 1.
+ return Result.Val.getInt() > MaxLoopIterations;
----------------
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).
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/altera-unroll-loops.rst:4
+altera-unroll-loops
+=================
+
----------------
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/altera-unroll-loops.cpp:359
+}
+// There are no fix-its for this check
----------------
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?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72235/new/
https://reviews.llvm.org/D72235
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
