Skip to content

Conversation

@kunalspathak
Copy link
Contributor

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

@dotnet/jit-contrib @AndyAyersMS PTAL

@kunalspathak
Copy link
Contributor Author

/azp runtime-coreclr superpmi-replay

@azure-pipelines
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
Contributor

/azp run runtime-coreclr superpmi-replay

@azure-pipelines
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
@dotnet-policy-service
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
Contributor

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

@amanasifkhalid - anything else needed for this PR?

Copy link
Contributor

@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
// 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
Contributor 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
@github-actions github-actions bot locked and limited conversation to collaborators Jul 21, 2025
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.

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

3 participants