This is an archive of the discontinued LLVM Phabricator instance.

Merge empty landing pads in SimplifyCFG
ClosedPublic

Authored by reames on Mar 12 2015, 9:36 AM.

Details

Summary

This patch tries to merge duplicate landing pads when they branch to a common shared target.

Given IR that looks like this:
lpad1:

%exn = landingpad {i8*, i32} personality i32 (...)* @__gxx_personality_v0
       cleanup
br label %shared_resume

lpad2:

%exn2 = landingpad {i8*, i32} personality i32 (...)* @__gxx_personality_v0
        cleanup
br label %shared_resume

shared_resume:

call void @fn()
ret void

}

We can rewrite the users of both landing pad blocks to use one of them. This will generally allow the shared_resume block to be merged with the common landing pad as well.

Without this change, tail duplication would likely kick in - creating N (2 in this case) copies of the shared_resume basic block.

There are two subtitles with the choice of blocks to merge. From the comments:
/ We specifically choose to not worry about merging non-empty blocks
/ here. That is a PRE/scheduling problem and is best solved elsewhere. In
/ practice, the optimizer produces empty landing pad blocks quite frequently
/ when dealing with exception dense code. (see: instcombine, gvn, if-else
/ sinking in this file)
/
/ This is primarily a code size optimization. We need to avoid performing
/ any transform which might inhibit optimization (such as our ability to
/ specialize a particular handler via tail commoning). We do this by not
/ merging any blocks which require us to introduce a phi. Since the same
/ values are flowing through both blocks, we don't loose any ability to
/ specialize. If anything, we make such specialization more likely.

Diff Detail

Event Timeline

reames updated this revision to Diff 21843.Mar 12 2015, 9:36 AM
reames retitled this revision from to Merge empty landing pads in SimplifyCFG.
reames updated this object.
reames edited the test plan for this revision. (Show Details)
reames added a subscriber: Unknown Object (MLST).
reames updated this revision to Diff 21844.Mar 12 2015, 9:43 AM

Getting a correct diff (the test was screwed up in the last one)

andrew.w.kaylor accepted this revision.Mar 12 2015, 12:09 PM
andrew.w.kaylor added a reviewer: andrew.w.kaylor.
andrew.w.kaylor added a subscriber: andrew.w.kaylor.

LGTM (with a minor question about debug info intrinsics)

lib/Transforms/Utils/SimplifyCFG.cpp
4462

If I'm reading this right these debug intrinsics just get thrown away if a merge is performed. Do they not contain anything that might be different from the corresponding debug intrinsics in the potential merge block?

This revision is now accepted and ready to land.Mar 12 2015, 12:09 PM
reames added inline comments.Mar 12 2015, 3:11 PM
lib/Transforms/Utils/SimplifyCFG.cpp
4462

This is correct. dbg intriniscs are not allowed to modify optimization or code gen. You're right that we could contain them if they were the same though.

Actually, there's a bug here. I need to remove *both* sets of debug intrinsics if I they're not the same.

sanjoy added a subscriber: sanjoy.Mar 12 2015, 4:50 PM

Minor drive-by comments inline.

lib/Transforms/Utils/SimplifyCFG.cpp
4423

Isn't there only one successor, Succ?

4460

Nit: should be "equivalent"

This revision was automatically updated to reflect the committed changes.