Page MenuHomePhabricator

[CallSiteSplitting] Do not perform callsite splitting inside landing pad
ClosedPublic

Authored by twoh on Mar 31 2018, 3:19 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

twoh created this revision.Mar 31 2018, 3:19 PM
junbuml added inline comments.Apr 2 2018, 8:31 AM
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 added inline comments.Apr 2 2018, 8:36 AM
lib/Transforms/Scalar/CallSiteSplitting.cpp
486–487 ↗(On Diff #140556)

Please see line 209 instead of line 216.

twoh updated this revision to Diff 140736.Apr 2 2018, 9:59 PM

Addressing @junbuml's comments. Thanks!

fhahn added a comment.Apr 2 2018, 10:05 PM

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 updated this revision to Diff 140739.Apr 2 2018, 10:45 PM

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 added a comment.Apr 3 2018, 7:37 AM

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?

twoh retitled this revision from Do not perform callsite splitting inside landing pad to [CallSiteSplitting] Do not perform callsite splitting inside landing pad.Apr 3 2018, 8:13 AM

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 updated this revision to Diff 140909.Apr 4 2018, 12:24 AM

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

twoh added inline comments.Apr 4 2018, 12:29 AM
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 added inline comments.Apr 4 2018, 8:18 AM
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 added inline comments.Apr 4 2018, 9:14 PM
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.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 4 2018, 9:20 PM
This revision was automatically updated to reflect the committed changes.