Page MenuHomePhabricator

[InlineCost] Don't return early when allowSizeGrowth(CS) is false
Needs ReviewPublic

Authored by paquette on Mar 13 2018, 4:28 PM.



This patch concerns chains of calls like so:


foo() {

wibble() {

Suppose foo has internal linkage. Also let's say that the call to foo within wibble is the only call to foo within the module.

Normally, the inlining cost model will apply a large bonus to ensure that foo is inlined into wibble because there's basically no cost in doing so. However, if wibble is unreachable-terminated, this won't happen. This is because the cost model currently does the following:

  1. Check if the block containing the call is unreachable-terminated (allowSizeGrowth)
  2. If it is, set the threshold to 0 and return

This happens before the bonus is applied. Therefore, any "zero-cost" case relying on the bonus won't ever be inlined when we're dealing with unreachable-terminated blocks.

This commit

  • Removes the early return when allowSizeGrowth is false
  • Wraps the threshold tweaks in a conditional which is true only when size growth is allowed.

The tweaks are wrapped in a conditional to reflect that we only want to inline when the cost of inlining is truly 0 or better; any modifications to the threshold would break this assertion. The early return is removed to facilitate inlining the example case.

This produced some minor code size improvements for ARM, AArch64, and x86-64 at Oz.

Output from here for Oz:

Edit: More clarity. The word salad from before wasn't that great.

Diff Detail

Event Timeline

paquette created this revision.Mar 13 2018, 4:28 PM
paquette edited the summary of this revision. (Show Details)Mar 14 2018, 10:04 AM
eraman added inline comments.Mar 14 2018, 10:17 AM

Move the code applying last-call-to-static bonus to the top. Then, you could early exit after setting the threshold to 0 under the !allowSizeGrowth condition.

paquette updated this revision to Diff 138429.Mar 14 2018, 11:50 AM

Update patch to move the LastCallToStaticBonus logic to the top of updateThreshold.

fhahn added inline comments.Mar 15 2018, 8:26 AM

The changes after this point do not seem to be necessary, i.e. could they stay at the same place?

fhahn added a comment.Jan 11 2019, 9:17 AM

Jessica, are you still planing on pushing this change? :)

Wow, I entirely forgot about this somehow.

I'll take a look and see if it's still relevant...