[PATCH] D75865: Introduce unify-loop-exits pass.

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

This mostly looks good to me. I have some comments questions that affect the correctness of the code (at least in general); most of the rest is suggestions for possible simplifications / minor improvements.



================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:1151
+//
+// These are used by the Hub to transfer control to the outgoing blocks.
+static void collectPredicates(PredMap &Predicates,
----------------
How about: "These indicate that **if** the Hub is entered from InBB, then control is transferred to OutBB if the corresponding value is true."

This suggests a slight refactoring of the function to have an outer loop based on Incoming and then looking at the possible cases of the BranchInst terminator, to parallel the corresponding code in `redirectIncomingToHub`.


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:1175-1176
+      Predicates[Out][In] = Inv;
+      Inverted.push_back(Inv);
+      Inverted.push_back(Condition);
+    }
----------------
Since `Inverted` does not only contain inverted conditions, could you please consistently rename it? Maybe AllConditions?


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:1244-1257
+    // Optimization: Consider an incoming block A with both successors B1 and B2
+    // in the set of outgoing blocks. The predicates for B1 and B2 complement
+    // each other. If B1 is visited first in the loop below, control will branch
+    // to B1 using the corresponding predicate. But if that branch is not taken,
+    // then control must reach B2, which means that the predicate for B2 is
+    // always true. This optimization is performed as follows:
+    // 1. At each iteration, drop the predicates for the current outgoing block.
----------------
Smart. I wonder if you could simplify this further by transposing the loops.

Basically, first create all the phis in a loop over the outgoing blocks.

Then in a separate loop nest, iterate over the Incoming blocks, then iterate over all outgoing phis, and add the appropriate condition. If you combine this with my comment in `collectPredicates`, it may even be quite suitable to merge the two functions into one. This could then elide the creation of the "OldPredicates" entirely. The code doesn't need that structure for anything else, do we? (I suppose this would require doing the redirectIncomingToHub later.)


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:1261
+    // has a trivially true predicate.
+    auto Preds = OldPredicates[Out];
+    OldPredicates.erase(Out);
----------------
std::move?


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:1367-1368
+    }
+    Updates.push_back(
+        {DominatorTree::Insert, GuardBlocks.back(), Outgoing.back()});
+    DTU->applyUpdates(Updates);
----------------
Shouldn't there be an edge insertion from the last guard blocks to both the last and the second-last Outgoing block? What am I missing here?


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:3036
+    // Last option: Create a new instruction
+    return BinaryOperator::CreateNot(Condition, "", Parent->getTerminator());
+  }
----------------
I believe the `not` should be created directly after `Inst` instead of before its block's terminator, to ensure that the new instruction dominates everything that `Inst` dominates.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:3041-3042
+    BasicBlock &EntryBlock = Arg->getParent()->getEntryBlock();
+    return BinaryOperator::CreateNot(Condition, Arg->getName() + ".inv",
+                                     EntryBlock.getTerminator());
+  }
----------------
Same argument here: the `not` should be create at the *start* of the entry block rather than at the end.


================
Comment at: llvm/lib/Transforms/Utils/UnifyLoopExits.cpp:139-141
+  // Locate exits by examining the successors of the exiting blocks. This is
+  // cheaper than calling Loop::getExitBlocks(), since that goes through the
+  // entire list of blocks in the loop.
----------------
I don't understand this argument. getExitingBlocks also goes through all blocks in the loop... since you need both exiting *and* exit blocks, you may as well loop over all blocks in the loop directly. Though I guess this approach is not too bad, either, just the comment is confusing. If you want to argue that getting the exiting blocks and then enumerating successors to find exits is cheaper than enumerating exits and then enumerating predecessors, I suppose there are two arguments to be made:
* Eliminating duplicates is more natural the way you're doing it
* Iterating over successors is more efficient than iterating over predecessors in LLVM IR


================
Comment at: llvm/lib/Transforms/Utils/UnifyLoopExits.cpp:196
+  }
+  LI.verify(DT);
+
----------------
This should be guarded by `#ifndef NDEBUG`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75865