[PATCH] D17473: [LoopUnroll] Avoid unnecessary DT recomputation.

[Chandler Carruth via llvm-commits]

chandlerc added inline comments.

================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:536
@@ +535,3 @@
+            ExitIDom ? DT->findNearestCommonDominator(ExitIDom, *BI) : *BI;
+      }
+      DT->changeImmediateDominator(Exit, ExitIDom);
----------------
mzolotukhin wrote:
> The reason we're doing this is that dominator for an exit block could change after unrolling. Consider a diamond-like loop body with header `H`, side blocks  `A` and `B`, and latchblock  `L`. Suppose `B` is exiting to `E`.
> 
> If `B` is the immediate dominator of `E` before unrolling, it's not the case after unrolling - we'll have several blocks exiting to `E`, so we have to actually find their common dominator.
> 
> Probably, there is a more efficient way of doing this, but even in this form it's a pure win over what we have now.
Ahh, I see.

So, the reason that this seemed odd to me is that all of these blocks that now branch to E come from unrolled copies of the loop, and so they should all have the same IDom -- the IDom of B from the first copy of the loop (which I think pretty much has to be the header, but I've not thought very hard about that).

But have we done any CFG simplification during unrolling at this point? (I know we talked about that, not sure any of it landed...) If so, that would of course potentially invalidate the idea of basing this purely on the loop structure and structural nature of unrolling.

It's not so much that this is ever going to be expensive at runtime (the domtree should make this pattern quite fast), it was just that I wanted to understand the complexity.

I think explaining some of the context of how to think about this code in comments would be very useful here.

================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:562-568
@@ -525,7 +561,9 @@
 
-  // FIXME: Reconstruct dom info, because it is not preserved properly.
-  // Incrementally updating domtree after loop unrolling would be easy.
-  if (DT)
-    DT->recalculate(*L->getHeader()->getParent());
+  if (DT) {
+    // FIXME: Reconstruct dom info, because it is not preserved properly.
+    // Incrementally updating domtree after loop unrolling would be easy.
+    if (!CompletelyUnroll)
+      DT->recalculate(*L->getHeader()->getParent());
+  }
 
   // Simplify any new induction variables in the partially unrolled loop.
----------------
Also, what do you think about running verifyDomTree here to help flush out any bugs?


http://reviews.llvm.org/D17473