-
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
Fix no struct promotion condition for long vars on 32bit platforms. #53067
Conversation
PTAL @dotnet/jit-contrib small cleaning change that came from another work item. |
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
src/coreclr/jit/jitconfigvalues.h
Outdated
@@ -116,7 +116,8 @@ CONFIG_INTEGER(JitNoHoist, W("JitNoHoist"), 0) | |||
CONFIG_INTEGER(JitNoInline, W("JitNoInline"), 0) // Disables inlining of all methods | |||
CONFIG_INTEGER(JitNoMemoryBarriers, W("JitNoMemoryBarriers"), 0) // If 1, don't generate memory barriers | |||
CONFIG_INTEGER(JitNoRegLoc, W("JitNoRegLoc"), 0) | |||
CONFIG_INTEGER(JitNoStructPromotion, W("JitNoStructPromotion"), 0) // Disables struct promotion in Jit32 | |||
CONFIG_INTEGER(JitNoStructPromotion, W("JitNoStructPromotion"), 0) // Disables struct promotion &1 - for all, &2 - for |
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: What does &1
and &2
mean?
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.
I do not know how to describe it, it means
if ((JitNoStructPromotion & 1) != 0) fgNoStructPromotion = true
if ((JitNoStructPromotion & 2) != 0) fgNoStructParamPromotion= true
we use a similar "hash" approach for JitStress and JitMinOpts but they don't have such comments.
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.
I see. Why not just pass JitNoStructPromotion=1
, etc. and then check for if (JitNoStructPromotion == 1) fgNoStructPromotion = true
and so forth?
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
The condition was
(fgNoStructPromotion && varDsc->lvIsParam)
when it should befgNoStructPromotion || (fgNoStructParamPromotion && varDsc->lvIsParam))
.While I am here also move
lvaPromoteLongVars
toDecomposeLongs
and add a commentJitNoStructPromotion
.There are tiny diffs in spmi tests on x86 from tests because we disable struct promotions in 2 cases:
runtime/src/coreclr/jit/importer.cpp
Lines 16474 to 16476 in b18be1e
and
runtime/src/coreclr/jit/importer.cpp
Lines 12130 to 12135 in b18be1e
and now, in these cases, we don't promote longs.