[PATCH] D22630: Loop rotation

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya marked 7 inline comments as done.
hiraditya added a comment.

In https://reviews.llvm.org/D22630#689237, @mzolotukhin wrote:

> Hi,
>
> Please find some comments inline. Also, please reinspect the tests - in many of them the comments don't correspond to test bodies, they are not minimal, and some tests duplicate others.
>
> Thanks,
> Michael


Thank you for the detailed review. I did not try to remove so much from tests from file other than loop-rotate.ll because they were there from the past.
For loop-rotate.ll I think the tests are a combination of multiple scenarios, and it will be good to have them.



================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:319
+          // Handle uses in the loop-closed-phi.
+          PHINode *ClosePhi = cast<PHINode>(UserInst);
+          BasicBlock *Pred = ClosePhi->getIncomingBlock(U.getOperandNo());
----------------
mzolotukhin wrote:
> Why is UserInst guaranteed to be a PHINode?
Because Inst does not dominate UserInst it must be the loop-closed-phi as the loop is in loop-closed-ssa form.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:356-357
+// (bounded by \p OrigH and \p OrigLatch), which are exiting the loop in
+// \p Blocks and collect all the exits from the region in \p Exits. Returns the
+// first basic block which was not copied.
+BasicBlock *LoopRotate::collectSEMEBlocks(BasicBlock *OrigH,
----------------
mzolotukhin wrote:
> I find it a bit confusing that you're saying about blocks being copied while the function in fact just populates the list.
I've updated the comment.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:369
 
-        // If the dominator changed, this may have an effect on other
-        // predecessors, continue until we reach a fixpoint.
-      } while (Changed);
+    // Copy until any BB where the branch does not exit loop, or the loop-latch.
+    if (OrigLatch == *BB || !L->isLoopExiting(*BB) ||
----------------
mzolotukhin wrote:
> I think this check is the reason you kept that `OrigH->isExiting()` check. I think that what we rather should do here is to copy all blocks up to an exiting block that we're going to make a new latch. Does it make sense?
> 
> In other words, I'd like the following case to be rotated:
> Entry -> {A}
> A -> {B}
> B -> {Exit, C}
> C -> {A}
> 
> I see that current implementation is generic enough to handle such cases, and the only reasons why it won't do it is that (IMHO unnecessary) limitations.
We can certainly do that. From other reviewers (including you) there were comments in the past that the loop-rotation should have a way to reach idempotence. If we keep rotating when the header is not exiting, there will be no way to stabilize loop rotation.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:444-445
+    Loop *BBParentLoop = BBLoop->getParentLoop();
+    if (BBParentLoop)
+      BBParentLoop->addBasicBlockToLoop(NewBB, *LI);
+    VMap[BB] = NewBB;
----------------
mzolotukhin wrote:
> Why don't we need to update LI if there is no parent loop?
For the current loop under rotation, there are no new basic blocks added. Only when the loop-header is changed around line 539 L->moveToHeader is called.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:604
+        DEBUG(dbgs() << "\nChanging IDom of " << *ExitBB << "to" << *NewIDom);
+        I = DTBB->begin();
+      } else
----------------
mzolotukhin wrote:
> Can we avoid quadratic complexity here?
Actually this while loop is bounded by the number of exiting basicblocks from the loop which were cloned.


================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate.ll:73
+
+; Check that the loop with indirect branches is rotated.
+; CHECK-LABEL: @foo3
----------------
mzolotukhin wrote:
> This function doesn't have any indirect branches.
I'll update the comment.


================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate.ll:149-153
+bb10:
+  switch i32 %arg3, label %bb11 [
+    i32 46, label %bb19
+    i32 32, label %bb19
+  ]
----------------
mzolotukhin wrote:
> Why do we need two switches in this test?
This switch statement is a loop preheader as it is branching to the loop header. So I think, it would exercise if the dominators are updated properly.


https://reviews.llvm.org/D22630