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

Less aggressive stfld spilling #71663

Merged
merged 2 commits into from
Jul 8, 2022
Merged

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Jul 5, 2022

The spilling logic for st[s]fld was quite quite complex, and not complete. Replace it with the simple spilling of side effects and global data accesses, which is all that's needed.

The new logic is less conservative because it doesn't spill invariant nodes when the assign is to a struct field, so we see a good number of positive diffs.

Fixes #71638.

Diffs.

@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 Jul 5, 2022
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 5, 2022
@ghost
Copy link

ghost commented Jul 5, 2022

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Using the CI to see what breaks.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member

/azp run Antigen, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@SingleAccretion SingleAccretion marked this pull request as ready for review July 7, 2022 13:12
@SingleAccretion
Copy link
Contributor Author

Fuzzer failures look preexisting.

@dotnet/jit-contrib

@AndyAyersMS
Copy link
Member

Can you look into some of the larger diffs? eg x64 win libraries pmi:

Top method regressions (percentages):
         266 (30.09 % of base) : 242426.dasm - ILCompiler.DependencyAnalysis.ReadyToRun.ImportThunk:EmitCode(ILCompiler.DependencyAnalysis.NodeFactory,byref,bool):this

@SingleAccretion
Copy link
Contributor Author

The larger regressions are due to us inlining more as we now have fewer temps.

@AndyAyersMS
Copy link
Member

The larger regressions are due to us inlining more as we now have fewer temps.

I can see that for some of the larger methods, but the one above is fairly small.

@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Jul 7, 2022

the one above is fairly small.

Hmm, for ILCompiler.DependencyAnalysis.ReadyToRun.ImportThunk:EmitCode specifically, I am pretty sure it's the inlining heuristic:

./diff-dasm 242426

 ; rsp based frame
 ; partially interruptible
 ; No matching PGO data
-; 0 inlinees with PGO data; 25 single block inlinees; 17 inlinees without PGO data
+; 0 inlinees with PGO data; 30 single block inlinees; 21 inlinees without PGO data
 ; Final local variable assignments

However, the other methods from the list do not have inlining diffs. Will dig deeper, though I suspect these are the usual consequences of reducing/altering the number and uses of temps.

Edit: curiously enough, local SPMI for Windows x64 specifically is visibly nicer, suggesting that there has been a change between the my fork and upstream which affects the numbers...

@AndyAyersMS
Copy link
Member

How many temps do we end up with in that case? I though we only backed off inlining once we got near the tracking limit.

@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Jul 8, 2022

How many temps do we end up with in that case?

Overall in the diff (i. e. with the new inlinees) we have only 83 locals.

I though we only backed off inlining once we got near the tracking limit.

I think it's this heuristic that's responsible:

// Slow down if there are already too many locals
if (m_RootCompiler->lvaTableCnt > 64)
{
// E.g. MaxLocalsToTrack = 1024 and lvaTableCnt = 512 -> multiplier *= 0.5;
const double lclFullness = min(1.0, (double)m_RootCompiler->lvaTableCnt / JitConfig.JitMaxLocalsToTrack());
multiplier *= (1.0 - lclFullness);
JITDUMP("\nCaller has %d locals. Multiplier decreased to %g.", m_RootCompiler->lvaTableCnt, multiplier);
}

@AndyAyersMS
Copy link
Member

I think it's this heuristic that's responsible:

Ah, right. Thanks for investigating.

@AndyAyersMS AndyAyersMS merged commit 77e7dda into dotnet:main Jul 8, 2022
@SingleAccretion SingleAccretion deleted the Stfld-Spilling branch July 8, 2022 18:40
@ghost ghost locked as resolved and limited conversation to collaborators Aug 7, 2022
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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Importer's spilling logic for stfld is not correct
3 participants