From 84efbfef89b40540704137475633e9d5d18baa52 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 12 Jun 2026 12:05:30 -0700 Subject: [PATCH 1/3] JIT: don't claim rotates set ZF on xarch ROL/ROR only update CF (and OF in the 1-bit form); SF/ZF/AF/PF are not affected for any count, so they don't belong in SupportsSettingZeroFlag's shift group. Fixes #129288. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/gentree.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 1a9afef97e5f18..3f734f8c04c0c7 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -21695,11 +21695,14 @@ bool GenTree::SupportsSettingZeroFlag() } #if defined(TARGET_XARCH) - if (OperIs(GT_LSH, GT_RSH, GT_RSZ, GT_ROL, GT_ROR)) + if (OperIs(GT_LSH, GT_RSH, GT_RSZ)) { - // Shift/Rotate instructions do not update the flags in case of count being zero. + // Shift instructions do not update the flags in case of count being zero. return gtGetOp2()->IsNeverZero(); } + // Note: GT_ROL / GT_ROR are intentionally NOT in this list. On xarch, + // ROL/ROR/RCL/RCR only update CF (and OF in the 1-bit form); SF/ZF/AF/PF + // are not affected for any count. if (OperIs(GT_AND, GT_OR, GT_XOR, GT_ADD, GT_SUB, GT_NEG)) { From d629caea28e54e1fedc015bd831478d4ee354408 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sat, 13 Jun 2026 08:11:43 -0700 Subject: [PATCH 2/3] JIT: add regression test for #129288 Verified to fail (exit 107 = 6 subtests wrong) under the pre-fix JIT and pass (exit 100) under the fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../JitBlue/Runtime_129288/Runtime_129288.cs | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_129288/Runtime_129288.cs diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_129288/Runtime_129288.cs b/src/tests/JIT/Regression/JitBlue/Runtime_129288/Runtime_129288.cs new file mode 100644 index 00000000000000..821cb79817b4b2 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_129288/Runtime_129288.cs @@ -0,0 +1,114 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// GenTree::SupportsSettingZeroFlag treated GT_ROL/GT_ROR as members of the +// shift group on xarch, which let lowering drop the TEST/CMP before a branch +// that compares the rotate's result to zero. ROL/ROR only modify CF (and OF +// for the 1-bit form) on xarch -- SF/ZF/AF/PF are not affected. So the +// branch read stale ZF from whatever flag-setting instruction the JIT had +// scheduled immediately before the rotate (a bounds check, in the original +// repro) and went the wrong direction. +// +// Each test below places a bounds check immediately before a rotate-then- +// branch-on-zero pattern. With x == 0 the rotate yields 0 and the +// (rotate == 0) branch must be taken. Pre-fix, the buggy code reads the +// bounds-check's ZF (zero == "indices equal", which they aren't) and falls +// through. + +namespace Runtime_129288; + +using System.Numerics; +using System.Runtime.CompilerServices; +using Xunit; + +public static class Runtime_129288 +{ + private static readonly int[] s_sink = new int[16]; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int RorUlongConst(ulong x, int idx) + { + s_sink[idx] = 1; // bounds check sets ZF + ulong r = BitOperations.RotateRight(x, 1); // ROR does not touch ZF + if (r == 0) return 0; // pre-fix: stale ZF -> wrong branch + return 1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int RolUlongConst(ulong x, int idx) + { + s_sink[idx] = 1; + ulong r = BitOperations.RotateLeft(x, 1); + if (r == 0) return 0; + return 1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int RorUintConst(uint x, int idx) + { + s_sink[idx] = 1; + uint r = BitOperations.RotateRight(x, 1); + if (r == 0) return 0; + return 1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int RolUintConst(uint x, int idx) + { + s_sink[idx] = 1; + uint r = BitOperations.RotateLeft(x, 1); + if (r == 0) return 0; + return 1; + } + + // Exercise the GT_LT-against-1 pattern from the original repro, which + // the JIT lowers to the same branch-on-rotate-zero shape for unsigned. + [MethodImpl(MethodImplOptions.NoInlining)] + private static int RorUlongLessThanOne(ulong x, int idx) + { + s_sink[idx] = 1; + ulong r = BitOperations.RotateRight(x, 1); + if (r < 1UL) return 0; + return 1; + } + + // Same shape with !=, which exercises the opposite branch polarity. + [MethodImpl(MethodImplOptions.NoInlining)] + private static int RorUlongConstNeZero(ulong x, int idx) + { + s_sink[idx] = 1; + ulong r = BitOperations.RotateRight(x, 1); + if (r != 0) return 1; + return 0; + } + + private static int Drive(ulong u64, uint u32, int idx, int passingValue) + { + int errors = 0; + if (RorUlongConst(u64, idx) != passingValue) errors++; + if (RolUlongConst(u64, idx) != passingValue) errors++; + if (RorUintConst(u32, idx) != passingValue) errors++; + if (RolUintConst(u32, idx) != passingValue) errors++; + if (RorUlongLessThanOne(u64, idx) != passingValue) errors++; + if (RorUlongConstNeZero(u64, idx) != passingValue) errors++; + return errors; + } + + [Fact] + public static int TestEntryPoint() + { + // CI runs JIT regression tests with tiering disabled, so the first + // call is FullOpts and the buggy lowering decision is exercised + // immediately. + int errors = 0; + + // x == 0: rotate yields 0, "== 0" branch must be taken. + errors += Drive(0UL, 0U, 0, 0); + + // Sanity: with a non-zero input the rotate is non-zero and the + // opposite branch must be taken. + errors += Drive(1UL, 1U, 1, 1); + + return errors == 0 ? 100 : 101 + errors; + } +} From 31b9e4058e5e5c77172c0c5a9bddd4db1320919c Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sat, 13 Jun 2026 08:27:35 -0700 Subject: [PATCH 3/3] JIT: clarify rotate-flags comment per review feedback Reword to match the IR: GT_ROL/GT_ROR cover only ROL/ROR (not RCL/RCR), and CF/OF on those are only updated when the rotate count is non-zero. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/gentree.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 3f734f8c04c0c7..d175d39a6912a2 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -21701,8 +21701,11 @@ bool GenTree::SupportsSettingZeroFlag() return gtGetOp2()->IsNeverZero(); } // Note: GT_ROL / GT_ROR are intentionally NOT in this list. On xarch, - // ROL/ROR/RCL/RCR only update CF (and OF in the 1-bit form); SF/ZF/AF/PF - // are not affected for any count. + // ROL/ROR leave SF/ZF/AF/PF unchanged regardless of the rotate count, so + // they cannot be used to skip a TEST/CMP before a branch on the rotate + // result. (Per the Intel SDM, CF -- and OF in the 1-bit form -- are only + // updated when the count is non-zero, but those aren't useful for a + // zero-flag check either.) if (OperIs(GT_AND, GT_OR, GT_XOR, GT_ADD, GT_SUB, GT_NEG)) {