[PATCH] D22630: Loop rotation

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Aditya,

Thanks for updating the patch, some comments are inline. Also, please try to merge tests with the same command line into one file and make sure all asserts have messages.

Thanks,
Michael


================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:302
@@ -301,2 +301,3 @@
   MPM.add(createReassociatePass());           // Reassociate expressions
   // Rotate Loop - disable header duplication at -Oz
+  if (SizeLevel != 2)
----------------
Please update the comment: this is not only a header anymore.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:15
@@ -11,1 +14,3 @@
+// 4. Add phi to the new loop header
+// 5. Update DOM
 //
----------------
I think DomTree or DT is used more often for dominator tree in our codebase.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:57-59
@@ +56,5 @@
+
+static cl::opt<int> MaxLoopsRotated(
+    "max-loops-rotated", cl::init(-1), cl::Hidden,
+    cl::desc("The maximum loops to be rotated (-1 means no limit)"));
+
----------------
This should be redundant, as we now have a pass bisection possibility. It basically does the same.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:77-79
@@ +76,5 @@
+      BLI->setSuccessor(I, NewBB);
+      break;
+    }
+  // Move NewBB physically from the end of the block list.
+  Function *F = Succ->getParent();
----------------
But why do we need it in release builds? I assume you used it for debugging, but opt-bisect should better fit for this, and there should be no reason to reimplement it locally.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:194
@@ +193,3 @@
+
+    // TODO: Once this is crossed we can copy until the prev. BB.
+    if (LoopSize + Metrics.NumInsts >= RotationMaxSize)
----------------
Why is this 'TODO'? Also, I don't see a need in shortening the word 'previous' here.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:263-266
@@ -387,27 +262,6 @@
 
-    // Right now OrigPreHeader has two successors, NewHeader and ExitBlock, and
-    // thus is not a preheader anymore.
-    // Split the edge to form a real preheader.
-    BasicBlock *NewPH = SplitCriticalEdge(
-        OrigPreheader, NewHeader,
-        CriticalEdgeSplittingOptions(DT, LI).setPreserveLCSSA());
-    NewPH->setName(NewHeader->getName() + ".lr.ph");
-
-    // Preserve canonical loop form, which means that 'Exit' should have only
-    // one predecessor. Note that Exit could be an exit block for multiple
-    // nested loops, causing both of the edges to now be critical and need to
-    // be split.
-    SmallVector<BasicBlock *, 4> ExitPreds(pred_begin(Exit), pred_end(Exit));
-    bool SplitLatchEdge = false;
-    for (BasicBlock *ExitPred : ExitPreds) {
-      // We only need to split loop exit edges.
-      Loop *PredLoop = LI->getLoopFor(ExitPred);
-      if (!PredLoop || PredLoop->contains(Exit))
-        continue;
-      if (isa<IndirectBrInst>(ExitPred->getTerminator()))
-        continue;
-      SplitLatchEdge |= L->getLoopLatch() == ExitPred;
-      BasicBlock *ExitSplit = SplitCriticalEdge(
-          ExitPred, Exit,
-          CriticalEdgeSplittingOptions(DT, LI).setPreserveLCSSA());
-      ExitSplit->moveBefore(Exit);
+BasicBlock *LoopRotate::collectSEMEBlocks(BasicBlock *OrigH,
+                                          BasicBlock *OrigLatch,
+                                          SmallVecBB &Blocks,
+                                          SmallPtrSetBB &Exits) const {
+  BasicBlock *NewH = nullptr;
----------------
A brief comment about what this function expects and returns would be helpful (for other functions too).

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:330-331
@@ +329,4 @@
+  // SplitEdge does not work properly with single-entry PHIs.
+  BasicBlock *NewPH = BasicBlock::Create(
+      NewH->getContext(), NewH->getName() + ".lr.ph", F, BeforeLoop);
+
----------------
Maybe it's possible to fix phis before we add `NewPH`, so that we can use `SplitEdge`? Or maybe we can split it before we copy blocks breaking phis?

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:403-404
@@ +402,4 @@
+
+  BasicBlock *OrigH = L->getHeader();
+  BasicBlock *LoopLatch = L->getLoopLatch();
+
----------------
We definitely want to have some thresholds, like we do now - maybe just rename the function somehow? `ProfitableToRotate` sounds to me like there would be a benefit right from loop-rotate, while in reality we just hope that a later pass would take advantage of it. As I see it, we want to rotate as many loops as possible, being limited only by thresholds, and not considering profitability of the rotation itself. BTW, are we in agreement on this?

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:419
@@ +418,3 @@
+      return false;
+    ++LoopsRotated;
+  }
----------------
Wouldn't it make sense to count rotated loops even if there is no threshold?

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:430
@@ +429,3 @@
+    DEBUG(dbgs() << "\nLoop in canonical form:"; L->dumpVerbose(););
+    // After splitting the blocks need to be recollected.
+    Blocks.clear();
----------------
Yes, I think it should be possible to update SEME instead of recomputing it from scratch.

================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate-4.ll:5
@@ +4,3 @@
+
+; Check that the loop with indirect branches is rotated.
+; CHECK-LABEL: foo
----------------
Is there any indirect branches in this test?

================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate-6.ll:5
@@ +4,3 @@
+
+; Check that the loop with header having a switch block is rotated.
+; CHECK-LABEL: bar
----------------
Do we have to have several switches to test this?


https://reviews.llvm.org/D22630