JIT: Rematerialize spilled FP/SIMD/mask constants instead of stack spill/reload

[tannergooding]

Floating-point, SIMD, and mask constants (CNS_DBL, CNS_VEC, CNS_MSK) have no GC liveness and, for a well-defined subset of values, can be rematerialized cheaply without needing a scratch register. Rather than spilling such a value to the stack and reloading it, this PR skips creating the spill temp/store in genProduceReg and instead rematerializes the constant at the reload point in genUnspillRegIfNeeded.

Details

A shared predicate isRematerializableConstant gates both the skip-store site and the rematerialization site. It only returns true for values that genSetRegToConst can encode directly, i.e. that need no scratch register:

• xarch: zero (xorps), all-bits-set (pcmpeqd/vpternlogd), or a RIP-relative load from the constant''s memory home.
• Arm64: 0.0/fmov-immediate doubles, zero/all-bits-set/movi-broadcast vectors, and all masks (pfalse/ptrue). Values that Arm64 would otherwise load from the constant pool via adrp/ldr need a scratch address register that is not reserved at the reload point, so those remain spilled.

Rematerialization goes through the GenTree* overload of genSetRegToConst, which honors the explicit target register. Values consumed directly from the spill temp as a contained memory operand (GTF_NOREG_AT_USE) continue to be spilled normally.

Diffs

SPMI asmdiffs (coreclr_tests, windows-arm64): 554 methods improved, 0 regressions, -5432 bytes, with spill store/reload pairs replaced by movi/fmov/mvni/pfalse/ptrue.

Note

This PR description was generated with the assistance of GitHub Copilot.

[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 changes CoreCLR JIT codegen to avoid stack spill/reload for certain spilled FP/SIMD/mask constants by rematerializing them at the reload point instead. It introduces a shared predicate to ensure the optimization is only applied when the constant can be materialized without requiring scratch registers (and without GC bookkeeping).

Changes:

• Update CodeGen::genUnspillRegIfNeeded to rematerialize eligible spilled GT_CNS_DBL / GT_CNS_VEC / GT_CNS_MSK into the reload target register (xarch/arm64) instead of loading from a spill temp.
• Add CodeGen::isRematerializableConstant to centralize the eligibility logic and keep the spill-skip and reload-remat sites consistent.
• Update CodeGen::genProduceReg to skip creating a spill temp / emitting the spill store for eligible constants (while preserving the existing behavior for GTF_NOREG_AT_USE cases that must read from the spill temp as a contained memory operand).

Show a summary per file

File	Description
src/coreclr/jit/codegenlinear.cpp	Implements the rematerialization path on unspill, adds the shared predicate, and skips spill-temp/store creation for eligible constants.
src/coreclr/jit/codegen.h	Declares the new isRematerializableConstant(GenTree*) helper on CodeGen.

Copilot's findings

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

[Copilot]

Copilot's findings

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

[tannergooding]

tannergooding requested review from AaronRobinsonMSFT, dotnet-diag and janvorli code owners

Sorry for the noise. The src/native/external/libunwind/README.md got automatically changed from a symlink to a regular file and then auto-committed by copilot.

[Copilot]

Copilot's findings

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

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (1)

src/coreclr/jit/emit.cpp:6970

• With the new dsOffset-based layout (and AlignUp in emitDataGenBeg/emitBBTableDataGenBeg), emitConsDsc.dsdOffs can now include inter-section alignment padding. Summing sec->dsSize undercounts the logical read-only data size, so Metrics.ReadOnlyDataBytes may no longer reflect the actual data-section footprint.

    {
        comp->Metrics.ReadOnlyDataBytes += sec->dsSize;

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

[Copilot]

Copilot's findings

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

[Copilot]

Copilot's findings

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

[tannergooding]

CC. @dotnet/jit-contrib, @EgorBo, @AndyAyersMS this should be ready for review.

This PR does two things....

---

First, it fixes a bug in method local constant allocation that did not actually respect the alignment. This meant that data was not necessarily aligned as expected and could result in worse perf, potentially bugs on some platforms that required alignment, and was preventing the constant reuse from kicking in. Fixing this reduces the amount of ReadOnlyDataBytes allocated by [-6.56%, -1.43%] on x64 and by [-5.03%, -0.14%] on Arm64

Concretely, for Linux x64 this is -281k ReadOnlyDataBytes. Similar numbers exist for other collections.

---

Secondly, it adds support for avoiding spilling constants to the stack and instead consuming them directly from their existing method local home. This doesn't happen on EVEX based Windows or Arm64 due to the existence of callee save registers. However, it is more common on Linux where we have no callee save floating-point registers. -- We may be able to optimize this more on Windows/Arm64 in the future by avoiding callee save registers altogether, which will avoid the prologue spill/restore of that register, but that should be a separate PR and may not pan out.

[EgorBo]

I took a quick look and it seems to be fine to me, but am no expert in LSRA, so perhaps someone else from @dotnet/jit-contrib can have another look.

Do we effectively do such rematerialization for int constants today? I'd assume it has to be some generic algorithm

[EgorBo]

I wonder what is better - reload a SIMD16 constant from data section after a call or use callee-saved lower parts of two SIMD regs (v8 - v15) to keep it & restore then 🤔

[tannergooding]

I wonder what is better - reload a SIMD16 constant from data section after a call or use callee-saved lower parts of two SIMD regs (v8 - v15) to keep it & restore then

We have some logic for this already, as part of NI_SIMD_UpperSave/Restore. However, it's tricky because the TYP_SIMD32 isn't already in a callee-saved register it requires a move in addition to the upper extract. The use of a callee-saved register at all also means the prologue/epilogue expands to spill/restore that register for the whole method. Since we have both an upper and lower half, this now becomes two registers to spill/restore. It then also requires the insert after the call.

So for constants, we basically have to consider what this PR does: no spill and restoring directly from RWD section; which assuming its still in the L1 cache from the previous use is roughly 4-8 cycles and +4 bytes of codegen if a contained load or +8 bytes of codegen if it has to be a standalone reload

vs actually spilling/reloading or splitting into two callee save registers and recombining across the callsite. Where a memory load/restore is 6 bytes of codegen and 4-8 latency and a insert/extract is also 6 bytes of codegen and roughly 3 cycles of latency. Given us a minimum comparison of +12 bytes and 6 cycles of latency to the inner loop

---

Given that, I believe that trying to save the theoretical 2 cycles per iteration at the cost of even larger codegen and more instructions isn't worth the complexity (and may not pay off since more iterations means more likely being in L1 cache and lower latency for the load); particularly since for it to be "optimal" we'd need to go and add some preferencing logic so that non-constants are preferenced for that splitting and to have constants only do it if there was free space.

[tannergooding]

Do we effectively do such rematerialization for int constants today? I'd assume it has to be some generic algorithm

I don't think so and I didn't include it as part of this work. We don't end up with many integer constants coming from memory at all given that x64 has a mov reg, literal64 and arm64 has their movz; movk sequence

It may be worth doing that as well, but I'd prefer to check in a different PR; particularly since it also impacts various edges around icon handles, GC liveness, etc.

[tannergooding]

CC. @jakobbotsch or @AndyAyersMS this is just pending a second pair of jitcontrib eyes on the LSRA impacting changes.

[Copilot]

Copilot's findings

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

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "f05decb038e7a547fb3ad091deca11af0a89a796",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "051c1fd3c13fea8c1e60b68d9c7f63c3173e5593",
  "last_reviewed_commit": "f05decb038e7a547fb3ad091deca11af0a89a796",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "051c1fd3c13fea8c1e60b68d9c7f63c3173e5593",
  "last_recorded_worker_run_id": "29684018172",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "f05decb038e7a547fb3ad091deca11af0a89a796",
      "review_id": 4730637214
    }
  ]
}

