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

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

Merged
merged 3 commits into from
Nov 13, 2020

Conversation

CarolEidt
Copy link
Contributor

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

@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

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.

@@ -5596,8 +5596,14 @@ void LinearScan::allocateRegisters()
{
allocate = false;
}
else if (refType == RefTypeParamDef && varDsc->lvRefCntWtd() <= BB_UNITY_WEIGHT)
{
else if (refType == RefTypeParamDef &&
Copy link
Contributor

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?

(!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

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

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.
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