Skip to content

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 20, 2025
Merged

Use emitLastGroup that works for single/multi align instructions #116843

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/coreclr/jit/emit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2856,16 +2856,17 @@ void* emitter::emitAddLabel(VARSET_VALARG_TP GCvars, regMaskTP gcrefRegs, regMas
{
#if FEATURE_LOOP_ALIGN

if (!currIGWasNonEmpty && (emitAlignLast != nullptr) && (emitAlignLast->idaLoopHeadPredIG != nullptr) &&
(emitAlignLast->idaLoopHeadPredIG->igNext == emitCurIG))
if (!currIGWasNonEmpty && (emitAlignLastGroup != nullptr) &&
(emitAlignLastGroup->idaLoopHeadPredIG != nullptr) &&
(emitAlignLastGroup->idaLoopHeadPredIG->igNext == emitCurIG))
{
// 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;
Copy link
Member

@AndyAyersMS AndyAyersMS Jun 20, 2025

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?

Copy link
Member

@AndyAyersMS AndyAyersMS Jun 20, 2025

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.

Copy link
Contributor Author

@kunalspathak kunalspathak Jun 20, 2025

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;

}
#endif // FEATURE_LOOP_ALIGN

Expand Down
Loading