From 3eb376ca93474c15c8c0efa3101742ae59c65371 Mon Sep 17 00:00:00 2001 From: Julie Lee Date: Wed, 17 Jun 2026 12:26:28 -0700 Subject: [PATCH 01/10] JIT: fix opt-repeat divide-by-zero miscompile (#129386) GTF_DIV_MOD_NO_BY_ZERO and GTF_DIV_MOD_NO_OVERFLOW are flow-sensitive annotations that assertion propagation sets on a divide once a dominating check proves the divisor safe at that node's location. They are "set once" facts. Under JitOptRepeat, a later optimization iteration re-runs (flow-insensitive) value numbering while these stale flags are still present; VN then models the divide as non-throwing and clears GTF_EXCEPT, after which CSE/loop hoisting moves a loop-invariant `0 % p0` into the loop preheader, above the dominating `p0 == 0` guard, where it executes unconditionally and throws DivideByZeroException. Clear these flags in ResetOptAnnotations (which already discards other "set once" annotations such as VNs, assertions, and CSE numbers between opt-repeat iterations) so each iteration's assertion propagation re-derives them from scratch. Recompute statement side effects afterwards so GTF_EXCEPT is restored to a consistent state for the cleared divides. Adds a regression test that reproduces the miscompile under JitOptRepeat. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/compiler.cpp | 24 ++++ .../JitBlue/Runtime_129386/Runtime_129386.cs | 121 ++++++++++++++++++ .../Runtime_129386/Runtime_129386.csproj | 17 +++ 3 files changed, 162 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.csproj diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index a70495eca6f19e..464592e2915823 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5822,11 +5822,35 @@ void Compiler::ResetOptAnnotations() { for (Statement* const stmt : block->Statements()) { + bool clearedDivModFlags = false; for (GenTree* const tree : stmt->TreeList()) { tree->ClearVN(); tree->ClearAssertion(); tree->gtCSEnum = NO_CSE; + + // GTF_DIV_MOD_NO_BY_ZERO and GTF_DIV_MOD_NO_OVERFLOW are flow-sensitive + // annotations set by assertion propagation once a dominating check proves + // the divisor safe at the node's location. They are "set once" facts that + // become stale when we re-run the optimizer, and must not be observed by + // the next iteration's (flow-insensitive) value numbering: doing so would + // let VN model the divide as non-throwing and allow CSE/loop hoisting to + // move it above the check that justified the flag (see + // https://github.com/dotnet/runtime/issues/129386). Clear them here so each + // iteration's assertion propagation re-derives them from scratch. + if (tree->OperIs(GT_DIV, GT_UDIV, GT_MOD, GT_UMOD) && + ((tree->gtFlags & (GTF_DIV_MOD_NO_BY_ZERO | GTF_DIV_MOD_NO_OVERFLOW)) != 0)) + { + tree->gtFlags &= ~(GTF_DIV_MOD_NO_BY_ZERO | GTF_DIV_MOD_NO_OVERFLOW); + clearedDivModFlags = true; + } + } + + // Clearing the no-throw flags above can make a divide throwing again, so + // recompute the statement's side effects to restore GTF_EXCEPT consistency. + if (clearedDivModFlags) + { + gtUpdateStmtSideEffects(stmt); } } } diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.cs b/src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.cs new file mode 100644 index 00000000000000..c14f8b493c7b84 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.cs @@ -0,0 +1,121 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Under JitOptRepeat (STRESS_OPT_REPEAT), a second value-numbering + CSE round +// could observe a divide whose DivideByZeroException had been suppressed by the +// flow-sensitive GTF_DIV_MOD_NO_BY_ZERO flag (set by assertion propagation under +// a dominating zero-check). CSE/loop-hoisting then moved the division into the +// loop preheader, above the dominating check, where it executed unconditionally +// and divided by zero. The division's exception must be modeled from value-based +// information only, so it is not hoisted above the check that proves it safe. + +using System; +using System.Numerics; +using System.Runtime.CompilerServices; +using Xunit; + +public class Runtime_129386 +{ + private static volatile uint s_inputP0 = 0u; + private static volatile uint s_inputP1 = 0xFFFFFFFFu; + private static int[] s_trace = new int[64]; + private static int s_traceLen = 0; + + [MethodImpl(MethodImplOptions.NoInlining)] + public static uint Fn30000002142(uint p0, uint p1) + { + unchecked + { + uint v1 = 0, v3 = 0, v4 = 0, v5 = 0, v7 = 0, v11 = 0, v13 = 0, v14 = 0, v19 = 0, + v22 = 0, v23 = 0, v24 = 0, v25 = 0, v33 = 0, v35 = 0, v39 = 0, v42 = 0, + v50 = 0, v51 = 0, v58 = 0, v61 = 0, v63 = 0, v66 = 0, v67 = 0, v68 = 0, + v69 = 0, v71 = 0, v72 = 0, v73 = 0, v74 = 0, v75 = 0, v78 = 0, v80 = 0, + v82 = 0, v83 = 0, v84 = 0, v85 = 0, v86 = 0, v87 = 0, v88 = 0, v93 = 0, + v119 = 0, v120 = 0, v121 = 0, v123 = 0, v124 = 0; + int v6 = 0, v10 = 0, v17 = 0, v21 = 0, v26 = 0, v70 = 0, v76 = 0, v77 = 0, + v79 = 0, v81 = 0, v118 = 0, v122 = 0; + + b0: + v6 = (0u < p0) ? 1 : 0; + if (v6 != 0) goto b1; + goto b3; + b1: + v10 = BitOperations.IsPow2(v7) ? 1 : 0; + v11 = (uint)v10; + v14 = v13 - 0x000D3403u; + v17 = (v14 > v4) ? 1 : 0; + if (v17 != 0) goto b2; + goto b10; + b2: + s_trace[s_traceLen++] = 2; + v19 = v5 % p0; + v21 = (p0 <= v11) ? 1 : 0; + if (v21 != 0) goto b10; + goto b11; + b3: + v22 = v14 - 0x000CBA13u; + v23 = v11 | 0xFFFC3D1Au; + v24 = v4 & v22; + v25 = 0u & v1; + v26 = (v24 > p0) ? 1 : 0; + if (v26 != 0) goto b10; + goto b12; + b10: + s_trace[s_traceLen++] = 10; + v66 = v51 >>> (int)v61; + v67 = v58 - 0x0004A8B9u; + v68 = Math.Max(0xFFFA3E09u, 0xFFFFFFFDu); + v69 = v33 - v50; + v70 = BitOperations.PopCount(0xFFFBC8AAu); + v71 = (uint)v70; + v72 = v4 | 0x00004D11u; + goto b19; + b11: + s_trace[s_traceLen++] = 11; + v73 = v51 + 0x000003E8u; + v74 = BitOperations.RotateRight(v39, (int)v42); + v75 = 0xFFFAF3CAu + v58; + v76 = (v68 == v19) ? 1 : 0; + if (v76 != 0) goto b13; + goto b0; + b12: + s_trace[s_traceLen++] = 12; + v77 = (v4 < v23) ? 1 : 0; + v78 = (uint)v77; + v79 = (v35 <= 0x000B9492u) ? 1 : 0; + v80 = (uint)v79; + v81 = BitOperations.TrailingZeroCount(0xFFFFFFFFu); + v82 = (uint)v81; + return v82; + b13: + s_trace[s_traceLen++] = 13; + v83 = ~v23; + v84 = v25 + v1; + v85 = Math.Max(0x80000000u, p1); + v86 = 0xFFFF5264u % v84; + v87 = p0 & 0xFFFFFFF9u; + v88 = ~p0; + goto b2; + b19: + s_trace[s_traceLen++] = 19; + v118 = (v83 <= 0x000BF458u) ? 1 : 0; + v119 = (uint)v118; + v120 = 0x0000001Fu * v93; + v121 = ~v75; + v122 = BitOperations.Log2(v24); + v123 = (uint)v122; + v124 = v63 + v3; + return v124; + } + } + + [Fact] + public static int TestEntryPoint() + { + // With p0 == 0 the source-correct path is b0 -> b3 -> b12 -> return 0; + // block b2 (which evaluates 'v5 % p0', a divide-by-zero) is unreachable. + // The miscompile produced a DivideByZeroException instead. + uint result = Fn30000002142(s_inputP0, s_inputP1); + return result == 0 ? 100 : 101; + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.csproj new file mode 100644 index 00000000000000..0d0006402d04b1 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.csproj @@ -0,0 +1,17 @@ + + + + true + + + True + + + + + + + + + + From 2fd6a658d2807c61f6b7ef164ea16a2ac9d51cff Mon Sep 17 00:00:00 2001 From: Julie Lee Date: Thu, 18 Jun 2026 08:54:54 -0700 Subject: [PATCH 02/10] Update regression test comment to describe the actual fix The header comment described the earlier (superseded) value-based exception modeling approach. Update it to reflect the current fix: clearing the stale flow-sensitive GTF_DIV_MOD_NO_BY_ZERO / GTF_DIV_MOD_NO_OVERFLOW flags between opt-repeat iterations in ResetOptAnnotations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.cs b/src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.cs index c14f8b493c7b84..309ec580c3743f 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.cs @@ -6,8 +6,10 @@ // flow-sensitive GTF_DIV_MOD_NO_BY_ZERO flag (set by assertion propagation under // a dominating zero-check). CSE/loop-hoisting then moved the division into the // loop preheader, above the dominating check, where it executed unconditionally -// and divided by zero. The division's exception must be modeled from value-based -// information only, so it is not hoisted above the check that proves it safe. +// and divided by zero. The fix clears these stale flow-sensitive flags +// (GTF_DIV_MOD_NO_BY_ZERO / GTF_DIV_MOD_NO_OVERFLOW) between opt-repeat iterations +// in ResetOptAnnotations, so a later value-numbering pass no longer sees the +// divide as non-throwing and cannot hoist it above its guard. using System; using System.Numerics; From bd866042afddb76580cecc8ad9c835c81057dcd9 Mon Sep 17 00:00:00 2001 From: Julie Lee Date: Tue, 23 Jun 2026 12:37:39 -0700 Subject: [PATCH 03/10] JIT: pin proven-safe div/mod with GTF_ORDER_SIDEEFF instead of clearing flags Per review feedback (@EgorBo): rather than clearing the flow-sensitive GTF_DIV_MOD_NO_BY_ZERO/GTF_DIV_MOD_NO_OVERFLOW flags between opt-repeat iterations in ResetOptAnnotations, mark the divide with an ordering side effect when assertion propagation proves it non-throwing. This pins the node below the dominating check that justified the flag, so CSE/loop hoisting cannot move it above the guard even though it now looks non-throwing. This is the same mechanism used elsewhere (bounds checks, indirs, null checks). - gentree.cpp: GT_DIV/GT_UDIV/GT_MOD/GT_UMOD support GTF_ORDER_SIDEEFF - assertionprop.cpp: set ordering side effect when marking div/mod no-throw - compiler.cpp: revert the ResetOptAnnotations flag-clearing workaround - update Runtime_129386 regression comment to describe the new mechanism Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/assertionprop.cpp | 10 ++++++++ src/coreclr/jit/compiler.cpp | 24 ------------------- src/coreclr/jit/gentree.cpp | 8 +++++++ .../JitBlue/Runtime_129386/Runtime_129386.cs | 8 +++---- 4 files changed, 22 insertions(+), 28 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 827cf671c91990..af4954db0ee250 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -4143,6 +4143,16 @@ GenTree* Compiler::optAssertionProp_ModDiv(ASSERT_VALARG_TP assertions, changed = true; } + // GTF_DIV_MOD_NO_BY_ZERO/GTF_DIV_MOD_NO_OVERFLOW make the divide appear non-throwing, + // which would otherwise let CSE/loop hoisting move it above the dominating check that + // proved the divisor safe (the flag is only valid at this node's location). Pin the + // node with an ordering side effect so it cannot be reordered above that check, even + // across opt-repeat iterations (see https://github.com/dotnet/runtime/issues/129386). + if ((tree->gtFlags & (GTF_DIV_MOD_NO_BY_ZERO | GTF_DIV_MOD_NO_OVERFLOW)) != 0) + { + tree->SetHasOrderingSideEffect(); + } + return changed ? optAssertionProp_Update(tree, tree, stmt) : nullptr; } diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 464592e2915823..a70495eca6f19e 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5822,35 +5822,11 @@ void Compiler::ResetOptAnnotations() { for (Statement* const stmt : block->Statements()) { - bool clearedDivModFlags = false; for (GenTree* const tree : stmt->TreeList()) { tree->ClearVN(); tree->ClearAssertion(); tree->gtCSEnum = NO_CSE; - - // GTF_DIV_MOD_NO_BY_ZERO and GTF_DIV_MOD_NO_OVERFLOW are flow-sensitive - // annotations set by assertion propagation once a dominating check proves - // the divisor safe at the node's location. They are "set once" facts that - // become stale when we re-run the optimizer, and must not be observed by - // the next iteration's (flow-insensitive) value numbering: doing so would - // let VN model the divide as non-throwing and allow CSE/loop hoisting to - // move it above the check that justified the flag (see - // https://github.com/dotnet/runtime/issues/129386). Clear them here so each - // iteration's assertion propagation re-derives them from scratch. - if (tree->OperIs(GT_DIV, GT_UDIV, GT_MOD, GT_UMOD) && - ((tree->gtFlags & (GTF_DIV_MOD_NO_BY_ZERO | GTF_DIV_MOD_NO_OVERFLOW)) != 0)) - { - tree->gtFlags &= ~(GTF_DIV_MOD_NO_BY_ZERO | GTF_DIV_MOD_NO_OVERFLOW); - clearedDivModFlags = true; - } - } - - // Clearing the no-throw flags above can make a divide throwing again, so - // recompute the statement's side effects to restore GTF_EXCEPT consistency. - if (clearedDivModFlags) - { - gtUpdateStmtSideEffects(stmt); } } } diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index c6a571db0df753..b6688a88a466ad 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -8801,6 +8801,14 @@ bool GenTree::OperSupportsOrderingSideEffect() const case GT_LOCKADD: case GT_CMPXCHG: case GT_MEMORYBARRIER: + // DIV/MOD support an ordering side effect so that, once assertion propagation + // proves the divide cannot throw (GTF_DIV_MOD_NO_BY_ZERO/GTF_DIV_MOD_NO_OVERFLOW) + // and the node would otherwise look movable, it stays pinned below the dominating + // check that justified the flag (see https://github.com/dotnet/runtime/issues/129386). + case GT_DIV: + case GT_UDIV: + case GT_MOD: + case GT_UMOD: case GT_CATCH_ARG: case GT_ASYNC_CONTINUATION: case GT_RETURN_SUSPEND: diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.cs b/src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.cs index 309ec580c3743f..7148da9d54b60b 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_129386/Runtime_129386.cs @@ -6,10 +6,10 @@ // flow-sensitive GTF_DIV_MOD_NO_BY_ZERO flag (set by assertion propagation under // a dominating zero-check). CSE/loop-hoisting then moved the division into the // loop preheader, above the dominating check, where it executed unconditionally -// and divided by zero. The fix clears these stale flow-sensitive flags -// (GTF_DIV_MOD_NO_BY_ZERO / GTF_DIV_MOD_NO_OVERFLOW) between opt-repeat iterations -// in ResetOptAnnotations, so a later value-numbering pass no longer sees the -// divide as non-throwing and cannot hoist it above its guard. +// and divided by zero. The fix marks the divide with an ordering side effect +// (GTF_ORDER_SIDEEFF) when assertion propagation proves it non-throwing, so it +// stays pinned below the dominating check and cannot be reordered/hoisted above +// its guard even though it now looks non-throwing. using System; using System.Numerics; From bd9de8f6fc70942c45a2aa7bf48771c5adab6ffa Mon Sep 17 00:00:00 2001 From: Julie Lee Date: Tue, 23 Jun 2026 14:37:03 -0700 Subject: [PATCH 04/10] JIT: only pin div/mod when the no-throw proof is assertion-based Per review feedback: optAssertionProp_RangeProperties proves non-zero/ non-negative from both value-based predicates (IsNeverZero/IsNeverNegative) and flow-sensitive assertions. Only the latter are location-dependent and need the ordering side effect; a value-based proof (e.g. a constant non-zero divisor) holds everywhere, so pinning it would needlessly block CSE/hoisting. Gate SetHasOrderingSideEffect on the proof not being value-based. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/assertionprop.cpp | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index af4954db0ee250..c0fafdd155502e 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -4143,12 +4143,17 @@ GenTree* Compiler::optAssertionProp_ModDiv(ASSERT_VALARG_TP assertions, changed = true; } - // GTF_DIV_MOD_NO_BY_ZERO/GTF_DIV_MOD_NO_OVERFLOW make the divide appear non-throwing, - // which would otherwise let CSE/loop hoisting move it above the dominating check that - // proved the divisor safe (the flag is only valid at this node's location). Pin the - // node with an ordering side effect so it cannot be reordered above that check, even - // across opt-repeat iterations (see https://github.com/dotnet/runtime/issues/129386). - if ((tree->gtFlags & (GTF_DIV_MOD_NO_BY_ZERO | GTF_DIV_MOD_NO_OVERFLOW)) != 0) + // The no-throw flags above make the divide look non-throwing. When the safety proof + // is location-dependent (derived from a dominating assertion rather than from the + // operand's own value), the divide is only safe at this position, so CSE/loop hoisting + // must not move it above the check that justified the flag. Pin it with an ordering + // side effect in that case (see https://github.com/dotnet/runtime/issues/129386). A + // value-based proof (e.g. a constant non-zero divisor) holds everywhere, so such a + // divide stays freely movable and is not pinned. + const bool byZeroNeedsPin = op2IsNotZero && !op2->IsNeverZero(); + const bool overflowNeedsPin = (op1IsNotNegative || op2IsNotNegative) && + !op1->IsNeverNegative(this) && !op2->IsNeverNegative(this); + if (byZeroNeedsPin || overflowNeedsPin) { tree->SetHasOrderingSideEffect(); } From b7f1a28c31b2a20b0be3ee4d4a53ca3cfb2f2576 Mon Sep 17 00:00:00 2001 From: Julie Lee Date: Tue, 23 Jun 2026 14:39:11 -0700 Subject: [PATCH 05/10] Remove GitHub issue links from JIT code comments Address review feedback: drop the issue URLs from the div/mod ordering side-effect comments in assertionprop.cpp and gentree.cpp. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/assertionprop.cpp | 5 ++--- src/coreclr/jit/gentree.cpp | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index c0fafdd155502e..47ac997acae079 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -4147,9 +4147,8 @@ GenTree* Compiler::optAssertionProp_ModDiv(ASSERT_VALARG_TP assertions, // is location-dependent (derived from a dominating assertion rather than from the // operand's own value), the divide is only safe at this position, so CSE/loop hoisting // must not move it above the check that justified the flag. Pin it with an ordering - // side effect in that case (see https://github.com/dotnet/runtime/issues/129386). A - // value-based proof (e.g. a constant non-zero divisor) holds everywhere, so such a - // divide stays freely movable and is not pinned. + // side effect in that case. A value-based proof (e.g. a constant non-zero divisor) + // holds everywhere, so such a divide stays freely movable and is not pinned. const bool byZeroNeedsPin = op2IsNotZero && !op2->IsNeverZero(); const bool overflowNeedsPin = (op1IsNotNegative || op2IsNotNegative) && !op1->IsNeverNegative(this) && !op2->IsNeverNegative(this); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index b6688a88a466ad..746504d5e1b5e5 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -8804,7 +8804,7 @@ bool GenTree::OperSupportsOrderingSideEffect() const // DIV/MOD support an ordering side effect so that, once assertion propagation // proves the divide cannot throw (GTF_DIV_MOD_NO_BY_ZERO/GTF_DIV_MOD_NO_OVERFLOW) // and the node would otherwise look movable, it stays pinned below the dominating - // check that justified the flag (see https://github.com/dotnet/runtime/issues/129386). + // check that justified the flag. case GT_DIV: case GT_UDIV: case GT_MOD: From 80845e114ebf68ff22318ec55cfbfa8e8f210323 Mon Sep 17 00:00:00 2001 From: Julie Lee Date: Tue, 23 Jun 2026 15:08:54 -0700 Subject: [PATCH 06/10] Fix misleading JITDUMP message in optAssertionProp_ModDiv The message in the op2IsNotZero block said 'never negative'; it should say 'never zero' (it guards GTF_DIV_MOD_NO_BY_ZERO). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/assertionprop.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 47ac997acae079..f688d80739bc18 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -4131,7 +4131,7 @@ GenTree* Compiler::optAssertionProp_ModDiv(ASSERT_VALARG_TP assertions, if (op2IsNotZero) { - JITDUMP("Divisor for DIV/MOD is proven to be never negative...\n") + JITDUMP("Divisor for DIV/MOD is proven to be never zero...\n") tree->gtFlags |= GTF_DIV_MOD_NO_BY_ZERO; changed = true; } From 2b075f4b90bf238d66aea07f20b24a4422f2b3f5 Mon Sep 17 00:00:00 2001 From: Julie Lee Date: Wed, 24 Jun 2026 10:21:57 -0700 Subject: [PATCH 07/10] Apply jit-format to optAssertionProp_ModDiv pin check Fixes the 'Format jit codebase' CI failures: clang-format wants no = alignment and the overflowNeedsPin expression on a single wrapped line. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/assertionprop.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index f688d80739bc18..e7beab5ed78002 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -4149,9 +4149,9 @@ GenTree* Compiler::optAssertionProp_ModDiv(ASSERT_VALARG_TP assertions, // must not move it above the check that justified the flag. Pin it with an ordering // side effect in that case. A value-based proof (e.g. a constant non-zero divisor) // holds everywhere, so such a divide stays freely movable and is not pinned. - const bool byZeroNeedsPin = op2IsNotZero && !op2->IsNeverZero(); - const bool overflowNeedsPin = (op1IsNotNegative || op2IsNotNegative) && - !op1->IsNeverNegative(this) && !op2->IsNeverNegative(this); + const bool byZeroNeedsPin = op2IsNotZero && !op2->IsNeverZero(); + const bool overflowNeedsPin = + (op1IsNotNegative || op2IsNotNegative) && !op1->IsNeverNegative(this) && !op2->IsNeverNegative(this); if (byZeroNeedsPin || overflowNeedsPin) { tree->SetHasOrderingSideEffect(); From 93c88d55ca579585c70d3f170ad102636ef5f239 Mon Sep 17 00:00:00 2001 From: Julie Lee Date: Wed, 24 Jun 2026 10:49:57 -0700 Subject: [PATCH 08/10] Address review: gate overflow pin to signed div/mod; clarify comment - overflowNeedsPin now only applies to GT_DIV/GT_MOD: GTF_DIV_MOD_NO_OVERFLOW affects exceptions only for signed div/mod, so pinning unsigned nodes for an overflow proof was unnecessary and could inhibit CSE/hoisting. The by-zero pin still covers unsigned div/mod. - gentree.cpp: comment now names DIV/UDIV/MOD/UMOD to match the enabled ops. Formatted with clang-format 17.0.6. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/assertionprop.cpp | 6 ++++-- src/coreclr/jit/gentree.cpp | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index e7beab5ed78002..93fe4ced7b34eb 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -4150,8 +4150,10 @@ GenTree* Compiler::optAssertionProp_ModDiv(ASSERT_VALARG_TP assertions, // side effect in that case. A value-based proof (e.g. a constant non-zero divisor) // holds everywhere, so such a divide stays freely movable and is not pinned. const bool byZeroNeedsPin = op2IsNotZero && !op2->IsNeverZero(); - const bool overflowNeedsPin = - (op1IsNotNegative || op2IsNotNegative) && !op1->IsNeverNegative(this) && !op2->IsNeverNegative(this); + // Overflow (ArithmeticException) is only possible for signed div/mod, so only those need + // pinning for an assertion-based no-overflow proof. + const bool overflowNeedsPin = tree->OperIs(GT_DIV, GT_MOD) && (op1IsNotNegative || op2IsNotNegative) && + !op1->IsNeverNegative(this) && !op2->IsNeverNegative(this); if (byZeroNeedsPin || overflowNeedsPin) { tree->SetHasOrderingSideEffect(); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 746504d5e1b5e5..5dde92d3ee1538 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -8801,7 +8801,7 @@ bool GenTree::OperSupportsOrderingSideEffect() const case GT_LOCKADD: case GT_CMPXCHG: case GT_MEMORYBARRIER: - // DIV/MOD support an ordering side effect so that, once assertion propagation + // DIV/UDIV/MOD/UMOD support an ordering side effect so that, once assertion propagation // proves the divide cannot throw (GTF_DIV_MOD_NO_BY_ZERO/GTF_DIV_MOD_NO_OVERFLOW) // and the node would otherwise look movable, it stays pinned below the dominating // check that justified the flag. From 8d68d27b22daa02abbb3592cef53997b81cfe0ce Mon Sep 17 00:00:00 2001 From: Julie Lee Date: Wed, 24 Jun 2026 10:56:28 -0700 Subject: [PATCH 09/10] Clarify div/mod ordering-side-effect comment for signed vs unsigned GTF_DIV_MOD_NO_OVERFLOW only applies to signed div/mod; unsigned div/mod is pinned via GTF_DIV_MOD_NO_BY_ZERO only. Make the comment precise. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/gentree.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 5dde92d3ee1538..b311e090270872 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -8801,10 +8801,11 @@ bool GenTree::OperSupportsOrderingSideEffect() const case GT_LOCKADD: case GT_CMPXCHG: case GT_MEMORYBARRIER: - // DIV/UDIV/MOD/UMOD support an ordering side effect so that, once assertion propagation - // proves the divide cannot throw (GTF_DIV_MOD_NO_BY_ZERO/GTF_DIV_MOD_NO_OVERFLOW) - // and the node would otherwise look movable, it stays pinned below the dominating - // check that justified the flag. + // DIV/UDIV/MOD/UMOD support an ordering side effect so that, once assertion + // propagation proves the divide cannot throw (GTF_DIV_MOD_NO_BY_ZERO for any of + // them, plus GTF_DIV_MOD_NO_OVERFLOW for the signed ones) and the node would + // otherwise look movable, it stays pinned below the dominating check that + // justified the flag. case GT_DIV: case GT_UDIV: case GT_MOD: From 8d39cff28201862700f6f5dd1008c39237fb4a26 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Wed, 24 Jun 2026 19:58:38 +0200 Subject: [PATCH 10/10] Update assertionprop.cpp --- src/coreclr/jit/assertionprop.cpp | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 93fe4ced7b34eb..3ebf0b0492e1ec 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -4129,34 +4129,20 @@ GenTree* Compiler::optAssertionProp_ModDiv(ASSERT_VALARG_TP assertions, changed = true; } - if (op2IsNotZero) + if (op2IsNotZero && ((tree->gtFlags & GTF_DIV_MOD_NO_BY_ZERO) == 0)) { - JITDUMP("Divisor for DIV/MOD is proven to be never zero...\n") + JITDUMP("Divisor for DIV/MOD is proven to be never negative...\n") tree->gtFlags |= GTF_DIV_MOD_NO_BY_ZERO; + tree->SetHasOrderingSideEffect(); changed = true; } - if (op1IsNotNegative || op2IsNotNegative) + if ((op1IsNotNegative || op2IsNotNegative) && ((tree->gtFlags & GTF_DIV_MOD_NO_OVERFLOW) == 0)) { JITDUMP("DIV/MOD is proven to never overflow...\n") tree->gtFlags |= GTF_DIV_MOD_NO_OVERFLOW; - changed = true; - } - - // The no-throw flags above make the divide look non-throwing. When the safety proof - // is location-dependent (derived from a dominating assertion rather than from the - // operand's own value), the divide is only safe at this position, so CSE/loop hoisting - // must not move it above the check that justified the flag. Pin it with an ordering - // side effect in that case. A value-based proof (e.g. a constant non-zero divisor) - // holds everywhere, so such a divide stays freely movable and is not pinned. - const bool byZeroNeedsPin = op2IsNotZero && !op2->IsNeverZero(); - // Overflow (ArithmeticException) is only possible for signed div/mod, so only those need - // pinning for an assertion-based no-overflow proof. - const bool overflowNeedsPin = tree->OperIs(GT_DIV, GT_MOD) && (op1IsNotNegative || op2IsNotNegative) && - !op1->IsNeverNegative(this) && !op2->IsNeverNegative(this); - if (byZeroNeedsPin || overflowNeedsPin) - { tree->SetHasOrderingSideEffect(); + changed = true; } return changed ? optAssertionProp_Update(tree, tree, stmt) : nullptr;