Cleanup gen tree compare by AndyAyersMS · Pull Request #125357 · dotnet/runtime

AndyAyersMS · 2026-03-10T01:37:24Z

Add some missing checks

- GT_CNS_INT: Compare icon handle flags and field sequences, not just the integer value. Two constants with the same value but different handle types or field sequences are not equivalent. - GT_ALLOCOBJ: Add missing case in the unary ExOp switch. Previously hit the default assert in debug builds. - GT_ARR_ELEM: Compare gtArrElemSize. Resolves existing TODO comment. - GT_SELECT: Add missing case in the special-node switch. Compare all three operands (condition, true value, false value). Co-authored-by: Copilot <[email protected]>

- GT_INDEX_ADDR: Compare gtElemType, gtLenOffset, and gtElemOffset in addition to gtElemSize. - GT_FTN_ADDR: Add leaf case comparing gtFptrMethod. Previously returned false for equal function address nodes. - GT_PHI_ARG: Add leaf case comparing local number, predecessor block, and SSA number. Previously returned false for equal phi arg nodes. Co-authored-by: Copilot <[email protected]>

- GT_JCC/GT_SETCC: Add leaf cases comparing gtCondition. - GT_PHYSREG: Add leaf case comparing gtSrcReg. - GT_JCMP/GT_JTEST/GT_SELECTCC: Compare gtCondition for binary nodes that carry a condition code but are not ExOp. - GT_SELECT_INCCC/GT_SELECT_INVCC/GT_SELECT_NEGCC (ARM64): Compare gtCondition for ARM64-specific conditional select variants. - GT_CCMP (ARM64/AMD64): Compare gtCondition and gtFlagsVal. Co-authored-by: Copilot <[email protected]>

dotnet-policy-service · 2026-03-10T01:38:27Z

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

AndyAyersMS · 2026-03-10T01:39:20Z

@EgorBo PTAL
FYI @dotnet/jit-contrib

Just a few diffs, but it looks like the old behavior can lead to improperly optimized multi-guess GDVs in NAOT.

Copilot

Pull request overview

This PR tightens GenTree::Compare in CoreCLR JIT to treat more node-specific fields as part of structural equality, improving correctness for optimizations that rely on tree equivalence.

Changes:

Extend constant and leaf-node comparisons to include additional distinguishing fields (e.g., handle flags, field sequences, conditions, physregs).
Add missing comparisons for several operators (GT_PHI_ARG, GT_FTN_ADDR, GT_JCC/GT_SETCC, GT_PHYSREG, GT_ALLOCOBJ, GT_INDEX_ADDR, GT_ARR_ELEM, GT_SELECT).
Add condition-code comparisons for CC-carrying binops (GT_JCMP, GT_JTEST, GT_SELECTCC, target-specific selects, GT_CCMP).

src/coreclr/jit/gentree.cpp

- GT_CNS_INT: add gtCompileTimeHandle comparison - GT_FTN_ADDR: add gtFptrDelegateTarget and gtEntryPoint (R2R) comparisons - GT_ALLOCOBJ: add gtHelperHasSideEffects and gtEntryPoint (R2R) comparisons Co-authored-by: Copilot <[email protected]>

Co-authored-by: Copilot <[email protected]>

…nupGenTreeCompare

Copilot

Pull request overview

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

Copilot · 2026-03-10T03:25:56Z

src/coreclr/jit/gentree.cpp

+#ifdef FEATURE_READYTORUN
+                if (op1->AsFptrVal()->gtEntryPoint.addr != op2->AsFptrVal()->gtEntryPoint.addr)
+                {
+                    break;
+                }
+#endif


For FEATURE_READYTORUN comparisons of GT_FTN_ADDR, this only checks gtEntryPoint.addr. CORINFO_CONST_LOOKUP semantics also depend on accessType (and the union value may be interpreted differently based on it). To avoid treating different entry point forms as equal, include accessType in the comparison (and compare the relevant union member accordingly).

Copilot · 2026-03-10T03:25:57Z

src/coreclr/jit/gentree.cpp

