-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
JIT: Remove a quirk in regular promotion #88130
Conversation
Also fix a bug in LSRA: if we undo promotion of one field of a multi-reg struct and as a result undo for the other fields as well, we must also clear out the largeVectorVars bitset to indicate these are no longer candidates.
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsAlso fix a bug in LSRA: if we undo promotion of one field of a multi-reg struct and as a result undo for the other fields as well, we must also clear out the largeVectorVars bitset to indicate these are no longer candidates.
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr outerloop |
Azure Pipelines successfully started running 3 pipeline(s). |
cc @dotnet/jit-contrib PTAL @kunalspathak Some decent diffs (mostly in tests, but a few KB in other collections too). |
Ping @kunalspathak, can you please take a look at this simple change? It will likely end up blocking #88238 otherwise. |
@@ -1922,6 +1922,9 @@ void LinearScan::identifyCandidates() | |||
fieldVarDsc->lvLRACandidate = 0; | |||
localVarIntervals[fieldVarDsc->lvVarIndex] = nullptr; | |||
VarSetOps::RemoveElemD(compiler, registerCandidateVars, fieldVarDsc->lvVarIndex); | |||
#if FEATURE_PARTIAL_SIMD_CALLEE_SAVE | |||
VarSetOps::RemoveElemD(compiler, largeVectorVars, fieldVarDsc->lvVarIndex); |
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.
Not sure if it makes sense to check if fieldVarDsc->lvVarIndex
was part of largeVectorVars
to begin with.
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.
Hmm, I think just clearing it unconditionally should be ok.
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
Also fix a bug in LSRA: if we undo promotion of one field of a multi-reg struct and as a result undo for the other fields as well, we must also clear out the largeVectorVars bitset to indicate these are no longer candidates. This was hitting crashes with some of the new promotions done by this change.