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

Inliner uses capacity of locals table instead of count #69280

Open
jakobbotsch opened this issue May 12, 2022 · 4 comments
Open

Inliner uses capacity of locals table instead of count #69280

jakobbotsch opened this issue May 12, 2022 · 4 comments
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@jakobbotsch
Copy link
Member

jakobbotsch commented May 12, 2022

I noticed in #69117 (comment) that we are using lvaTableCnt instead of lvaCount here:

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);
}

That seems strange and might cause significantly different inlining decisions just by adding or removing very few locals, depending on whether we grow the table one more time or not.

category:correctness
theme:inlining
skill-level:intermediate
cost:medium
impact:medium

@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 12, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@jakobbotsch jakobbotsch added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 12, 2022
@ghost
Copy link

ghost commented May 12, 2022

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

Issue Details

I noticed in #69117 (comment) that we are using lvaTableCnt instead of lvaCount here:

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);
}

That seems strange and might cause significantly different inlining decisions just by adding or removing very few locals, depending on whether we grow the table one more time or not.

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr, untriaged

Milestone: -

@jakobbotsch jakobbotsch added this to the 7.0.0 milestone May 12, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label May 12, 2022
@AndyAyersMS
Copy link
Member

@EgorBo please take a look once you're back.

@EgorBo
Copy link
Member

EgorBo commented Jun 20, 2022

Decided to postpone this to 8.0 where I plan more changes for the inliner. I tried this locally and it was too impactful

@EgorBo EgorBo modified the milestones: 7.0.0, 8.0.0 Jun 20, 2022
@EgorBo EgorBo modified the milestones: 8.0.0, Future Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

No branches or pull requests

3 participants