[PATCH] D32699: [PM/Unswitch] Teach the new simple loop unswitch to handle loop invariant PHI inputs and to rewrite PHI nodes during the actual unswitching.

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

lgtm

I have some minor comments, but they're of the form "if *I* wrote this, this is what I'd do", and don't necessarily need to be addressed.



================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:58
 static void updateLoopExitIDom(BasicBlock *LoopExitBB, Loop &L,
                                             DominatorTree &DT) {
   assert(pred_begin(LoopExitBB) != pred_end(LoopExitBB) &&
----------------
JFYI: clang format is needed here too.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:142
+/// incoming values along this edge.
+bool checkIfLCSSAPHIsAreLoopInvariant(Loop &L, BasicBlock &ExitingBB,
+                                      BasicBlock &ExitBB) {
----------------
Make this static?  Same for `rewritePHINodesForUnswitchedExitBlock` and `rewritePHINodesForExitAndUnswitchedBlocks`.

I'd also call this `areLoopExitPHIsLoopInvariant`, since nothing in this is specific to lcssa PHIs.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:165
+/// terminator.
+void rewritePHINodesForUnswitchedExitBlock(BasicBlock &UnswitchedBB,
+                                BasicBlock &ExitingBB,
----------------
I'd have used a slightly different interface.  Instead of `rewritePHINodesForUnswitchedExitBlock` and `rewritePHINodesForExitAndUnswitchedBlocks`, I'd have a single function `rewriteExitPHIs` with the signature of `rewritePHINodesForExitAndUnswitchedBlocks` that did different things based on whether `ExitBB` and `UnswitchedBB` were equal.  Right now the PHI handling logic is effectively split between the caller (that decides whether we need a split PHI) and the callee `rewritePHINodesForUnswitchedExitBlock` or `rewritePHINodesForExitAndUnswitchedBlocks` that does the rewrite.



================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:167
+                                BasicBlock &ExitingBB,
+                                BasicBlock &OldPH) {
+  for (Instruction &I : UnswitchedBB) {
----------------
FYI: indent seems off here, but clang-format is the hammer.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:194
+                                               BasicBlock &UnswitchedBB,
+                                               BasicBlock &ExitingBB,
+                                               BasicBlock &OldPH) {
----------------
I'd s/`ExitingBB`/`OldExitingBB`/

Same for `rewritePHINodesForUnswitchedExitBlock`.



================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:217
+    // required to produce identical results.
+    for (int64_t i = PN->getNumIncomingValues() - 1; i >= 0; --i) {
+      if (PN->getIncomingBlock(i) != &ExitingBB)
----------------
Why `int64_t` ?


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:302
     (void)PredBB;
     assert(PredBB == BI.getParent() && "A branch's parent is't a predecessor!");
     UnswitchedBB = LoopExitBB;
----------------
Nit: s/is't/isn't/


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:491
+    if (pred_empty(ExitBB)) {
+      // Only rewrite once.
+      if (UnswitchedExitBBs.insert(ExitBB).second)
----------------
Instead of adding a new `UnswitchedExitBBs`, I'd re-use `SplitExitBBMap` for this -- for unswitched block `BB` that doesn't need splitting, I'd have `SplitExitBBMap[BB] = BB`.  For instance, if you go with the unified `rewriteExitPHIs` routine above, the the code will look like:

```
    if (!SplitExitBB) {
      // If this is the first time we see this, do the split and remember it.
      SplitExitBB = pred_empty(ExitBB) ? ExitBB : SplitBlock(ExitBB, &ExitBB->front(), &DT, &LI);
      rewriteExitPHIs(*ExitBB, *SplitExitBB, *ParentBB, *OldPH);
      updateLoopExitIDom(ExitBB, L, DT);
    }
```

with an appropriate relaxation in `updateLoopExitIDom`.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:507
     }
     ExitBB = SplitExitBB;
   }
----------------
This looked fine initially, but this time around I scratched my head a bit -- do you mind making this explicitly an assignment into `CasePair.second`?


https://reviews.llvm.org/D32699