[EH] Create removeUnwindEdge utility

[JosephTremoulet]

Summary

Factor the code that rewrites invokes to calls and rewrites WinEH
terminators to their "unwind to caller" equivalents into a helper in
Utils/Local, and use it in the three places I'm aware of that need to do
this.

[JosephTremoulet]

fix typo

[JosephTremoulet]

Remove a stale comment and simplify the code that updates a removed cleanup's predecessors' outgoing edges. I looked at introducing EHTerminatorInst as suggested in the FIXME comment (D13184), but the complexity seemed unnecessary, especially since this case can be handled with replaceUsesOfWith.

[rnk]

lgtm

Thanks for the refactor!

lib/Transforms/Utils/Local.cpp

1266

I think each if here can be something like:

NewTI = ...::Create(...);
OldPad = ...->getUnwindDest();

And then you can share the name taking and edge erasing code for all the non-call terminators.

[JosephTremoulet]

Incorporate Reid's feedback

include/llvm/Transforms/Utils/Local.h

 DbgDeclareInst *FindAllocaDbgDeclare(Value *V);
 
 /// \brief Replaces llvm.dbg.declare instruction when an alloca is replaced with
 /// a new value.  If Deref is true, tan additional DW_OP_deref is prepended to
 /// the expression.
 bool replaceDbgDeclareForAlloca(AllocaInst *AI, Value *NewAllocaAddress,
                                 DIBuilder &Builder, bool Deref);
 
+/// Replace 'BB's terminator with one that does not have an unwind successor
+/// block.  Rewrites `invoke` to `call`, `catchendpad unwind label %foo` to
+/// `catchendpad unwind to caller`, etc.  Updates any PHIs in unwind successor.
+///
+/// \param BB  Block whose terminator will be replaced.  Its terminator must
+///            have an unwind successor.
+void removeUnwindEdge(BasicBlock *BB);
+
 /// \brief Remove all blocks that can not be reached from the function's entry.
 ///
 /// Returns true if any basic block was removed.
 bool removeUnreachableBlocks(Function &F);
 
 /// \brief Combine the metadata of two instructions so that K can replace J
 ///
 /// Metadata not listed as known via KnownIDs is removed

lib/CodeGen/WinEHPrepare.cpp

