JIT: Strip redundant shift-count mask earlier in gtFoldExprBinary

[tannergooding]

Summary

IL specifies that the shift count is masked to the operand bit width, so foo >> shift is imported as x >> (shift & 31) (or & 63 for 64-bit). LowerShift currently tries to strip the redundant AND, but it only pattern-matches the literal shift(x, n & C) shape at lowering time. After CSE, (shift & 31) becomes a temp — shift(x, cse0) where cse0 = shift & 31 — so the mask is no longer stripped and a redundant and is emitted.

This PR removes the redundant mask earlier, in gtFoldExprBinary, before CSE runs.

Changes

gtFoldExprShiftCountMask (new helper in gentree.cpp)

Called from gtFoldExprBinary for every GT_LSH/GT_RSH/GT_RSZ node (opt-enabled paths only, same guard as the surrounding fold logic):

• Strip the AND: if the shift count is (n & C) where C covers the hardware width mask (0x1f for 32-bit, 0x3f for 64-bit), replace the count with n directly. Handles commutative AND (constant on either side).
• Normalize out-of-range constants: once the AND is gone the count is no longer implicitly bounded, so an out-of-range constant shift count is normalized to cns & width. This also lets identities like x >> 0 => x fold.

Guard mirrors LowerShift: x64, ARM64, LoongArch64, RISC-V (architectures whose hardware shift already masks the count). ARM32 is excluded; LowerShift still re-inserts the AND there.

LowerShift is kept as a backstop for MinOpts and any cases that slip through (e.g. explicit user-written & 31 on the count expression).

VN normalization (valuenum.cpp)

VNForFunc for binary shifts now strips a GT_AND wrapper on the count VN when the mask covers the width. This gives x << lcl (where lcl was assigned y & 31) the same VN as x << y, letting VN-based CSE and copy-prop collapse them for the already-CSE'd case.

The 32-bit EvalOp shift paths are also brought in line with the 64-bit paths: count is masked to 0x1f to avoid C++ UB on out-of-range shift counts derived from VN constant folding.

HIR convention

A second commit documents the new HIR convention: shift counts in GT_LSH/GT_RSH/GT_RSZ no longer carry a redundant width-mask AND in optimized code. LowerShift is updated with a comment noting it remains a backstop for MinOpts and ARM32.

SPMI asmdiffs (2,676,835 contexts, 55 missed = 0.00%)

Collection	Δ bytes	Δ PerfScore
libraries	-50	-0.25%
libraries_tests	-160	-0.60%
libraries_tests_no_tiered_compilation	+11,660	-4.73%
runtime	-192	-0.53%
asp_net	-10	-0.30%
other 6 collections	-1,145	-0.25...−1.26%
Total	+10,103	improves all 11

The +11,660 byte regression is isolated to libraries_tests_no_tiered_compilation and traced to CSE churn in giant auto-generated Vector64/128/256 test methods (e.g. CountIndexOfLastIndexOfTest): removing a node during morph shifts VN/CSE budgets in bodies that go straight to FullOpts because no-tiered disables tier-0 filtering. PerfScore improves most (-4.73%) in that same collection. Real-code wins:

