Invoke GetInlineCost for legality check before inline functions in SampleProfileLoader.
ClosedPublic

Authored by danielcdh on Sep 12 2017, 4:27 PM.

Details

Summary

SampleProfileLoader inlines hot functions if it is inlined in the profiled binary. However, the inline needs to be guarded by legality check, otherwise it could lead to correctness issues.

danielcdh created this revision.Sep 12 2017, 4:27 PM
eraman added inline comments.Sep 12 2017, 4:42 PM
lib/Transforms/IPO/SampleProfile.cpp
89

Do you really need an option for this? All you want is to pick a large enough number such that the cost never exceeds the threshold.

760

This needs detailed comments. State that you want to check if there is anything in the reachable portion of the callee at this callsite that makes this inlining potentially illegal and that's why you use a large threshold (large enough to return true in ~all cases except when it is potentially incorrect)

761

Differentiate the InlineCost::isNever and the case where cost exceeds threshold. It is useful to log the latter case for debugging.

danielcdh updated this revision to Diff 114941.Sep 12 2017, 4:57 PM
danielcdh marked 3 inline comments as done.

update

eraman accepted this revision.Sep 12 2017, 5:36 PM

LGTM

This revision is now accepted and ready to land.Sep 12 2017, 5:36 PM

Discussed this offline with Dehao. Instead of using an arbitrary threshold of 100000, it is better to piggyback on the the option in InlineCost to compute cost analysis in full. That way, if the analysis returns NeverInlineCost, Sample loader shouldn't inline. Anything else (doesn't matter if the cost >= threshold), it can do the inlining. I will refactor InlineCost.cpp to enable the feature to be used without requiring an option and Dehao will base this patch on top of that.

eraman accepted this revision.Sep 13 2017, 2:21 PM

LGTM

lib/Transforms/IPO/SampleProfile.cpp
763–778

Nit: I would use Params instead (or IP.)

768

Extend the comment to say that the actual threshold doesn't matter since you only check for isNever.

danielcdh closed this revision.Sep 13 2017, 2:24 PM
vitalybuka reopened this revision.Sep 13 2017, 10:44 PM
vitalybuka added a subscriber: vitalybuka.

There is a uninitialized value, so I've reverted the patch with r313230 to fix bots.
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/7858/steps/check-llvm%20msan/logs/stdio

This revision is now accepted and ready to land.Sep 13 2017, 10:44 PM

fix the msan uninitialized memory access error.

danielcdh closed this revision.Sep 14 2017, 10:31 AM