[PATCH] D28676: Makes incremental dominator calculation in Loop Unroll pass

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a reviewer: efriedma.
mkuper added a comment.

This looks like it has a lot of overlap with https://reviews.llvm.org/D28073.
Please sync with Eli.

One thing https://reviews.llvm.org/D28073 doesn't do is handle peeling (I guess mostly because Eli has better things to do than fix my code :-) ), so that's a part of this patch we want regardless. Added a couple of comments on that.



================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:197
+        DT->addNewBlock(NewBB,
+                        cast<BasicBlock>(VMap[cast<Value>(IDom->getBlock())]));
+      }
----------------
Why do you need the cast<Value>()?
Also, I'd a comment here that the reason VMap[IDom->getBlock()] is guaranteed to point to the right block is because the iteration order is RPO, so we've already made a copy of the IDom.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:221
+    BasicBlock *CurIDom = DT->getNode(Exit)->getIDom()->getBlock();
+    BasicBlock *NewIDom = DT->findNearestCommonDominator(CurIDom, NewLatch);
+    DT->changeImmediateDominator(Exit, NewIDom);
----------------
Do we really need to findNearestCommonDominator() here?
IIRC, we are guaranteed to be in loop-simplify form here, which means we have dedicated exit blocks. Can we figure out what the right IDom is without findNearestCommonDominator, a la D28073?


https://reviews.llvm.org/D28676