Arm64: keep SP in base operand for large-offset adds

[a74nh]

Fixes #130437

When generating large stack offsets, the base register can be SP. Therefore ensure that base is not used for reg3.

[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 ARM64 add operand ordering in a couple of codegen/emitter paths so that when the base register is SP (possible for large stack offsets), SP is used in the “base” operand position rather than the third operand position, avoiding the assert(isGeneralRegister(reg3)) constraint in emitIns_R_R_R.

Changes:

• In emitInsLoadStoreOp, swap the add operands for the large-offset materialization so the base register is reg2 and the temp remains reg3.
• In CodeGen::instGen_Set_Reg_To_Base_Plus_Imm, swap the add operands in the large-immediate fallback for the same reason.

Reviewed changes

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

File	Description
src/coreclr/jit/emitarm64.cpp	Reorders add operands in the large-offset address materialization path to keep SP out of reg3.
src/coreclr/jit/codegenarm64.cpp	Reorders add operands in the large-immediate fallback helper to keep SP out of reg3.

[JulieLeeMSFT]

LGTM.
nit: I wonder if we need to add a test, e.g., a test forcing a large stack frame with an SP-relative access at a ≥4KB offset.

[dhartglassMSFT]

Did something change here recently? It seems like jitstressregs should have hit large spill+mask reg spill pretty easily

[a74nh]

Did something change here recently? It seems like jitstressregs should have hit large spill+mask reg spill pretty easily

The bug itself isn't new, but it was #128037 that exposed it.

[a74nh]

Did something change here recently? It seems like jitstressregs should have hit large spill+mask reg spill pretty easily

The bug itself isn't new, but it was #128037 that exposed it.

More specifically: #128037 passes SP into instGen_Set_Reg_To_Base_Plus_Imm(), which causes the invalid add using SP in arg2 to be generated

[a74nh]

LGTM. nit: I wonder if we need to add a test, e.g., a test forcing a large stack frame with an SP-relative access at a ≥4KB offset.

Added a test.
I extracted the code from the failing test, but could no longer get the failing scenario to trigger. I let codex try, and it got stuck trying to reproduce the exact conditions. Eventually it ended up writing a specific targeted test using il. I'd ideally rather not use il, but it does ensure it will always trigger the correct scenario.

[dhartglassMSFT]

(github wont let me leave a comment on line 2157)

anyway, I looked at this bug Friday before I realized you were as well - Copilot flagged the "imm < 4096" line thinking it should capture "imm < -4096" as well, I guess for FP-based frames? Is this a real issue we want to fix in the future?

Can make a new bug if so since this has nothing to do with your change.

Change LGTM either way