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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Thanks for the reviews! Updated patch and responses inline.

In https://reviews.llvm.org/D32409#737397, @davide wrote:

> In https://reviews.llvm.org/D32409#737387, @davide wrote:
>
> > I focused on the logic to update the dominator and I think it's correct. Some comments below.
> >  Also, recently there have been some changes wrt divergent conditions. Do you plan to integrate them in the new pass?
>
>
> The reason why I'm asking is that I'm not an expert about DivergenceAnalysis so I can't really reason about all the implications there.


I do plan to integrate them. I'm just starting with what seemed like a reasonably useful to review subset. Does this seem like a reasonable thing to do in a follow-up?



================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:50
+STATISTIC(NumSwitches, "Number of switches unswitched");
+STATISTIC(NumTrivial, "Number of unswitches that are trivial");
+
----------------
davide wrote:
> Do we really need `NumTrivial`? From what I see here it's just `NumBranches + NumSwitches`
Well, in the next patch we start doing non-trivial ones... I can wait until then if you prefer, I don't have a strong opinion.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:122
+  // exit block.
+  DT.changeImmediateDominator(UnswitchedNode, OldPHNode);
+
----------------
davide wrote:
> sanjoy wrote:
> > Instead of special casing here, why not just
> > 
> > ```
> >   SmallSetVector<BasicBlock *, 4> Worklist;
> >   Worklist.insert(UnswitchedNode);
> > ```
> > 
> > and let nature run its course?
> +1
I mean, I still need the comment on that line, and it is no shorter, and it avoids a pointless query to the DomChain set?

I can make the change if you want, just seems odd.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:179-184
+  // FIXME: We should compute this once at the start and update it!
+  SmallVector<BasicBlock *, 16> ExitBlocks;
+  L.getExitBlocks(ExitBlocks);
+  SmallPtrSet<BasicBlock *, 16> ExitBlockSet(ExitBlocks.begin(),
+                                             ExitBlocks.end());
+
----------------
davide wrote:
> Instead of having `getExitBlocks()` taking a `SmallVector<>` can we have it or a variant of it which takes a `SmallPtrSet` ? 
> I could use it in `LCSSA` to compute the dominance chain as well, FWIW.
Sure, could I add that in a follow-up patch?


================
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;
----------------
sanjoy wrote:
> 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.
It isn't at all. I just left this in from the old pass. I'd like to fix this in a subsequent patch though, added a comment.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:223
+  if (llvm::all_of(predecessors(LoopExitBB), [&](BasicBlock *PredBB) {
+        return PredBB == BI.getParent();
+      })) {
----------------
sanjoy wrote:
> 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);
> }
> ```
> 
Oooo, I like getSinglePredecessor. Done!


================
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,
----------------
sanjoy wrote:
> What does this routine change in the cases where nothing is unswitched?
This comment predates me making it *not* change anything. Updated comment.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:485
+      if (ConstantInt *Cond = dyn_cast<ConstantInt>(SI->getCondition())) {
+        CurrentBB = SI->findCaseValue(Cond)->getCaseSuccessor();
+        continue;
----------------
sanjoy wrote:
> 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`).
No, this is a holdover from when we restarted this entire thing on each unswitch. Thanks, updated.


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


https://reviews.llvm.org/D32409