• BitArray.LeftShift / RightShift: -22B / -25B (3× and rNd,31 removed each)
• Number+BigInteger.ShiftLeft: -21B (3× and,31)
• ThreadPool.SetInt16Value: -3B (CSE count drops from Get core-setup building in the consolidated repo. #2 → WIP: repo consolidation scouting kick-off - make clr build locally on Windows #1 — the exact CSE scenario from the issue)
• Array.CanAssignArrayType, DecCalc.VarDecDiv, Grisu3.TryRun: several more and,63/and,31 removals

Test

Added src/tests/JIT/Directed/shift/ShiftMaskCSE.cs (+ _ro/_do projects) — a functional regression test covering the issue's ShiftAndCSE shape and several variants. Hooked up via the existing Directed_2.csproj merged test runner.

Note

This PR description was drafted by GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[tannergooding]

This is explicitly not for .NET 11, it is a draft to get some numbers and see how beneficial this early handling is.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR adjusts the JIT’s handling of IL shift-count masking so redundant (count & 31/63) masks can be stripped earlier (before CSE), while keeping correctness on targets that don’t naturally implement the IL masking semantics (notably ARM32). It also updates value numbering to treat masked and unmasked shift counts as equivalent when the mask fully covers the operand-width mask, and adds a directed regression test.

Changes:

• Add gtFoldExprShiftCountMask and invoke it from gtFoldExprBinary to strip/normalize redundant shift-count masks early.
• Update lowering to always route shifts through LowerShift, and (re)introduce explicit masking on ARM32 when needed.
• Update value numbering and add a directed test to validate correctness when the masked count is CSE’d.

Show a summary per file

File	Description
src/coreclr/jit/gentree.cpp	Introduces early shift-count-mask stripping/normalization during folding.
src/coreclr/jit/compiler.h	Declares the new gtFoldExprShiftCountMask helper.
src/coreclr/jit/lower.cpp	Ensures shifts are lowered consistently via LowerShift, with ARM32 correctness handling.
src/coreclr/jit/valuenum.cpp	Masks shift counts in eval and normalizes VNs for masked/unmasked equivalent shift counts.
src/tests/JIT/Directed/shift/ShiftMaskCSE.cs	Adds a regression test exercising CSE of the masked shift count across edge-case counts.
src/tests/JIT/Directed/shift/ShiftMaskCSE_ro.csproj	Adds the optimized “ro” test project for the new regression.
src/tests/JIT/Directed/shift/ShiftMaskCSE_do.csproj	Adds the optimized-with-debug “do” test project for the new regression.

Copilot's findings

• Files reviewed: 7/7 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

• Files reviewed: 8/8 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

• Files reviewed: 8/8 changed files
• Comments generated: 2

[Copilot]

Copilot's findings

Suppressed comments (3)

src/coreclr/jit/lower.cpp:8855

• On ARM32, the "already masked" check only looks for a constant in gtGetOp2(). Since GT_AND is commutative, a constant mask may appear on op1 (e.g., after some transformations), and we'd insert an extra AND(…,31) unnecessarily.

            if (shiftBy->OperIs(GT_AND))
            {
                GenTree* andOp2 = shiftBy->gtGetOp2();
                if (andOp2->IsCnsIntOrI() && ((static_cast<size_t>(andOp2->AsIntCon()->IconValue()) & ~mask) == 0))
                {
                    alreadyMasked = true;
                }
            }

src/coreclr/jit/lower.cpp:8833

• LowerShift no longer strips redundant shift-count ANDs on targets where the hardware already masks the count. Since gtFoldExprBinary (and thus gtFoldExprShiftCountMask) doesn't run under MinOpts (Tier0OptimizationEnabled is false), this change reintroduces a redundant AND in MinOpts, contradicting the PR description that LowerShift remains a backstop for MinOpts.

    assert(shift->OperIs(GT_LSH, GT_RSH, GT_RSZ));

#if defined(TARGET_XARCH) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64)
    // Nothing to do: gtFoldExprShiftCountMask already stripped any redundant width mask
    // from the count operand.
#elif defined(TARGET_ARM)
    // ARM32 uses Rs[7:0] as the shift count; counts in [32, 255] give 0 (LSL/LSR) or

src/coreclr/jit/morph.cpp:12407

• fgRecognizeAndMorphBitwiseRotation no longer tolerates the importer-emitted shift-count AND masks, but gtFoldExprShiftCountMask is only run via gtFoldExprBinary (Tier0+). In MinOpts (Tier0OptimizationEnabled == false), the masks will remain and this rotation recognition will stop matching. Consider stripping the redundant masks locally so this optimization doesn't regress under MinOpts.

        ssize_t   rotatedValueBitSize    = genTypeSize(rotatedValueActualType) * 8;
        noway_assert((rotatedValueBitSize == 32) || (rotatedValueBitSize == 64));
        GenTree* leftShiftIndex  = leftShiftTree->gtGetOp2();
        GenTree* rightShiftIndex = rightShiftTree->gtGetOp2();

• Files reviewed: 8/8 changed files
• Comments generated: 2

[Copilot]

Copilot's findings

Suppressed comments (2)

src/coreclr/jit/lower.cpp:8855

• The ARM32 alreadyMasked check only looks for a constant in gtGetOp2(). Since GT_AND is commutative, a count written/canonicalized as AND(31, x) will be treated as not masked and will get an extra AND inserted, producing redundant work. It’s cheap to handle both operand orders here.

            // 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<size_t>(andOp2->AsIntCon()->IconValue()) & ~mask) == 0))
                {
                    alreadyMasked = true;
                }
            }

