[PATCH] D121084: [NewPM][Inliner] Make inlined calls to functions in same SCC as callee exponentially expensive

Mon Mar 7 15:23:40 PST 2022

aeubanks added a comment.

In D121084#3365306 <https://reviews.llvm.org/D121084#3365306>, @nikic wrote:

> In D121084#3365187 <https://reviews.llvm.org/D121084#3365187>, @aeubanks wrote:
>
>> I did see a consistent 0.75-1% regression on an important internal benchmark with the other approach (which I believe we actually care about), although some benchmarks are sensitive to alignment.
>
> And the regressions is not present with this variant? If so, yeah, let's go with this.

I haven't tested this variant yet and the benchmarks take hours to run. I'm going to start it now, but given that 14.0.0 is tomorrow I'll submit this tonight if the benchmark isn't done by then and request a cherry-pick since davidxl is happy with this and it helps with many of the worst compile time issues. If this does turn out to be similar to the noinline patch in terms of performance then we can do something with the noinline patch.

================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:971
+                  CalleeSCC == CG.lookupSCC(CG.get(*NewCallee))) {
+                Attribute NewCBCostMult = Attribute::get(
+                    M.getContext(),
----------------
nikic wrote:
> Should we also fetch the previous multiplier of ICB here and multiply it in? Not sure if it's really relevant in practice, but currently we're only preserving the existing multiplier from CB, but not ICB.
if we had 

a -> b <-> c

z -> a <-> d

where both the a -> b and z -> a inlinings were problematic, then multiplying the value in ICB could help, else we'd restart the multipliers

as you said, probably very unlikely, we can do that in a follow-up change if we think it's important

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121084/new/

https://reviews.llvm.org/D121084