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

[aeubanks]

Reverts #157363

Causes crashes, see #157363 (comment)

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Arthur Eubanks (aeubanks)

Changes

Reverts llvm/llvm-project#157363

Causes crashes, see #157363 (comment)

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Utils/BasicBlockUtils.cpp (+1-43)
• (removed) llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll (-169)

diff --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
index d2391e166f942..cad0b4c12b54e 100644
--- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
+++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
@@ -58,19 +58,6 @@ 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) {
-  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 (isa<CatchReturnInst>(ReturnInstr) ||
-        isa<CleanupReturnInst>(ReturnInstr)) {
-      BasicBlock *ReturnInstrBB = ReturnInstr->getParent();
-      ReturnInstr->eraseFromParent();
-      new UnreachableInst(ReturnInstrBB->getContext(), ReturnInstrBB);
-    }
-  }
-}
-
 void llvm::detachDeadBlocks(
     ArrayRef<BasicBlock *> BBs,
     SmallVectorImpl<DominatorTree::UpdateType> *Updates,
@@ -88,36 +75,7 @@ void llvm::detachDeadBlocks(
     // Zap all the instructions in the block.
     while (!BB->empty()) {
       Instruction &I = BB->back();
-      // 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
-      // basic blocks than the pad instruction. If we would only delete the
-      // 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();
-          }
-        }
-      }
-
+      // 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
diff --git a/llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll b/llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll
deleted file mode 100644
index d2fccae6770db..0000000000000
--- a/llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll
+++ /dev/null
@@ -1,169 +0,0 @@
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
-; RUN: opt -passes=simplifycfg -S < %s | FileCheck %s
-
-; cleanuppad/cleanupret
-
-define void @unreachable_cleanuppad_linear(i64 %shapes.1) personality ptr null {
-; CHECK-LABEL: define void @unreachable_cleanuppad_linear(
-; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
-; CHECK-NEXT:  [[START:.*:]]
-; CHECK-NEXT:    [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
-; CHECK-NEXT:    ret void
-;
-start:
-  %_7 = icmp ult i64 0, %shapes.1
-  ret void
-
-funclet:
-  %cleanuppad = cleanuppad within none []
-  br label %funclet_end
-
-funclet_end:
-  cleanupret from %cleanuppad unwind to caller
-}
-
-define void @unreachable_cleanuppad_multiple_predecessors(i64 %shapes.1) personality ptr null {
-; CHECK-LABEL: define void @unreachable_cleanuppad_multiple_predecessors(
-; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
-; CHECK-NEXT:  [[START:.*:]]
-; CHECK-NEXT:    [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
-; CHECK-NEXT:    ret void
-;
-start:
-  %_7 = icmp ult i64 0, %shapes.1
-  ret void
-
-funclet:
-  %cleanuppad = cleanuppad within none []
-  switch i64 %shapes.1, label %otherwise [ i64 0, label %one
-  i64 1, label %two
-  i64 42, label %three ]
-one:
-  br label %funclet_end
-
-two:
-  br label %funclet_end
-
-three:
-  br label %funclet_end
-
-otherwise:
-  br label %funclet_end
-
-funclet_end:
-  cleanupret from %cleanuppad unwind to caller
-}
-
-; catchpad/catchret
-
-define void @unreachable_catchpad_linear(i64 %shapes.1) personality ptr null {
-; CHECK-LABEL: define void @unreachable_catchpad_linear(
-; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
-; CHECK-NEXT:  [[START:.*:]]
-; CHECK-NEXT:    [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
-; CHECK-NEXT:    ret void
-;
-start:
-  %_7 = icmp ult i64 0, %shapes.1
-  ret void
-
-dispatch:
-  %cs = catchswitch within none [label %funclet] unwind to caller
-
-funclet:
-  %cleanuppad = catchpad within %cs []
-  br label %funclet_end
-
-
-funclet_end:
-  catchret from %cleanuppad to label %unreachable
-
-unreachable:
-  unreachable
-}
-
-define void @unreachable_catchpad_multiple_predecessors(i64 %shapes.1) personality ptr null {
-; CHECK-LABEL: define void @unreachable_catchpad_multiple_predecessors(
-; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
-; CHECK-NEXT:  [[START:.*:]]
-; CHECK-NEXT:    [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
-; CHECK-NEXT:    ret void
-;
-start:
-  %_7 = icmp ult i64 0, %shapes.1
-  ret void
-
-dispatch:
-  %cs = catchswitch within none [label %funclet] unwind to caller
-
-funclet:
-  %cleanuppad = catchpad within %cs []
-  switch i64 %shapes.1, label %otherwise [ i64 0, label %one
-  i64 1, label %two
-  i64 42, label %three ]
-one:
-  br label %funclet_end
-
-two:
-  br label %funclet_end
-
-three:
-  br label %funclet_end
-
-otherwise:
-  br label %funclet_end
-
-funclet_end:
-  catchret from %cleanuppad to label %unreachable
-
-unreachable:
-  unreachable
-}
-
-; Issue reproducer
-
-define void @gh148052(i64 %shapes.1) personality ptr null {
-; CHECK-LABEL: define void @gh148052(
-; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
-; CHECK-NEXT:  [[START:.*:]]
-; CHECK-NEXT:    [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
-; CHECK-NEXT:    call void @llvm.assume(i1 [[_7]])
-; CHECK-NEXT:    ret void
-;
-start:
-  %_7 = icmp ult i64 0, %shapes.1
-  br i1 %_7, label %bb1, label %panic
-
-bb1:
-  %_11 = icmp ult i64 0, %shapes.1
-  br i1 %_11, label %bb3, label %panic1
-
-panic:
-  unreachable
-
-bb3:
-  ret void
-
-panic1:
-  invoke void @func(i64 0, i64 0, ptr null)
-  to label %unreachable unwind label %funclet_bb14
-
-funclet_bb14:
-  %cleanuppad = cleanuppad within none []
-  br label %bb13
-
-unreachable:
-  unreachable
-
-bb10:
-  cleanupret from %cleanuppad5 unwind to caller
-
-funclet_bb10:
-  %cleanuppad5 = cleanuppad within none []
-  br label %bb10
-
-bb13:
-  cleanupret from %cleanuppad unwind label %funclet_bb10
-}
-
-declare void @func(i64, i64, ptr)

[spaits]

Hi @aeubanks . I am checking the issue. Please wait a bit with the revert. If I can't do it in a few hours, I will approve this and merge this.

If I can fix it fast have would rather commit that, than reland the whole patch again.

[spaits]

I think I have found the problem:

bb10:                                             ; preds = %bb9, %bb
  %phi11 = phi ptr [ %arg, %bb9 ], [ null, %bb ]

Here bb9 is ureachable, and my patch replaces the cleanupret in it with in it with unreachable.

(If we wouldn't do that, it would crash in some other scenarion: see #148052)

I also see that the problem is that, I don't update the successors of the extra deleted EH related blocks.

[spaits]

I think I can come up with a fix in 1-2 hours.

[spaits]

I have a version, in which your LLVM-IR works. I will do some testing with it. If it works fine there I will create the PR.

[spaits]

@aeubanks I have created the PR: #158435
It fixes the issue. Also, it has some experimentation :). Your LLVM-IR code is also added to the tests. Let's wait for the CI and reviews.

[aeubanks]

looks like the premerge flaked on lldb tests

when a PR breaks something, usually we revert to green, and then you are free to reland with a fix: https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy