[llvm] [BasicBlockUtils] Remove broken support for eh pads in SplitEdge() (PR #137816)

[Nikita Popov via llvm-commits]

https://github.com/nikic created https://github.com/llvm/llvm-project/pull/137816

d81d9e8b8604c85709de0a22bb8dd672a28f0401 changed SplitEdge() to make use of ehAwareSplitEdge() for critical edges where the target is an eh pad. However, the implementation is incorrect at least for landing pads. What is currently produced for the code in the modified unit test is something like this:

    continue:
      invoke void @sink()
              to label %normal unwind label %new_bb

    new_bb:
      %cp = cleanuppad within %exception []
      cleanupret from %cp unwind label %exception

    exception:
      %cleanup = landingpad i8 cleanup
      br label %trivial-eh-handler

This mixes different exception handling mechanisms in a nonsensical way, and is not well-formed IR. To actually "split" the landingpad edge (for a rather loose definition of "split"), I think we'd have to generate something along these lines:

      exception.split:
        %cleanup.split = landingpad i8 cleanup
        br label %exception.cont

      exception:
        %cleanup.orig = landingpad i8 cleanup
        br label %exception.cont
      
      exception.cont:
        %cleanup = phi i8 [ %cleanup.split, %exception_split ], [ %cleanup.orig, %exception ]

I didn't bother actually implementing that, seeing as how nobody noticed the existing codegen being broken in the last four years, so clearly nobody actually needs this function to work with EH edges. Just return nullptr instead.

>From 1cebd2e81ea89843190fc650e6ae5c60165ce2e3 Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Tue, 29 Apr 2025 15:41:29 +0200
Subject: [PATCH] [BasicBlockUtils] Remove support for eh pads in SplitEdge()

d81d9e8b8604c85709de0a22bb8dd672a28f0401 changed SplitEdge() to
make use of ehAwareSplitEdge() for critical edges where the
target is an eh pad. However, the implementation is incorrect for
landing pads, because this requires passing additional information
to the function and performing non-local updates.

What is currently produced for the code in the modified unit test
is this:

    continue:
      invoke void @sink()
              to label %normal unwind label %0

    0:
      %1 = cleanuppad within %exception []
      cleanupret from %1 unwind label %exception

    exception:
      %cleanup = landingpad i8
              cleanup
      br label %trivial-eh-handler

This is not well-formed IR. To actually "split" the landingpad
edge we would have to do something along the lines of what CoroFrame
does, where we'd generate multiple landingpads, which then go to
a common phi in the original block.

I could try to implement this here, but I find it rather dubious
(we'd still have a landingpad at the start of the "split" edge so
it's not *really* "split") and clearly nobody actually uses
SplitEdge() on eh edges, because then somebody would have noticed
that it generates broken code.
---
 llvm/lib/Transforms/Utils/BasicBlockUtils.cpp  |  5 -----
 .../Transforms/Utils/BasicBlockUtilsTest.cpp   | 18 ++----------------
 2 files changed, 2 insertions(+), 21 deletions(-)

diff --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
index 429037ab9da47..dce10c0ecd56c 100644
--- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
+++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
@@ -768,11 +768,6 @@ BasicBlock *llvm::SplitEdge(BasicBlock *BB, BasicBlock *Succ, DominatorTree *DT,
       CriticalEdgeSplittingOptions(DT, LI, MSSAU).setPreserveLCSSA();
 
   if ((isCriticalEdge(LatchTerm, SuccNum, Options.MergeIdenticalEdges))) {
-    // If it is a critical edge, and the succesor is an exception block, handle
-    // the split edge logic in this specific function
-    if (Succ->isEHPad())
-      return ehAwareSplitEdge(BB, Succ, nullptr, nullptr, Options, BBName);
-
     // If this is a critical edge, let SplitKnownCriticalEdge do it.
     return SplitKnownCriticalEdge(LatchTerm, SuccNum, Options, BBName);
   }
diff --git a/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp b/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp
index c9a6a32851775..e58daed887855 100644
--- a/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp
+++ b/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp
@@ -285,22 +285,8 @@ declare void @sink_alt() cold
   EXPECT_TRUE(Ehpad);
 
   BasicBlock *NewBB = SplitEdge(SrcBlock, DestBlock, &DT, &LI, &MSSAU, "");
-
-  MSSA.verifyMemorySSA();
-  EXPECT_TRUE(DT.verify());
-  EXPECT_NE(NewBB, nullptr);
-  EXPECT_EQ(NewBB->getSinglePredecessor(), SrcBlock);
-  EXPECT_EQ(NewBB, SrcBlock->getTerminator()->getSuccessor(SuccNum));
-  EXPECT_EQ(NewBB->getParent(), F);
-
-  bool BBFlag = false;
-  for (BasicBlock &BB : *F) {
-    if (BB.getName() == NewBB->getName()) {
-      BBFlag = true;
-      break;
-    }
-  }
-  EXPECT_TRUE(BBFlag);
+  // SplitEdge cannot split an eh pad edge.
+  EXPECT_EQ(NewBB, nullptr);
 }
 
 TEST(BasicBlockUtils, splitBasicBlockBefore_ex1) {