[PATCH] D15931: [LoopUnswitch] Create a PHINode for the original landingpad only if it has some uses

[Chen Li via llvm-commits]

chenli added inline comments.

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:1064
@@ +1063,3 @@
+      auto *OriginalLPad = ExitBlocks[i]->getLandingPadInst();
+      if (!OriginalLPad->use_empty()) {
+        assert(!LPad->getType()->isTokenTy() &&
----------------
reames wrote:
> chenli wrote:
> > reames wrote:
> > > I believe this code is wrong as written. If you don't insert the PHI, you still need to replace the original LandingPad with the new one.  
> > The original LandingPad is in the original loop, and the new LandingPad is in the cloned loop after switch. They will join into the same loop exit block where requires a PHINode which takes both the original landingPad and new LandingPad as incoming values. The original LandingPad should be independent of the new LandingPad and not be replaced. 
> Oh, you're right.  I misread the code as walking the successors rather than the predecessors for some reason.  
> 
> Though, I gotta admit I'm not quite clear what this code is doing even without your change.  The ExitBlock is the one outside the loop right?  Why do we believe that all other predecessors to the successor of an exit block are going to also have landing pads just because it does?  I'm not following that chain of logic.
> 
> Also, the logic in SplitEditBlocks appears like it might be a nop?  The docs for SplitBlockPredecessors indicates that Preds is supposed to be a subset of the predecessor blocks.  I'm not really sure what this is doing in this case.  
>From my understanding, when UnswitchNontrivialCondition happens, it first split loop preheader and loop exits and push them into the clone list. And then it create the new loop by cloning all blocks inside the clone list, where loop preheader and loop exits are all cloned. In other words, the original loop and the newly cloned loop don't share the same loop exits. They each have their own loop exits and joint the exit blocks into exit successors. Therefore, if NewExit has a landingpad, the original exit block should also have a landingpad since that's where NewExit cloned from. 

SplitExitBlocks should not be a nop. It basically splits all exit blocks and creates the joint blocks to be used after creating the new loop. In SplitExitBlocks, it does pass all predecessors for each loop exit block into SplitBlockPredecessors function.


http://reviews.llvm.org/D15931