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

[Michael Zolotukhin via Phabricator via llvm-commits]

mzolotukhin 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"));
----------------
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)?


================
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,
----------------
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?



================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:639-642
-  // FIXME: We only preserve DT info for complete unrolling now. Incrementally
-  // updating domtree after partial loop unrolling should also be easy.
-  if (DT && !CompletelyUnroll)
-    DT->recalculate(*L->getHeader()->getParent());
----------------
Thanks for fixing this!


================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:644-646
+  if (DT && UnrollVerifyDomtree)
+    DT->verifyDomTree();
+
----------------
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`?


================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:700-701
   if (DT) {
-    if (!OuterL && !CompletelyUnroll)
-      OuterL = L;
     if (OuterL) {
----------------
Why can we remove this?


Repository:
  rL LLVM

https://reviews.llvm.org/D28073