[PATCH] D28073: Preserve domtree and loop-simplify for runtime unrolling.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:55
+static cl::opt<bool>
+UnrollVerifyDomtree("unroll-verify-domtree", cl::init(false), cl::Hidden,
+                    cl::desc("Verify domtree after unrolling"));
----------------
mzolotukhin wrote:
> mkuper wrote:
> > efriedma wrote:
> > > mkuper wrote:
> > > > We want to enforce this being on in new tests too, I assume?
> > > We probably want this on in new tests, yes, but I have no idea how we would enforce it.  
> > In code review, I meant. :-)
> Can we make the default value `true` for debug builds (it will match the existing behavior)?
Okay.  (I initially didn't do this because it's O(N^2) in theory, but it turns out it doesn't really matter that much.)


================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:619
+      if (BB == LatchBlock) {
+        // The latch is special because we emit unconditional branches in
+        // some cases.  Since the latch is always at the bottom of the loop,
----------------
mzolotukhin wrote:
> mkuper wrote:
> > efriedma wrote:
> > > mkuper wrote:
> > > > Isn't it the case that when the latch ends with an unconditional branch, that branch is towards the header block?
> > > > If so, we should not have any children outside the loop.
> > > Maybe this is more clear?  "The latch is special because we can emit an unconditional branch in the unrolled loop even if the original latch block ends in a conditional branch."
> > Ah, ok, this makes sense.
> > This still looks a bit weird to me, though. Any chance you can give an example of when this ends up different than the nearest common dominator with Latches[0]?
> > 
> > (Feel free to ignore me - I'm not sure I understand this enough to LGTM it anyway. :-) )
> > 
> I'm also curious about an example.
> 
> Also, this part is not clear to me: "Since the latch is always at the bottom of the loop, new dominator must also be a latch." Why can't a mid-block from the body be a dominator of a cloned latch?
> 
This shows up for example, if you completely unroll a single-block loop with a constant trip count; the immediate dominator of the exit is the last iteration of the loop, not the first iteration.  (foldBlockIntoPredecessor hides this problem because the block in question gets folded away when a loop is completely unrolled.)

> Why can't a mid-block from the body be a dominator of a cloned latch?

I'm not sure what you're asking here.  ChildrenToUpdate is the set of loop exit blocks which are dominated by BB, so this code only touches the idom for blocks outside the loop.  The latch's idom isn't relevant.


================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:644-646
+  if (DT && UnrollVerifyDomtree)
+    DT->verifyDomTree();
+
----------------
mzolotukhin wrote:
> Do I understand it correctly that using `-verify-dom-info` wasn't sufficient because of `foldBlockIntoPredecessor`, and that's why we're now introducing `unroll-verify-domtree`?
Yes... we want to verify at this point to try and catch as many mistakes as possible.


================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:700-701
   if (DT) {
-    if (!OuterL && !CompletelyUnroll)
-      OuterL = L;
     if (OuterL) {
----------------
mzolotukhin wrote:
> Why can we remove this?
There are three possibilities for unrolling: we could be completely unrolling a loop, we could be runtime unrolling a loop, or we could be peeling a loop.  If we're completely unrolling, these lines are a no-op.  If we're runtime unrolling, we now correctly preserve LoopSimplify for L (and we don't break it for any loop outside of L).  If we're peeling, we explicitly recreate LoopSimplify earlier.  


Repository:
  rL LLVM

https://reviews.llvm.org/D28073