Reland "[BasicBlockUtils] Handle funclets when detaching EH pad blocks"

[spaits]

Reland #157363.
Fixes #148052.

This PR differs in three thing from #157363:

• Properly detaching and emptying the catchret/cleanupret block.
• Not explicitly handling catchswitches anymore.
• Move some deletion and detaching related code into static function and reuse them for EH Pad handling.

The first and third points are pretty self explanatory :)

The second point is not that much. If I remove my handling code for the catchswithc, then we may have catchpad within poison, which is similar semanitic (or at leas I imagine, not offically, since on paper there shouldn't be poison value) to catchpad within none. But here is the catch: catchpad within none is not valid llvm-ir. I think, temporarily, when removing basic blocks it may work.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Gábor Spaits (spaits)

Changes

This is a fix for a crash reported under PR #157363 that fixes issue #148052.

This PR contains two things:

• Properly detaching and emptying the catchret/cleanupret block.
• Not handling catchswitches anymore.

The first point is pretty explanatory :)

The second point is not that much. If I remove my handling code for the catchswithc, then we may have catchpad within poison, which is similar semanitic (or at leas I imagine, not offically) to catchpad within none. But here is the catch: catchpad within none is not valid llvm-ir. I think, temporarily, when removing basic blocks it may work.

---

Full diff: https://github.com/llvm/llvm-project/pull/158435.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Utils/BasicBlockUtils.cpp (+51-53)
• (modified) llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll (+37)

diff --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
index d2391e166f942..7cd9dbfb56095 100644
--- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
+++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
@@ -58,36 +58,64 @@ static cl::opt<unsigned> MaxDeoptOrUnreachableSuccessorCheckDepth(
              "is followed by a block that either has a terminating "
              "deoptimizing call or is terminated with an unreachable"));
 
