[PATCH] D43176: [LoopInterchange] Incrementally update the dominator tree.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:588
       Changed |= Interchanged;
     }
     return Changed;
----------------
kuhar wrote:
> Is there a reason why the updates are applied int the destructor instead of at the end of this loop?
The first version of the patch had to collect updates from multiple code paths, but I've simplified things so there actually is no need to have DTUpdates as member variable. The only point where we adjust branches is in adjustLoopLinks, at the end we can apply the updates. I think we have to apply the updates after every transform, so we have a valid DT when we deal with loops with nests > 2.


================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:1285
+      DTUpdates.push_back(
+          {DominatorTree::UpdateKind::Insert, BI->getParent(), NewBB});
+      DTUpdates.push_back(
----------------
kuhar wrote:
> Can you have multiple successors that are the same BB? If so, you would have to exclude the redundant ones here.
That should never happen I think. I've added a break and also an assertion. Please let me know if this is exactly the case you meant


https://reviews.llvm.org/D43176