Invoke GetInlineCost for legality check before inline functions in SampleProfileLoader.

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



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

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.


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)


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.


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


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



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


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.

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