+#ifdef FEATURE_READYTORUN
+                    if (op1->AsAllocObj()->gtEntryPoint.addr != op2->AsAllocObj()->gtEntryPoint.addr)
+                    {
+                        return false;
+                    }
+#endif


For FEATURE_READYTORUN comparisons of GT_ALLOCOBJ, this only checks gtEntryPoint.addr. CORINFO_CONST_LOOKUP equality should also consider accessType (and the union contents as interpreted by it), otherwise nodes with different entry point access modes can be considered identical.

Copilot · 2026-03-10T03:25:57Z

src/coreclr/jit/gentree.cpp

+                    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))


The GT_INDEX_ADDR ExOp comparison now checks several structural fields, but it still doesn't account for GT_INDEX_ADDR-specific semantic flags like GTF_INX_RNGCHK and GTF_INX_ADDR_NONNULL. These flags affect whether a bounds check / null-faulting behavior is required, so Compare should ensure the relevant bits match to avoid considering semantically different nodes identical.

EgorBo · 2026-03-10T10:07:50Z

Any idea what produced the massive diffs?

EgorBo · 2026-03-10T10:10:50Z

src/coreclr/jit/gentree.cpp

+                if ((op1->AsIntCon()->gtIconVal == op2->AsIntCon()->gtIconVal) &&
+                    (op1->GetIconHandleFlag() == op2->GetIconHandleFlag()) &&
+                    (op1->AsIntCon()->gtFieldSeq == op2->AsIntCon()->gtFieldSeq) &&
+                    (op1->AsIntCon()->gtCompileTimeHandle == op2->AsIntCon()->gtCompileTimeHandle))


I suspect e.g. assert-prop never propagates compile-time handle for constants

I think we should add tests for the actual issues this fixes. And remove the comparisons that we don't have failing tests for.

For the field sequence check, I think it can be something like:

