Skip to content

Don't force unused reg arg to the stack. #44555

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.

Sign up for GitHub

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

Jump to bottom
Merged
CarolEidt merged 3 commits into from
Nov 13, 2020
Merged

Don't force unused reg arg to the stack. #44555

CarolEidt merged 3 commits into from
Nov 13, 2020

Conversation

@CarolEidt
Copy link
Contributor

@CarolEidt CarolEidt commented Nov 11, 2020

No description provided.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 11, 2020
@CarolEidt
Copy link
Contributor Author

CarolEidt commented Nov 12, 2020

@dotnet/jit-contrib PTAL
This is one of those "I can't believe I haven't noticed this before" issues. I found it while working on #43870.
When we had an unused incoming register parameter, we would choose not to allocate a register for it, but that meant that it was uselessly stored to the stack - adding both code and frame size.
Instead, we want to "allocate" a register for it, i.e. the one it's in, and because it's a "last use" it will then be freed.
This only impacts "independently promoted" structs; unused scalar args in registers are not considered candidates. (It might be better, in the long run, to "depromote" unused structs, but this is a cheap and straightforward way to handle them for now).
Here are the diffs. No diffs for x86 (no structs passed in regs) or for arm32 (presumably no unused HFAs):

Arch OS What Delta Methods Improved Methods Regressed
Arm64 Windows Crossgen fx+benchmarks -1448 (-0.00%) 261 0
x64 windows Crossgen fx+benchmarks -11519 (-0.03%) 1486 0
x64 Linux Crossgen fx+benchmarks -2487 (-0.01%) 256 0

sandreenko
sandreenko approved these changes Nov 12, 2020
View reviewed changes
Copy link
Contributor

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few nits.

src/coreclr/src/jit/lsra.cpp Outdated
}
else if (refType == RefTypeParamDef && varDsc->lvRefCntWtd() <= BB_UNITY_WEIGHT)
{
else if (refType == RefTypeParamDef &&
Copy link
Contributor

@sandreenko sandreenko Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: would it be correct to write the condition as

            else if ((refType == RefTypeParamDef) && (varDsc->lvRefCntWtd() <= BB_UNITY_WEIGHT) &&
                     (!currentRefPosition->lastUse || (currentInterval->physReg == REG_STK)))

so it takes 2 lines instead of 3?

src/coreclr/src/jit/lsra.cpp Outdated
(!currentRefPosition->lastUse || (currentInterval->physReg == REG_STK)) &&
varDsc->lvRefCntWtd() <= BB_UNITY_WEIGHT)
{
// If this is a low ref-count parameter, and either it is *not* a last used (i.e. unused) or it's
Copy link
Contributor

@sandreenko sandreenko Nov 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It was not clear for me if not was applicable to the word in the brackets: it is *not* a last used (i.e. unused) should be read as it is not unused or it is unused? From the context, it looks like the first but maybe then we can delete double negation?

Suggested change
// If this is a low ref-count parameter, and either it is *not* a last used (i.e. unused) or it's
// If this is a low ref-count parameter, and either it is used (def is not the last use) or it's

@CarolEidt
Copy link
Contributor Author

CarolEidt commented Nov 13, 2020

Thanks for the review @sandreenko!

@CarolEidt CarolEidt merged commit fcce160 into dotnet:master Nov 13, 2020
@CarolEidt CarolEidt deleted the EnregUnusedArg branch November 19, 2020 18:48
@ghost ghost locked as resolved and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@sandreenko sandreenko sandreenko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@CarolEidt @sandreenko @Dotnet-GitSync-Bot