-static void replaceFuncletPadsRetWithUnreachable(Instruction &I) {
+static void zapAllInstructionInDeadBasicBlock(BasicBlock *BB) {
+  while (!BB->empty()) {
+    Instruction &I = BB->back();
+    // If this instruction is used, replace uses with an arbitrary value.
+    // Because control flow can't get here, we don't care what we replace the
+    // value with. Note that since this block is unreachable, and all values
+    // contained within it must dominate their uses, that all uses will
+    // eventually be removed (they are themselves dead).
+    if (!I.use_empty())
+      I.replaceAllUsesWith(PoisonValue::get(I.getType()));
+    BB->back().eraseFromParent();
+  }
+  new UnreachableInst(BB->getContext(), BB);
+  assert(BB->size() == 1 && isa<UnreachableInst>(BB->getTerminator()) &&
+           "The successor list of BB isn't empty before "
+           "applying corresponding DTU updates.");
+}
+
+static void deleteBasicBlockFromSuccessor(
+    BasicBlock *BB, SmallVectorImpl<DominatorTree::UpdateType> *Updates,
+    bool KeepOneInputPHIs) {
+  SmallPtrSet<BasicBlock *, 4> UniqueSuccessors;
+  for (BasicBlock *Succ : successors(BB)) {
+    Succ->removePredecessor(BB, KeepOneInputPHIs);
+    if (Updates && UniqueSuccessors.insert(Succ).second)
+      Updates->push_back({DominatorTree::Delete, BB, Succ});
+  }
+}
+
+static void replaceFuncletPadsRetWithUnreachable(
+    Instruction &I, SmallVectorImpl<DominatorTree::UpdateType> *Updates,
+    bool KeepOneInputPHIs) {
   assert(isa<FuncletPadInst>(I) && "Instruction must be a funclet pad!");
   for (User *User : make_early_inc_range(I.users())) {
     Instruction *ReturnInstr = dyn_cast<Instruction>(User);
+    // If we have a cleanupret or catchret block, replace it with just an
+    // unreachable.
     if (isa<CatchReturnInst>(ReturnInstr) ||
         isa<CleanupReturnInst>(ReturnInstr)) {
       BasicBlock *ReturnInstrBB = ReturnInstr->getParent();
-      ReturnInstr->eraseFromParent();
-      new UnreachableInst(ReturnInstrBB->getContext(), ReturnInstrBB);
+      // This catchret or catchpad basic block is detached now. Let the
+      // successors know it.
+      deleteBasicBlockFromSuccessor(ReturnInstrBB, Updates, KeepOneInputPHIs);
+      zapAllInstructionInDeadBasicBlock(ReturnInstrBB);
     }
   }
 }
 
-void llvm::detachDeadBlocks(
-    ArrayRef<BasicBlock *> BBs,
-    SmallVectorImpl<DominatorTree::UpdateType> *Updates,
-    bool KeepOneInputPHIs) {
+void llvm::detachDeadBlocks(ArrayRef<BasicBlock *> BBs,
+                            SmallVectorImpl<DominatorTree::UpdateType> *Updates,
+                            bool KeepOneInputPHIs) {
   for (auto *BB : BBs) {
     // Loop through all of our successors and make sure they know that one
     // of their predecessors is going away.
     SmallPtrSet<BasicBlock *, 4> UniqueSuccessors;
-    for (BasicBlock *Succ : successors(BB)) {
-      Succ->removePredecessor(BB, KeepOneInputPHIs);
-      if (Updates && UniqueSuccessors.insert(Succ).second)
-        Updates->push_back({DominatorTree::Delete, BB, Succ});
-    }
-
-    // Zap all the instructions in the block.
-    while (!BB->empty()) {
-      Instruction &I = BB->back();
+    auto NonFirstPhiIt = BB->getFirstNonPHIIt();
+    if (NonFirstPhiIt != BB->end()) {
+      Instruction &I = *NonFirstPhiIt;
       // Exception handling funclets need to be explicitly addressed.
       // These funclets must begin with cleanuppad or catchpad and end with
       // cleanupred or catchret. The return instructions can be in different
@@ -95,42 +123,12 @@ void llvm::detachDeadBlocks(
       // first block, the we would have possible cleanupret and catchret
       // instructions with poison arguments, which wouldn't be valid.
       if (isa<FuncletPadInst>(I))
-        replaceFuncletPadsRetWithUnreachable(I);
-
-      // Catchswitch instructions have handlers, that must be catchpads and
-      // an unwind label, that is either a catchpad or catchswitch.
-      if (CatchSwitchInst *CSI = dyn_cast<CatchSwitchInst>(&I)) {
-        // Iterating over the handlers and the unwind basic block and processing
-        // catchpads. If the unwind label is a catchswitch, we just replace the
-        // label with poison later on.
-        for (unsigned I = 0; I < CSI->getNumSuccessors(); I++) {
-          BasicBlock *SucBlock = CSI->getSuccessor(I);
-          Instruction &SucFstInst = *(SucBlock->getFirstNonPHIIt());
-          if (isa<FuncletPadInst>(SucFstInst)) {
-            replaceFuncletPadsRetWithUnreachable(SucFstInst);
-            // There may be catchswitch instructions using the catchpad.
-            // Just replace those with poison.
-            if (!SucFstInst.use_empty())
-              SucFstInst.replaceAllUsesWith(
-                  PoisonValue::get(SucFstInst.getType()));
-            SucFstInst.eraseFromParent();
-          }
-        }
-      }
-
-      // Because control flow can't get here, we don't care what we replace the
-      // value with.  Note that since this block is unreachable, and all values
-      // contained within it must dominate their uses, that all uses will
-      // eventually be removed (they are themselves dead).
-      if (!I.use_empty())
-        I.replaceAllUsesWith(PoisonValue::get(I.getType()));
-      BB->back().eraseFromParent();
+        replaceFuncletPadsRetWithUnreachable(I, Updates, KeepOneInputPHIs);
     }
-    new UnreachableInst(BB->getContext(), BB);
-    assert(BB->size() == 1 &&
-           isa<UnreachableInst>(BB->getTerminator()) &&
-           "The successor list of BB isn't empty before "
-           "applying corresponding DTU updates.");
+
+    // Detaching and emptying the current basic block.
+    deleteBasicBlockFromSuccessor(BB, Updates, KeepOneInputPHIs);
+    zapAllInstructionInDeadBasicBlock(BB);
   }
 }
 
@@ -1704,8 +1702,8 @@ BranchInst *llvm::GetIfCondition(BasicBlock *BB, BasicBlock *&IfTrue,
 
   // We can only handle branches.  Other control flow will be lowered to
   // branches if possible anyway.
-  BranchInst *Pred1Br = dyn_cast<BranchInst>(Pred1->getTerminator());
-  BranchInst *Pred2Br = dyn_cast<BranchInst>(Pred2->getTerminator());
+  BranchInst *Pred1Br = dyn_cast_or_null<BranchInst>(Pred1->getTerminator());
+  BranchInst *Pred2Br = dyn_cast_or_null<BranchInst>(Pred2->getTerminator());
   if (!Pred1Br || !Pred2Br)
     return nullptr;
 
diff --git a/llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll b/llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll
index d2fccae6770db..d915b0c5272ee 100644
--- a/llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll
+++ b/llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll
@@ -166,4 +166,41 @@ bb13:
   cleanupret from %cleanuppad unwind label %funclet_bb10
 }
 
+%struct.foo = type { ptr, %struct.eggs, ptr }
+%struct.eggs = type { ptr, ptr, ptr }
+
+declare x86_thiscallcc ptr @quux(ptr, ptr, i32)
+
+define x86_thiscallcc ptr @baz(ptr %arg, ptr %arg1, ptr %arg2, i1 %arg3, ptr %arg4) personality ptr null {
+bb:
+  %alloca = alloca [2 x %struct.foo], align 4
+  %invoke = invoke x86_thiscallcc ptr @quux(ptr null, ptr null, i32 0)
+          to label %bb5 unwind label %bb10
+
+bb5:                                              ; preds = %bb
+  %getelementptr = getelementptr i8, ptr %arg, i32 20
+  %call = call x86_thiscallcc ptr null(ptr null, ptr null, i32 0)
+  br label %bb6
+
+bb6:                                              ; preds = %bb10, %bb5
+  %phi = phi ptr [ null, %bb10 ], [ null, %bb5 ]
+  ret ptr %phi
+
+bb7:                                              ; No predecessors!
+  %cleanuppad = cleanuppad within none []
+  %getelementptr8 = getelementptr i8, ptr %arg2, i32 -20
+  %icmp = icmp eq ptr %arg, null
+  br label %bb9
+
+bb9:                                              ; preds = %bb7
+  cleanupret from %cleanuppad unwind label %bb10
+
+bb10:                                             ; preds = %bb9, %bb
+  %phi11 = phi ptr [ %arg, %bb9 ], [ null, %bb ]
+  %cleanuppad12 = cleanuppad within none []
+  %getelementptr13 = getelementptr i8, ptr %phi11, i32 -20
+  store i32 0, ptr %phi11, align 4
+  br label %bb6
+}
+
 declare void @func(i64, i64, ptr)

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[rnk]

Thanks!

[spaits]

Thank you very much @rnk for reviewing my PR. I will probably merge it today.

I have added a few NFC commits. Just cleaning up and more testing.

• Added another test case.
• Added some more comments, when deleting the uses of catchpad/cleanuppad.
• Cleaned up the code a bit:
  • Merged two static functions, that are always called after each other (zapAllInstructionInDeadBasicBlock + deleteBasicBlockFromSuccessor -> emptyAndDetachBlock)
  • Deleted the static function, that was only called from one place after no more explicit catchswitch handling (replaceFuncletPadsRetWithUnreachable).