Fix for out of range rotate amount during lower

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,

src/coreclr/jit/importercalls.cpp

@@ -6048,6 +6048,82 @@ 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.
+//
+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())
+    {
+#if LOWER_DECOMPOSE_LONGS
+        if (varTypeIsLong(baseType))
+        {
+            // TODO-CQ: variable-sized long rotates need special handling on 32-bit.
+            return nullptr;
+        }
+#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.
+        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<uint32_t>(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<uint64_t>(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<uint32_t>(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,47 +6681,7 @@ GenTree* Compiler::impPrimitiveNamedIntrinsic(NamedIntrinsic        intrinsic,
             assert(sig->numArgs == 2);
             assert(!varTypeIsSmall(retType) && !varTypeIsSmall(baseType));
 
-            GenTree* op2 = impStackTop().val;
-
-            if (!op2->IsIntegralConst())
-            {
-                // TODO-CQ: ROL currently expects op2 to be a constant
-                break;
-            }
-
-            // Pop the value from the stack
-            impPopStack();
-
-            GenTree* op1  = impPopStack().val;
-            uint32_t cns2 = static_cast<uint32_t>(op2->AsIntConCommon()->IconValue());
-
-            // Mask the offset to ensure deterministic xplat behavior for overshifting
-            cns2 &= varTypeIsLong(baseType) ? 0x3F : 0x1F;
-
-            if (cns2 == 0)
-            {
-                // No rotation is a nop
-                return op1;
-            }
-
-            if (op1->IsIntegralConst())
-            {
-                if (varTypeIsLong(baseType))
-                {
-                    uint64_t cns1 = static_cast<uint64_t>(op1->AsIntConCommon()->LngValue());
-                    result        = gtNewLconNode(BitOperations::RotateLeft(cns1, cns2));
-                }
-                else
-                {
-                    uint32_t cns1 = static_cast<uint32_t>(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;
         }
 
@@ -6654,47 +6690,7 @@ GenTree* Compiler::impPrimitiveNamedIntrinsic(NamedIntrinsic        intrinsic,
             assert(sig->numArgs == 2);
             assert(!varTypeIsSmall(retType) && !varTypeIsSmall(baseType));
 
-            GenTree* op2 = impStackTop().val;
-
-            if (!op2->IsIntegralConst())
-            {
-                // TODO-CQ: ROR currently expects op2 to be a constant
-                break;
-            }
-
-            // Pop the value from the stack
-            impPopStack();
-
-            GenTree* op1  = impPopStack().val;
-            uint32_t cns2 = static_cast<uint32_t>(op2->AsIntConCommon()->IconValue());
-
-            // Mask the offset to ensure deterministic xplat behavior for overshifting
-            cns2 &= varTypeIsLong(baseType) ? 0x3F : 0x1F;
-
-            if (cns2 == 0)
-            {
-                // No rotation is a nop
-                return op1;
-            }
-
-            if (op1->IsIntegralConst())
-            {
-                if (varTypeIsLong(baseType))
-                {
-                    uint64_t cns1 = static_cast<uint64_t>(op1->AsIntConCommon()->LngValue());
-                    result        = gtNewLconNode(BitOperations::RotateRight(cns1, cns2));
-                }
-                else
-                {
-                    uint32_t cns1 = static_cast<uint32_t>(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;
         }
 

src/coreclr/jit/lower.cpp

@@ -535,6 +535,13 @@ GenTree* Lowering::LowerNode(GenTree* node)
                 return next;
             }
 
+#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());
+#endif
+
             LowerRotate(node);
             break;
         }
@@ -8812,30 +8819,30 @@ 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).
+//    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::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 !LOWER_DECOMPOSE_LONGS
+    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 +8856,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);
 

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

src/coreclr/jit/morph.cpp

@@ -12534,6 +12534,26 @@ GenTree* Compiler::fgRecognizeAndMorphBitwiseRotation(GenTree* tree)
         {
             noway_assert(GenTree::OperIsRotate(rotateOp));
 
+            // Explicitly mask the rotate amount to the range [0, bitsize-1]. Otherwise, a later
+            // 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())
+            {
+                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

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

src/tests/JIT/Regression/Regression_ro_2.csproj

@@ -106,6 +106,7 @@
     <Compile Include="JitBlue\Runtime_129076\Runtime_129076.cs" />
     <Compile Include="JitBlue\Runtime_129176\Runtime_129176.cs" />
     <Compile Include="JitBlue\Runtime_129288\Runtime_129288.cs" />
+    <Compile Include="JitBlue\Runtime_129298\Runtime_129298.cs" />
     <Compile Include="JitBlue\Runtime_129527\Runtime_129527.cs" />
     <Compile Include="JitBlue\Runtime_129642\Runtime_129642.cs" />
     <Compile Include="JitBlue\Runtime_129970\Runtime_129970.cs" />