From d458b0641e9561619480f97922769f182eaf00d7 Mon Sep 17 00:00:00 2001 From: David Hartglass Date: Wed, 5 Aug 2026 11:46:19 -0700 Subject: [PATCH 01/11] add testcase --- .../JitBlue/Runtime_129298/Runtime_129298.cs | 62 +++++++++++++++++++ .../JIT/Regression/Regression_ro_2.csproj | 1 + 2 files changed, 63 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_129298/Runtime_129298.cs diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_129298/Runtime_129298.cs b/src/tests/JIT/Regression/JitBlue/Runtime_129298/Runtime_129298.cs new file mode 100644 index 00000000000000..5b437d37beeeb9 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_129298/Runtime_129298.cs @@ -0,0 +1,62 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Testcase exposed an ROR node with an out of range operand on arm64, +// asserted in lowering + +using System; +using System.Numerics; +using System.Runtime.CompilerServices; +using Xunit; + +public class Runtime_129298 +{ + private static volatile uint Input_p0 = 1; + private static volatile uint Input_p1 = 1; + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + public static uint Fn(uint p0, uint p1) + { + unchecked + { + uint v1, v3, v5, v6, v8, v11, v15, v16, v22, v26, v28; + uint v33 = 0, v40, v44; + uint v21 = 0; + int v2, v4, v7, v9, v10, v29; + v1 = p0 % p1; + v2 = BitOperations.LeadingZeroCount(v1); v3 = (uint)v2; + v4 = BitOperations.LeadingZeroCount(v1); v5 = (uint)v4; + v6 = BitOperations.RotateLeft(p0, 0); + v7 = BitOperations.IsPow2(0x80000000u) ? 1 : 0; v8 = (uint)v7; + v9 = (p1 < p0) ? 1 : 0; + if (v9 == 0) + { + v22 = Math.Min(0xFFFFFFFEu, 0xFFFFFFFFu); + return v22; + } + v10 = (v1 > p1) ? 1 : 0; v11 = (uint)v10; + v15 = v6 + 0x12345u; v16 = v11 + p1; + v26 = BitOperations.RotateLeft(0xFFFFFFFFu, (int)v21); + v28 = p1 ^ 3u; + v29 = (v15 <= v16) ? 1 : 0; + if (v29 != 0) + { + v33 = (uint)BitOperations.TrailingZeroCount(v16); + return v33 ^ v5; + } + // unreached at runtime; v33 = 0 (default-init) reaches Lowering. + v40 = BitOperations.RotateLeft(p0, (int)(0x7FFFFFFEu % v33)); + v44 = v40 ^ v5; + return v44; + } + } + + [Fact] + public static void TestEntryPoint() + { + // The JIT must compile Fn (including the dead rotate block) under + // FullOpts without asserting. For the given inputs Fn takes the first + // early return, so the result is Math.Min(0xFFFFFFFE, 0xFFFFFFFF). + Assert.Equal(0xFFFFFFFEu, Fn(Input_p0, Input_p1)); + } +} diff --git a/src/tests/JIT/Regression/Regression_ro_2.csproj b/src/tests/JIT/Regression/Regression_ro_2.csproj index 4a5a409958c8cb..9b821d1dff3bfa 100644 --- a/src/tests/JIT/Regression/Regression_ro_2.csproj +++ b/src/tests/JIT/Regression/Regression_ro_2.csproj @@ -106,6 +106,7 @@ + From 7c2c432226f054a84ab6d9d655df5e2503f37edf Mon Sep 17 00:00:00 2001 From: David Hartglass Date: Wed, 5 Aug 2026 12:08:53 -0700 Subject: [PATCH 02/11] initial impl --- src/coreclr/jit/lower.cpp | 47 ++++++++++++++++++++++++++++++--------- src/coreclr/jit/lower.h | 1 + src/coreclr/jit/morph.cpp | 25 +++++++++++++++++++++ 3 files changed, 62 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 54bca2b4a8995c..5ebc5d011ebb09 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -535,6 +535,12 @@ GenTree* Lowering::LowerNode(GenTree* node) return next; } +#if defined(TARGET_XARCH) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) + // These targets mask the rotate amount implicitly, so strip a redundant + // AND(amount, mask) before lowering the rotate. + TryRemoveShiftRotateMask(node->AsOp()); +#endif + LowerRotate(node); break; } @@ -8812,30 +8818,32 @@ bool Lowering::TryFoldBinop(GenTreeOp* node) } //------------------------------------------------------------------------ -// LowerShift: Lower shift nodes +// TryRemoveShiftRotateMask: Remove a redundant mask on a shift or rotate count. // // Arguments: -// shift - the shift node (GT_LSH, GT_RSH or GT_RSZ) +// op - the shift or rotate node (GT_LSH, GT_RSH, GT_RSZ, GT_ROL or GT_ROR) // // Notes: -// Remove unnecessary shift count masking, xarch shift instructions -// mask the shift count to 5 bits (or 6 bits for 64 bit operations). +// On the targets that call this, the shift/rotate instructions mask the +// count to 5 bits (or 6 bits for 64 bit operations), so an explicit +// AND(count, mask) that keeps at least those low bits is redundant and can +// be removed. // -void Lowering::LowerShift(GenTreeOp* shift) +void Lowering::TryRemoveShiftRotateMask(GenTreeOp* op) { - assert(shift->OperIs(GT_LSH, GT_RSH, GT_RSZ)); + assert(op->OperIs(GT_LSH, GT_RSH, GT_RSZ, GT_ROL, GT_ROR)); size_t mask = 0x1f; #ifdef TARGET_64BIT - if (varTypeIsLong(shift->TypeGet())) + if (varTypeIsLong(op->TypeGet())) { mask = 0x3f; } #else - assert(!varTypeIsLong(shift->TypeGet())); + assert(!varTypeIsLong(op->TypeGet())); #endif - for (GenTree* andOp = shift->gtGetOp2(); andOp->OperIs(GT_AND); andOp = andOp->gtGetOp1()) + for (GenTree* andOp = op->gtGetOp2(); andOp->OperIs(GT_AND); andOp = andOp->gtGetOp1()) { GenTree* maskOp = andOp->gtGetOp2(); @@ -8849,12 +8857,29 @@ void Lowering::LowerShift(GenTreeOp* shift) break; } - shift->gtOp2 = andOp->gtGetOp1(); + op->gtOp2 = andOp->gtGetOp1(); BlockRange().Remove(andOp); BlockRange().Remove(maskOp); // The parent was replaced, clear contain and regOpt flag. - shift->gtOp2->ClearContained(); + op->gtOp2->ClearContained(); } +} + +//------------------------------------------------------------------------ +// LowerShift: Lower shift nodes +// +// Arguments: +// shift - the shift node (GT_LSH, GT_RSH or GT_RSZ) +// +// Notes: +// Remove unnecessary shift count masking, xarch shift instructions +// mask the shift count to 5 bits (or 6 bits for 64 bit operations). +// +void Lowering::LowerShift(GenTreeOp* shift) +{ + assert(shift->OperIs(GT_LSH, GT_RSH, GT_RSZ)); + + TryRemoveShiftRotateMask(shift); ContainCheckShiftRotate(shift); diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index eb7b257709ac3f..1fd1435d676222 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -481,6 +481,7 @@ class Lowering final : public Phase GenTree* LowerStoreLoc(GenTreeLclVarCommon* tree); void LowerRotate(GenTree* tree); void LowerShift(GenTreeOp* shift); + void TryRemoveShiftRotateMask(GenTreeOp* op); bool TryFoldBinop(GenTreeOp* node); #ifdef FEATURE_HW_INTRINSICS GenTree* LowerHWIntrinsic(GenTreeHWIntrinsic* node); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 9ccd9f1ceafa4c..2f3ce7f0e30332 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -12534,6 +12534,31 @@ GenTree* Compiler::fgRecognizeAndMorphBitwiseRotation(GenTree* tree) { noway_assert(GenTree::OperIsRotate(rotateOp)); + // The rotate amount must be in the range [0, bitsize - 1]. IL masks shift/rotate + // amounts explicitly, but the recognition above stripped that mask off the index. + // Reintroduce it so a later constant fold cannot leave an out-of-range amount that + // trips downstream invariants (e.g. arm64 containment in lowering): + // - an in-range constant needs no change, + // - an out-of-range constant is masked in place, + // - a non-constant amount is wrapped in AND(amount, bitsize - 1). + // Targets whose rotate instructions mask the amount implicitly strip the AND again + // in lowering. + if (rotateIndex->IsCnsIntOrI()) + { + ssize_t rotateAmount = rotateIndex->AsIntCon()->IconValue(); + + if ((rotateAmount < 0) || (rotateAmount > minimalMask)) + { + rotateIndex->AsIntCon()->SetIconValue(rotateAmount & minimalMask); + } + } + else + { + rotateIndex = + gtNewOperNode(GT_AND, genActualType(rotateIndex), rotateIndex, gtNewIconNode(minimalMask)); + rotateIndex->SetMorphed(this, /* doChildren */ true); + } + GenTreeFlags inputTreeEffects = tree->gtFlags & GTF_ALL_EFFECT; // We can use the same tree only during global morph; reusing the tree in a later morph From 3593535003adc045f2ab3649a9a3629b5bd77f5b Mon Sep 17 00:00:00 2001 From: David Hartglass Date: Wed, 5 Aug 2026 13:07:22 -0700 Subject: [PATCH 03/11] explicit masking for some operands in importercalls --- src/coreclr/jit/importercalls.cpp | 46 ++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 775339048a6af1..d97290199db784 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6607,9 +6607,28 @@ GenTree* Compiler::impPrimitiveNamedIntrinsic(NamedIntrinsic intrinsic, GenTree* op2 = impStackTop().val; + unsigned rotateMask = varTypeIsLong(baseType) ? 0x3F : 0x1F; + if (!op2->IsIntegralConst()) { - // TODO-CQ: ROL currently expects op2 to be a constant +#ifndef TARGET_64BIT + if (varTypeIsLong(baseType)) + { + // TODO-X86-CQ: variable-sized long rotates need special handling on 32-bit. + break; + } +#endif // !TARGET_64BIT + + // The rotate amount is not a constant. Import ROL(op1, AND(op2, mask)) directly + // instead of bailing to the managed fallback. The native rotate instructions mask + // the amount implicitly on the platforms we care about; the explicit AND keeps the + // IR in the masked form the rest of the JIT expects and is stripped again in + // lowering where possible. + impPopStack(); + GenTree* rotateValue = impPopStack().val; + GenTree* rotateAmount = gtNewOperNode(GT_AND, genActualType(op2), op2, + gtNewIconNode(rotateMask, genActualType(op2))); + result = gtNewOperNode(GT_ROL, baseType, rotateValue, rotateAmount); break; } @@ -6620,7 +6639,7 @@ GenTree* Compiler::impPrimitiveNamedIntrinsic(NamedIntrinsic intrinsic, uint32_t cns2 = static_cast(op2->AsIntConCommon()->IconValue()); // Mask the offset to ensure deterministic xplat behavior for overshifting - cns2 &= varTypeIsLong(baseType) ? 0x3F : 0x1F; + cns2 &= rotateMask; if (cns2 == 0) { @@ -6656,9 +6675,28 @@ GenTree* Compiler::impPrimitiveNamedIntrinsic(NamedIntrinsic intrinsic, GenTree* op2 = impStackTop().val; + unsigned rotateMask = varTypeIsLong(baseType) ? 0x3F : 0x1F; + if (!op2->IsIntegralConst()) { - // TODO-CQ: ROR currently expects op2 to be a constant +#ifndef TARGET_64BIT + if (varTypeIsLong(baseType)) + { + // TODO-X86-CQ: variable-sized long rotates need special handling on 32-bit. + break; + } +#endif // !TARGET_64BIT + + // The rotate amount is not a constant. Import ROR(op1, AND(op2, mask)) directly + // instead of bailing to the managed fallback. The native rotate instructions mask + // the amount implicitly on the platforms we care about; the explicit AND keeps the + // IR in the masked form the rest of the JIT expects and is stripped again in + // lowering where possible. + impPopStack(); + GenTree* rotateValue = impPopStack().val; + GenTree* rotateAmount = gtNewOperNode(GT_AND, genActualType(op2), op2, + gtNewIconNode(rotateMask, genActualType(op2))); + result = gtNewOperNode(GT_ROR, baseType, rotateValue, rotateAmount); break; } @@ -6669,7 +6707,7 @@ GenTree* Compiler::impPrimitiveNamedIntrinsic(NamedIntrinsic intrinsic, uint32_t cns2 = static_cast(op2->AsIntConCommon()->IconValue()); // Mask the offset to ensure deterministic xplat behavior for overshifting - cns2 &= varTypeIsLong(baseType) ? 0x3F : 0x1F; + cns2 &= rotateMask; if (cns2 == 0) { From c833b2d8ee34485c60d20f98e2e5c5969c52b9a4 Mon Sep 17 00:00:00 2001 From: David Hartglass Date: Wed, 5 Aug 2026 13:27:21 -0700 Subject: [PATCH 04/11] move importer rotate logic to a common place --- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/importercalls.cpp | 205 +++++++++++++----------------- 2 files changed, 86 insertions(+), 120 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index f9cf55b8feeecb..c8288e4be4f049 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5366,6 +5366,7 @@ class Compiler CORINFO_SIG_INFO* sig R2RARG(CORINFO_CONST_LOOKUP* entryPoint), bool mustExpand); + GenTree* impRotateHelper(var_types baseType, genTreeOps rotateOper); #ifdef FEATURE_HW_INTRINSICS bool IsValidForShuffle(GenTree* indices, diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index d97290199db784..a5649d5ea444ea 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6048,6 +6048,89 @@ GenTree* Compiler::impSRCSUnsafeIntrinsic(NamedIntrinsic intrinsic, } } +//------------------------------------------------------------------------ +// impRotateHelper: import a NI_PRIMITIVE_RotateLeft or +// NI_PRIMITIVE_RotateRight intrinsic. +// +// Arguments: +// baseType - the type being rotated (TYP_INT or TYP_LONG) +// rotateOper - GT_ROL for RotateLeft, GT_ROR for RotateRight +// +// Returns: +// IR tree to use in place of the call, or nullptr if the jit should treat +// the intrinsic call like a normal call. +// +// Notes: +// Expects the value to rotate and the rotate amount to be on the top of the +// stack (rotate amount on top). Pops both when it produces a result. +// +GenTree* Compiler::impRotateHelper(var_types baseType, genTreeOps rotateOper) +{ + assert((rotateOper == GT_ROL) || (rotateOper == GT_ROR)); + + GenTree* op2 = impStackTop().val; + + unsigned rotateMask = varTypeIsLong(baseType) ? 0x3F : 0x1F; + + if (!op2->IsIntegralConst()) + { +#ifndef TARGET_64BIT + if (varTypeIsLong(baseType)) + { + // TODO-X86-CQ: variable-sized long rotates need special handling on 32-bit. + return nullptr; + } +#endif // !TARGET_64BIT + + // The rotate amount is not a constant. Import ROL/ROR(op1, AND(op2, mask)) directly + // instead of bailing to the managed fallback. The native rotate instructions mask + // the amount implicitly on the platforms we care about; the explicit AND keeps the + // IR in the masked form the rest of the JIT expects and is stripped again in + // lowering where possible. + impPopStack(); + GenTree* rotateValue = impPopStack().val; + GenTree* rotateAmount = gtNewOperNode(GT_AND, genActualType(op2), op2, + gtNewIconNode(rotateMask, genActualType(op2))); + return gtNewOperNode(rotateOper, baseType, rotateValue, rotateAmount); + } + + // Pop the value from the stack + impPopStack(); + + GenTree* op1 = impPopStack().val; + uint32_t cns2 = static_cast(op2->AsIntConCommon()->IconValue()); + + // Mask the offset to ensure deterministic xplat behavior for overshifting + cns2 &= rotateMask; + + if (cns2 == 0) + { + // No rotation is a nop + return op1; + } + + if (op1->IsIntegralConst()) + { + if (varTypeIsLong(baseType)) + { + uint64_t cns1 = static_cast(op1->AsIntConCommon()->LngValue()); + uint64_t res = + (rotateOper == GT_ROL) ? BitOperations::RotateLeft(cns1, cns2) : BitOperations::RotateRight(cns1, cns2); + return gtNewLconNode(res); + } + else + { + uint32_t cns1 = static_cast(op1->AsIntConCommon()->IconValue()); + uint32_t res = + (rotateOper == GT_ROL) ? BitOperations::RotateLeft(cns1, cns2) : BitOperations::RotateRight(cns1, cns2); + return gtNewIconNode(res, baseType); + } + } + + op2->AsIntConCommon()->SetIconValue(cns2); + return gtFoldExpr(gtNewOperNode(rotateOper, baseType, op1, op2)); +} + //------------------------------------------------------------------------ // impPrimitiveNamedIntrinsic: import a NamedIntrinsic representing a primitive operation // @@ -6605,66 +6688,7 @@ GenTree* Compiler::impPrimitiveNamedIntrinsic(NamedIntrinsic intrinsic, assert(sig->numArgs == 2); assert(!varTypeIsSmall(retType) && !varTypeIsSmall(baseType)); - GenTree* op2 = impStackTop().val; - - unsigned rotateMask = varTypeIsLong(baseType) ? 0x3F : 0x1F; - - if (!op2->IsIntegralConst()) - { -#ifndef TARGET_64BIT - if (varTypeIsLong(baseType)) - { - // TODO-X86-CQ: variable-sized long rotates need special handling on 32-bit. - break; - } -#endif // !TARGET_64BIT - - // The rotate amount is not a constant. Import ROL(op1, AND(op2, mask)) directly - // instead of bailing to the managed fallback. The native rotate instructions mask - // the amount implicitly on the platforms we care about; the explicit AND keeps the - // IR in the masked form the rest of the JIT expects and is stripped again in - // lowering where possible. - impPopStack(); - GenTree* rotateValue = impPopStack().val; - GenTree* rotateAmount = gtNewOperNode(GT_AND, genActualType(op2), op2, - gtNewIconNode(rotateMask, genActualType(op2))); - result = gtNewOperNode(GT_ROL, baseType, rotateValue, rotateAmount); - break; - } - - // Pop the value from the stack - impPopStack(); - - GenTree* op1 = impPopStack().val; - uint32_t cns2 = static_cast(op2->AsIntConCommon()->IconValue()); - - // Mask the offset to ensure deterministic xplat behavior for overshifting - cns2 &= rotateMask; - - if (cns2 == 0) - { - // No rotation is a nop - return op1; - } - - if (op1->IsIntegralConst()) - { - if (varTypeIsLong(baseType)) - { - uint64_t cns1 = static_cast(op1->AsIntConCommon()->LngValue()); - result = gtNewLconNode(BitOperations::RotateLeft(cns1, cns2)); - } - else - { - uint32_t cns1 = static_cast(op1->AsIntConCommon()->IconValue()); - result = gtNewIconNode(BitOperations::RotateLeft(cns1, cns2), baseType); - } - break; - } - - op2->AsIntConCommon()->SetIconValue(cns2); - result = gtFoldExpr(gtNewOperNode(GT_ROL, baseType, op1, op2)); - + result = impRotateHelper(baseType, GT_ROL); break; } @@ -6673,66 +6697,7 @@ GenTree* Compiler::impPrimitiveNamedIntrinsic(NamedIntrinsic intrinsic, assert(sig->numArgs == 2); assert(!varTypeIsSmall(retType) && !varTypeIsSmall(baseType)); - GenTree* op2 = impStackTop().val; - - unsigned rotateMask = varTypeIsLong(baseType) ? 0x3F : 0x1F; - - if (!op2->IsIntegralConst()) - { -#ifndef TARGET_64BIT - if (varTypeIsLong(baseType)) - { - // TODO-X86-CQ: variable-sized long rotates need special handling on 32-bit. - break; - } -#endif // !TARGET_64BIT - - // The rotate amount is not a constant. Import ROR(op1, AND(op2, mask)) directly - // instead of bailing to the managed fallback. The native rotate instructions mask - // the amount implicitly on the platforms we care about; the explicit AND keeps the - // IR in the masked form the rest of the JIT expects and is stripped again in - // lowering where possible. - impPopStack(); - GenTree* rotateValue = impPopStack().val; - GenTree* rotateAmount = gtNewOperNode(GT_AND, genActualType(op2), op2, - gtNewIconNode(rotateMask, genActualType(op2))); - result = gtNewOperNode(GT_ROR, baseType, rotateValue, rotateAmount); - break; - } - - // Pop the value from the stack - impPopStack(); - - GenTree* op1 = impPopStack().val; - uint32_t cns2 = static_cast(op2->AsIntConCommon()->IconValue()); - - // Mask the offset to ensure deterministic xplat behavior for overshifting - cns2 &= rotateMask; - - if (cns2 == 0) - { - // No rotation is a nop - return op1; - } - - if (op1->IsIntegralConst()) - { - if (varTypeIsLong(baseType)) - { - uint64_t cns1 = static_cast(op1->AsIntConCommon()->LngValue()); - result = gtNewLconNode(BitOperations::RotateRight(cns1, cns2)); - } - else - { - uint32_t cns1 = static_cast(op1->AsIntConCommon()->IconValue()); - result = gtNewIconNode(BitOperations::RotateRight(cns1, cns2), baseType); - } - break; - } - - op2->AsIntConCommon()->SetIconValue(cns2); - result = gtFoldExpr(gtNewOperNode(GT_ROR, baseType, op1, op2)); - + result = impRotateHelper(baseType, GT_ROR); break; } From 72cfcebdae1c33da44b8870b9275e0a6498f87f4 Mon Sep 17 00:00:00 2001 From: David Hartglass Date: Wed, 5 Aug 2026 14:45:32 -0700 Subject: [PATCH 05/11] remove comment --- src/coreclr/jit/importercalls.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index a5649d5ea444ea..12433fcb632de5 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6060,10 +6060,6 @@ GenTree* Compiler::impSRCSUnsafeIntrinsic(NamedIntrinsic intrinsic, // IR tree to use in place of the call, or nullptr if the jit should treat // the intrinsic call like a normal call. // -// Notes: -// Expects the value to rotate and the rotate amount to be on the top of the -// stack (rotate amount on top). Pops both when it produces a result. -// GenTree* Compiler::impRotateHelper(var_types baseType, genTreeOps rotateOper) { assert((rotateOper == GT_ROL) || (rotateOper == GT_ROR)); From bd64b0fdd6d993df45a16a33b8d49bc7ee8154c5 Mon Sep 17 00:00:00 2001 From: David Hartglass Date: Wed, 5 Aug 2026 15:20:20 -0700 Subject: [PATCH 06/11] reword comments --- src/coreclr/jit/lower.cpp | 6 ++---- src/coreclr/jit/morph.cpp | 13 ++++--------- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 5ebc5d011ebb09..cf52581da513e1 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -8824,10 +8824,8 @@ bool Lowering::TryFoldBinop(GenTreeOp* node) // op - the shift or rotate node (GT_LSH, GT_RSH, GT_RSZ, GT_ROL or GT_ROR) // // Notes: -// On the targets that call this, the shift/rotate instructions mask the -// count to 5 bits (or 6 bits for 64 bit operations), so an explicit -// AND(count, mask) that keeps at least those low bits is redundant and can -// be removed. +// Some targets' shift/rotate instructions mask their count to bitsize. +// Remove the explicit AND(count, mask) that keeps at least those low bits. // void Lowering::TryRemoveShiftRotateMask(GenTreeOp* op) { diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 2f3ce7f0e30332..edbf188b8ac88f 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -12534,15 +12534,10 @@ GenTree* Compiler::fgRecognizeAndMorphBitwiseRotation(GenTree* tree) { noway_assert(GenTree::OperIsRotate(rotateOp)); - // The rotate amount must be in the range [0, bitsize - 1]. IL masks shift/rotate - // amounts explicitly, but the recognition above stripped that mask off the index. - // Reintroduce it so a later constant fold cannot leave an out-of-range amount that - // trips downstream invariants (e.g. arm64 containment in lowering): - // - an in-range constant needs no change, - // - an out-of-range constant is masked in place, - // - a non-constant amount is wrapped in AND(amount, bitsize - 1). - // Targets whose rotate instructions mask the amount implicitly strip the AND again - // in lowering. + // Explicitly mask the rotate amount to the range [0, bitsize-1]. Otherwise, a later + // tranform can stick an out of range constant here and trip up lowering. If the + // target's rotate or shift instructions mask their operand implicitly, those targets + // remove this mask again during lowering. if (rotateIndex->IsCnsIntOrI()) { ssize_t rotateAmount = rotateIndex->AsIntCon()->IconValue(); From 74b848f05df8fc5c67d6c7a3b86b38b88df6890c Mon Sep 17 00:00:00 2001 From: David Hartglass Date: Wed, 5 Aug 2026 15:31:59 -0700 Subject: [PATCH 07/11] transform spelling typo in comment --- src/coreclr/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index edbf188b8ac88f..6dc43b1198125f 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -12535,7 +12535,7 @@ GenTree* Compiler::fgRecognizeAndMorphBitwiseRotation(GenTree* tree) noway_assert(GenTree::OperIsRotate(rotateOp)); // Explicitly mask the rotate amount to the range [0, bitsize-1]. Otherwise, a later - // tranform can stick an out of range constant here and trip up lowering. If the + // transform can stick an out of range constant here and trip up lowering. If the // target's rotate or shift instructions mask their operand implicitly, those targets // remove this mask again during lowering. if (rotateIndex->IsCnsIntOrI()) From 4fbaaa2b72c02cf96df5c617d3687f29d5744e6e Mon Sep 17 00:00:00 2001 From: David Hartglass Date: Thu, 6 Aug 2026 09:33:10 -0700 Subject: [PATCH 08/11] jit format --- src/coreclr/jit/importercalls.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 12433fcb632de5..63652672972460 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6084,9 +6084,9 @@ GenTree* Compiler::impRotateHelper(var_types baseType, genTreeOps rotateOper) // IR in the masked form the rest of the JIT expects and is stripped again in // lowering where possible. impPopStack(); - GenTree* rotateValue = impPopStack().val; - GenTree* rotateAmount = gtNewOperNode(GT_AND, genActualType(op2), op2, - gtNewIconNode(rotateMask, genActualType(op2))); + GenTree* rotateValue = impPopStack().val; + GenTree* rotateAmount = + gtNewOperNode(GT_AND, genActualType(op2), op2, gtNewIconNode(rotateMask, genActualType(op2))); return gtNewOperNode(rotateOper, baseType, rotateValue, rotateAmount); } From a74014d0d3691cafd6ca1c492bdceaeb9c4f8d61 Mon Sep 17 00:00:00 2001 From: David Hartglass Date: Thu, 6 Aug 2026 11:42:24 -0700 Subject: [PATCH 09/11] reword comment --- src/coreclr/jit/importercalls.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 63652672972460..2831f5d69214fb 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6078,11 +6078,8 @@ GenTree* Compiler::impRotateHelper(var_types baseType, genTreeOps rotateOper) } #endif // !TARGET_64BIT - // The rotate amount is not a constant. Import ROL/ROR(op1, AND(op2, mask)) directly - // instead of bailing to the managed fallback. The native rotate instructions mask - // the amount implicitly on the platforms we care about; the explicit AND keeps the - // IR in the masked form the rest of the JIT expects and is stripped again in - // lowering where possible. + // Import non-constant rotates as an explicitly masked ROL/ROR(op1, AND(op2, mask)) instead. + // Lowering will remove this mask if the target's rotate implicitly masks its operand. impPopStack(); GenTree* rotateValue = impPopStack().val; GenTree* rotateAmount = From d1984cdb53dbe71f6e353f4b540200c77825d1f9 Mon Sep 17 00:00:00 2001 From: David Hartglass Date: Fri, 7 Aug 2026 12:40:17 -0700 Subject: [PATCH 10/11] WASM PR feedback --- src/coreclr/jit/importercalls.cpp | 6 +++--- src/coreclr/jit/lower.cpp | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 2831f5d69214fb..c3c29a6a948a6e 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6070,13 +6070,13 @@ GenTree* Compiler::impRotateHelper(var_types baseType, genTreeOps rotateOper) if (!op2->IsIntegralConst()) { -#ifndef TARGET_64BIT +#if LOWER_DECOMPOSE_LONGS if (varTypeIsLong(baseType)) { - // TODO-X86-CQ: variable-sized long rotates need special handling on 32-bit. + // TODO-CQ: variable-sized long rotates need special handling on 32-bit. return nullptr; } -#endif // !TARGET_64BIT +#endif // LOWER_DECOMPOSE_LONGS // Import non-constant rotates as an explicitly masked ROL/ROR(op1, AND(op2, mask)) instead. // Lowering will remove this mask if the target's rotate implicitly masks its operand. diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index cf52581da513e1..793729d04582b2 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -535,7 +535,7 @@ GenTree* Lowering::LowerNode(GenTree* node) return next; } -#if defined(TARGET_XARCH) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) +#if defined(TARGET_XARCH) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) || defined(TARGET_WASM) // These targets mask the rotate amount implicitly, so strip a redundant // AND(amount, mask) before lowering the rotate. TryRemoveShiftRotateMask(node->AsOp()); @@ -8832,7 +8832,7 @@ void Lowering::TryRemoveShiftRotateMask(GenTreeOp* op) assert(op->OperIs(GT_LSH, GT_RSH, GT_RSZ, GT_ROL, GT_ROR)); size_t mask = 0x1f; -#ifdef TARGET_64BIT +#if !LOWER_DECOMPOSE_LONGS if (varTypeIsLong(op->TypeGet())) { mask = 0x3f; From df2ebf96e4e1f2c589e9ea5106aa0e85fb0e7339 Mon Sep 17 00:00:00 2001 From: David Hartglass Date: Fri, 7 Aug 2026 13:40:59 -0700 Subject: [PATCH 11/11] git format changes --- src/coreclr/jit/lower.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 793729d04582b2..6a17733504a6dd 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -535,7 +535,8 @@ GenTree* Lowering::LowerNode(GenTree* node) return next; } -#if defined(TARGET_XARCH) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) || defined(TARGET_WASM) +#if defined(TARGET_XARCH) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) || \ + defined(TARGET_WASM) // These targets mask the rotate amount implicitly, so strip a redundant // AND(amount, mask) before lowering the rotate. TryRemoveShiftRotateMask(node->AsOp());