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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/UnifyLoopExits.cpp:163-164
       Exits.insert(S);
+      BasicBlock *Succ = S->getSingleSuccessor();
+      if (Succ)
+        CommonSuccs[Succ].insert(S);
----------------
This can be rewritten as "if (auto *Succ = ...) "


================
Comment at: llvm/lib/Transforms/Utils/UnifyLoopExits.cpp:182
+
+      dbgs() << "Common exit successor join blocks\n";
+      for (auto CS : CommonSuccs) {
----------------
Can you rephrase a bit? The join blocks are for what?


================
Comment at: llvm/lib/Transforms/Utils/UnifyLoopExits.cpp:207
+      Exits.remove(Exit);
+      ExitingBlocks.remove(Exit->getSinglePredecessor());
+      ExitingBlocks.insert(Exit);
----------------
Is it true that an exit block always has a single predecessor? This will return nullptr otherwise. Maybe a comment will help.


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


================
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);
+        }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123230