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

Fix no struct promotion condition for long vars on 32bit platforms. #53067

Merged
merged 3 commits into from
May 26, 2021

Conversation

sandreenko
Copy link
Contributor

The condition was (fgNoStructPromotion && varDsc->lvIsParam) when it should be fgNoStructPromotion || (fgNoStructParamPromotion && varDsc->lvIsParam)).

While I am here also move lvaPromoteLongVars to DecomposeLongs and add a comment JitNoStructPromotion.

There are tiny diffs in spmi tests on x86 from tests because we disable struct promotions in 2 cases:

// Being lazy here. Refanys are tricky in terms of gc tracking.
// Since it is uncommon, just don't perform struct promotion in any method that contains mkrefany.

and
/* Set this flag to make sure register arguments have a location assigned
* even if we don't use them inside the method */
compJmpOpUsed = true;
fgNoStructPromotion = true;

and now, in these cases, we don't promote longs.

@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 21, 2021
@sandreenko
Copy link
Contributor Author

PTAL @dotnet/jit-contrib small cleaning change that came from another work item.

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -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
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

Lgtm

@sandreenko sandreenko merged commit d2072e2 into dotnet:main May 26, 2021
@sandreenko sandreenko deleted the fixNoPromoCondition branch May 26, 2021 02:37
@ghost ghost locked as resolved and limited conversation to collaborators Jun 25, 2021
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.

2 participants