[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 ↗

(On Diff #140556)

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 ↗

(On Diff #140556)

Can you add FileCheck to check no callsite splitting happened ?

[junbuml]

lib/Transforms/Scalar/CallSiteSplitting.cpp

486–487 ↗

(On Diff #140556)

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 ↗

(On Diff #140736)

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 ↗

(On Diff #140556)

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 ↗

(On Diff #140739)

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

7 ↗

(On Diff #140739)

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 ↗

(On Diff #140739)

@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 ↗

(On Diff #140739)

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 ↗

(On Diff #140739)

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.

llvm/trunk/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;
 
+  // Do not split a call-site in an exception handling block. This check
+  // prevents triggering an assertion in SplitEdge used via
+  // DuplicateInstructionsInSplitBetween. 
+  if (CallSiteBB->isEHPad())
+    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);

llvm/trunk/test/Transforms/CallSiteSplitting/lpad.ll

+; RUN: opt -S -callsite-splitting < %s | FileCheck %s
+;
+; Make sure that the callsite is not splitted by checking that there's only one
+; call to @callee.
+
+; CHECK-LABEL: @caller
+; CHECK-LABEL: lpad
+; CHECK: call void @callee
+; CHECK-NOT: call void @callee
+
+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
+}