[PATCH] D123230: [UnifyLoopExits] Reduce number of guard blocks

[Brendon Cahoon via Phabricator via llvm-commits]

bcahoon marked 5 inline comments as done.
bcahoon added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/UnifyLoopExits.cpp:207
+      Exits.remove(Exit);
+      ExitingBlocks.remove(Exit->getSinglePredecessor());
+      ExitingBlocks.insert(Exit);
----------------
sameerds wrote:
> Is it true that an exit block always has a single predecessor? This will return nullptr otherwise. Maybe a comment will help.
Great point. I've added a check when CommonSuccs is created that checks for a single predecessor. It's the common case to try to optimize, along with the changes needed to StructurzeCFG. It's possible to allow multiple predecessors though that could complicate the node ordering changes in StructurizeCFG.


================
Comment at: llvm/lib/Transforms/Utils/UnifyLoopExits.cpp:215
   DomTreeUpdater DTU(DT, DomTreeUpdater::UpdateStrategy::Eager);
+  DTU.applyUpdates(Updates);
+
----------------
sameerds wrote:
> What does this do? We haven't done any updates yet, right?
Thanks for catching this.


================
Comment at: llvm/lib/Transforms/Utils/UnifyLoopExits.cpp:235-242
+      for (auto Pred : predecessors(G)) {
+        auto PredLoop = LI.getLoopFor(Pred);
+        if (!ParentLoop->contains(PredLoop)) {
+          if (PredLoop)
+            LI.removeBlock(Pred);
+          ParentLoop->addBasicBlockToLoop(Pred, LI);
+        }
----------------
sameerds wrote:
> Scary stuff. Could you please add a comment? I think this is related to the test "unreachable_from_inner_loop" ... but an explanation in English will help a lot.
Agreed. Please let me know if this requires additional clarification.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123230/new/

https://reviews.llvm.org/D123230