Page MenuHomePhabricator

[InlineCost] cleanup calculations of Cost and Threshold
ClosedPublic

Authored by fedor.sergeev on Apr 15 2019, 5:16 PM.

Details

Summary

Doing better separation of Cost and Threshold.
Cost counts the abstract complexity of live instructions, while Threshold is an upper bound of complexity that inlining is comfortable to pay.
There are two parts:

  • huge 15K last-call-to-static bonus is no longer subtracted from Cost but rather is now added to Threshold.

    That makes much more sense, as the cost of inlining (Cost) is not changed by the fact that internal function is called once. It only changes the likelyhood of this inlining being profitable (Threshold).
  • bonus for calls proved-to-be-inlinable into callee is no longer subtracted from Cost but added to Threshold instead.

While calculations are somewhat different, overall InlineResult should stay the same since Cost >= Threshold compares the same.

Diff Detail

Repository
rL LLVM

Event Timeline

fedor.sergeev created this revision.Apr 15 2019, 5:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2019, 5:16 PM

I would split this change into NFC and last-call-to-static.

lib/Analysis/InlineCost.cpp
295 ↗(On Diff #195275)

Cost is int, why do we use int64_t? May be UpperBound should be int?

316 ↗(On Diff #195275)

may be all const initialization can be moved to field declarations like you do with Cost, VectorBonus, ...

fedor.sergeev marked an inline comment as done.Apr 15 2019, 9:45 PM
fedor.sergeev added inline comments.
lib/Analysis/InlineCost.cpp
295 ↗(On Diff #195275)

This is inspired by the original visitSwitchInst usages, where calculation specifically used std::min to avoid int32 overflows.
I decided that it should be ok to use the same sequence elsewhere.

moving NFC part out of this change

fedor.sergeev marked an inline comment as done.
fedor.sergeev edited the summary of this revision. (Show Details)
fedor.sergeev edited the summary of this revision. (Show Details)
fedor.sergeev edited the summary of this revision. (Show Details)Apr 16 2019, 3:12 AM
fedor.sergeev edited the summary of this revision. (Show Details)
apilipenko added inline comments.Apr 26 2019, 6:21 PM
lib/Analysis/InlineCost.cpp
162–167 ↗(On Diff #195296)

I don't think we need to store it here. Other bonuses, like VectorBonus, are "speculative", i.e. we apply them first and revoke if the conditions don't hold.

258–263 ↗(On Diff #195296)

If you don't store it in a field, you don't need a setter function.

fedor.sergeev edited the summary of this revision. (Show Details)

removing InlineCallBonus

fedor.sergeev edited the summary of this revision. (Show Details)Apr 27 2019, 11:33 AM
apilipenko accepted this revision.Apr 28 2019, 10:54 PM
This revision is now accepted and ready to land.Apr 28 2019, 10:54 PM
This revision was automatically updated to reflect the committed changes.
chandlerc added inline comments.Jul 2 2019, 8:03 PM
llvm/trunk/lib/Analysis/InlineCost.cpp
983–984

The change doesn't say anything about how it addresses this last sentence in the comment that seems to be indicating that the decision to update cost (instead of threshold) was not made arbitrarily.

How did you verify that this doesn't in fact change the way bonuses end up being applied?

Pointing out the (serious) bug in this change below.

llvm/trunk/lib/Analysis/InlineCost.cpp
1337

I think this is the source of the (huge) behavior change.

We reduced the cost to apply some forms of bonus because it only decreases the *cost* of inlining, but doesn't increase the *benefit* of inlining. Now that is reversed.

So in this case, if the inner call is to a function where this is the last call, the threshold is now *HUGE*, and the cost is small. We take that huge threshold, and apply it blindly to our threshold here.

This will cause us to inline waaay too much along call stacks that (eventually) have an reachable, inlinable call to function which is the last call to that function.

Thanks for the explanation. Temporarily reverted in r365000.

Thanks for the explanation. Temporarily reverted in r365000.

Thanks for @chandlerc doing the analysis and @rupprecht doing a revert!
I will surely get to addressing this issue now.