-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Use emitLastGroup that works for single/multi align instructions #116843
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
Use emitLastGroup that works for single/multi align instructions #116843
Conversation
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.
Pull Request Overview
This PR fixes an issue where the incorrect align instruction pointer (emitAlignLast) was used for arm64, replacing it with emitAlignLastGroup to better handle both single and multi align instructions.
- Updated condition and assignment to use emitAlignLastGroup instead of emitAlignLast
- Addresses the bug reported in #114569
Comments suppressed due to low confidence (2)
src/coreclr/jit/emit.cpp:2869
- Consider adding or updating tests to verify that the use of emitAlignLastGroup properly updates the loop head prediction for both single and multi align scenarios, particularly ensuring correct behavior on arm64.
emitAlignLastGroup->idaLoopHeadPredIG = emitCurIG;
src/coreclr/jit/emit.cpp:2859
- The condition updated to use emitAlignLastGroup correctly reflects the intended behavior for arm64. Ensure that this change is consistently applied in all align instruction updates if similar logic exists elsewhere in the code.
if (!currIGWasNonEmpty && (emitAlignLastGroup != nullptr) &&
@dotnet/jit-contrib @AndyAyersMS PTAL |
/azp runtime-coreclr superpmi-replay |
Command 'runtime-coreclr' is not supported by Azure Pipelines. Supported commands
See additional documentation. |
/azp run runtime-coreclr superpmi-replay |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Looks like CI is being uncooperative, though superpmi-diffs is clean. @kunalspathak do you know why we started seeing this failure all of a sudden? |
This is another case of stressing of block layout where the backedges are spread, enclosing other loops that are alignment candidate. |
yeah, we cannot trigger this anymore. I even tried to do it manually but didn't see a way to do it. |
@amanasifkhalid - anything else needed for this PR? |
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.
No, LGTM
{ | ||
// If the emitCurIG was thought to be a loop-head, but if it didn't turn out that way and we end up | ||
// creating a new IG from which the loop starts, make sure to update the LoopHeadPred of last align | ||
// instruction emitted. This will guarantee that the information stays up-to-date. Later if we | ||
// notice a loop that encloses another loop, this information helps in removing the align field from | ||
// such loops. | ||
// We need to only update emitAlignLast because we do not align intermingled or overlapping loops. | ||
emitAlignLast->idaLoopHeadPredIG = emitCurIG; | ||
emitAlignLastGroup->idaLoopHeadPredIG = emitCurIG; |
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.
You should update the last line in the comment just above.
Does emitAlignLast
need to be reset too?
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.
Thought I had submitted this yesterday, sorry for not getting to you sooner.
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.
You should update the last line in the comment just above.
sent out #116869
Does
emitAlignLast
need to be reset too?
Not needed, emitAlignLast
and emitAlignLastGroup
should point to the same object (for xarch). It is only different for arm, and we were incorrectly referring to the last align instruction instead of the first one of the 4-align instructions we insert.
runtime/src/coreclr/jit/emit.cpp
Lines 5738 to 5741 in 296780d
// Since we never align overlapping instructions, it is always guaranteed that | |
// the emitAlignLastGroup points to the loop that is in process of getting aligned. | |
emitAlignLastGroup->idaLoopHeadPredIG = emitCurIG; |
In #114569, I was relying on
emitAlignLast
to update the pred head, if we end up creating an extra IG. However, for arm64, since we emit 4 align instructions and just keep the first of those align instructions with updates, it is not accurate to rely onemitAlignLast
because it points to the last of the 4 align instructions. Instead useemitAlignLastGroup
which is more accurate and designed for such purposes.Fixes: #116836