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

[reames]

Summary

This patch adds a check in UnswitchNontrivialCondition to see if the original landingpad instruction has any uses. If not, we don't need to create a PHINode for it after SplitExitEdges. Please refer to D15835 for the motivation.

[reames]

Please briefly explain the motivation in this description as well. You want the commit comment to be self explanatory to reviewers and readers of the commit message.

lib/Transforms/Scalar/LoopUnswitch.cpp

1064

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.

[• chenli]

lib/Transforms/Scalar/LoopUnswitch.cpp

1064

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.

[reames]

lib/Transforms/Scalar/LoopUnswitch.cpp

1064

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.

[• chenli]

lib/Transforms/Scalar/LoopUnswitch.cpp

1064

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.

[• chenli]

ping...

[reames]

Closing a long abandoned review.

lib/Transforms/Scalar/LoopUnswitch.cpp

for (BasicBlock::iterator I = ExitSucc->begin();
          PHINode *PN = dyn_cast<PHINode>(I); ++I) {
       Value *V = PN->getIncomingValueForBlock(ExitBlocks[i]);
       ValueToValueMapTy::iterator It = VMap.find(V);
       if (It != VMap.end()) V = It->second;
       PN->addIncoming(V, NewExit);
     }
 
     if (LandingPadInst *LPad = NewExit->getLandingPadInst()) {
+      auto *OriginalLPad = ExitBlocks[i]->getLandingPadInst();
+      if (!OriginalLPad->use_empty()) {
+        assert(!LPad->getType()->isTokenTy() &&
+               "Loop unswitch cannot be applied if LPad is token type. "
+               "Otherwise an invalid PHINode of token type would be created.");
         PHINode *PN = PHINode::Create(LPad->getType(), 0, "",
                                       &*ExitSucc->getFirstInsertionPt());
 
         for (pred_iterator I = pred_begin(ExitSucc), E = pred_end(ExitSucc);
              I != E; ++I) {
           BasicBlock *BB = *I;
           LandingPadInst *LPI = BB->getLandingPadInst();
           LPI->replaceAllUsesWith(PN);
           PN->addIncoming(LPI, BB);
         }
       }
     }
+  }
 
   // Rewrite the code to refer to itself.
   for (unsigned i = 0, e = NewBlocks.size(); i != e; ++i)
     for (BasicBlock::iterator I = NewBlocks[i]->begin(),
            E = NewBlocks[i]->end(); I != E; ++I)
       RemapInstruction(&*I, VMap,
                        RF_NoModuleLevelChanges | RF_IgnoreMissingEntries);
 

test/Transforms/LoopUnswitch/token.ll

+; RUN: opt < %s -loop-unswitch -S 2>&1 | FileCheck %s
+
+define void @test1(i1 %cond) personality i32 ()* @"dummy_personality" {
+entry:
+  br label %for.cond
+
+for.cond:
+  call void @dummy_call()           
+  br i1 %cond, label %for.body, label %for.end
+  
+for.body:                              
+  invoke void @dummy_invoke()
+          to label %invoke.cont unwind label %lpad
+
+invoke.cont:                                      
+  br label %for.inc
+
+lpad:                                             
+  %a = landingpad token
+          cleanup
+  unreachable
+  
+for.inc:                                          
+  br label %for.cond
+
+for.end:                                          
+  ret void
+}
+
+; CHECK: lpad.us-lcssa.us:                                 ; preds = %for.body.us
+; CHECK-NEXT:  %lpad.us-lcssa1.us = landingpad token
+
+; CHECK: lpad.us-lcssa:                                    ; preds = %for.body
+; CHECK-NEXT:  %lpad.us-lcssa1 = landingpad token
+
+; CHECK: lpad:                                             ; preds = %lpad.us-lcssa.us, %lpad.us-lcssa
+; CHECK-NOT: phi token
+
+declare void @dummy_call()
+
+declare void @dummy_invoke()
+
+declare i32 @"dummy_personality"()