[PATCH] D22630: Loop rotation

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:604
+        DEBUG(dbgs() << "\nChanging IDom of " << *ExitBB << "to" << *NewIDom);
+        I = DTBB->begin();
+      } else
----------------
mzolotukhin wrote:
> hiraditya wrote:
> > 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.
> Can you elaborate? I don't see any exits from the loop, but I see how we start from the beginning.
This part of code executes only for the basic blocks which are not in the loop, i.e., exit basic blocks. For the exit basic blocks dominator tree may not be up to date and hence is being updated here. Once the exit basic blocks have their dominators up-to-date, and since their sub-trees are already up-to-date => NewIDom == StaleIDom (line:581) for all successors of exit basic blocks.


================
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:
> hiraditya wrote:
> > 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.
> Loop simplify splits the edge to the loop anyway. Thus, this test, if checking anything, checks only loop simplify. For testing loop-rotation the first switch is absolutely unnecessary.
There is no loop-simplify in the new loop rotation.


https://reviews.llvm.org/D22630