[PATCH] D22630: Loop rotation

[Eli Friedman via llvm-commits]

eli.friedman added a subscriber: eli.friedman.
eli.friedman added a comment.

It looks like you're adding a bunch of bugpoint-reduced testcases without any comments... tests should at least explain what specifically they're trying to test.

----

This might be easier to review if it were split up into multiple patches, something like:

1. Split checking whether rotation is legal/profitable into a separate function.
2. Replace the actual transform code with your generalized version.
3. Change the rules for when rotation is legal/profitable given the more flexible implementation.

As it is, you're mixing up a bunch of different changes to the way rotation works, so it's hard to tell whether certain changes are intentional.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:149
@@ -202,2 +148,3 @@
+  const BasicBlock *OrigHeader = L->getHeader();
   if (!L->isLoopExiting(OrigHeader))
     return false;
----------------
Did you mean to change this check? Given the way you've implemented cloning, you should be able to do something more aggressive.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:150
@@ -202,3 +149,3 @@
   if (!L->isLoopExiting(OrigHeader))
     return false;
 
----------------
It seems like you could rotate a loop where the loop header doesn't exit the loop?  Might require some changes to figure out whether it's profitable.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:154
@@ -213,2 +153,3 @@
+  if (!BI || BI->isUnconditional())
     return false;
 
----------------
Why does this matter?  (At the very least, needs a comment.)

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:162
@@ +161,3 @@
+  const BasicBlock *LoopLatch = L->getLoopLatch();
+  if (!isSEME(OrigHeader, LoopLatch, DT))
+    return false;
----------------
Isn't a loop an SEME region by definition?

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:202
@@ +201,3 @@
+    else
+      return false;
+  }
----------------
This is not the way to count the size of a loop... see the existing code using CodeMetrics.  (In particular, you can never use size() as a threshold for anything because it's affected by debug info.)

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:327
@@ +326,3 @@
+
+  // Remove this code.
+  BasicBlock *BeforeLoop = nullptr;
----------------
?

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:431
@@ +430,3 @@
+    DEBUG(dbgs() << "\nSplitting the edge of Loop:"; L->dumpVerbose(););
+    LoopLatch = SplitEdge(LoopLatch, L->getHeader(), DT, LI);
+  }
----------------
The loop latch could be an indirectbr.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:553
@@ -552,3 @@
-/// I don't believe this invalidates SCEV.
-bool LoopRotate::simplifyLoopLatch(Loop *L) {
-  BasicBlock *Latch = L->getLoopLatch();
----------------
Why are you getting rid of this?


https://reviews.llvm.org/D22630