[PATCH] D72235: [clang-tidy] new altera unroll loops check

[Aaron Ballman via Phabricator via llvm-commits]

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