diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index f9cf55b8feeecb..8fb51c8576565d 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4050,6 +4050,7 @@ class Compiler GenTree* gtFoldExprUnaryConstDbl(GenTreeUnOp* tree, GenTreeDblCon* dblCon); GenTree* gtFoldExprBinary(GenTreeOp* tree); + GenTree* gtFoldExprShiftCountMask(GenTreeOp* shift); GenTree* gtFoldExprBinaryConst(GenTreeOp* tree); GenTree* gtFoldExprBinaryConstInt(GenTreeOp* tree, GenTreeIntCon* intCon1, GenTreeIntCon* intCon2); GenTree* gtFoldExprBinaryConstLng(GenTreeOp* tree, GenTreeIntConCommon* intConCommon1, GenTreeIntConCommon* intConCommon2); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 0cb12dfca24794..b81d2a1743e1c1 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -15564,6 +15564,67 @@ GenTree* Compiler::gtFoldExprUnary(GenTreeUnOp* tree) return tree; } +//------------------------------------------------------------------------ +// gtFoldExprShiftCountMask: strip or normalize a redundant shift-count mask +// +// Arguments: +// shift - a GT_LSH, GT_RSH or GT_RSZ node +// +// Returns: +// The shift's op2 after any rewrite (the caller reloads its cached copy). +// +GenTree* Compiler::gtFoldExprShiftCountMask(GenTreeOp* shift) +{ + assert(shift->OperIs(GT_LSH, GT_RSH, GT_RSZ)); + + size_t width = varTypeIsLong(shift->TypeGet()) ? 0x3f : 0x1f; + + GenTree* count = shift->gtGetOp2(); + + if (count->OperIs(GT_AND)) + { + GenTree* andOp1 = count->gtGetOp1(); + GenTree* andOp2 = count->gtGetOp2(); + + GenTree* maskCns = nullptr; + GenTree* shiftCnt = nullptr; + + if (andOp2->IsCnsIntOrI()) + { + maskCns = andOp2; + shiftCnt = andOp1; + } + else if (andOp1->IsCnsIntOrI()) + { + maskCns = andOp1; + shiftCnt = andOp2; + } + if ((maskCns != nullptr) && ((static_cast(maskCns->AsIntCon()->IconValue()) & width) == width)) + { + JITDUMP("Removing redundant shift-count mask [%06u]\n", count->gtTreeID); + count = shiftCnt; + shift->gtOp2 = count; + // No need to call gtUpdateNodeSideEffects: AND(count, C) propagates exactly + // count's flags (C is a side-effect-free constant), so shift's flags are unchanged. + } + } + + // Normalize out-of-range constant counts. This also handles the case where the AND + // above was stripped and the inner count was itself a constant (e.g. AND(cns, cns)). + if (count->IsCnsIntOrI()) + { + size_t value = static_cast(count->AsIntCon()->IconValue()); + + if ((value & width) != value) + { + count->AsIntCon()->SetIconValue(static_cast(value & width)); + fgUpdateConstTreeValueNumber(count); + } + } + + return count; +} + //------------------------------------------------------------------------ // gtFoldExprBinary: see if a binary operation is foldable // @@ -15591,6 +15652,16 @@ GenTree* Compiler::gtFoldExprBinary(GenTreeOp* tree) return tree; } + if (tree->OperIs(GT_LSH, GT_RSH, GT_RSZ) && opts.OptimizationEnabled()) + { + // IL masks the shift count to the operand bit width, so `x >> n` is imported as + // `x >> (n & 31)` (or `& 63` for 64-bit). Strip the redundant mask in optimized + // compilations so it disappears before CSE and later phases never have to account + // for it. For targets where the hardware does not mask (currently ARM32), + // LowerShift re-inserts the AND so that lowered code remains correct. + op2 = gtFoldExprShiftCountMask(tree); + } + if (op1->OperIsConst()) { if (op2->OperIsConst()) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 54bca2b4a8995c..4df8f6e2b62b2e 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -556,11 +556,7 @@ GenTree* Lowering::LowerNode(GenTree* node) return next; } -#if defined(TARGET_XARCH) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) LowerShift(node->AsOp()); -#else - ContainCheckShiftRotate(node->AsOp()); -#endif break; } @@ -8818,13 +8814,19 @@ bool Lowering::TryFoldBinop(GenTreeOp* node) // 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). +// On targets where hardware masks the shift count to the operand width (XARCH, ARM64, +// LOONGARCH64, RISCV64), nothing to do -- gtFoldExprShiftCountMask strips the redundant +// AND earlier. +// +// On ARM32 the hardware uses Rs[7:0] without masking mod 32, so counts >= 32 give 0 +// rather than wrapping. Insert AND(count, 31) for variable-count shifts to restore the +// masking semantics that were stripped in gtFoldExprShiftCountMask. // void Lowering::LowerShift(GenTreeOp* shift) { assert(shift->OperIs(GT_LSH, GT_RSH, GT_RSZ)); +#if defined(TARGET_XARCH) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) size_t mask = 0x1f; #ifdef TARGET_64BIT if (varTypeIsLong(shift->TypeGet())) @@ -8852,9 +8854,44 @@ void Lowering::LowerShift(GenTreeOp* shift) shift->gtOp2 = andOp->gtGetOp1(); BlockRange().Remove(andOp); BlockRange().Remove(maskOp); - // The parent was replaced, clear contain and regOpt flag. shift->gtOp2->ClearContained(); } +#elif defined(TARGET_ARM) + // ARM32 uses Rs[7:0] as the shift count; counts in [32, 255] give 0 (LSL/LSR) or + // replicated sign (ASR) rather than masking mod 32. Insert AND(count, 31) so the + // hardware sees a value in [0, 31]. Skip when the count is already provably in + // [0, 31]: a constant (handled above) or AND(x, C) where C fits in 5 bits. + { + assert(!varTypeIsLong(shift->TypeGet())); + constexpr ssize_t mask = 0x1f; + + GenTree* shiftBy = shift->gtGetOp2(); + if (!shiftBy->IsCnsIntOrI()) + { + // Skip insertion when the count is already AND(x, C) where C <= 31, + // which guarantees the result is in [0, 31]. A bare GT_AND is not + // sufficient: user-written masks like (y & 0xFF) don't bound to [0, 31]. + bool alreadyMasked = false; + if (shiftBy->OperIs(GT_AND)) + { + GenTree* andOp2 = shiftBy->gtGetOp2(); + if (andOp2->IsCnsIntOrI() && ((static_cast(andOp2->AsIntCon()->IconValue()) & ~mask) == 0)) + { + alreadyMasked = true; + } + } + + if (!alreadyMasked) + { + GenTree* maskCns = m_compiler->gtNewIconNode(mask); + GenTree* andNode = m_compiler->gtNewOperNode(GT_AND, TYP_INT, shiftBy, maskCns); + BlockRange().InsertBefore(shift, maskCns); + BlockRange().InsertBefore(shift, andNode); + shift->AsOp()->gtOp2 = andNode; + } + } + } +#endif ContainCheckShiftRotate(shift); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 29016915de484d..cec9e059b699a2 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -12342,9 +12342,7 @@ GenTree* Compiler::fgRecognizeAndMorphBitwiseRotation(GenTree* tree) // / \ / \. // LSH RSZ -> x y // / \ / \. - // x AND x AND - // / \ / \. - // y 31 ADD 31 + // x y x ADD // / \. // NEG 32 // | @@ -12354,7 +12352,7 @@ GenTree* Compiler::fgRecognizeAndMorphBitwiseRotation(GenTree* tree) // (x >>> ((-y + N) & M)) op (x << (y & M)) // // (x << y) op (x >>> (-y + N)) - // (x >> > (-y + N)) op (x << y) + // (x >>> (-y + N)) op (x << y) // // (x >>> (y & M)) op (x << ((-y + N) & M)) // (x << ((-y + N) & M)) op (x >>> (y & M)) @@ -12416,46 +12414,69 @@ GenTree* Compiler::fgRecognizeAndMorphBitwiseRotation(GenTree* tree) GenTree* rightShiftIndex = rightShiftTree->gtGetOp2(); // The shift index may be masked. At least (rotatedValueBitSize - 1) lower bits - // shouldn't be masked for the transformation to be valid. If additional - // higher bits are not masked, the transformation is still valid since the result - // of MSIL shift instructions is unspecified if the shift amount is greater or equal - // than the width of the value being shifted. + // must be unmasked for the transformation to be valid. ssize_t minimalMask = rotatedValueBitSize - 1; ssize_t leftShiftMask = -1; ssize_t rightShiftMask = -1; if (leftShiftIndex->OperIs(GT_AND)) { - if (leftShiftIndex->gtGetOp2()->IsCnsIntOrI()) + GenTree* andOp1 = leftShiftIndex->gtGetOp1(); + GenTree* andOp2 = leftShiftIndex->gtGetOp2(); + + GenTree* maskOp = nullptr; + GenTree* indexOp = nullptr; + + if (andOp2->IsCnsIntOrI()) { - leftShiftMask = leftShiftIndex->gtGetOp2()->AsIntCon()->IconValue(); - leftShiftIndex = leftShiftIndex->gtGetOp1(); + maskOp = andOp2; + indexOp = andOp1; } - else + else if (andOp1->IsCnsIntOrI()) + { + maskOp = andOp1; + indexOp = andOp2; + } + + if (maskOp == nullptr) { return nullptr; } + + leftShiftMask = maskOp->AsIntCon()->IconValue(); + leftShiftIndex = indexOp; } if (rightShiftIndex->OperIs(GT_AND)) { - if (rightShiftIndex->gtGetOp2()->IsCnsIntOrI()) + GenTree* andOp1 = rightShiftIndex->gtGetOp1(); + GenTree* andOp2 = rightShiftIndex->gtGetOp2(); + + GenTree* maskOp = nullptr; + GenTree* indexOp = nullptr; + + if (andOp2->IsCnsIntOrI()) { - rightShiftMask = rightShiftIndex->gtGetOp2()->AsIntCon()->IconValue(); - rightShiftIndex = rightShiftIndex->gtGetOp1(); + maskOp = andOp2; + indexOp = andOp1; } - else + else if (andOp1->IsCnsIntOrI()) + { + maskOp = andOp1; + indexOp = andOp2; + } + + if (maskOp == nullptr) { return nullptr; } + + rightShiftMask = maskOp->AsIntCon()->IconValue(); + rightShiftIndex = indexOp; } if (((minimalMask & leftShiftMask) != minimalMask) || ((minimalMask & rightShiftMask) != minimalMask)) { - // The shift index is overmasked, e.g., we have - // something like (x << y & 15) or - // (x >> (32 - y) & 15 with 32 bit x. - // The transformation is not valid. return nullptr; } @@ -12488,12 +12509,9 @@ GenTree* Compiler::fgRecognizeAndMorphBitwiseRotation(GenTree* tree) if (GenTree::Compare(shiftIndexWithAdd->gtGetOp1()->gtGetOp1(), shiftIndexWithoutAdd)) { // We found one of these patterns: - // (x << (y & M)) | (x >>> ((-y + N) & M)) // (x << y) | (x >>> (-y + N)) - // (x >>> (y & M)) | (x << ((-y + N) & M)) // (x >>> y) | (x << (-y + N)) - // where N == bitsize(x), M is const, and - // M & (N - 1) == N - 1 + // where N == bitsize(x) #ifndef TARGET_64BIT if (!shiftIndexWithoutAdd->IsCnsIntOrI() && (rotatedValueBitSize == 64)) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 84b21479f40c61..6a8fd6bbf3919b 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -733,7 +733,7 @@ T ValueNumStore::EvalOpSpecialized(VNFunc vnf, T v0, T v1) } else { - return v0 << v1; + return v0 << (v1 & 0x1F); } case GT_RSH: if (sizeof(T) == 8) @@ -742,7 +742,7 @@ T ValueNumStore::EvalOpSpecialized(VNFunc vnf, T v0, T v1) } else { - return v0 >> v1; + return v0 >> (v1 & 0x1F); } case GT_RSZ: if (sizeof(T) == 8) @@ -751,7 +751,7 @@ T ValueNumStore::EvalOpSpecialized(VNFunc vnf, T v0, T v1) } else { - return UINT32(v0) >> v1; + return UINT32(v0) >> (v1 & 0x1F); } case GT_ROL: if (sizeof(T) == 8) @@ -13695,6 +13695,7 @@ void Compiler::fgValueNumberTree(GenTree* tree) ValueNumPair op2vnp; ValueNumPair op2Xvnp; vnStore->VNPUnpackExc(tree->AsOp()->gtOp2->gtVNPair, &op2vnp, &op2Xvnp); + ValueNumPair excSetPair = vnStore->VNPExcSetUnion(op1Xvnp, op2Xvnp); ValueNum newVN = ValueNumStore::NoVN; diff --git a/src/tests/JIT/Directed/shift/ShiftMaskCSE.cs b/src/tests/JIT/Directed/shift/ShiftMaskCSE.cs new file mode 100644 index 00000000000000..a201a6e430e747 --- /dev/null +++ b/src/tests/JIT/Directed/shift/ShiftMaskCSE.cs @@ -0,0 +1,106 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// + +using System; +using System.Runtime.CompilerServices; +using Xunit; + +// The shift count is masked to the operand bit width by IL semantics, so `x >> n` +// is imported as `x >> (n & 31)` (or `& 63` for 64-bit). When that masked count is +// shared between two shifts it gets CSE'd, and the JIT must still produce the same +// result as an unshared masked shift for every count, including counts past the +// operand bit width where the masking is observable. +namespace ShiftMaskCSE +{ + public static class Test + { + [MethodImpl(MethodImplOptions.NoInlining)] + static uint ShiftAndCSE_U32(uint foo, int shift) + { + uint res = foo >> shift; + res <<= shift; + return res; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static uint ShiftAndCSE_U32_Ref(uint foo, int shift) + { + int s = shift & 31; + uint res = foo >> s; + res <<= s; + return res; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int ShiftAndCSE_I32(int foo, int shift) + { + int res = foo >> shift; + res <<= shift; + return res; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int ShiftAndCSE_I32_Ref(int foo, int shift) + { + int s = shift & 31; + int res = foo >> s; + res <<= s; + return res; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static ulong ShiftAndCSE_U64(ulong foo, int shift) + { + ulong res = foo >> shift; + res <<= shift; + return res; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static ulong ShiftAndCSE_U64_Ref(ulong foo, int shift) + { + int s = shift & 63; + ulong res = foo >> s; + res <<= s; + return res; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static long ShiftAndCSE_I64(long foo, int shift) + { + long res = foo >> shift; + res <<= shift; + return res; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static long ShiftAndCSE_I64_Ref(long foo, int shift) + { + int s = shift & 63; + long res = foo >> s; + res <<= s; + return res; + } + + [Fact] + public static void TestEntryPoint() + { + // Counts span past the bit width (and negative), exercising the masking. + foreach (int shift in new int[] { 0, 1, 5, 31, 32, 33, 63, 64, 65, 100, -1, -31, -32 }) + { + foreach (uint foo in new uint[] { 0u, 1u, 0xF0F0F0F0u, 0xFFFFFFFFu, 0x12345678u }) + { + Assert.Equal(ShiftAndCSE_U32_Ref(foo, shift), ShiftAndCSE_U32(foo, shift)); + Assert.Equal(ShiftAndCSE_I32_Ref((int)foo, shift), ShiftAndCSE_I32((int)foo, shift)); + } + + foreach (ulong foo in new ulong[] { 0ul, 1ul, 0xF0F0F0F0F0F0F0F0ul, 0xFFFFFFFFFFFFFFFFul, 0x123456789ABCDEF0ul }) + { + Assert.Equal(ShiftAndCSE_U64_Ref(foo, shift), ShiftAndCSE_U64(foo, shift)); + Assert.Equal(ShiftAndCSE_I64_Ref((long)foo, shift), ShiftAndCSE_I64((long)foo, shift)); + } + } + } + } +} diff --git a/src/tests/JIT/Directed/shift/ShiftMaskCSE_do.csproj b/src/tests/JIT/Directed/shift/ShiftMaskCSE_do.csproj new file mode 100644 index 00000000000000..0f324b66bf7f5c --- /dev/null +++ b/src/tests/JIT/Directed/shift/ShiftMaskCSE_do.csproj @@ -0,0 +1,12 @@ + + + 1 + + + Full + True + + + + + diff --git a/src/tests/JIT/Directed/shift/ShiftMaskCSE_ro.csproj b/src/tests/JIT/Directed/shift/ShiftMaskCSE_ro.csproj new file mode 100644 index 00000000000000..1812ab3306a855 --- /dev/null +++ b/src/tests/JIT/Directed/shift/ShiftMaskCSE_ro.csproj @@ -0,0 +1,12 @@ + + + 1 + + + None + True + + + + +