From 16d054debc9a9e0ca5ee05963aaee49243d23f94 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 28 Jul 2026 11:30:49 -0700 Subject: [PATCH 1/2] JIT: include element info when comparing array address trees 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. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/gentree.cpp | 40 +++++- .../JitBlue/Runtime_131459/Runtime_131459.cs | 116 ++++++++++++++++++ .../JIT/Regression/Regression_ro_2.csproj | 1 + 3 files changed, 151 insertions(+), 6 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_131459/Runtime_131459.cs diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index a1357523a02804..f612fa9137d826 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -2976,10 +2976,18 @@ bool GenTree::Compare(GenTree* op1, GenTree* op2, bool swapOK) } break; + case GT_ARR_ADDR: + if ((op1->AsArrAddr()->GetElemType() != op2->AsArrAddr()->GetElemType()) || + (op1->AsArrAddr()->GetElemClassHandle() != op2->AsArrAddr()->GetElemClassHandle()) || + (op1->AsArrAddr()->GetFirstElemOffset() != op2->AsArrAddr()->GetFirstElemOffset())) + { + return false; + } + break; + // For the ones below no extra argument matters for comparison. case GT_BOX: case GT_RUNTIMELOOKUP: - case GT_ARR_ADDR: break; default: @@ -3034,7 +3042,11 @@ bool GenTree::Compare(GenTree* op1, GenTree* op2, bool swapOK) } break; case GT_INDEX_ADDR: - if (op1->AsIndexAddr()->gtElemSize != op2->AsIndexAddr()->gtElemSize) + if ((op1->AsIndexAddr()->gtElemSize != op2->AsIndexAddr()->gtElemSize) || + (op1->AsIndexAddr()->gtElemType != op2->AsIndexAddr()->gtElemType) || + (op1->AsIndexAddr()->gtStructElemClass != op2->AsIndexAddr()->gtStructElemClass) || + (op1->AsIndexAddr()->gtLenOffset != op2->AsIndexAddr()->gtLenOffset) || + (op1->AsIndexAddr()->gtElemOffset != op2->AsIndexAddr()->gtElemOffset)) { return false; } @@ -3112,7 +3124,10 @@ bool GenTree::Compare(GenTree* op1, GenTree* op2, bool swapOK) return false; } - // NOTE: gtArrElemSize may need to be handled + if (op1->AsArrElem()->gtArrElemSize != op2->AsArrElem()->gtArrElemSize) + { + return false; + } unsigned dim; for (dim = 0; dim < op1->AsArrElem()->gtArrRank; dim++) @@ -3530,9 +3545,6 @@ unsigned Compiler::gtHashValue(GenTree* tree) case GT_CAST: hash ^= tree->AsCast()->gtCastType; break; - case GT_INDEX_ADDR: - hash += tree->AsIndexAddr()->gtElemSize; - break; case GT_ALLOCOBJ: hash = genTreeHashAdd(hash, static_cast( reinterpret_cast(tree->AsAllocObj()->gtAllocObjClsHnd))); @@ -3554,7 +3566,13 @@ unsigned Compiler::gtHashValue(GenTree* tree) // For the ones below no extra argument matters for comparison. case GT_BOX: + break; + case GT_ARR_ADDR: + hash = genTreeHashAdd(hash, tree->AsArrAddr()->GetElemType()); + hash = genTreeHashAdd(hash, static_cast(reinterpret_cast( + tree->AsArrAddr()->GetElemClassHandle()))); + hash = genTreeHashAdd(hash, tree->AsArrAddr()->GetFirstElemOffset()); break; default: @@ -3600,7 +3618,15 @@ unsigned Compiler::gtHashValue(GenTree* tree) // For the ones below no extra argument matters for comparison. case GT_QMARK: + break; + case GT_INDEX_ADDR: + hash = genTreeHashAdd(hash, tree->AsIndexAddr()->gtElemSize); + hash = genTreeHashAdd(hash, tree->AsIndexAddr()->gtElemType); + hash = genTreeHashAdd(hash, static_cast(reinterpret_cast( + tree->AsIndexAddr()->gtStructElemClass))); + hash = genTreeHashAdd(hash, tree->AsIndexAddr()->gtLenOffset); + hash = genTreeHashAdd(hash, tree->AsIndexAddr()->gtElemOffset); break; #ifdef FEATURE_HW_INTRINSICS @@ -3656,6 +3682,8 @@ unsigned Compiler::gtHashValue(GenTree* tree) case GT_ARR_ELEM: hash = genTreeHashAdd(hash, gtHashValue(tree->AsArrElem()->gtArrObj)); + hash = genTreeHashAdd(hash, tree->AsArrElem()->gtArrRank); + hash = genTreeHashAdd(hash, tree->AsArrElem()->gtArrElemSize); unsigned dim; for (dim = 0; dim < tree->AsArrElem()->gtArrRank; dim++) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_131459/Runtime_131459.cs b/src/tests/JIT/Regression/JitBlue/Runtime_131459/Runtime_131459.cs new file mode 100644 index 00000000000000..5f15fd7715ae84 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_131459/Runtime_131459.cs @@ -0,0 +1,116 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Runtime_131459; + +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using Xunit; + +[StructLayout(LayoutKind.Sequential, Size = 16)] +internal struct S16 +{ + public byte Value; +} + +[StructLayout(LayoutKind.Sequential, Size = 252)] +internal struct S252 +{ + public byte Value; +} + +[StructLayout(LayoutKind.Sequential, Size = 256)] +internal struct S256 +{ + public byte Value; +} + +[StructLayout(LayoutKind.Sequential, Size = 512)] +internal struct S512 +{ + public byte Value; +} + +public class Runtime_131459 +{ + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static ref byte Address(bool small, object array) + { + if (small) + { + ref S16 element = ref Unsafe.As(array)[0, 1]; + return ref Unsafe.As(ref element); + } + else + { + ref S252 element = ref Unsafe.As(array)[0, 1]; + return ref Unsafe.As(ref element); + } + } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static ref byte AddressBig(bool small, object array) + { + if (small) + { + ref S256 element = ref Unsafe.As(array)[0, 1]; + return ref Unsafe.As(ref element); + } + else + { + ref S512 element = ref Unsafe.As(array)[0, 1]; + return ref Unsafe.As(ref element); + } + } + + + [Fact] + public static void TestEntryPoint() + { + S16[,] small = new S16[1, 2]; + small[0, 1].Value = 16; + + S252[,] large = new S252[1, 2]; + large[0, 1].Value = 252; + + Assert.Equal(16, Address(true, small)); + Assert.Equal(252, Address(false, large)); + + S256[,] smallBig = new S256[1, 2]; + smallBig[0, 1].Value = 16; + + S512[,] largeBig = new S512[1, 2]; + largeBig[0, 1].Value = 252; + + Assert.Equal(16, AddressBig(true, smallBig)); + Assert.Equal(252, AddressBig(false, largeBig)); + } + + // GT_ARR_ADDR carries the array element type and class handle. If GenTree::Compare ignores + // them, tail merge folds the two arms below into one and the surviving node claims long[], + // which lets value numbering treat the store through 'r' as not aliasing da[1]. + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static double ArrayElementTypeMerge(bool useLongArray, double[] da, object array) + { + ref byte r = ref Unsafe.NullRef(); + if (useLongArray) + { + r = ref Unsafe.As(ref Unsafe.As(array)[1]); + } + else + { + r = ref Unsafe.As(ref Unsafe.As(array)[1]); + } + + double before = da[1]; + Unsafe.WriteUnaligned(ref r, 0x4045000000000000L); + return da[1] - before; + } + + [Fact] + public static void ArrayElementType() + { + double[] da = new double[2]; + Assert.Equal(42.0, ArrayElementTypeMerge(false, da, da)); + } +} diff --git a/src/tests/JIT/Regression/Regression_ro_2.csproj b/src/tests/JIT/Regression/Regression_ro_2.csproj index 9c69e5397bb479..5f7385e48dff4b 100644 --- a/src/tests/JIT/Regression/Regression_ro_2.csproj +++ b/src/tests/JIT/Regression/Regression_ro_2.csproj @@ -120,6 +120,7 @@ + From 1ad1ee220d0cad0f1ea054c951088741834bf9dd Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 28 Jul 2026 15:14:46 -0700 Subject: [PATCH 2/2] JIT: also compare the IndexAddr range check and non-null bits 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> --- src/coreclr/jit/gentree.cpp | 6 +++- .../JitBlue/Runtime_131459/Runtime_131459.cs | 33 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index f612fa9137d826..8de8ea59ea2b2c 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -3046,7 +3046,9 @@ bool GenTree::Compare(GenTree* op1, GenTree* op2, bool swapOK) (op1->AsIndexAddr()->gtElemType != op2->AsIndexAddr()->gtElemType) || (op1->AsIndexAddr()->gtStructElemClass != op2->AsIndexAddr()->gtStructElemClass) || (op1->AsIndexAddr()->gtLenOffset != op2->AsIndexAddr()->gtLenOffset) || - (op1->AsIndexAddr()->gtElemOffset != op2->AsIndexAddr()->gtElemOffset)) + (op1->AsIndexAddr()->gtElemOffset != op2->AsIndexAddr()->gtElemOffset) || + ((op1->gtFlags & (GTF_INX_RNGCHK | GTF_INX_ADDR_NONNULL)) != + (op2->gtFlags & (GTF_INX_RNGCHK | GTF_INX_ADDR_NONNULL)))) { return false; } @@ -3627,6 +3629,8 @@ unsigned Compiler::gtHashValue(GenTree* tree) tree->AsIndexAddr()->gtStructElemClass))); hash = genTreeHashAdd(hash, tree->AsIndexAddr()->gtLenOffset); hash = genTreeHashAdd(hash, tree->AsIndexAddr()->gtElemOffset); + hash = genTreeHashAdd(hash, static_cast(tree->gtFlags & + (GTF_INX_RNGCHK | GTF_INX_ADDR_NONNULL))); break; #ifdef FEATURE_HW_INTRINSICS diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_131459/Runtime_131459.cs b/src/tests/JIT/Regression/JitBlue/Runtime_131459/Runtime_131459.cs index 5f15fd7715ae84..7a12c6dbd33e69 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_131459/Runtime_131459.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_131459/Runtime_131459.cs @@ -3,6 +3,7 @@ namespace Runtime_131459; +using System; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using Xunit; @@ -113,4 +114,36 @@ public static void ArrayElementType() double[] da = new double[2]; Assert.Equal(42.0, ArrayElementTypeMerge(false, da, da)); } + + // GT_INDEX_ADDR also carries the "needs a range check" and "known non-null" bits. + // MemoryMarshal.GetArrayDataReference expands to an IndexAddr with the range check + // cleared, so if GenTree::Compare ignores those bits, tail merge can fold the two arms + // below and drop the range check on the a[0] path. The two trees only line up when the + // index constants have the same type, which is the case on 32-bit targets. + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static int BoundsCheckMerge(bool useDataReference, S16[] array) + { + S16 element; + if (useDataReference) + { + element = MemoryMarshal.GetArrayDataReference(array); + } + else + { + element = array[0]; + } + + return element.Value + 1; + } + + [Fact] + public static void BoundsCheck() + { + S16[] nonEmpty = new S16[2]; + Assert.Equal(1, BoundsCheckMerge(true, nonEmpty)); + Assert.Equal(1, BoundsCheckMerge(false, nonEmpty)); + + S16[] empty = new S16[0]; + Assert.Throws(() => BoundsCheckMerge(false, empty)); + } }