if (obj.GetType() == typeof(ClassOne) { Unsafe.As<ClassOne>(obj).Field = 1; } else { Unsafe.As<ClassTwo>(obj).Field = 2; // Same offset as ClassOne::Field }

being tail-merged (the field sequence affects semantics of ADD(x, field seq).

I don't think gtCompileTimeHandle does or should affect semantics.

I can't think of any cases where GetIconHandleFlag would differentiate two different handles with the same value. If we want to support such cases, we will need to rationalize what that 'means' first.

Thanks. Copilot (with a little help from me) found a case where field seqs matter.

SingleAccretion · 2026-03-10T10:54:55Z

src/coreclr/jit/gentree.cpp

+                if ((op1->AsIntCon()->gtIconVal == op2->AsIntCon()->gtIconVal) &&
+                    (op1->GetIconHandleFlag() == op2->GetIconHandleFlag()) &&
+                    (op1->AsIntCon()->gtFieldSeq == op2->AsIntCon()->gtFieldSeq) &&
+                    (op1->AsIntCon()->gtCompileTimeHandle == op2->AsIntCon()->gtCompileTimeHandle))


I think we should add tests for the actual issues this fixes. And remove the comparisons that we don't have failing tests for.

For the field sequence check, I think it can be something like:

if (obj.GetType() == typeof(ClassOne) { Unsafe.As<ClassOne>(obj).Field = 1; } else { Unsafe.As<ClassTwo>(obj).Field = 2; // Same offset as ClassOne::Field }

being tail-merged (the field sequence affects semantics of ADD(x, field seq).

I don't think gtCompileTimeHandle does or should affect semantics.

I can't think of any cases where GetIconHandleFlag would differentiate two different handles with the same value. If we want to support such cases, we will need to rationalize what that 'means' first.

SingleAccretion · 2026-03-10T11:02:05Z

src/coreclr/jit/gentree.cpp


    if (kind & GTK_BINOP)
    {
+        if (op1->OperIs(GT_JCMP, GT_JTEST, GT_SELECTCC))


These operators look like they're missing GTK_EXOP.

Added missing cases and fixed up the compare logic for them.

SingleAccretion · 2026-03-10T11:04:23Z

src/coreclr/jit/gentree.cpp

+                        (op1->AsAllocObj()->gtAllocObjClsHnd != op2->AsAllocObj()->gtAllocObjClsHnd) ||
+                        (op1->AsAllocObj()->gtHelperHasSideEffects != op2->AsAllocObj()->gtHelperHasSideEffects))


Suggested change

(op1->AsAllocObj()->gtAllocObjClsHnd != op2->AsAllocObj()->gtAllocObjClsHnd) ||

(op1->AsAllocObj()->gtHelperHasSideEffects != op2->AsAllocObj()->gtHelperHasSideEffects))

(op1->AsAllocObj()->gtAllocObjClsHnd != op2->AsAllocObj()->gtAllocObjClsHnd))

gtHelperHasSideEffects is a pure function of gtAllocObjClsHnd.

AndyAyersMS · 2026-03-10T17:02:45Z

Any idea what produced the massive diffs?

Diffs were pretty minimal at first but the gaggle of copilot reviewers added more checks. Let me claw some of these back.

Exercises a scenario where tail merge incorrectly merges stores with different field sequences (ClassOne.Field vs ClassTwo.Field via Unsafe.As at the same offset). After inlining, redundant branch opts uses the liberal VN to fold branches, exposing incorrect alias analysis from the wrong field sequence. Without the gtFieldSeq check in GenTree::Compare, the test returns 42 instead of the expected 999. Co-authored-by: Copilot <[email protected]>

GT_JCMP, GT_JTEST, GT_SELECTCC, GT_CCMP, GT_SELECT_INCCC, GT_SELECT_INVCC, and GT_SELECT_NEGCC all extend GenTreeOp with extra fields (gtCondition, gtFlagsVal) but were not marked GTK_EXOP. This meant their extra field comparisons in GenTree::Compare were outside the IsExOp switch, inconsistent with other extended operators. Move the condition/flags comparisons into the ExOp switch block. Co-authored-by: Copilot <[email protected]>

gtCompileTimeHandle does not affect runtime semantics. SPMI diffs confirm removing the check enables ~5500 additional optimizations across all collections with no regressions, saving ~306KB total. Co-authored-by: Copilot <[email protected]>

AndyAyersMS · 2026-03-11T16:17:13Z

I suspect e.g. assert-prop never propagates compile-time handle for constants

This was the source of the extra diffs. Removing that there are just a handful of diffs: one case where we can now fold some function pointer checks, and a couple where the new field seq check blocks a tail merge. In those cases the field offsets ended up the same so the new code is less efficient.

I suppose we could relax the field seq check once we're past the point where it could impact VN based optimizations, but these opportunities seem rare.

gtHelperHasSideEffects is a pure function of gtAllocObjClsHnd (both are derived from getNewHelper(classHandle)). Since gtAllocObjClsHnd is already compared, the gtHelperHasSideEffects check is redundant. Co-authored-by: Copilot <[email protected]>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Copilot · 2026-03-11T16:45:25Z

src/coreclr/jit/gentree.cpp

+                case GT_ALLOCOBJ:
+                    if ((op1->AsAllocObj()->gtNewHelper != op2->AsAllocObj()->gtNewHelper) ||
+                        (op1->AsAllocObj()->gtAllocObjClsHnd != op2->AsAllocObj()->gtAllocObjClsHnd))
+                    {
+                        return false;
+                    }


GT_ALLOCOBJ extra-field comparison doesn’t include gtHelperHasSideEffects, which is later used to set GTF_CALL_M_ALLOC_SIDE_EFFECTS when the node is morphed into a helper call. Include gtHelperHasSideEffects in the comparison so nodes with different side-effect behavior are not treated as identical.

SingleAccretion

LGTM modulo comments (the test doesn't look right).

SingleAccretion · 2026-03-11T17:13:53Z

src/tests/JIT/Regression/JitBlue/Runtime_125357/Runtime_125357.cs

+            Unsafe.As<ClassTwo>(obj).Field = 999;
+            return Unsafe.As<ClassOne>(obj).Field;


This is UB.

Also, the comment at the top doesn't make sense to me.

// After inlining, redundant branch opts (which uses the liberal VN) can fold // the type check using the wrong field-based alias information, producing // incorrect results.

What type check?

Trying to do all this via copilot, so...

Maybe if we add a second type check at the use we can expose this without UB?

SingleAccretion · 2026-03-11T17:14:08Z

src/tests/JIT/Regression/JitBlue/Runtime_125357/Runtime_125357.csproj

+    <RequiresProcessIsolation>true</RequiresProcessIsolation>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+    <CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />


Suggested change

<RequiresProcessIsolation>true</RequiresProcessIsolation>

<Optimize>True</Optimize>

</PropertyGroup>

<ItemGroup>

<Compile Include="$(MSBuildProjectName).cs" />

<CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />

<Optimize>True</Optimize>

</PropertyGroup>

<ItemGroup>

<Compile Include="$(MSBuildProjectName).cs" />

The whole csproj should be deleted and the test added into one of the merged csprojs instead.

Exercises a scenario where tail merge incorrectly merges stores with different field sequences (ClassOne.Field vs ClassTwo.Field via Unsafe.As at the same offset). After inlining, redundant branch opts uses the liberal VN to fold branches, exposing incorrect alias analysis from the wrong field sequence. Without the gtFieldSeq check in GenTree::Compare, the test returns 42 instead of the expected 999. Co-authored-by: Copilot <[email protected]>

…nupGenTreeCompare

AndyAyersMS and others added 3 commits March 9, 2026 16:23

Copilot AI review requested due to automatic review settings March 10, 2026 01:37

github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 10, 2026

dotnet-policy-service bot assigned AndyAyersMS Mar 10, 2026

Copilot started reviewing on behalf of AndyAyersMS March 10, 2026 01:38 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

src/coreclr/jit/gentree.cpp Show resolved Hide resolved

src/coreclr/jit/gentree.cpp Show resolved Hide resolved

src/coreclr/jit/gentree.cpp Show resolved Hide resolved

AndyAyersMS and others added 4 commits March 9, 2026 20:01

Add missing gtStructElemClass comparison in GT_INDEX_ADDR

b639514

Co-authored-by: Copilot <[email protected]>

Merge remote-tracking branch 'origin/CleanupGenTreeCompare' into Clea…

2be8435

…nupGenTreeCompare

Copilot AI review requested due to automatic review settings March 10, 2026 03:21

Copilot started reviewing on behalf of AndyAyersMS March 10, 2026 03:22 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

EgorBo reviewed Mar 10, 2026

View reviewed changes

SingleAccretion reviewed Mar 10, 2026

View reviewed changes

AndyAyersMS and others added 3 commits March 11, 2026 07:39

Copilot AI review requested due to automatic review settings March 11, 2026 16:39

Copilot started reviewing on behalf of AndyAyersMS March 11, 2026 16:40 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

SingleAccretion reviewed Mar 11, 2026

View reviewed changes

AndyAyersMS and others added 2 commits March 11, 2026 10:46

Merge remote-tracking branch 'origin/CleanupGenTreeCompare' into Clea…

6e78d4e

…nupGenTreeCompare

This was referenced Mar 11, 2026

[android] Android.Device_Emulator.JIT.Test failing on emulators with CoreCLR #112633

Open

Native abort (SIGABRT, exit 134) in JIT/SIMD/Vector3Interop_r on Linux x64 #123957

Open

		(op1->AsAllocObj()->gtAllocObjClsHnd != op2->AsAllocObj()->gtAllocObjClsHnd) \|\|
		(op1->AsAllocObj()->gtHelperHasSideEffects != op2->AsAllocObj()->gtHelperHasSideEffects))

		Unsafe.As<ClassTwo>(obj).Field = 999;
		return Unsafe.As<ClassOne>(obj).Field;

Conversation

AndyAyersMS commented Mar 10, 2026

Uh oh!

dotnet-policy-service bot commented Mar 10, 2026

Uh oh!

AndyAyersMS commented Mar 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

EgorBo commented Mar 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Mar 10, 2026

Uh oh!

AndyAyersMS commented Mar 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

SingleAccretion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants