Fix for out of range rotate amount during lower

[dhartglassMSFT]

Assert during arm64 lower that rotate amount on a rotate node (ROR, ROL) was an out of range integer

Change to instead add explicit masking onto rotate nodes in the jit (AND op (CNS_INT 63) for ex.), and to strip the mask during lower like is done already for shifts

Also change importercalls to allow non-constant rotate amounts onto the node. Gives a couple diffs where we can generate ROR now instead of a call.

There's a single bad diff caused by CSE deduplicating the explicit masks, which ends up defeating the lowering mask-stripping logic. This bug exists already for SHIFTS (not rotates) so this is an extant issue, I'll try and open a bug.

fixes #129298

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

[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 updates the JIT’s handling of rotate counts to avoid out-of-range rotate amounts reaching lowering, including enabling import of variable rotate amounts for primitive intrinsics, and adds a regression test for the arm64 lowering assert scenario.

Changes:

• Mask rotate counts explicitly in morphing so rotate nodes always have counts normalized to [0, bitsize-1].
• Strip redundant AND(count, mask) for rotates during lowering on targets that implicitly mask rotate counts.
• Refactor RotateLeft/RotateRight intrinsic importing into a shared helper and allow non-constant rotate amounts (with explicit masking), plus add a regression test.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/coreclr/jit/morph.cpp	Adds explicit masking of rotate counts when recognizing/morphing rotation patterns.
src/coreclr/jit/lower.cpp	Adds TryRemoveShiftRotateMask and uses it to strip redundant rotate-count masks prior to lowering rotates on relevant targets.
src/coreclr/jit/lower.h	Declares TryRemoveShiftRotateMask helper used by lowering.
src/coreclr/jit/importercalls.cpp	Introduces impRotateHelper and uses it for RotateLeft/RotateRight, including masked variable rotate counts.
src/coreclr/jit/compiler.h	Adds the impRotateHelper declaration.
src/tests/JIT/Regression/JitBlue/Runtime_129298/Runtime_129298.cs	Adds regression test ensuring JIT compiles the dead rotate block without asserting.
src/tests/JIT/Regression/Regression_ro_2.csproj	Includes the new regression test source file in the project.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/coreclr/jit/lower.cpp:8875

• The LowerShift doc comment still refers specifically to xarch, but this helper is used for multiple targets (see the TARGET_XARCH || TARGET_ARM64 || TARGET_LOONGARCH64 || TARGET_RISCV64 call sites). Updating the comment would avoid misleading future readers about why the mask removal is safe here.

// Notes:
//    Remove unnecessary shift count masking, xarch shift instructions
//    mask the shift count to 5 bits (or 6 bits for 64 bit operations).
//

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

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

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

[dhartglassMSFT]

example diff

@@ -97,24 +97,19 @@ G_M27122_IG06:        ; bbWeight=1, gcrefRegs=0001 {x0}, byrefRegs=0000 {}, byre
             str     w0, [fp, #0x14]	// [V06 tmp2]
             ldr     w0, [fp, #0x14]	// [V06 tmp2]
             ldr     w1, [fp, #0x24]	// [V02 arg2]
-            movz    x2, #0xD1FFAB1E      // code for <unknown method>
-            movk    x2, #0xD1FFAB1E LSL #16
-            movk    x2, #0xD1FFAB1E LSL #32
-            ldr     x2, [x2]
-            blr     x2      // code for <unknown method>
-            ; gcr arg pop 0
+            ror     w0, w0, w1
             str     w0, [fp, #0x10]	// [V07 tmp3]
             ldr     w0, [fp, #0x10]	// [V07 tmp3]
             str     w0, [fp, #0x20]	// [V03 loc0]
             nop     
             ldr     w0, [fp, #0x20]	// [V03 loc0]
-						;; size=92 bbWeight=1 PerfScore 29.00
+						;; size=76 bbWeight=1 PerfScore 24.00
 G_M27122_IG07:        ; bbWeight=1, epilog, nogc, extend
             ldp     fp, lr, [sp], #0x30
             ret     lr
                         ;; size=8 bbWeight=1 PerfScore 2.00

[AndyAyersMS]

Can you make the noted adjustments for Wasm?

[AndyAyersMS]

Also TARGET_WASM

[dhartglassMSFT]

sure, fyi this same ifdef appears something like 5 times in this file in (unchanged) places, I assume at some point we'd want to allow WASM through this path at least?

#if defined(TARGET_XARCH) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64)
            LowerShift(node->AsOp());
#else
            ContainCheckShiftRotate(node->AsOp());
#endif

[AndyAyersMS]

If you'd rather defer fixing the Wasm aspects and open a follow-up issue for Wasm that's fine by me.

[dhartglassMSFT]

I'll push the WASM changes, if stuff fails though I'll probably back it out and instead do it as a follow up

Would existing test coverage in PR hit that shift path, hypothetically?

[AndyAyersMS]

Possibly, but there isn't much that gets tested yet.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/tests/JIT/Regression/JitBlue/Runtime_129298/Runtime_129298.cs:48

• The comment claims this block is "unreached at runtime", but it is only unreached for the specific test inputs. For other inputs where p1 < p0, v33 remains 0 and 0x7FFFFFFE % v33 would execute (and throw DivideByZeroException). Consider updating the comment to avoid implying the code is generally unreachable at runtime.

            // unreached at runtime; v33 = 0 (default-init) reaches Lowering.
            v40 = BitOperations.RotateLeft(p0, (int)(0x7FFFFFFEu % v33));

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

[AndyAyersMS]

Please open a follow-up to review the similar cases you saw in lower.

In the follow-up, maybe we could use something like TARGET_MASKS_SHIFTS to capture the actual semantic we are relying on....