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 <223556219+Copilot@users.noreply.github.com>

- 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 <223556219+Copilot@users.noreply.github.com>

- 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 <223556219+Copilot@users.noreply.github.com>

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 <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

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

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.

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.

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 Outdated 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 <223556219+Copilot@users.noreply.github.com>

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

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

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!

EgorBo Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

SingleAccretion Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

SingleAccretion Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

SingleAccretion Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

SingleAccretion Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants