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

[Philip Reames via llvm-commits]

reames 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() &&
----------------
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.  


http://reviews.llvm.org/D15931