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.

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

Conversation

kunalspathak
Copy link
Member

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 on emitAlignLast because it points to the last of the 4 align instructions. Instead use emitAlignLastGroup which is more accurate and designed for such purposes.

Fixes: #116836

@Copilot Copilot AI review requested due to automatic review settings June 20, 2025 00:14
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 20, 2025
Copy link
Contributor

@Copilot Copilot AI left a 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) &&

@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib @AndyAyersMS PTAL

@kunalspathak
Copy link
Member Author

/azp runtime-coreclr superpmi-replay

Copy link

Command 'runtime-coreclr' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@amanasifkhalid
Copy link
Member

/azp run runtime-coreclr superpmi-replay

Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@amanasifkhalid amanasifkhalid added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 20, 2025
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@amanasifkhalid
Copy link
Member

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?

@kunalspathak
Copy link
Member Author

@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.

@kunalspathak
Copy link
Member Author

/azp run runtime-coreclr superpmi-replay

yeah, we cannot trigger this anymore. I even tried to do it manually but didn't see a way to do it.

@kunalspathak
Copy link
Member Author

@amanasifkhalid - anything else needed for this PR?

Copy link
Member

@amanasifkhalid amanasifkhalid left a comment

Choose a reason for hiding this comment

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

No, LGTM

@kunalspathak kunalspathak merged commit 8dbc826 into dotnet:main Jun 20, 2025
110 of 112 checks passed
@kunalspathak kunalspathak deleted the multialign-update-predhead branch June 20, 2025 16:06
{
// 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

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

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
Member Author

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.

// 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;

@kunalspathak kunalspathak mentioned this pull request Jun 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

Assertion failed 'markedLastLoop && markedCurrLoop' during 'Generate code'
3 participants