Page MenuHomePhabricator

Avoid inlining call sites in unreachable-terminated block
ClosedPublic

Authored by junbuml on Jan 26 2016, 5:43 PM.

Details

Summary

If the normal destination of the invoke or the parent block of the call site is unreachable-terminated, there is little point in inlining the call site unless there is literally zero cost. Unlike my previous change (D15289), this change specifically handle the call sites followed by unreachable in the same basic block for call or in the normal destination for the invoke. This change could be a reasonable first step to conservatively inline call sites leading to an unreachable-terminated block while BFI / BPI is not yet available in inliner.

Diff Detail

Event Timeline

junbuml updated this revision to Diff 46084.Jan 26 2016, 5:43 PM
junbuml retitled this revision from to Avoid inlining CallSites in unreachable-terminated block.
junbuml updated this object.
junbuml added a subscriber: llvm-commits.
davidxl edited edge metadata.Jan 26 2016, 8:26 PM

The patch is straightforward and looks reasonable to me.

lib/Analysis/InlineCost.cpp
1217–1219

If the normal destination of the invoke or the parent block of the call site is unreachable-terminated, ...

junbuml updated this revision to Diff 46135.Jan 27 2016, 7:33 AM
junbuml retitled this revision from Avoid inlining CallSites in unreachable-terminated block to Avoid inlining call sites in unreachable-terminated block.
junbuml updated this object.
junbuml edited edge metadata.

Addressed David's comment.

mcrosier edited edge metadata.Jan 28 2016, 10:27 AM

LGTM, but perhaps @davidxl or @eraman should provide the official approval.

eraman edited edge metadata.Jan 28 2016, 11:20 AM

The only concern I have is the suppression of inlining of hot calls in the following hypothetical example:

main() {

hot_call_1();
...
hot_call_N()
exit(0);

}

Even though each of the hot_call_X calls are executed once, inlining may be beneficial (due to constprop for example). Short of checking for caller to be main (as you've done in your previous patch), is there a way to distinguish this case? I suppose this pattern is very rare in real code to worry about this.

Thanks Easwaran for the comment.
I can see that the case you mention could happen with this change. Although I tried to come up with a workaround without BPI, nothing seems to show desirable results in my test environment. To cover this case, I may want to wait until BPI is hooked in inliner and revisit this to elaborate it by checking the actual coldness of the block. So, in future, we may only suppress inlining a call site in a block that is leading to an unreachable and dominated by a block of which all edges from predecessors have zero branch probability.

I mentioned this in another reply. This scenario is rare so this patch
is still good in general. If we want to handle it in the future, this
heuristics needs to be moved up and can be followed by other threshold
adjusting heuristics -- but that should not be required for this
patch.

David

junbuml updated this revision to Diff 46541.Feb 1 2016, 8:33 AM
junbuml added a reviewer: dblaikie.
junbuml added a subscriber: dblaikie.

Thanks David for the comment.
Based on @davidxl's comment, I added FIXME to describe the corner case mention by @dblaikie and @eraman. I will be happy to hear any opinion or suggestion in this change.

I mentioned this in another reply. This scenario is rare so this patch
is still good in general. If we want to handle it in the future, this
heuristics needs to be moved up and can be followed by other threshold
adjusting heuristics -- but that should not be required for this
patch.

David

Sounds reasonable. I also don't expect this kind of code to appear in real life, but one can imagine microbenchmarks or toy programs to have this pattern.

eraman accepted this revision.Feb 1 2016, 11:54 AM
eraman edited edge metadata.

LGTM

This revision is now accepted and ready to land.Feb 1 2016, 11:54 AM
This revision was automatically updated to reflect the committed changes.