[PATCH] D32409: [PM/LoopUnswitch] Introduce a new, simpler loop unswitch pass.

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

Looks good overall, mostly minor comments inline.



================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:71
+/// exit block.
+static void updateLoopExitIDomAfterUnswitch(BasicBlock *LoopExitBB, Loop &L,
+                                            DominatorTree &DT) {
----------------
I'd prefer calling this function just `updateLoopExitIDom`, since nothing in it is specific to  unswitching.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:75
+         "Cannot have empty predecessors of the loop exit block if we split "
+         "off a block to unswitch1");
+
----------------
s/unswitch1/unswitch!/


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:80
+  // until all predecessors are covered or we reach the loop header. The loop
+  // header necesasrily dominates all loop exit blocks in loop simplified form
+  // so we can early-exit the moment we hit that block.
----------------
s/necesasrily/necessarily/


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:122
+  // exit block.
+  DT.changeImmediateDominator(UnswitchedNode, OldPHNode);
+
----------------
Instead of special casing here, why not just

```
  SmallSetVector<BasicBlock *, 4> Worklist;
  Worklist.insert(UnswitchedNode);
```

and let nature run its course?


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:136
+    assert(!DomChain.count(Node) &&
+           "Cannot be dominated by a block you can reach1");
+    // If this block doesn't have an immediate dominator somewhere in the chain
----------------
s/reach1/reach!/


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:200
+  }
+  auto *ContinueBB = BI.getSuccessor((LoopExitSuccIdx + 1) % 2);
+  assert(L.contains(ContinueBB) &&
----------------
This is very minor, but I'm used to seeing `1 - LoopExitSuccIdx` in cases like these.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:205
+  // If the loop exit block contains phi nodes, this isn't trivial.
+  if (isa<PHINode>(LoopExitBB->begin()))
+    return false;
----------------
Why is a PHI on the exit block a problem?  Are you saying this because the PHI can require you to copy some computation into `OldPH`?  If so, perhaps we can be a bit more aggressive here, and allow PHIs that have constant incoming values for the edge we're unswitching?  It would be fine to do in a later patch, but please add a quick TODO or explanation if that's the reason.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:223
+  if (llvm::all_of(predecessors(LoopExitBB), [&](BasicBlock *PredBB) {
+        return PredBB == BI.getParent();
+      })) {
----------------
I found this bit of code a bit convoluted to understand.  I'd have written this as (if I understood the intent correctly):

```
if (LoopExitBB->getSinglePredecessor()) {
  assert(LoopExitBB->getSinglePredecessor() == BI.getParent());
  UnswitchedBB = LoopExitBB
} else {
  // We know something other than BI is branching to LoopExitBB and since the 
  // loop is simplified, we know that all those other things are from within
  // the loop.  Hence the unswitched branch needs to branch to the a split out
  // block.
  UnswitchedBB = SplitBlock(LoopExitBB, &LoopExitBB->front(), &DT, &LI);
}
```



================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:238
+  BI.setSuccessor(LoopExitSuccIdx, UnswitchedBB);
+  BI.setSuccessor((LoopExitSuccIdx + 1) % 2, NewPH);
+
----------------
Same suggestion here regarding `1 - LoopExitSuccIdx`.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:261
+
+/// Unswitch a trivial switcih if the condition is loop invariant.
+///
----------------
s/switcih/switch/


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:365
+  // Split the preheader, so that we know that there is a safe
+  // place to insert the conditional branch. We will change the preheader to
+  // have a conditional branch on Cond.
----------------
Minor, but s/conditional branch/switch/


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:445
+
+/// This routine cans the loop to find a branch or switch which occurs before
+/// any side effects occur. These can potentially be unswitched without
----------------
s/cans/scans/


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:453
+/// The return value indicates whether any changes were made, not whether
+/// anything was actually unswitched.
+static bool unswitchAllTrivialConditions(Loop &L, DominatorTree &DT,
----------------
What does this routine change in the cases where nothing is unswitched?


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:475
+    // stores, calls, volatile loads) in the part of the loop that the code
+    // *would* execute. Check the header first.
+    for (Instruction &I : *CurrentBB)
----------------
The ` Check the header first.` comment seems out of place.  I'd also be tempted to use `llvm::any_of` here.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:485
+      if (ConstantInt *Cond = dyn_cast<ConstantInt>(SI->getCondition())) {
+        CurrentBB = SI->findCaseValue(Cond)->getCaseSuccessor();
+        continue;
----------------
Are handling these cases here necessary?  I'd imagine `LoopSimplifyCFG` to be a better place for these (especially the second one, which is just an unconditional `br` written as a `switch`).


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:518
+      // We do not understand other terminator instructions.
+      return false;
+
----------------
Should this be `return Changed;`?


https://reviews.llvm.org/D32409