Skip to content

JIT: include element info when comparing array address trees - #131485

Merged
AndyAyersMS merged 2 commits into
dotnet:mainfrom
AndyAyersMS:issue-131459
Jul 29, 2026
Merged

JIT: include element info when comparing array address trees#131485
AndyAyersMS merged 2 commits into
dotnet:mainfrom
AndyAyersMS:issue-131459

Conversation

@AndyAyersMS

Copy link
Copy Markdown
Member

GenTree::Compare ignored the element size on GT_ARR_ELEM and all of the element metadata on GT_ARR_ADDR and GT_INDEX_ADDR, so tail merge could fold two array element addresses that differ only in element type or scale. Also hash these fields, and move the unreachable GT_INDEX_ADDR case in gtHashValue to the binop arm where it can actually run.

Fixes #131459.

GenTree::Compare ignored the element size on GT_ARR_ELEM and all of the
element metadata on GT_ARR_ADDR and GT_INDEX_ADDR, so tail merge could
fold two array element addresses that differ only in element type or
scale. Also hash these fields, and move the unreachable GT_INDEX_ADDR
case in gtHashValue to the binop arm where it can actually run.

Fixes dotnet#131459.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 28, 2026 19:30
@AndyAyersMS

AndyAyersMS commented Jul 28, 2026

Copy link
Copy Markdown
Member Author

@EgorBo PTAL
fyi @dotnet/jit-contrib

No diffs expected.

@github-actions github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 28, 2026
@AndyAyersMS
AndyAyersMS requested a review from EgorBo July 28, 2026 19:30
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 a JIT correctness issue where GenTree::Compare (and related hashing) did not include key element metadata for array-address-related nodes, allowing tail-merge/CSE to incorrectly treat distinct array element address computations as identical. It also adds a JIT regression test covering the reported miscompile and a related GT_ARR_ADDR metadata scenario.

Changes:

  • Extend GenTree::Compare to include element metadata when comparing GT_ARR_ELEM, GT_ARR_ADDR, and GT_INDEX_ADDR.
  • Extend Compiler::gtHashValue to hash the same element metadata for GT_ARR_ELEM, GT_ARR_ADDR, and GT_INDEX_ADDR (and move GT_INDEX_ADDR hashing to the correct operator-kind arm).
  • Add a regression test (Runtime_131459) and include it in Regression_ro_2.csproj.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/coreclr/jit/gentree.cpp Include array element/type/scale metadata in GenTree::Compare and gtHashValue for relevant tree kinds to prevent incorrect folding/merging.
src/tests/JIT/Regression/JitBlue/Runtime_131459/Runtime_131459.cs Add regression coverage for GT_ARR_ELEM element-size compare and GT_ARR_ADDR element-type/class-handle metadata compare.
src/tests/JIT/Regression/Regression_ro_2.csproj Add the new regression test file to the compilation list.

Comment thread src/coreclr/jit/gentree.cpp Outdated
Comment thread src/coreclr/jit/gentree.cpp
GenTree::Compare looked at the element metadata on GT_INDEX_ADDR but not
at GTF_INX_RNGCHK / GTF_INX_ADDR_NONNULL, so tail merge could fold a
GetArrayDataReference expansion with an ordinary ldelema and drop the
range check. Compare and hash those bits too.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 28, 2026 22:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

src/coreclr/jit/gentree.cpp:3624

  • Similarly, in the binary ExOp arm of gtHashValue, the comment "For the ones below no extra argument matters for comparison." is no longer accurate now that GT_INDEX_ADDR hashes multiple metadata fields. Please update the comment to reflect the new structure.
    src/tests/JIT/Regression/JitBlue/Runtime_131459/Runtime_131459.cs:56
  • The AddressBig coverage looks ineffective for this fix: S256/S512 have element sizes 256/512, but the JIT importer rejects expanding mdarray intrinsics when arrayElemSize doesn’t fit in unsigned char (see src/coreclr/jit/importercalls.cpp around the FitsIn<unsigned char>(arrayElemSize) check). That means these cases likely won’t produce GT_ARR_ELEM, so they won’t exercise the new GenTree::Compare/gtHashValue element-size handling. Consider removing this section or changing the types to sizes <= 255 so it stays on the intrinsic/GT_ARR_ELEM path.
    [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
    private static ref byte AddressBig(bool small, object array)
    {
        if (small)
        {

src/coreclr/jit/gentree.cpp:3569

  • In gtHashValue, the comment "For the ones below no extra argument matters for comparison." is now misleading because GT_ARR_ADDR immediately below does contribute additional metadata to the hash. Updating the comment avoids future confusion when adding more ExOp cases.

This issue also appears on line 3621 of the same file.

Comment thread src/coreclr/jit/gentree.cpp
@AndyAyersMS
AndyAyersMS merged commit d7e2960 into dotnet:main Jul 29, 2026
140 of 142 checks passed
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.

JIT: GenTree comparison ignores GT_ARR_ELEM element size

3 participants