Skip to content

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

Merged
CarolEidt merged 3 commits into
dotnet:masterfrom
CarolEidt:EnregUnusedArg
Nov 13, 2020
Merged

Don't force unused reg arg to the stack.#44555
CarolEidt merged 3 commits into
dotnet:masterfrom
CarolEidt:EnregUnusedArg

Conversation

@CarolEidt

Copy link
Copy Markdown
Contributor

No description provided.

@Dotnet-GitSync-BotDotnet-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
Copy Markdown
ContributorAuthor

@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):

ArchOSWhatDeltaMethods ImprovedMethods Regressed
Arm64WindowsCrossgen fx+benchmarks-1448 (-0.00%)2610
x64windowsCrossgen fx+benchmarks-11519 (-0.03%)14860
x64LinuxCrossgen fx+benchmarks-2487 (-0.01%)2560

@sandreenkosandreenko left a comment

Copy link
Copy Markdown
Contributor

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.

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

Copy link
Copy Markdown
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?

Comment threadsrc/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

@sandreenkosandreenkoNov 12, 2020

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

Thanks for the review @sandreenko!

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

Labels

area-CodeGen-coreclrCLR 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

@CarolEidt@sandreenko@Dotnet-GitSync-Bot