[Inliner] Do not apply any bonus for cold callsites.
ClosedPublic

Authored by eraman on Jul 24 2017, 4:38 PM.

Details

Summary

Inlining cold callsites into a not-cold caller could prevent further
inlining of the caller.

Diff Detail

Repository
rL LLVM
eraman created this revision.Jul 24 2017, 4:38 PM

This change makes lots of sense to me.... However, the change description should really spell out more details about what's going on. You should assume that some of the readers of the change description don't know (or have forgotten) some of the context in your head about why this is all important and how it fits together.

I'm guessing that far and away the biggest difference is the last-call bonus? I suspect it is worth calling out that specific case.

Does function-attrs mark these as coldcc so that we use a cheaper calling convention for them? If not, we should do that. We should also discount the cost of calling them when computing other functions' inline cost. Probably just worth a FIXME comment here or a mention in the description as future work.

lib/Analysis/InlineCost.cpp
712 ↗(On Diff #107994)

Would it make more sense to just update the bonuses to reflect what they should be for cold call sites?

I mostly find the boolean interaction with all the bonuses very brittle and easy got get wrong. Are there other ways of structuring this?

If not, I might set the boolean based on semantics rather than logic -- specifically that this is a *cold* call edge. That will make it more obvious why we would skip bonuses below for that case.

1328–1329 ↗(On Diff #107994)

However you end up changing around the logic above, you should update the comments here to make it *really clear* what tradeoff you're making with this code as it is different now.

Notably, we have a bool whose only purpose is to organize the predicates going into this if, and now we have a new predicate that is not in the bool... which I think makes sense, but is a little odd to be omitted from the comment.

Does function-attrs mark these as coldcc so that we use a cheaper calling convention for them?

No. In InlineCost, calls with coldcc get a high penalty (2000). So marking them as coldcc in function-attrs (which comes before inlining) will have the effect of not inlining them.

If not, we should do that. We should also discount the cost of calling them when computing other functions' inline cost.

It's not clear to me why we should do this. Is it because coldcc calls are cheaper (or even free)?
>Probably just worth a FIXME comment here or a mention in the description as future work.

lib/Analysis/InlineCost.cpp
712 ↗(On Diff #107994)

I've attempted to do this by keeping the bonus percentages as variables that are reset when the callsite is cold.

1328–1329 ↗(On Diff #107994)

I've added comments saying why we disable this even if it might reduce code size. Let me know if you are expecting something more.

eraman updated this revision to Diff 108157.Jul 25 2017, 2:13 PM

Address Chandler's comments.

eraman updated this revision to Diff 108706.Jul 28 2017, 1:52 PM

Replace the two vector bonus variables by one.

The smaller vector bonus is half of the larger one, so I've used just one VectorBonus variable.

chandlerc accepted this revision.Jul 28 2017, 1:59 PM

LGTM, thanks!

This revision is now accepted and ready to land.Jul 28 2017, 1:59 PM
This revision was automatically updated to reflect the committed changes.