[CallSiteSplitting] Do not perform callsite splitting inside landing pad

[twoh]

Summary

If the callsite is inside landing pad, do not perform callsite splitting.

Callsite splitting uses utility function llvm::DuplicateInstructionsInSplitBetween, which eventually calls llvm::SplitEdge. llvm::SplitEdge calls llvm::SplitCriticalEdge with an assumption that the function returns nullptr only when the target edge is not a critical edge (and further assumes that if the return value was not nullptr, the predecessor of the original target edge always has a single successor because critical edge splitting was successful). However, this assumtion is not true because SplitCriticalEdge returns nullptr if the destination block is a landing pad. This invalid assumption results assertion failure.

Fundamental solution might be fixing llvm::SplitEdge to not to rely on the invalid assumption. However, it'll involve a lot of work because current API assumes that llvm::SplitEdge never fails. Instead, this patch makes callsite splitting to not to attempt splitting if the callsite is in a landing pad.

Attached test case will crash with assertion failure without the fix.

[junbuml]

lib/Transforms/Scalar/CallSiteSplitting.cpp

486–487

Can you move this check in canSplitCallSite()? You can use !CallSiteBB->isEHPad() instead of CallSiteBB->canSplitPredecessors() in line 216.

test/Transforms/CallSiteSplitting/lpad.ll

1

Can you add FileCheck to check no callsite splitting happened ?

[junbuml]

lib/Transforms/Scalar/CallSiteSplitting.cpp

486–487

Please see line 209 instead of line 216.

[twoh]

Addressing @junbuml's comments. Thanks!

[fhahn]

Thank you very much for your patch. Using DuplicateInstructionsInSplitBetween here has surfaced a few edge cases already.

LGTM

lib/Transforms/Scalar/CallSiteSplitting.cpp

209

Is there any reason besides the duplicateInstructionsInSplitBetween, that prevents us from splitting the callsite? It would be great if you could mention why we cannot do splitting here.

test/Transforms/CallSiteSplitting/lpad.ll

1

Could you also check that there is only on call to callee? Checking for not split is slightly more fragile.

[twoh]

Thanks @fhahn for the comments! I added comments about why we don't perform splitting for the callsites inside the landing pad. Please let me know if you think it is too verbose.

[fhahn]

In D45130#1055295, @twoh wrote:

Thanks @fhahn for the comments! I added comments about why we don't perform splitting for the callsites inside the landing pad. Please let me know if you think it is too verbose.

Thanks. It is quite verbose and I think it would be enough to state that this check prevents triggering an assertion in SplitEdge used via DuplicateInstructionsInSplitBetween. It is fine to just update the comment before committing. Could you also prefix [CallSiteSplitting] to the commit message?

[junbuml]

Thanks for handling this. Just minor comments inlined.

lib/Transforms/Scalar/CallSiteSplitting.cpp

217–220

Since canSplitPredecessors() simply return false for EHPad except LandingPad, you can return false for EHPad here like :

return !CallSiteBB->isEHPad();

test/Transforms/CallSiteSplitting/lpad.ll

8

Could you please also check the basic block name ?

[twoh]

Addressing comments from @fhahn and @junbuml's comments. Thank you!

[twoh]

lib/Transforms/Scalar/CallSiteSplitting.cpp

217–220

@junbuml Got it. I think it is still worth to leave calling canSplitPredecessors here, in case of more cases to be added to the function in the future (the function name seems generic enough to check more than EHPad). What do you think?

[junbuml]

lib/Transforms/Scalar/CallSiteSplitting.cpp

217–220

I understand. In this patch, using these two seems okay for now.
However,I think we should ultimately have a single function to do canSplitPredecessors, which works for SplitEdge() as well as SplitBlockPredecessors(). I didn't look at in great detail, but I guess it must be okay to specifically handle LandingPad in SplitEdge().

[twoh]

lib/Transforms/Scalar/CallSiteSplitting.cpp

217–220

Agree. I think the issue is that while SplitBlockPredecessors() may return nullptr if canSplitPredecessors returns false, SplitEdge() (and SplitBlock()) is not expected to return nullptr.

lib/Transforms/Scalar/CallSiteSplitting.cpp

static bool canSplitCallSite(CallSite CS, TargetTransformInfo &TTI) {
   }
 
   // Need 2 predecessors and cannot split an edge from an IndirectBrInst.
   SmallVector<BasicBlock *, 2> Preds(predecessors(CallSiteBB));
   if (Preds.size() != 2 || isa<IndirectBrInst>(Preds[0]->getTerminator()) ||
       isa<IndirectBrInst>(Preds[1]->getTerminator()))
     return false;
 
   return CallSiteBB->canSplitPredecessors();
 }
 
 static Instruction *cloneInstForMustTail(Instruction *I, Instruction *Before,
                                          Value *V) {
   Instruction *Copy = I->clone();
   Copy->setName(I->getName());
   Copy->insertBefore(Before);
   if (V)
     Copy->setOperand(0, V);
   return Copy;
 }
 
 /// Copy mandatory `musttail` return sequence that follows original `CI`, and
 /// link it up to `NewCI` value instead:
 ///
 ///   * (optional) `bitcast NewCI to ...`
 ///   * `ret bitcast or NewCI`
 ///
 /// Insert this sequence right before `SplitBB`'s terminator, which will be
for (Function::iterator BI = F.begin(), BE = F.end(); BI != BE;) {
     // can replace BB's terminator in case BB is a successor of itself. In that
     // case, IE will be invalidated and we also have to check the current
     // terminator.
     while (II != IE && &*II != BB.getTerminator()) {
       Instruction *I = &*II++;
       CallSite CS(cast<Value>(I));
       if (!CS || isa<IntrinsicInst>(I) || isInstructionTriviallyDead(I, &TLI))
         continue;
+      if (I->getParent()->isEHPad())
+        continue;
 
       Function *Callee = CS.getCalledFunction();
       if (!Callee || Callee->isDeclaration())
         continue;
 
       // Successful musttail call-site splits result in erased CI and erased BB.
       // Check if such path is possible before attempting the splitting.
       bool IsMustTail = CS.isMustTailCall();

test/Transforms/CallSiteSplitting/lpad.ll

+; RUN: opt -S -callsite-splitting < %s
+
+declare void @foo(i1* %p);
+declare void @bar(i1* %p);
+declare dso_local i32 @__gxx_personality_v0(...)
+
+define void @caller(i1* %p) personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+entry:
+  %0 = icmp eq i1* %p, null
+  br i1 %0, label %bb1, label %bb2
+
+bb1:
+  invoke void @foo(i1* %p) to label %end1 unwind label %lpad
+
+bb2:
+  invoke void @bar(i1* %p) to label %end2 unwind label %lpad
+
+lpad:
+  %1 = landingpad { i8*, i32 } cleanup
+  call void @callee(i1* %p)
+  resume { i8*, i32 } %1
+
+end1:
+  ret void
+
+end2:
+  ret void
+}
+
+define internal void @callee(i1* %p) {
+  ret void
+}