[SimplifyCFG] Do not merge landingpads for invokes with deopt state.

[dantrushin]

Summary

Invokes with deopt states on them must have unique landingpads
for gc.relocate placement (see RewriteStatepointsForGC.cpp)

Disable landingpad merging for invokes with deopt states in SimplifyCFG,
to avoid need of undoing this optimization later.

[lebedev.ri]

I'm not sure i understand why this is required. Going through all the invokes
and ensuring that every deopt invoke has a dedicated (empty) landingpad
seems both trivial to do and something that you'd want to do a part of lowering later on anyway?.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

6659

Why is only the first predecessor checked?

6660

Can landingpad has any other predecessor than an invoke?

llvm/test/Transforms/SimplifyCFG/duplicate-landingpad.ll

1

Please use the script.

[dantrushin]

In D119266#3305457, @lebedev.ri wrote:

I'm not sure i understand why this is required. Going through all the invokes
and ensuring that every deopt invoke has a dedicated (empty) landingpad
seems both trivial to do and something that you'd want to do a part of lowering later on anyway?.

I want to avoid doing optimization which I will undo later.
In our downstream pipeline repeated cycles of simplifycfg + loopsimplify (which undoes this transform, too) finally triggers opt blowup (which is different problem, yes).

[dantrushin]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

6659

Mainly because I anticipated this question.
If I'd checked all predecessor, I'd been asked why I check all of them.

Initially we start with unique landingpads, so it has single predecessor. If we do not merge them, then it will stay that way.

6660

For the problem I'm interested in, no

[dantrushin]

Update test checks

[lebedev.ri]

Hot take: then the deopt is simply misdesigned.
The landingpad should simply be non-empty, it should contain a call to some intrinsic with nomerge attribute, so it stays unique.
(It sounds quite similar to the hasaddresstaken brokenness.)

More realistically, then we need to add the opposite transform into simplifycfg.

[reames]

Reading through the review, I see what look like two related issues.

1. RS4GC expects unique landing pads for calls being wrapped in statepoints. I don't get why this review phrases this as a property of deopt bundles; this seems like either a) a bug in RS4GC we should just fix or b) a restriction of the abstract machine model (i.e. a function of the GC attribute on the function.)

I'd also think this would be really easy to do in RS4GC as we already have all the utilities for splitting edges. Assuming there's not a deeper semantic issue I'm missing on first glance, I'd strongly prefer fixing RS4GC.

2. There's a reported infinite loop bug that sounds completely unrelated. A reproducer for that should be filed as it's own bug asap.

[dantrushin]

RS4GC knows how to split edges. so it can do it.
What I've been trying to do is to avoid repeatable merging (performed by SimplifyCFG) and splitting (performed by LoopSimplify) of landingpads.
Since these passes are run before RS4GC, we do not have statepoints in IR yet.

As for opt blowup I mentioned, it is not infinite loop, but an IR produced by repeatable application of above simplifyCFG + LoopSimplify.
When splitting landingpads, LoopSimplify adds '.split-lp' suffix to block name. After several repetitions, we get an IR which is cannot be read back by LLVM (fails verification), but which is correct by eye inspection.
Presumably, length of generated labels leads to such behavior. Unfortunately, I was unable to prepare test case which I can publish, so let's ignore that part for now.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

 /// values are flowing through both blocks, we don't lose any ability to
 /// specialize.  If anything, we make such specialization more likely.
 ///
 /// TODO - This transformation could remove entries from a phi in the target
 /// block when the inputs in the phi are the same for the two blocks being
 /// merged.  In some cases, this could result in removal of the PHI entirely.
 static bool TryToMergeLandingPad(LandingPadInst *LPad, BranchInst *BI,
                                  BasicBlock *BB, DomTreeUpdater *DTU) {
+
+  // Invokes with deopt bundles must have unique landingpads
+  // (See RewriteStatepointsForGC.cpp). Prohibit landingpad merging
+  // for such invokes.
+  BasicBlock *Pred = *pred_begin(BB);
+  if (auto *Invoke = dyn_cast<InvokeInst>(Pred->getTerminator()))
+    if (Invoke->getOperandBundle(LLVMContext::OB_deopt))
+      return false;
+
   auto Succ = BB->getUniqueSuccessor();
   assert(Succ);
   // If there's a phi in the successor block, we'd likely have to introduce
   // a phi into the merged landing pad block.
   if (isa<PHINode>(*Succ->begin()))
     return false;
 
   for (BasicBlock *OtherPred : predecessors(Succ)) {

llvm/test/Transforms/SimplifyCFG/duplicate-landingpad.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -simplifycfg -simplifycfg-require-and-preserve-domtree=1 -S | FileCheck %s
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 
 declare i32 @__gxx_personality_v0(...)
 declare void @fn()
 
 
 define void @test1() personality i32 (...)* @__gxx_personality_v0 {
lpad2:
   %exn2 = landingpad {i8*, i32}
   cleanup
   br label %shared_resume
 
 shared_resume:
   call void @fn()
   ret void
 }
+
+; Should not trigger when invokes have deopt bundles
+define void @neg3() gc "statepoint-example" personality i32 (...)* @__gxx_personality_v0 {
+; CHECK-LABEL: @neg3(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    invoke void @fn() [ "deopt"() ]
+; CHECK-NEXT:    to label [[INVOKE2:%.*]] unwind label [[LPAD1:%.*]]
+; CHECK:       invoke2:
+; CHECK-NEXT:    invoke void @fn() [ "deopt"() ]
+; CHECK-NEXT:    to label [[COMMON_RET:%.*]] unwind label [[LPAD2:%.*]]
+; CHECK:       common.ret:
+; CHECK-NEXT:    ret void
+; CHECK:       lpad1:
+; CHECK-NEXT:    [[EXN:%.*]] = landingpad { i8*, i32 }
+; CHECK-NEXT:    cleanup
+; CHECK-NEXT:    br label [[SHARED_RESUME:%.*]]
+; CHECK:       lpad2:
+; CHECK-NEXT:    [[EXN2:%.*]] = landingpad { i8*, i32 }
+; CHECK-NEXT:    cleanup
+; CHECK-NEXT:    br label [[SHARED_RESUME]]
+; CHECK:       shared_resume:
+; CHECK-NEXT:    call void @fn()
+; CHECK-NEXT:    br label [[COMMON_RET]]
+;
+entry:
+  invoke void @fn() [ "deopt"() ]
+  to label %invoke2 unwind label %lpad1
+
+invoke2:
+  invoke void @fn() [ "deopt"() ]
+  to label %invoke.cont unwind label %lpad2
+
+invoke.cont:
+  ret void
+
+lpad1:
+  %exn = landingpad {i8*, i32}
+  cleanup
+  br label %shared_resume
+
+lpad2:
+  %exn2 = landingpad {i8*, i32}
+  cleanup
+  br label %shared_resume
+
+shared_resume:
+  call void @fn()
+  ret void
+}