Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: fixes for mixed PGO/nonPGO compiles #50633

Merged
merged 3 commits into from
Apr 4, 2021

Conversation

AndyAyersMS
Copy link
Member

Always scale inlinee counts, independent of the pgo status of the
call site and inlinee.

Get rid of setBBWeight and modifyBBWeight as they are attractive
nuisances that lead to bad habits. Use inherit and scale instead as
new counts should always be based on old.

Account for cases where there are methods in the inline tree with PGO
data but the root method doesn't have PGO, and vice versa.

Always scale inlinee counts, independent of the pgo status of the
call site and inlinee.

Get rid of `setBBWeight` and `modifyBBWeight` as they are attractive
nuisances that lead to bad habits. Use `inherit` and `scale` instead as
new counts should always be based on old.

Account for cases where there are methods in the inline tree with PGO
data but the root method doesn't have PGO, and vice versa.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 2, 2021
@AndyAyersMS
Copy link
Member Author

cc @dotnet/jit-contrib

Leads to a fair number of diffs, will try and summarize later.

@AndyAyersMS
Copy link
Member Author

Still tweaking this, but here's where things stand right now.

Generally smaller code (note this is with SPMI -- many more methods had no diffs). Mainly smaller because we're now often scaling down PGO counts when we inline a PGO method into a non-PGO method, and this reduces knock-on inlines.

collection diff pct changed improved regressed unchanged
aspnet.run -407 -0.03% 1291 670 621 301
benchmarks.run -3848 -0.32% 424 283 141 536
libraries.crossgen -8349 -0.37% 1621 1045 576 1030
libraries.crossgen2 -4972 -0.25% 1332 972 360 1087
libraries.pmi -5366 -0.11% 2308 1709 599 2527
tests.pmi 328 0.12% 158 54 104 106

@AndyAyersMS
Copy link
Member Author

Updated stats (clears up some of the bigger size increases in asp.net, where we had inlinees that should have scaled to zero not get scaled, and thus enabling things like GDV which we shouldn't have bothered with).

I'm not thrilled with summarily setting inlinee counts to zero if the call site count is zero, but for now it's the best we can do (note we will still inline at such sites because always inlines and force inlines currently bypass all profitability checking).

collection diff pct changed improved regressed unchanged
aspnet.run -6343 -0.39% 1314 708 606 300
benchmarks.run -3846 -0.32% 426 284 142 535
libraries.crossgen -8529 -0.37% 1700 1089 611 1032
libraries.crossgen2 -4980 -0.25% 1333 973 360 1087
libraries.pmi -5167 -0.11% 2318 1713 605 2525
tests.pmi 328 0.12% 158 54 104 106

@EgorBo
Copy link
Member

EgorBo commented Apr 2, 2021

I'm not thrilled with summarily setting inlinee counts to zero if the call site count is zero, but for now it's the best we can do

I guess it means if the profile for callsite's block is wrong - we're also losing usefull profile of the inlinee?

@AndyAyersMS
Copy link
Member Author

I guess it means if the profile for callsite's block is wrong - we're also losing usefull profile of the inlinee?

Yes. Profile data can be "wrong" in several different ways.

  1. we might have bugs in our processing that set block counts to zero when they should be nonzero
  2. we might be collecting on too narrow of a scenario

Hopefully we find and fix most of the first kind of problem.

For the second kind we need to be careful not to "overfit" to the profile data we have and assume all other behavior is unlikely. The proposal for that is to also run synthesis in tandem, it will give a less extreme view of what is possible at runtime, and then blend the profile data and the synthesized data together. Thus only if both systems agree a block is rare will it end up being rare.

@AndyAyersMS
Copy link
Member Author

I think what's here is a clear improvement, in particular in the ASP net collection I now see more sensible block orderings and such. There are a number of things still to clean up but this should get us headed in the right direction.

There is one prominent source of diffs for non-pgo, which is that we no longer down-weight internal blocks of inlinees (since in almost all cases we compute a scale and the inlinee blocks have equal weight). The decision to do so was somewhat arbitrary. At some point we'll get around to running synthesis and make up new numbers anyways.

That also leads to a lot of the "unchanged" cases in the results above, as the more consistent inlinee weights lead to less IG breaking later on as block weights are somewhat more uniform (recall we now break IGs if block weights change).

@AndyAyersMS
Copy link
Member Author

@dotnet/jit-contrib I would like to get this in in time for some of the weekend runs, so if anyone has time to review...

@AndyAyersMS AndyAyersMS merged commit 245cd97 into dotnet:main Apr 4, 2021
@AndyAyersMS AndyAyersMS deleted the PgoNoPgoFixes branch April 4, 2021 18:18
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this pull request Apr 6, 2021
Changes made in dotnet#50633 were simply wrong, and lead to division by
infinite values.

Fixes dotnet#50743.
AndyAyersMS added a commit that referenced this pull request Apr 9, 2021
Changes made in #50633 were simply wrong, and lead to division by
infinite values.

Fixes #50743.
@AndyAyersMS AndyAyersMS mentioned this pull request Apr 12, 2021
54 tasks
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Apr 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants