JIT: Secondary frame pointer for x64 unoptimized methods - #128795
JIT: Secondary frame pointer for x64 unoptimized methods#128795AndyAyersMS wants to merge 15 commits into
Conversation
On x64, addressing modes only encode a signed 8-bit displacement cheaply (disp8, -128..+127); larger offsets require a 4-byte disp32. Methods with large stack frames therefore emit many oversized stack-local references. This adds an optional secondary frame/stack pointer, reserved in a callee-saved register (RBX), offset by a configurable number of bytes from the primary base. Stack locals that fall outside disp8 range of the primary base but inside disp8 range of the secondary pointer are rewritten to use the secondary pointer, shrinking those references from disp32 to disp8. Gated behind the default-off config JitSecondFramePtr (the byte offset; 0x100 = 256) and restricted to OptimizationDisabled() (MinOpts/Tier0) on x64 only. LSRA reserves the register; codegen sets it up in the prolog after unwindEndProlog(); emit rewrites eligible SV refs to [rbx+disp8]. EH/funclet support: EH methods always use RBP frames on x64. The secondary pointer is re-established (lea rbx,[rbp-offset]) only in filter funclet prologs, since the VM's CallEHFilterFunclet restores only RBP. Catch/ finally/fault funclets need no re-establishment because CallEHFunclet restores all nonvolatiles (including RBX) from the establisher context. SuperPMI asmdiffs across all x64 collections (JitSecondFramePtr=0x100): overall -7,353,695 bytes, 100% in MinOpts, FullOpts unchanged (0 diffs). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When an access is redirected through the secondary frame pointer
(REG_OPT_RSVD2/RBX) the emitted bytes use [rbx+disp8], but emitDispFrameRef
previously still printed the canonical [rbp/rsp+disp], so the listing did not
match the encoded instruction. Print the actual [rbx+disp8] operand and append
the canonical reference as a parenthesized suffix, e.g.
mov qword ptr [rbx+0x78] (rbp-0x88), rax
Plumb the instruction through emitDispFrameRef so it can reuse
emitIsSecondFramePtrCandidate (the same decision emitOutputSV makes), keeping
display and emitted bytes in lockstep. Display-only change; no codegen impact.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When a stack access is redirected through the secondary frame pointer (REG_OPT_RSVD2), the disassembly now prints the real [rbx+disp8] operand and emits the canonical frame reference (e.g. rbp-0x88) as an end-of-line ';' comment, rather than an inline parenthetical that could be misread as sitting between operands. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The LSRA-time reservation keyed only off total frame size, so ~27% of methods reserved RBX (push/lea/pop plus unwind data) but never used it: no local actually landed in the secondary disp8 band. The band can't be tested at LSRA, since REGALLOC-layout offsets have no base-register flag and are inflated by an over-estimated callee-save area. Reserve RBX at LSRA only as a cheap candidate (out of allocation), then make the precise band-occupancy decision in genFinalizeFrame once FINAL offsets are known. If nothing lands in the band, cancel the reservation so no push/lea/unwind is emitted; otherwise mark the register modified and redo the frame layout to account for the push. aspnet2 asmdiffs: unused-RBX setups drop from 13 to 0; size win improves from -3864 to -3982 bytes. No replay failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
Adds an AMD64-only "secondary frame pointer" optimization (gated by JitConfig.JitSecondFramePtr, default 0x100) for unoptimized methods with large frames. RBX is reserved during LSRA, conditionally established (RBP - offset or RSP + offset) in the prolog after a profitability check on FINAL frame offsets, restored in filter funclets, and used by the xarch emitter to redirect far stack accesses from [rbp/rsp + disp32] to [rbx + disp8] (saving 3 bytes per redirected access). Disassembly is updated to show the redirected operand plus a trailing canonical-frame-reference comment, and emitDispFrameRef gains an instruction parameter on all targets.
Changes:
- Reserve RBX (
REG_OPT_RSVD2) as candidate secondary FP during LSRA when MinOpts, large frame, EH-compatible, non-OSR; finalize/cancel ingenFinalizeFramevia newgenSecondFramePtrIsProfitable. - Emit prolog
leato establish RBX from RBP/RSP, re-establish it in AMD64 filter funclets, and redirect eligible stack-var encodings (non-EVEX/SSE38/3A/crc32) inemitInsSizeSVCalcDisp/emitOutputSV. - Plumb
instruction insthroughemitDispFrameRefon all targets, and render[rbx+disp8] ; rbp-0xNNin xarch disassembly.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/targetamd64.h | Define REG_OPT_RSVD2 / RBM_OPT_RSVD2 as RBX. |
| src/coreclr/jit/jitconfigvalues.h | Add JitSecondFramePtr config (default 256). |
| src/coreclr/jit/codegeninterface.h | Add genSecondFramePtrReg/Offset/FPbased state (AMD64). |
| src/coreclr/jit/codegen.h | Declare genSecondFramePtrIsProfitable; fix #endif comment for TARGET_XARCH. |
| src/coreclr/jit/lsra.cpp | Reserve RBX candidate when conditions hold (MinOpts, fixed base, large frame, EH/OSR ok). |
| src/coreclr/jit/codegencommon.cpp | Profitability check after FINAL layout; if profitable, mark RBX modified, redo layout, emit prolog lea. |
| src/coreclr/jit/codegenxarch.cpp | Re-establish RBX in FILTER funclet prologs. |
| src/coreclr/jit/emitxarch.h | Declare emitIsSecondFramePtrCandidate and display state for trailing comment. |
| src/coreclr/jit/emitxarch.cpp | Redirect candidate check in size calc/output; modrm `0x40 |
| src/coreclr/jit/emit.h | Add instruction ins = INS_none parameter to emitDispFrameRef. |
| src/coreclr/jit/emitarm.cpp / emitarm64.cpp / emitloongarch64.cpp / emitriscv64.cpp | Update emitDispFrameRef signature on other targets (parameter unused). |
The reservation gate ran lvaFrameSize(REGALLOC_FRAME_LAYOUT) eagerly for every method -- a full layout pass even in fullopts, where the feature never engages. Replace it with a cheap O(numLocals) estimate (outgoing-arg space plus stack-home sizes, bailing once past the disp8 window); the precise band test in genSecondFramePtrIsProfitable still re-checks with FINAL offsets. Also skip the per-stack-access candidate check in the common case via a cached emitter flag, set once in emitBegFN, instead of dereferencing codeGen on every access. No code-size change; recovers the throughput regressions flagged in CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
No-opt TP is a bit high, but the very high numbers are from thin collections. Realistically it's about 0.5%. Still seems more costly than I'd expect. |
Test the cheap disp8 band arithmetic before the costlier instruction-class predicates in emitIsSecondFramePtrCandidate. The band check rejects the common in-window access first, so the EVEX/APX/SSE38-3A/crc32 checks only run for band-matching accesses. Byte-identical codegen; cuts MinOpts throughput cost. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Refreshed the diffs About 12MB less code (~2%) for ~0.5% TP. Was hoping this would be substantially cheaper than #127780 but having to compute frame layout twice and the extra encoding checks are not free. This is complementary with #127780; the two together are nearly additive in code size savings. Relies on RBX being preserved across most EH transitions (except filters). Tier0 / minopts only, we could make it be tier0 only perhaps. |
|
@dotnet/jit-contrib here's yet another idea for trying to cope with x64 displacement limits -- add in a secondary frame pointer. Like its sibling #127780 (which sorted locals to maximize use of small displacements) this mostly reduces unoptimized code size, but it has more TP cost than I'd like. Not sure there is any easily observable benefit either; I suspect this can help very large code bases reduce ICache/ITLB costs since last I knew all tiers allocated from the same code heap so the various tier code intermingles. I could try and measure perhaps. Thoughts? |
| auto frameLikelyLargeEnough = [this]() -> bool { | ||
| unsigned size = m_compiler->lvaOutgoingArgSpaceSize; | ||
| for (unsigned lclNum = 0; lclNum < m_compiler->lvaCount; lclNum++) | ||
| { | ||
| size += m_compiler->lvaLclStackHomeSize(lclNum); | ||
| if (size > 256) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| }; |
| if ((secondFramePtrOffset != 0) && optDisabled && haveFixedBase && ehCompatible && notOsr && | ||
| frameLikelyLargeEnough()) | ||
| { |
| const int adjusted = EBPbased ? (dsp + codeGen->genSecondFramePtrOffset) : (dsp - codeGen->genSecondFramePtrOffset); | ||
| const bool rawFits = ((signed char)dsp == (ssize_t)dsp); | ||
| const bool adjFits = ((signed char)adjusted == (ssize_t)adjusted); | ||
|
|
||
| if (rawFits || !adjFits) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Instructions with special displacement/encoding handling cannot use the plain [reg+disp8] form. | ||
| if (IsEvexEncodableInstruction(ins) || IsApxExtendedEvexInstruction(ins) || EncodedBySSE38orSSE3A(ins) || | ||
| (ins == INS_crc32)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| *pAdjustedDsp = adjusted; | ||
| return true; | ||
| } |
|
Overall the changes LGTM. I don't have a strong preference on taking it or not, but did leave a few questions/comments about maybe ways to improve throughput... |
Commit to saving the reserved register before the single FINAL layout, so the offsets already account for the push. If the secondary pointer turns out not worth establishing, demote it but leave the register saved (a small wasted push/pop) instead of redoing the frame layout. Removes the double lvaAssignFrameOffsets(FINAL) on engaging methods, erasing the residual MinOpts throughput cost; a few methods grow by one push/pop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ndaryFramePointer
- Initialize emitSecondFramePtrActive at declaration (avoid indeterminate read). - Use codeGen->genSecondFramePtrReg instead of hard-coded REG_OPT_RSVD2 in the ModRM emission and disassembly, so the emitter stays correct if the chosen register ever changes. - Compute the secondary displacement in ssize_t to avoid signed int overflow (UB) when JitSecondFramePtr is set to a large value. - Reject non-positive JitSecondFramePtr offsets (a negative offset would invert the adjustment math and prolog LEA). - Stop double-counting the outgoing-arg-space region in the frame-size estimate used to gate the reservation. - Refresh the stale genFinalizeFrame comment to match the single-layout-pass flow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
An EnC remap resumes execution in the updated method without re-running its prolog and only preserves RBM_ENC_CALLEE_SAVED (RSI|RDI on Windows, none on SysV) across the transition. The secondary frame pointer is a callee-saved register (REG_OPT_RSVD2/RBX) established only in the prolog and is not in that set, so after a remap it would hold a stale value and corrupt every redirected [rbx+disp8] local access. Gate the reservation off when opts.compDbgEnC, and assert the invariant where the register is committed as modified. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- genSecondFramePtrIsProfitable: compute the adjusted-fits-disp8 range in ssize_t to avoid signed int overflow (UB) for large JitSecondFramePtr values (mirrors the emitter fix). - frameLikelyLargeEnough: tie the cheap frame-size threshold to the configured offset instead of a hard-coded 256, so the gate tracks the band actually targeted (default offset 256 preserves prior behavior). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| unsigned size = m_compiler->lvaOutgoingArgSpaceSize; | ||
| for (unsigned lclNum = 0; lclNum < m_compiler->lvaCount; lclNum++) | ||
| { | ||
| // Skip the outgoing-arg-space var: its stack home size is lvaOutgoingArgSpaceSize, which | ||
| // is already seeded into `size` above; counting it again would double-count that region. | ||
| if (lclNum == m_compiler->lvaOutgoingArgSpaceVar) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| size += m_compiler->lvaLclStackHomeSize(lclNum); | ||
| if (size > (unsigned)secondFramePtrOffset) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| }; |
| // When non-zero, reserve a callee-saved register as a secondary stack base pointer, offset by this | ||
| // many bytes from the primary base, to address far locals with a disp8 displacement. 0 disables. | ||
| // 256 is canonical: it tiles the two disp8 windows contiguously for a 512-byte cheap range; other | ||
| // values overlap (less reach) or leave a gap. x64 only. | ||
| RELEASE_CONFIG_INTEGER(JitSecondFramePtr, "JitSecondFramePtr", 0x100) |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "4fb4edb6b177cfabbd79d4b7bc198a4c4c4408be",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "646ab80e65fcf3cb9f068157f644ead63761ee7c",
"last_reviewed_commit": "4fb4edb6b177cfabbd79d4b7bc198a4c4c4408be",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "646ab80e65fcf3cb9f068157f644ead63761ee7c",
"last_recorded_worker_run_id": "29684001643",
"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": "4fb4edb6b177cfabbd79d4b7bc198a4c4c4408be",
"review_id": 4730644779
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: The problem is real and well-established: x64 unoptimized (Tier0/MinOpts) frames frequently exceed the disp8 window, forcing disp32 stack-access encodings that bloat code. A secondary base pointer (RBX = primaryBase ± 256) lets far locals fold back into a cheap disp8 form, which the author's diffs show yields ~2% code-size savings for a modest TP cost.
Approach: The design is sound and carefully layered: LSRA tentatively reserves RBX as a candidate, genFinalizeFrame commits the push into the single FINAL layout and then cancels the establishment (via genSecondFramePtrIsProfitable) if no local lands in the secondary band, the prolog emits a no-unwind lea, filter funclets recompute RBX, and the emitter deterministically redirects qualifying accesses in both the size and output paths. Guards (opt-disabled only, fixed base, EH⇒RBP frame, not OSR, not EnC) are thorough and the rationale is documented inline. Overflow-safe ssize_t math and disp8-band determinism between sizing and output are handled correctly.
Summary: JitSecondFramePtr = 0x100), so it changes prologs, unwind, and EH re-establishment for essentially all x64 Tier0 methods at once — a broad blast radius that relies on the correctness of the funclet/unwind claims; and (2) there is no targeted regression test for the EH/filter re-establishment path. A human should confirm the CallEHFunclet/CallEHFilterFunclet nonvolatile-restore assumptions and decide whether default-on is intended for this PR or should bake behind an off-by-default switch first.
Detailed Findings
✅ Encoding, unwind, and EH correctness — verified
- The redirected ModRM encoding (
code | ((0x40 | (secondReg & 7)) << 8), no SIB, no REX.B) is valid because RBX < R8 and rm ≠ 4/5, andassert((unsigned)secondReg < 8)locks that in. - Size (
emitInsSizeSVCalcDisp) and output (emitOutputSV) both callemitIsSecondFramePtrCandidatewith the same post-FINALlvaFrameAddress-deriveddsp, so estimation and emission stay in lockstep. On AMD64FEATURE_FIXED_OUT_ARGSis set, soemitCurStackLvlis 0 and RSP-based redirects use a stable displacement. - The RBX push carries its own unwind code (marked modified before the FINAL layout), while the establishing
leasits afterunwindEndPrologand correctly needs no unwind data — mid-method unwind restores RBX to the caller value from its stack slot, which is the right semantics. - Redirect math is correct for both bases: FP frame RBX = RBP−offset with
secondDsp = dsp+offset; RSP frame RBX = RSP+offset withsecondDsp = dsp−offset. - localloc (which mutates RSP) forces an RBP frame, so RSP-based secondary pointers are never exposed to a shifting RSP.
⚠️ Enabled by default — large Tier0 blast radius
RELEASE_CONFIG_INTEGER(JitSecondFramePtr, "JitSecondFramePtr", 0x100) makes the feature active by default in every x64 MinOpts/Tier0 method whose frame is large enough. Because Tier0 is the first-tier code for nearly all methods, this touches prolog layout, unwind data, and EH funclet setup extremely broadly on day one. The guards look complete, but the correctness of the catch/finally/fault path depends entirely on the claim that CallEHFunclet restores RBX from the establisher CONTEXT (and that CallEHFilterFunclet restores only RBP). That claim is plausible and consistent with x64 EH, but it is load-bearing and hard to falsify by reading alone — a maintainer should confirm it, and confirm the intent to ship default-on rather than staging behind an off-by-default switch.
⚠️ Test gap — no targeted regression coverage
No test accompanies the change. The riskiest paths (filter-funclet RBX recomputation, catch/finally reliance on runtime restore, the profitability cancel-after-commit path, and the RSP-based variant) are only exercised incidentally by existing suites. Given the default-on scope, a focused test (e.g., a large-frame method with EH including a filter, plus GC/EH stress) would materially de-risk this. This is advisory rather than strictly blocking, since Tier0 codegen is heavily exercised by the existing pri1/EH/GCStress runs the author is presumably relying on.
💡 Minor observations
- The comment-only
#if defined(TARGET_AMD64)block ingenFinalizeFrame(documenting that the reservation is handled later) compiles to nothing; it's harmless documentation but could just be a plain comment. genSecondFramePtrIsProfitabledeliberately checks "at least one redirectable access" without counting sites, so a method with only one or two redirects can still regress a few bytes. The author documents this tradeoff explicitly; noting it here only so a reviewer weighing the TP/size numbers is aware it's an intentional approximation.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 246.9 AIC · ⌖ 11.1 AIC · ⊞ 10K
In un-optimized codegen on x64 only, introduce a secondary frame pointer in RBX, offset 256 bytes (configurable via
DOTNET_JitSecondFramePtr, default0x100) from either RSP or RBP, and use that to address locals in range. RBX survives most EH transitions so only needs to be set up in the prolog and re-established for filters.Substantial code size savings from smaller displacement encodings.
Note
Description edited with GitHub Copilot to reconcile the stated offset with the shipped default (256).