[github-actions]

Holistic Review

Motivation: FP/SIMD/mask constants (CNS_DBL, CNS_VEC, CNS_MSK) have no GC liveness and, for a well-defined subset of values, can be rematerialized with a single scratch-free instruction (xorps/pcmpeqd on xarch, movi/fmov/mvni/pfalse/ptrue on arm64) or read directly from their data-section home. Spilling such values to the stack and reloading them is strictly wasteful. This PR elides the spill store, the never-read register definition, and the reload, rematerializing instead. The SPMI numbers (554 methods improved, 0 regressions, -5432 bytes on windows-arm64) confirm the CQ win.

Approach: A shared predicate isRematerializableConstant gates both the skip-store site (genProduceReg) and the rematerialization site (genUnspillRegIfNeeded), and is kept deliberately in sync with the scratch-free fast paths of the target's genSetRegToConst. isRematerializedConstantSpill layers the GTF_SPILL/GTF_NOREG_AT_USE semantics on top so both the store and the definition can be dropped. A separate lowering guard (IsConstantMaterializableInRegWithoutMemory) prevents reg-optional/containment of register-only constants that have no data-section home. Finally, emitDataGenBeg and friends now record an explicit dsOffset per data section and align it up, replacing the previously-recomputed packed offset; this fixes the interaction where a rematerialized/contained constant is read from a properly-aligned data-section home and lets duplicate-constant matching reuse aligned entries. The layering is careful and the design comments are unusually thorough and accurate.

Summary: The change is correct and well-scoped. The three predicates correctly mirror the codegen fast paths I verified in genSetRegToConst (arm64 CNS_DBL zero/fmov, CNS_VEC SIMD8/12/16 zero/allbits/movi, all CNS_MSK; xarch all three operators), and the xarch spill-temp bypass in genOperandDesc/emitInsBinary/instr.cpp consistently routes GTF_NOREG_AT_USE constants to the data-section path. The data-section dsOffset refactor is applied consistently across emitEndCodeGen, emitDataGenFind, emitDataOffsetToPtr, and the arm disassembly helper. I have one minor, debug-only observation below; I found no correctness or performance concerns that block merge. Verdict: LGTM.

Detailed Findings

• Minor (debug-only): xarch switch-table disassembly still uses packed offset accumulation. In emitxarch.cpp emitDispIns (the INS_i_jmp block, around line 12379), the label-table lookup still walks dsdList accumulating offs += size and compares offs against idMemCookie (the value returned by emitBBTableDataGenBeg). Because emitBBTableDataGenBeg now returns an alignment-adjusted secOffs rather than a packed offset, this disassembly-only lookup can mismatch if inter-section alignment padding is ever introduced ahead of a jump table. The analogous arm helper in emitarm.cpp was correctly updated in this PR to compare against jdsc->dsOffset; the xarch switch-table path was not. This affects only --disasm/verbose JIT dumps, not generated code, so it is non-blocking, but updating it to use dsOffset for consistency would prevent a confusing disassembly label mismatch.

• Observation (no action): The tight coupling between isRematerializableConstant and each target's genSetRegToConst fast paths is a maintenance hazard — a future edit to the arm64 movi/fmov encoding conditions would need a matching edit here. The inline comments call this out explicitly and reference the mirrored code, which is the right mitigation given there is no cheap shared abstraction.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 179.2 AIC · ⌖ 10.9 AIC · ⊞ 10K