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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked 19 inline comments as done.
sameerds added inline comments.


================
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.
----------------
nhaehnle wrote:
> 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.)
That's brilliant! All three functions quickly collapsed into a single function now.


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:1367-1368
+    }
+    Updates.push_back(
+        {DominatorTree::Insert, GuardBlocks.back(), Outgoing.back()});
+    DTU->applyUpdates(Updates);
----------------
nhaehnle wrote:
> 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?
Yes, there should be, and there was one in the first version of this review. It got lost when I refactored into separate functions. This is fixed now. The whole thing manages to work correctly even if I "forget" to add any outgoing edge from the new guard blocks. My best guess is that the updater itself all new edges incident on a new node, and since I am updating the dom tree as late as possible, this always discovers all the new edges.


================
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.
----------------
nhaehnle wrote:
> 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
I assume you are okay with the call to getExitingBlocks. If I were to traverse the whole loop body myself, that would just duplicate functionality that's already easily available. I rewrote the comments for clarity.


================
Comment at: llvm/lib/Transforms/Utils/UnifyLoopExits.cpp:196
+  }
+  LI.verify(DT);
+
----------------
nhaehnle wrote:
> This should be guarded by `#ifndef NDEBUG`.
Wrapped this in EXPENSIVE_CHECKS instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75865