for (BasicBlock *BB : BlocksInFunclet) {
         IsUnreachableCleanupendpad = CEPI->getCleanupPad() != CleanupPad;
       if (IsUnreachableRet || IsUnreachableCatchret ||
           IsUnreachableCleanupret || IsUnreachableCleanupendpad) {
         // Loop through all of our successors and make sure they know that one
         // of their predecessors is going away.
         for (BasicBlock *SuccBB : TI->successors())
           SuccBB->removePredecessor(BB);
 
+        if (IsUnreachableCleanupendpad) {
+          // We can't simply replace a cleanupendpad with unreachable, because
+          // its predecessor edges are EH edges and unreachable is not an EH
+          // pad.  Change all predecessors to the "unwind to caller" form.
+          for (pred_iterator PI = pred_begin(BB), PE = pred_end(BB);
+               PI != PE;) {
+            BasicBlock *Pred = *PI++;
+            removeUnwindEdge(Pred);
+          }
+        }
+
         new UnreachableInst(BB->getContext(), TI);
         TI->eraseFromParent();
       }
+      // FIXME: Check for invokes/cleanuprets/cleanupendpads which unwind to
+      // implausible catchendpads (i.e. catchendpad not in immediate parent
+      // funclet).
     }
   }
 }
 
 void WinEHPrepare::cleanupPreparedFunclets(Function &F) {
   // Clean-up some of the mess we made by removing useles PHI nodes, trivial
   // branches, etc.
   for (Function::iterator FI = F.begin(), FE = F.end(); FI != FE;) {

lib/Transforms/Utils/Local.cpp

do {
     Changed |= ConstantFoldTerminator(BB, true);
     for (succ_iterator SI = succ_begin(BB), SE = succ_end(BB); SI != SE; ++SI)
       if (Reachable.insert(*SI).second)
         Worklist.push_back(*SI);
   } while (!Worklist.empty());
   return Changed;
 }
 
+void llvm::removeUnwindEdge(BasicBlock *BB) {
+  TerminatorInst *TI = BB->getTerminator();
+
+  if (auto *II = dyn_cast<InvokeInst>(TI)) {
+    changeToCall(II);
+  } else if (auto *CRI = dyn_cast<CleanupReturnInst>(TI)) {
+    auto *NewCRI =
+        CleanupReturnInst::Create(CRI->getCleanupPad(), nullptr, CRI);
+    NewCRI->takeName(CRI);
+    NewCRI->setDebugLoc(CRI->getDebugLoc());
+    CRI->getUnwindDest()->removePredecessor(BB);
+    CRI->eraseFromParent();
+  } else if (auto *CEP = dyn_cast<CleanupEndPadInst>(TI)) {
+    auto *NewCEP =
+        CleanupEndPadInst::Create(CEP->getCleanupPad(), nullptr, CEP);
+    NewCEP->takeName(CEP);
+    NewCEP->setDebugLoc(CEP->getDebugLoc());
+    CEP->getUnwindDest()->removePredecessor(BB);
+    CEP->eraseFromParent();
+  } else if (auto *CEP = dyn_cast<CatchEndPadInst>(TI)) {
+    auto *NewCEP = CatchEndPadInst::Create(CEP->getContext(), nullptr, CEP);
+    NewCEP->takeName(CEP);
+    NewCEP->setDebugLoc(CEP->getDebugLoc());
+    CEP->getUnwindDest()->removePredecessor(BB);
+    CEP->eraseFromParent();
+  } else if (auto *TPI = dyn_cast<TerminatePadInst>(TI)) {
+    SmallVector<Value *, 3> TerminatePadArgs;
+    for (Value *Operand : TPI->arg_operands())
+      TerminatePadArgs.push_back(Operand);
+    auto *NewTPI = TerminatePadInst::Create(TPI->getContext(), nullptr,
+                                            TerminatePadArgs, TPI);
+    NewTPI->takeName(TPI);
+    NewTPI->setDebugLoc(TPI->getDebugLoc());
+    TPI->getUnwindDest()->removePredecessor(BB);
+    TPI->eraseFromParent();
+  } else {
+    llvm_unreachable("Could not find unwind successor");
+  }
+}
+
 /// removeUnreachableBlocksFromFn - Remove blocks that are not reachable, even
 /// if they are in a dead cycle.  Return true if a change was made, false
 /// otherwise.
 bool llvm::removeUnreachableBlocks(Function &F) {
   SmallPtrSet<BasicBlock*, 128> Reachable;
   bool Changed = markAliveBlocks(F, Reachable);
 
   // If there are unreachable blocks in the CFG...

lib/Transforms/Utils/SimplifyCFG.cpp

static bool SimplifyBranchOnICmpChain(BranchInst *BI, IRBuilder<> &Builder,
 
   // Erase the old branch instruction.
   EraseTerminatorInstAndDCECond(BI);
 
   DEBUG(dbgs() << "  ** 'icmp' chain result is:\n" << *BB << '\n');
   return true;
 }
 
-// FIXME: This seems like a pretty common thing to want to do.  Consider
-// whether there is a more accessible place to put this.
-static void convertInvokeToCall(InvokeInst *II) {
-  SmallVector<Value*, 8> Args(II->op_begin(), II->op_end() - 3);
-  // Insert a call instruction before the invoke.
-  CallInst *Call = CallInst::Create(II->getCalledValue(), Args, "", II);
-  Call->takeName(II);
-  Call->setCallingConv(II->getCallingConv());
-  Call->setAttributes(II->getAttributes());
-  Call->setDebugLoc(II->getDebugLoc());
-
-  // Anything that used the value produced by the invoke instruction now uses
-  // the value produced by the call instruction.  Note that we do this even
-  // for void functions and calls with no uses so that the callgraph edge is
-  // updated.
-  II->replaceAllUsesWith(Call);
-  II->getUnwindDest()->removePredecessor(II->getParent());
-
-  // Insert a branch to the normal destination right before the invoke.
-  BranchInst::Create(II->getNormalDest(), II);
-
-  // Finally, delete the invoke instruction!
-  II->eraseFromParent();
-}
-
 bool SimplifyCFGOpt::SimplifyResume(ResumeInst *RI, IRBuilder<> &Builder) {
   // If this is a trivial landing pad that just continues unwinding the caught
   // exception then zap the landing pad, turning its invokes into calls.
   BasicBlock *BB = RI->getParent();
   LandingPadInst *LPInst = dyn_cast<LandingPadInst>(BB->getFirstNonPHI());
   if (RI->getValue() != LPInst)
     // Not a landing pad, or the resume is not unwinding the exception that
     // caused control to branch here.
     return false;
 
   // Check that there are no other instructions except for debug intrinsics.
   BasicBlock::iterator I = LPInst, E = RI;
   while (++I != E)
     if (!isa<DbgInfoIntrinsic>(I))
       return false;
 
   // Turn all invokes that unwind here into calls and delete the basic block.
   for (pred_iterator PI = pred_begin(BB), PE = pred_end(BB); PI != PE;) {
-    InvokeInst *II = cast<InvokeInst>((*PI++)->getTerminator());
-    convertInvokeToCall(II);
+    BasicBlock *Pred = *PI++;
+    removeUnwindEdge(Pred);
   }
 
   // The landingpad is now unreachable.  Zap it.
   BB->eraseFromParent();
   return true;
 }
 
 bool SimplifyCFGOpt::SimplifyCleanupReturn(CleanupReturnInst *RI) {
for (BasicBlock::iterator I = BB->begin(), IE = BB->getFirstNonPHI();
           PN->addIncoming(PN, pred);
       PN->moveBefore(InsertPt);
     }
   }
 
   for (pred_iterator PI = pred_begin(BB), PE = pred_end(BB); PI != PE;) {
     // The iterator must be updated here because we are removing this pred.
     BasicBlock *PredBB = *PI++;
-    TerminatorInst *TI = PredBB->getTerminator();
     if (UnwindDest == nullptr) {
-      if (auto *II = dyn_cast<InvokeInst>(TI)) {
-        // The cleanup return being simplified continues to the caller and this
-        // predecessor terminated with an invoke instruction.  Convert the
-        // invoke to a call.
-        // This call updates the predecessor/successor chain.
-        convertInvokeToCall(II);
-      } else {
-        // In the remaining cases the predecessor's terminator unwinds to the
-        // block we are removing.  We need to create a new instruction that
-        // unwinds to the caller.  Simply setting the unwind destination to
-        // nullptr would leave the objects internal data in an inconsistent
-        // state.
-        // FIXME: Consider whether it is better to update setUnwindDest to
-        //        keep things consistent.
-        if (auto *CRI = dyn_cast<CleanupReturnInst>(TI)) {
-          auto *NewCRI = CleanupReturnInst::Create(CRI->getCleanupPad(),
-                                                   nullptr, CRI);
-          NewCRI->takeName(CRI);
-          NewCRI->setDebugLoc(CRI->getDebugLoc());
-          CRI->eraseFromParent();
-        } else if (auto *CEP = dyn_cast<CatchEndPadInst>(TI)) {
-          auto *NewCEP = CatchEndPadInst::Create(CEP->getContext(), nullptr,
-                                                 CEP);
-          NewCEP->takeName(CEP);
-          NewCEP->setDebugLoc(CEP->getDebugLoc());
-          CEP->eraseFromParent();
-        } else if (auto *TPI = dyn_cast<TerminatePadInst>(TI)) {
-          SmallVector<Value *, 3> TerminatePadArgs;
-          for (Value *Operand : TPI->arg_operands())
-            TerminatePadArgs.push_back(Operand);
-          auto *NewTPI = TerminatePadInst::Create(TPI->getContext(), nullptr,
-                                                  TerminatePadArgs, TPI);
-          NewTPI->takeName(TPI);
-          NewTPI->setDebugLoc(TPI->getDebugLoc());
-          TPI->eraseFromParent();
-        } else {
-          llvm_unreachable("Unexpected predecessor to cleanup pad.");
-        }
-      }
+      removeUnwindEdge(PredBB);
     } else {
-      // If the predecessor did not terminate with an invoke instruction, it
-      // must be some variety of EH pad.
       TerminatorInst *TI = PredBB->getTerminator();
-      // FIXME: Introducing an EH terminator base class would simplify this.
-      if (auto *II = dyn_cast<InvokeInst>(TI))
-        II->setUnwindDest(UnwindDest);
-      else if (auto *CRI = dyn_cast<CleanupReturnInst>(TI))
-        CRI->setUnwindDest(UnwindDest);
-      else if (auto *CEP = dyn_cast<CatchEndPadInst>(TI))
-        CEP->setUnwindDest(UnwindDest);
-      else if (auto *TPI = dyn_cast<TerminatePadInst>(TI))
-        TPI->setUnwindDest(UnwindDest);
-      else
-        llvm_unreachable("Unexpected predecessor to cleanup pad.");
+      TI->replaceUsesOfWith(BB, UnwindDest);
     }
   }
 
   // The cleanup pad is now unreachable.  Zap it.
   BB->eraseFromParent();
   return true;
 }
 
if (BranchInst *BI = dyn_cast<BranchInst>(TI)) {
       for (SwitchInst::CaseIt i = SI->case_begin(), e = SI->case_end();
            i != e; ++i)
         if (i.getCaseSuccessor() == BB) {
           BB->removePredecessor(SI->getParent());
           SI->removeCase(i);
           --i; --e;
           Changed = true;
         }
-    } else if (InvokeInst *II = dyn_cast<InvokeInst>(TI)) {
-      if (II->getUnwindDest() == BB) {
-        // Convert the invoke to a call instruction.  This would be a good
-        // place to note that the call does not throw though.
-        BranchInst *BI = Builder.CreateBr(II->getNormalDest());
-        II->removeFromParent();   // Take out of symbol table
-
-        // Insert the call now...
-        SmallVector<Value*, 8> Args(II->op_begin(), II->op_end()-3);
-        Builder.SetInsertPoint(BI);
-        CallInst *CI = Builder.CreateCall(II->getCalledValue(),
-                                          Args, II->getName());
-        CI->setCallingConv(II->getCallingConv());
-        CI->setAttributes(II->getAttributes());
-        // If the invoke produced a value, the call does now instead.
-        II->replaceAllUsesWith(CI);
-        delete II;
-        Changed = true;
-      }
-    }
+    } else if ((isa<InvokeInst>(TI) &&
+                cast<InvokeInst>(TI)->getUnwindDest() == BB) ||
+               isa<CatchEndPadInst>(TI) || isa<TerminatePadInst>(TI)) {
+      removeUnwindEdge(TI->getParent());
+      Changed = true;
+    } else if (isa<CleanupReturnInst>(TI) || isa<CleanupEndPadInst>(TI) ||
+               isa<CatchReturnInst>(TI)) {
+      new UnreachableInst(TI->getContext(), TI);
+      TI->eraseFromParent();
+    // TODO: If TI is a CatchPadInst, then (BB must be its normal dest and)
+    // we can eliminate it, redirecting its preds to its unwind successor,
+    // or to the next outer handler if the removed catch is the last for its
+    // catchendpad.
   }
 
   // If this block is now dead, remove it.
   if (pred_empty(BB) &&
       BB != &BB->getParent()->getEntryBlock()) {
     // We know there are no successors, so just nuke the block.
     BB->eraseFromParent();
     return true;

test/CodeGen/WinEH/wineh-cloning.ll

 ; CHECK-NEXT:   %catch = catchpad []
 ; CHECK-NEXT:       to label %catch.body unwind label %endpad
 ; CHECK:      catch.body:
 ; CHECK-NEXT:   catchret %catch to label %exit
 ; CHECK:      endpad:
 ; CHECK-NEXT:   catchendpad unwind to caller
 ; CHECK:      exit:
 ; CHECK-NEXT:   ret void
+
+define void @test11() personality i32 (...)* @__CxxFrameHandler3 {
+entry:
+  invoke void @f()
+    to label %exit unwind label %cleanup.outer
+cleanup.outer:
+  %outer = cleanuppad []
+  invoke void @f()
+    to label %outer.cont unwind label %cleanup.inner
+outer.cont:
+  br label %merge
+cleanup.inner:
+  %inner = cleanuppad []
+  br label %merge
+merge:
+  invoke void @f()
+    to label %unreachable unwind label %merge.end
+unreachable:
+  unreachable
+merge.end:
+  cleanupendpad %outer unwind to caller
+exit:
+  ret void
+}
+; merge.end will get cloned for outer and inner, but is implausible
+; from inner, so the invoke @f() in inner's copy of merge should be
+; rewritten to call @f()
+; CHECK-LABEL: define void @test11()
+; CHECK:      %inner = cleanuppad []
+; CHECK-NEXT: call void @f()
+; CHECK-NEXT: unreachable

test/Transforms/SimplifyCFG/wineh-unreachable.ll

+; RUN: opt -S -simplifycfg < %s | FileCheck %s
+
+declare void @Personality()
+declare void @f()
+
+; CHECK-LABEL: define void @test1()
+define void @test1() personality i8* bitcast (void ()* @Personality to i8*) {
+entry:
+  ; CHECK: call void @f()
+  invoke void @f()
+    to label %exit unwind label %unreachable.unwind
+exit:
+  ret void
+unreachable.unwind:
+  cleanuppad []
+  unreachable  
+}
+
+; CHECK-LABEL: define void @test2()
+define void @test2() personality i8* bitcast (void ()* @Personality to i8*) {
+entry:
+  invoke void @f()
+    to label %exit unwind label %catch.pad
+catch.pad:
+  ; CHECK: catchpad []
+  ; CHECK-NEXT: to label %catch.body unwind label %catch.end
+  %catch = catchpad []
+    to label %catch.body unwind label %catch.end
+catch.body:
+  ; CHECK:      catch.body:
+  ; CHECK-NEXT:   call void @f()
+  ; CHECK-NEXT:   unreachable
+  call void @f()
+  catchret %catch to label %unreachable
+catch.end:
+  ; CHECK: catch.end:
+  ; CHECK-NEXT: catchendpad unwind to caller
+  catchendpad unwind label %unreachable.unwind
+exit:
+  ret void
+unreachable.unwind:
+  cleanuppad []
+  unreachable
+unreachable:
+  unreachable
+}
+
+; CHECK-LABEL: define void @test3()
+define void @test3() personality i8* bitcast (void ()* @Personality to i8*) {
+entry:
+  invoke void @f()
+    to label %exit unwind label %cleanup.pad
+cleanup.pad:
+  ; CHECK: %cleanup = cleanuppad []
+  ; CHECK-NEXT: call void @f()
+  ; CHECK-NEXT: unreachable
+  %cleanup = cleanuppad []
+  invoke void @f()
+    to label %cleanup.ret unwind label %cleanup.end
+cleanup.ret:
+  ; This cleanupret should be rewritten to unreachable,
+  ; and merged into the pred block.
+  cleanupret %cleanup unwind label %unreachable.unwind
+cleanup.end:
+  ; This cleanupendpad should be rewritten to unreachable,
+  ; causing the invoke to be rewritten to a call.
+  cleanupendpad %cleanup unwind label %unreachable.unwind
+exit:
+  ret void
+unreachable.unwind:
+  cleanuppad []
+  unreachable
+}
+
+; CHECK-LABEL: define void @test4()
+define void @test4() personality i8* bitcast (void ()* @Personality to i8*) {
+entry:
+  invoke void @f()
+    to label %exit unwind label %terminate.pad
+terminate.pad:
+  ; CHECK: terminatepad [] unwind to caller
+  terminatepad [] unwind label %unreachable.unwind
+exit:
+  ret void
+unreachable.unwind:
+  cleanuppad []
+  unreachable
+}
 No newline at end of file