src/coreclr/jit/lower.cpp:8832

• LowerShift no longer strips redundant shift-count masks on XARCH/ARM64/LOONGARCH64/RISCV64, but gtFoldExprShiftCountMask only removes them when OptimizationEnabled is true. This means MinOpts / non-optimized methods (and user-written count & 31 patterns that survive folding) will now keep emitting an extra AND, and the comment here incorrectly claims there is “nothing to do”. Consider keeping the lowering-time stripping as a backstop, as the Notes section describes.

This issue also appears on line 8844 of the same file.

// Notes:
//    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)
    // Nothing to do: gtFoldExprShiftCountMask already stripped any redundant width mask
    // from the count operand.
#elif defined(TARGET_ARM)

• Files reviewed: 8/8 changed files
• Comments generated: 0 new

[Copilot]

Copilot's findings

Suppressed comments (3)

src/coreclr/jit/gentree.cpp:15661

• The comment says shift nodes are "imported as" x >> (n & 31), but the importer builds the shift directly (see importer.cpp CEE_SHL/SHR/SHR_UN) and relies on shift semantics / lowering to provide masking. Wording here is misleading for future maintainers; suggest describing the semantic equivalence instead of claiming the tree shape.
  src/tests/JIT/Directed/shift/ShiftMaskCSE.cs:13
• This header comment says shifts are "imported as" x >> (n & 31), but the JIT importer builds shift nodes directly and the masking is a semantic property rather than a guaranteed IR shape. Adjusting the wording avoids baking in an invariant that isn’t actually enforced.

// 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.

src/coreclr/jit/lower.cpp:8882

• ARM32 path: the "already masked" check only looks for a constant in op2 of GT_AND. If the AND is commuted (constant in op1), we’ll insert an extra & 31 even though the count is already provably in [0,31]. This can happen after optimizations that canonicalize commutative ops, and it would create redundant instructions.

            if (shiftBy->OperIs(GT_AND))
            {
                GenTree* andOp2 = shiftBy->gtGetOp2();
                if (andOp2->IsCnsIntOrI() && ((static_cast<size_t>(andOp2->AsIntCon()->IconValue()) & ~mask) == 0))
                {

• Files reviewed: 8/8 changed files
• Comments generated: 0 new

[Copilot]

Copilot's findings

Suppressed comments (1)

src/coreclr/jit/lower.cpp:8882

• On ARM32, the "alreadyMasked" detection only checks shiftBy->gtGetOp2() for a constant. Since GT_AND is commutative, the constant mask can also be on op1 (this PR already updates morph rotation recognition to handle either side). Missing that case causes redundant nested ANDs to be inserted, increasing code size and potentially affecting containment/costing on ARM32.

            if (shiftBy->OperIs(GT_AND))
            {
                GenTree* andOp2 = shiftBy->gtGetOp2();
                if (andOp2->IsCnsIntOrI() && ((static_cast<size_t>(andOp2->AsIntCon()->IconValue()) & ~mask) == 0))
                {

• Files reviewed: 9/9 changed files
• Comments generated: 0 new

[Copilot]

Copilot's findings

Suppressed comments (2)

src/coreclr/jit/gentree.cpp:15661

• The comment says shift nodes are imported as x >> (n & 31)/& 63, but the importer currently builds GT_LSH/GT_RSH/GT_RSZ directly from the stack operands and relies on later phases/lowering for any masking (see importer.cpp around the shift opcodes). This wording is likely to mislead future readers about where the mask comes from; it would be clearer to describe the IL semantics (count is masked) and that this helper strips an explicit GT_AND when present.
  src/coreclr/jit/lower.cpp:8882
• On ARM32, the "already masked" fast-path only recognizes AND(x, C) when the constant is op2. If the AND has been commuted to AND(C, x), we’ll insert a second AND(count, 31) and end up with redundant masking in LIR. Consider making the detection commutative like the other AND-mask handling in this PR.

            if (shiftBy->OperIs(GT_AND))
            {
                GenTree* andOp2 = shiftBy->gtGetOp2();
                if (andOp2->IsCnsIntOrI() && ((static_cast<size_t>(andOp2->AsIntCon()->IconValue()) & ~mask) == 0))
                {

• Files reviewed: 8/8 changed files
• Comments generated: 0 new