[PATCH] D60266: [LoopUnroll] Rotate loop, when optimizing for size and can fully unroll a loop.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:1096
+  auto shouldRotate = [&](Loop *L) {
+    if (!L->getHeader()->getParent()->optForSize() || !TripCount ||
+        UP.Count != TripCount)
----------------
Can you just use the OptForSize you defined earlier in the function?


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:1108
+
+    // Loops with indirectbr cannot be cloned.
+    if (!L->isSafeToClone())
----------------
I feel like this comment doesn't really describe why you're avoiding loops that are unsafe to clone? Maybe a bit more detail here?


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:1119
+      BranchInst *HeaderBI = dyn_cast<BranchInst>(Header->getTerminator());
+      auto CheckSuccessors = [&](unsigned S1, unsigned S2) {
+        return HeaderBI->getSuccessor(S1) == LatchBlock &&
----------------
This could use a comment explaining what you're doing here?


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:1130
+  if (shouldRotate(L)) {
+    SimplifyQuery SQ(L->getHeader()->getParent()->getParent()->getDataLayout());
+    LoopRotation(L, LI, &TTI, &AC, &DT, &SE, nullptr, SQ, true, -1, true);
----------------
I think it's probably worth factoring `L->getHeader()->getParent()` out into a variable at this point, if only to reduce the number of `->`s. :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60266/new/

https://reviews.llvm.org/D60266