-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Conversation
@dotnet/jit-contrib PTAL
|
There was a problem hiding this 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
@@ -5596,8 +5596,14 @@ void LinearScan::allocateRegisters() | |||
{ | |||
allocate = false; | |||
} | |||
else if (refType == RefTypeParamDef && varDsc->lvRefCntWtd() <= BB_UNITY_WEIGHT) | |||
{ | |||
else if (refType == RefTypeParamDef && |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
// 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 |
Thanks for the review @sandreenko! |
No description provided.