This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG] Extend SimplifyResume to handle phi of trivial landing pad.
ClosedPublic

Authored by chenli on Oct 13 2015, 9:06 PM.

Details

Summary

Currently SimplifyResume can convert an invoke instruction to a call instruction if its landing pad is trivial. In practice we could have several invoke instructions with trivial landing pads and share a common rethrow block, and in the common rethrow block, all the landing pads join to a phi node. The patch extends SimplifyResume to check the phi of landing pad and their incoming blocks. If any of them is trivial, remove it from the phi node and convert the invoke instruction to a call instruction.

Diff Detail

Event Timeline

chenli updated this revision to Diff 37318.Oct 13 2015, 9:06 PM
chenli retitled this revision from to [SimplifyCFG] Extend SimplifyResume to handle phi of trivial landing pad..
chenli updated this object.
chenli added reviewers: hfinkel, reames.
chenli added a subscriber: llvm-commits.
reames requested changes to this revision.Oct 14 2015, 11:01 AM
reames edited edge metadata.

The general transform looks reasonable, but I'd suggest possibly tackling this a slightly different way. Rather than explicitly doing the detection of a trivial block and rewrite in a single step, replace the branch to the unified resume with a resume and then let the existing code handle it. Doing it this way could make the change less invasive and easier to reason about. Note that this is a suggestion to think about, not a requirement. If you feel the current approach is easier/better, we can run with this after cleanup.

lib/Transforms/Utils/SimplifyCFG.cpp
2926

This comment is slightly confusing. I thought at first *all* incoming blocks had to be trivial. Wording change to indicate checking for *any* trivial block would be good.

2932

You're not actually checking the input is a landing pad with this check. It could be another PHI. Use a dyn_cast, continue pattern.

2937

Why? Just push the add below the loop. Either use a lambda or a IsOkay condition flag to dedect which case you're in.

2951

Please move this into the loop below.

This revision now requires changes to proceed.Oct 14 2015, 11:01 AM

The general transform looks reasonable, but I'd suggest possibly tackling this a slightly different way. Rather than explicitly doing the detection of a trivial block and rewrite in a single step, replace the branch to the unified resume with a resume and then let the existing code handle it. Doing it this way could make the change less invasive and easier to reason about. Note that this is a suggestion to think about, not a requirement. If you feel the current approach is easier/better, we can run with this after cleanup.

I was sort of thinking about a similar approach as you pointed, which was tail duplicating the common resume block to all its predecessors. I worried that if SimplifyResume could not simply all the predecessors, it might not be able to merge them back to a common resume block and would cause code size increment.

I was sort of thinking about a similar approach as you pointed, which was tail duplicating the common resume block to all its predecessors. I worried that if SimplifyResume could not simply all the predecessors, it might not be able to merge them back to a common resume block and would cause code size increment.

I wouldn't want to blindly tail duplicate the resume. Doing so for a particular scenario (landing pad to resume contains only debug intrinsics) is reasonable, but we definitely want to be able to common suffixes into a shared rethrow and the easiest way to do that is to have a common resume at the end. In other words, we need to establish profitability before doing the transform.

Thinking about it further, I think my suggestion is just a bad idea. Go with the approach you had, let's just cleanup the code to make it clear what's happening.

chenli edited edge metadata.

Update patch w.r.t Philip's comments

reames added inline comments.Oct 15 2015, 4:54 PM
lib/Transforms/Utils/SimplifyCFG.cpp
2930

Call this IncomingValue since it's not necessarily a landing pad yet.

2959

Blocks that will be deleted...

2961

Using BasicBlock::removePredecessor is the idiomatic way to do this.

2966–2967

I think that if you're not careful here, you mind end up with a block with a zero entry PHI and no predecessors. You should probably delete that one too.

chenli updated this revision to Diff 37637.Oct 16 2015, 1:46 PM
chenli edited edge metadata.
chenli marked an inline comment as done.

Update patch w.r.t Philip's comments.

reames added inline comments.Oct 21 2015, 6:47 PM
lib/Transforms/Utils/SimplifyCFG.cpp
2968

BB->getParent should always be set? Is this defensive programming (remove it), or a bug (add test case)?

chenli added inline comments.Oct 21 2015, 9:45 PM
lib/Transforms/Utils/SimplifyCFG.cpp
2968

We could have already erased BB if it is in TrivialUnwindBlocks. In that case BB becomes a dangling pointer. My code here isn't actually safe though. I should reset BB to nullptr on line 2967 if TrivialBB equals to BB. I will change it.

Update patch.

reames accepted this revision.Oct 22 2015, 11:54 AM
reames edited edge metadata.

LGTM w/minor comment addressed.

lib/Transforms/Utils/SimplifyCFG.cpp
2957–2964

use auto * here.

2968

A comment correction is in other. Possibly: have been deleted, and we haven't already deleted it above when deleting the landing pad blocks.

This revision is now accepted and ready to land.Oct 22 2015, 11:54 AM
chenli closed this revision.Oct 22 2015, 1:50 PM
chenli updated this revision to Diff 38476.Oct 26 2015, 3:57 PM
chenli edited edge metadata.

Update the patch so it does not crash bug 25299.

majnemer added inline comments.
test/Transforms/SimplifyCFG/bug-25299.ll
1 ↗(On Diff #38476)

Why does this test also need the -gvn pass?

chenli added inline comments.Oct 26 2015, 8:49 PM
test/Transforms/SimplifyCFG/bug-25299.ll
1 ↗(On Diff #38476)

-gvn is not needed here. I forgot to remove it while testing locally. I will update it now, and thanks for spotting that.

chenli updated this revision to Diff 38493.Oct 26 2015, 8:51 PM

Correct test command line options.