[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 added inline comments.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:491
+    if (pred_empty(ExitBB)) {
+      // Only rewrite once.
+      if (UnswitchedExitBBs.insert(ExitBB).second)
----------------
chandlerc wrote:
> sanjoy wrote:
> > 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`.
> IMO, this is somewhat tied up in the other issue.
> 
> I actually wrote this version as well before writing the one you see here. The thing that bugged me about it was that splitting the exit blocks doesn't seem likely to be the common case. And so it seems weird to end up with a map that will often just be a set.
> 
> That, plus, we have a local and easy way to distinguish between the two and so it seemed like we should keep the data structures separate.
> 
> However, it does result in a lot more code. More efficient code, but more code all the same.
> 
> I'm somewhat divided on this. If my explanation convinces you to like this approach, I'll keep it. But if after hearing why I wasn't fond of the approach you still think its worth simplifying, let me know, and I'll do a follow-up patch to simplify everything.
As long as you tried the approach I mentioned and didn't like it, I'm okay with what you ultimately settled on.


Repository:
  rL LLVM

https://reviews.llvm.org/D32699