Route dynasm x64 calls and enhance setarrayitem with GC barrier - #19
Merged
Conversation
WalkthroughAdds an x86-64 ABI argument-placement and call-emission helper suite, refactors many call sites (generic calls, CALL_ASSEMBLER paths, allocation/mem ops) to use ABI helpers and aligned calls, enhances Setarrayitem GC write-barrier detection and staging, extends inline card-marking to accept reg/frame/immediate, and adds a translator cutover-known-unported case with a test. ChangesABI Helper Infrastructure and Call Emission Refactoring
Setarrayitem GC Write Barrier Enhancement
Write Barrier Card Marking and Helper Call Refactor
Translator cutover classification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 414177c34e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ; mov rax, [rbp + offset] | ||
| ; call rax | ||
| ); | ||
| self.emit_abi_call_rax(); |
There was a problem hiding this comment.
push rbp
In Windows builds, this call site executes push rbp and then invokes emit_abi_call_rax(), whose Win64 path always does sub rsp, 40. After the extra push, rsp is already 16-byte aligned, so subtracting 40 re-misaligns the stack to 8 bytes before call, violating the Win64 ABI requirement for 16-byte alignment at call boundaries. This can break callees that rely on aligned stack frames (for example SIMD spills) and should use the one-push variant (32-byte shadow reservation) instead.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majit/majit-backend-dynasm/src/x86/assembler.rs (1)
5258-5349:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHandle frame-spilled indices in the card-marking path.
At Line 5308, the inline card update only supports
Loc::RegandLoc::Immed. IfSETARRAYITEM_GCreaches this fallback with its index spilled to the frame, the helper can setCARDS_SETand fall through here, but the_ => {}arm skips the actual card-byte write. That loses the remembered-set entry for the stored element. A simple fix is to materializeLoc::Frameintor11and share the existing register path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@majit/majit-backend-dynasm/src/x86/assembler.rs` around lines 5258 - 5349, The card-marking arm in emit_write_barrier_fastpath_kind only handles Loc::Reg and Loc::Immed for the index and skips Loc::Frame, losing remembered-set updates when the index is spilled; add a Loc::Frame branch that materializes the spilled index into r11 and then reuses the same register-path sequence used for Loc::Reg (push rcx/rdx, mov r11, [frame+offset] or equivalent to load the frame slot, shr r11, not/sub adjustments, compute byte/bit and or BYTE [base + r11], dl, pop rdx/pop rcx) so the inline card byte write is executed for frame-spilled indices as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@majit/majit-backend-dynasm/src/x86/assembler.rs`:
- Around line 1155-1200: The Windows-only 40-byte stack reservation in
emit_abi_call_rax, emit_abi_call_rax_aligned, emit_abi_call_rax_after_one_push
and emit_abi_call_reg incorrectly assumes rsp%16==8 and breaks callers that
pushed an extra value (which require 32 bytes shadow space); change these
helpers to compute the correct shadow/adjustment instead of hardcoding 40—e.g.
add a parameter or internal check (call it extra_pushes or pre_call_alignment)
to choose 32 vs 40 (and keep the aligned variant’s 8-byte adjustments) and
update all callers to pass the correct context so the dynasm! sequences reserve
the right amount of stack on Windows and maintain 16-byte alignment for
subsequent call sites.
- Around line 1092-1153: The helpers emit_abi_arg_from_reg,
emit_abi_arg_from_mem and emit_abi_arg_from_imm currently drop arguments with
idx >= 4 on Windows and never handle XMM (floating) ABI regs; update these
helpers to model the ABI fully by: 1) computing the caller-home stack slot for
any argument index beyond the register count and emitting a store to [rbp +
home_offset_for_arg(idx)] (so overflow args are written to the stack/home area
instead of no-oping), and 2) adding handling for floating arguments so that when
the argument is a float/double you move into the appropriate XMM register
(xmm0..xmm3 on Windows, xmm0..xmm7 on System V where relevant) or into the home
stack slot when beyond the XMM register window; make these changes in
emit_abi_arg_from_reg, emit_abi_arg_from_mem and emit_abi_arg_from_imm and
adjust emit_call* call sites to supply argument type info if necessary so the
helpers can choose GPR vs XMM vs stack storage.
---
Outside diff comments:
In `@majit/majit-backend-dynasm/src/x86/assembler.rs`:
- Around line 5258-5349: The card-marking arm in
emit_write_barrier_fastpath_kind only handles Loc::Reg and Loc::Immed for the
index and skips Loc::Frame, losing remembered-set updates when the index is
spilled; add a Loc::Frame branch that materializes the spilled index into r11
and then reuses the same register-path sequence used for Loc::Reg (push rcx/rdx,
mov r11, [frame+offset] or equivalent to load the frame slot, shr r11, not/sub
adjustments, compute byte/bit and or BYTE [base + r11], dl, pop rdx/pop rcx) so
the inline card byte write is executed for frame-spilled indices as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eb4f2e76-5115-4730-b994-74941d081b34
📒 Files selected for processing (1)
majit/majit-backend-dynasm/src/x86/assembler.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@majit/majit-backend-dynasm/src/x86/assembler.rs`:
- Around line 5507-5537: The inline card-mark path can clobber loc_base when
loc_base is rcx/rdx and it ignores frame-spilled indexes in the `_ => {}` arm;
fix by (1) in the Reg arm (the dynasm block that currently pushes rcx/rdx, moves
Rq(loc_index) into r11, shifts and computes dl, then ORs BYTE [Rq(loc_base) +
r11], dl) avoid overwriting loc_base by not using rcx/rdx as temporaries — use a
dedicated temp (e.g., r11/r10) or spill/restore the actual loc_base if it
aliases rcx/rdx so the final memory operand still references the original base;
and (2) replace the `_ => {}` no-op with handling for
frame-spilled/index-in-stack cases (load the spilled index into the same temp
register used for the Reg path, compute byte_ofs/byte_val the same way and
perform the same OR BYTE [base + offset], value sequence), so Setarrayitem’s
frame-backed indexes also update the card. Ensure changes reference the same
symbols: arglocs, Loc::Reg/Loc::Immed (and the frame-spill variant), loc_base,
loc_index, Rq(...) and the dynasm blocks so the logic is unified and
preserves/restores registers correctly.
- Around line 1315-1366: The SysV (non-Windows) paths currently do a bare call
in emit_abi_call_rax() and emit_abi_call_reg(), causing ABI-unaligned stacks;
modify those helpers so their non-Windows branches adjust RSP by 8 before the
call and restore it after (same pattern used in emit_abi_call_rax_aligned and
the Windows branches that use emit_abi_call_*_with_extra_pushes), or route them
to emit_abi_call_rax_with_extra_pushes/emit_abi_call_reg_with_extra_pushes with
extra_pushes=0 so the emitter emits "sub rsp, 8; call ...; add rsp, 8" on
non-Windows targets to guarantee 16-byte alignment for SysV callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dda45fc3-ece2-4a8d-9873-46c1ee2cb800
📒 Files selected for processing (1)
majit/majit-backend-dynasm/src/x86/assembler.rs
youknowone
approved these changes
May 12, 2026
youknowone
left a comment
youknowone
left a comment
Owner
There was a problem hiding this comment.
👍 Thanks! x86 now seems working great
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majit/majit-backend-dynasm/src/x86/assembler.rs (1)
4881-4940:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftMarshal ABI args with a parallel move plan.
Lines 4896-4937 copy argument sources into ABI destinations in source order, so overlapping register assignments corrupt later arguments or the callee address. A simple SysV swap like
arg0 in rsi, arg1 in rdibecomesmov rdi, rsi; mov rsi, rdi, which passes the same value twice; the same problem exists for XMM swaps and whenarglocs[func_index]lives in a register reused by an earlier ABI destination. This path needs a staging/remap step instead of sequential moves.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@majit/majit-backend-dynasm/src/x86/assembler.rs` around lines 4881 - 4940, The current emit_call_from_arglocs copies argument sources into ABI destinations in source order, causing register/XMM/slot overwrite when sources and destinations overlap; update emit_call_from_arglocs to compute a parallel-move plan from placements (from build_abi_arg_placements) to ABI destinations and execute it safely by: 1) collecting (source loc, target ABI placement, type) tuples for indices (func_index+1)..arg_count and the callee operand; 2) topologically ordering non-cyclic moves and performing them with emit_abi_arg_from_reg/mem/imm; and 3) breaking cycles by spilling one operand to a temporary stack slot or reserved temp register (use emit_reserve_abi_call_area / stack_slots or a dedicated temp) then completing the remaining moves; ensure XMM and general-purpose moves are handled separately and that the same move logic is applied to the callee address placement to avoid clobbering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@majit/majit-backend-dynasm/src/x86/assembler.rs`:
- Around line 4881-4940: The current emit_call_from_arglocs copies argument
sources into ABI destinations in source order, causing register/XMM/slot
overwrite when sources and destinations overlap; update emit_call_from_arglocs
to compute a parallel-move plan from placements (from build_abi_arg_placements)
to ABI destinations and execute it safely by: 1) collecting (source loc, target
ABI placement, type) tuples for indices (func_index+1)..arg_count and the callee
operand; 2) topologically ordering non-cyclic moves and performing them with
emit_abi_arg_from_reg/mem/imm; and 3) breaking cycles by spilling one operand to
a temporary stack slot or reserved temp register (use emit_reserve_abi_call_area
/ stack_slots or a dedicated temp) then completing the remaining moves; ensure
XMM and general-purpose moves are handled separately and that the same move
logic is applied to the callee address placement to avoid clobbering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23d54a2f-85bb-41e9-a8e0-bac0ed2467d1
📒 Files selected for processing (2)
majit/majit-backend-dynasm/src/x86/assembler.rsmajit/majit-translate/src/translator/rtyper/cutover.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbce486016
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+5574
to
+5576
| let byte_index = loc_index.value >> wb.jit_wb_card_page_shift; | ||
| let byte_ofs = | ||
| !((byte_index >> 3) as i64) - majit_gc::header::GcHeader::SIZE as i64; |
There was a problem hiding this comment.
The new Loc::Immed card-marking path computes byte_index with Rust's signed right-shift on i64, while the register/frame paths use x86 shr (logical shift). If an immediate index is negative, this branch derives a different byte_ofs/bit than the runtime shr-based formula and can set the wrong card-table byte, corrupting GC write-barrier metadata for that store pattern. Use unsigned shift semantics (e.g., via u64) to match the existing register/frame behavior.
Useful? React with 👍 / 👎.
Owner
This was referenced May 15, 2026
Merged
Merged
youknowone
added a commit
that referenced
this pull request
Jul 9, 2026
…L_ASSEMBLER (#19) Replace the portal-depth merge-point no-op with the pyjitpl.py:1579-1602 else-branch cut and wire the recursive CALL_ASSEMBLER path for all four result kinds. dispatch.rs BC_JIT_MERGE_POINT arm: - The seen<0 depth>0 skip is de-gated to the pyjitpl.py:1547-1552 predicate (inline_depth()>0 && seen_loop_header_for_jdindex<0 && !no_loop_header); the MAJIT_PORTAL_INLINE conjunct is removed from the dispatch condition. jitdriver.rs:2787 vinfo seeding keeps its own gate. - A depth>0 merge point inside the close block runs finishframe( leave_portal_frame=false) (pop inline frame, restore scalar/vable, no return-wire), then exec_recursive_call_assembler, then a deferred LeavePortalFrame op, then returns TraceAction::Continue (raise ChangeFrame analog) so dispatch resumes in the caller. GuardFutureCondition stays on the depth==0 path. - The inline-portal push records the matching EnterPortalFrame op. exec_recursive_call_assembler: accept Int/Ref/Float/Void result kinds (was Int-only). Each kind drives its concrete executor, records call_assembler_{int,ref,float,void}, and writes the result into the matching register bank; Void records with no result. Add execute_recursive_assembler_ {ref,float,void} trait seams (default None) and wire the production ClosureRuntimeWithResolver via with_trace_ctx_and_token_resolver. assembler.rs: add recursive_call_{ref,float,void} builders alongside recursive_call_int. Read float greens from the float register bank: exec_recursive_call green decode uses read_float_reg and the recursive_call_* builders use touch_float_reg for JitArgKind::Float (prepare_list_of_boxes decodes argcode 'f' from registers_f). Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 9, 2026
…single-pass scalar write-back) (#434) * majit: PR#427 review follow-ups — ;state CI example, GFC hoist, single-pass scalar write-back Three changes on top of the merged #344 Phase B. Add majit/examples/spcount: a `jit_merge_point!(...; state)` single-pass whole-circuit-close example. Before this the `; state` close path had no coverage in this repo's CI (aheui, its only other consumer, is a separate repo). The crate is a tl-shaped stack machine (scalar stackpos + virt stack) with a countable @dont_look_inside residual; its jit_residual_not_double_executed test asserts the residual runs exactly once per iteration. Registered in the workspace members and the CI cranelift `-p` list. Hoist GUARD_FUTURE_CONDITION in the BC_JIT_MERGE_POINT reached_loop_header equivalent (dispatch.rs). pyjitpl.py:2993 emits the dummy guard unconditionally at the top of reached_loop_header, before the compile/close/append branching, so it fires on every outcome including the append-and-continue path. majit emitted it only in the two close branches; move the single emission above the header-match branch and drop the two per-branch copies. Fix writeback_scalar_state_fields no-op. merge_point set self.sym = None at the end of the CloseLoop arm before returning, so the macro's post-return writeback_scalar_state_fields read a None sym and never wrote the walk-final scalars into native state; a walk that advanced a scalar (e.g. a SEL-advanced selected) across a single-pass close left native state at the trace-start value. Capture the scalar values off the still-live sym inside the CloseLoop arm (via a new JitState::collect_scalar_state_field_values static method), stash them on MetaInterp, and have writeback_scalar_state_fields consume the stash through a new writeback_scalar_state_fields_from_values method. Macro call order (writeback -> recover -> try_resume) is unchanged. Adds a regression test that drives a scalar advance across a CloseLoop and asserts native state receives it. Assisted-by: Claude * majit: port recursive-portal merge-point cut and 4-kind recursive CALL_ASSEMBLER (#19) Replace the portal-depth merge-point no-op with the pyjitpl.py:1579-1602 else-branch cut and wire the recursive CALL_ASSEMBLER path for all four result kinds. dispatch.rs BC_JIT_MERGE_POINT arm: - The seen<0 depth>0 skip is de-gated to the pyjitpl.py:1547-1552 predicate (inline_depth()>0 && seen_loop_header_for_jdindex<0 && !no_loop_header); the MAJIT_PORTAL_INLINE conjunct is removed from the dispatch condition. jitdriver.rs:2787 vinfo seeding keeps its own gate. - A depth>0 merge point inside the close block runs finishframe( leave_portal_frame=false) (pop inline frame, restore scalar/vable, no return-wire), then exec_recursive_call_assembler, then a deferred LeavePortalFrame op, then returns TraceAction::Continue (raise ChangeFrame analog) so dispatch resumes in the caller. GuardFutureCondition stays on the depth==0 path. - The inline-portal push records the matching EnterPortalFrame op. exec_recursive_call_assembler: accept Int/Ref/Float/Void result kinds (was Int-only). Each kind drives its concrete executor, records call_assembler_{int,ref,float,void}, and writes the result into the matching register bank; Void records with no result. Add execute_recursive_assembler_ {ref,float,void} trait seams (default None) and wire the production ClosureRuntimeWithResolver via with_trace_ctx_and_token_resolver. assembler.rs: add recursive_call_{ref,float,void} builders alongside recursive_call_int. Read float greens from the float register bank: exec_recursive_call green decode uses read_float_reg and the recursive_call_* builders use touch_float_reg for JitArgKind::Float (prepare_list_of_boxes decodes argcode 'f' from registers_f). Assisted-by: Claude * majit: correct ResidualCall/abort parity comments in exec_recursive_call The ResidualCall-decision abort in exec_recursive_call carried comments claiming pyjitpl.py falls to do_residual_call for this case. That mapping is wrong: pyjitpl.py's residual path (do_recursive_call assembler_call=False) is reached only when warmrunnerstate.inlining is false, which is never the case here (inlining is always true). The callee-not-compiled case majit labels ResidualCall corresponds to pyjitpl.py:1417 assembler_call=True, which builds the callee token on demand via compile_tmp_callback (warmstate.py:714-722) and emits CALL_ASSEMBLER. majit has no compile_tmp_callback, so it aborts/retries until the callee compiles on its own. Rewrite the comments at the abort branch, decide_recursive_inline's non_inline selector, and the InlineDecision::ResidualCall variant doc to state this mapping. No behavior change. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 9, 2026
…single-pass scalar write-back) (#434) * majit: PR#427 review follow-ups — ;state CI example, GFC hoist, single-pass scalar write-back Three changes on top of the merged #344 Phase B. Add majit/examples/spcount: a `jit_merge_point!(...; state)` single-pass whole-circuit-close example. Before this the `; state` close path had no coverage in this repo's CI (aheui, its only other consumer, is a separate repo). The crate is a tl-shaped stack machine (scalar stackpos + virt stack) with a countable @dont_look_inside residual; its jit_residual_not_double_executed test asserts the residual runs exactly once per iteration. Registered in the workspace members and the CI cranelift `-p` list. Hoist GUARD_FUTURE_CONDITION in the BC_JIT_MERGE_POINT reached_loop_header equivalent (dispatch.rs). pyjitpl.py:2993 emits the dummy guard unconditionally at the top of reached_loop_header, before the compile/close/append branching, so it fires on every outcome including the append-and-continue path. majit emitted it only in the two close branches; move the single emission above the header-match branch and drop the two per-branch copies. Fix writeback_scalar_state_fields no-op. merge_point set self.sym = None at the end of the CloseLoop arm before returning, so the macro's post-return writeback_scalar_state_fields read a None sym and never wrote the walk-final scalars into native state; a walk that advanced a scalar (e.g. a SEL-advanced selected) across a single-pass close left native state at the trace-start value. Capture the scalar values off the still-live sym inside the CloseLoop arm (via a new JitState::collect_scalar_state_field_values static method), stash them on MetaInterp, and have writeback_scalar_state_fields consume the stash through a new writeback_scalar_state_fields_from_values method. Macro call order (writeback -> recover -> try_resume) is unchanged. Adds a regression test that drives a scalar advance across a CloseLoop and asserts native state receives it. Assisted-by: Claude * majit: port recursive-portal merge-point cut and 4-kind recursive CALL_ASSEMBLER (#19) Replace the portal-depth merge-point no-op with the pyjitpl.py:1579-1602 else-branch cut and wire the recursive CALL_ASSEMBLER path for all four result kinds. dispatch.rs BC_JIT_MERGE_POINT arm: - The seen<0 depth>0 skip is de-gated to the pyjitpl.py:1547-1552 predicate (inline_depth()>0 && seen_loop_header_for_jdindex<0 && !no_loop_header); the MAJIT_PORTAL_INLINE conjunct is removed from the dispatch condition. jitdriver.rs:2787 vinfo seeding keeps its own gate. - A depth>0 merge point inside the close block runs finishframe( leave_portal_frame=false) (pop inline frame, restore scalar/vable, no return-wire), then exec_recursive_call_assembler, then a deferred LeavePortalFrame op, then returns TraceAction::Continue (raise ChangeFrame analog) so dispatch resumes in the caller. GuardFutureCondition stays on the depth==0 path. - The inline-portal push records the matching EnterPortalFrame op. exec_recursive_call_assembler: accept Int/Ref/Float/Void result kinds (was Int-only). Each kind drives its concrete executor, records call_assembler_{int,ref,float,void}, and writes the result into the matching register bank; Void records with no result. Add execute_recursive_assembler_ {ref,float,void} trait seams (default None) and wire the production ClosureRuntimeWithResolver via with_trace_ctx_and_token_resolver. assembler.rs: add recursive_call_{ref,float,void} builders alongside recursive_call_int. Read float greens from the float register bank: exec_recursive_call green decode uses read_float_reg and the recursive_call_* builders use touch_float_reg for JitArgKind::Float (prepare_list_of_boxes decodes argcode 'f' from registers_f). Assisted-by: Claude * majit: correct ResidualCall/abort parity comments in exec_recursive_call The ResidualCall-decision abort in exec_recursive_call carried comments claiming pyjitpl.py falls to do_residual_call for this case. That mapping is wrong: pyjitpl.py's residual path (do_recursive_call assembler_call=False) is reached only when warmrunnerstate.inlining is false, which is never the case here (inlining is always true). The callee-not-compiled case majit labels ResidualCall corresponds to pyjitpl.py:1417 assembler_call=True, which builds the callee token on demand via compile_tmp_callback (warmstate.py:714-722) and emits CALL_ASSEMBLER. majit has no compile_tmp_callback, so it aborts/retries until the callee compiles on its own. Rewrite the comments at the abort branch, decide_recursive_inline's non_inline selector, and the InlineDecision::ResidualCall variant doc to state this mapping. No behavior change. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 11, 2026
… op count Adds `jit_tier_is_alive`, asserting four things about `straight_line_program`: the result is 333, the `VmState`-filtered `degraded_dispatch_arms()` set equals empty, `compiles >= 1`, and the optimized body is exactly 2 ops. The op count needs a new counter: `set_on_compile_loop` now also stores `ops_after` in `LAST_OPS_AFTER`. `COMPILES > 0` alone does not separate a real body from the single `Finish()` an empty dispatch compiles, which reports `COMPILES=1, ops_after=1`. The count is pinned by equality, not bounded — every inequality a real body satisfies, a bare `Finish()` satisfies too. `probe_with_ops` resets and reads both counters inside `PROBE_LOCK`. Both are process-global, so a read taken after the guard drops can observe another test's compile, and an unreset counter retains the last compile anywhere in the process. The degraded-arm assertion is an equality against the empty vector rather than `is_empty()`, so a name disappearing is caught as well as a name appearing. The gate pins the legacy bare `jit_merge_point!()`. Converting this example to `jit_merge_point!(driver, program, pc; state)` makes it answer 222 with final `regs [222, 0, 222]`, and that is filed as task #28, still inferred rather than proven. Every number here must be re-pinned against the converted build when that conversion lands; the merge point is unchanged by this commit. Verified at d6157375f37 with `cargo test -p i64env --release`, run as `--nocapture --test-threads=1`: 10 passed, 0 failed, `result=333 COMPILES=1 OPS_AFTER=2`. The working tree also carried uncommitted changes to majit-macros (`jit_interp/mod.rs`, `jitcode_lower/dispatch.rs`, `jitcode_lower/helpers.rs`, tasks #19/#20, owned by another agent), so the run links those; they do not reach this example's arms, which are `i64` constant paths plus a wildcard. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 11, 2026
`compile_probe` returned `(result, compiles)` with `COMPILES` loaded inside
`PROBE_LOCK`, while `jit_tier_is_alive` loaded `LAST_OPS_AFTER` at the call
site, after the guard had dropped. Both counters are process-global, so a
concurrent test's compile could land between the run and that load and the gate
would pin a body it never ran.
`LAST_OPS_AFTER` also had no counterpart to `COMPILES.store(0, ..)`, so it
retained whatever the last compile anywhere in the process left behind. That
half survives `--test-threads=1`: under sequential execution the value can come
from the test that ran before this one. It has not fired because
`assert!(compiles >= 1)` precedes the read, which makes the assertion order,
rather than the counter, carry the guarantee.
`compile_probe` now returns `(result, compiles, ops_after)`, resetting and
reading both counters inside the lock.
Both pinned op counts are unchanged at 10, measured after the reset, so they
were reporting the probe's own compile rather than an inherited value.
Verified at 543d24e79bb with `cargo test -p tla --release` (16 passed) and
`cargo test -p tlc --release` (23 passed), each `--nocapture --test-threads=1`.
HEAD, both files and all 23 majit-macros sources hashed identical before and
after the run; majit-macros/jit_interp/{mod.rs,jitcode_lower/dispatch.rs} were
uncommitted throughout (tasks #19/#20, owned by another agent).
Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 20, 2026
… names Comment-only change across `majit/majit-metainterp/src/` (excluding `src/optimizeopt/`) and `majit/majit-metainterp/tests/`. - Rewrite `<file>.rs:NNN` citations of pyre's own Rust sources to name the symbol that lives there, or drop the line number when the surrounding prose already names it. Line refs inside string literals and inside ```text panic transcripts are left untouched. - Upstream `.py:NNN` citations (rpython/, pypy/, lib-python/, lib_pypy/) are unchanged. - Strip session-local tracking tags (Slice X-D/X-G/X3-E/QQ-n/P3/T-final, Sub-slice B/C.x, F.n-orthodox, M2 Step n, Box Identity Phase E Step n, #19 Step n, Step 2e.2b, P1.5) from comment prose. - Replace "see above"/"see the header"/"same rationale as" pointers with the fact plus the symbol that holds the rest. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 20, 2026
…d deny through the warm state (#1364) * jit: name the fixture the inline-chain depth cap was measured on `FBW_INLINE_CHAIN_DEPTH`'s doc cited `depthN_inline_chain`, which no longer names any file: #829 deleted `depth2_`/`depth3_`/`depth7_inline_chain_typeflip.py` and added `inline_chain_depth_typeflip.py` in the same commit. Cite the surviving file and record that the ~2.0-2.3x number was taken before the consolidation. Assisted-by: Claude * majit: publish the preview short-preamble export as one PreviewShortState Replace the four OptContext fields `exported_short_boxes`, `exported_short_inputargs`, `exported_short_inputarg_refs` and `exported_short_args_state` with a single `Option<PreviewShortState>` holding the three vectors plus an `Option` args_state. `preamble_end_args` stays a separate `Option<Vec<OpRef>>`. optimizer.rs binds `create_short_inputargs`, `create_short_inputarg_refs`, the args-state tuple and the filtered short boxes to locals and assigns the struct once, after the majit_log dump. `force_box_for_end_of_preamble` and its `preamble_end_args` write are unmoved. unroll.rs::export_state_with_bounds reads the args-state through `preview_short_state.and_then(|p| p.args_state)`, and takes the short inputargs / inputarg refs / short boxes from one match on the `Option` instead of an `is_empty()` test on a defaulted vector. The `debug_assert_eq!` cross-checking `exported_short_inputarg_refs` against `exported_short_inputargs` is dropped; the two are now built and published together. The length check against the export-site `label_args + virtuals` recompute is kept. unroll.rs tests gain `publish_preview_short_state` and `mint_short_inputargs` helpers; the four fixtures that wrote the ctx fields directly now construct the struct. Assisted-by: Claude * docs: correct the jit.virtual_ref emit note and two fbw fixture headers gate-triage.md claimed `opimpl_virtual_ref` / `_finish` have no caller outside a `#[test]`. They do: `walker_ec_enter` / `walker_ec_leave` call them on the live inline-push path in `pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`, which the same file already says 100 lines later. The parenthetical now states the narrower residue instead -- `vrefs_before_residual_call` / `vrefs_after_residual_call` iterate zero times over a level the walker inlines without seeding a frame -- and points at `jitcode_dispatch/mod.rs` item (a), where that residue is recorded. It also notes that `mod.rs`'s upstream citation for the residue is wrong: `perform_call` (pyjitpl.py:2445-2449) is `newframe` + `setup_call` and never touches `virtualref_boxes`; upstream's vref comes from `ExecutionContext.enter` (pypy/interpreter/executioncontext.py:88-89), traced through on an inlined call. getframe_root_loop_force_blackhole_crn.py said "this file adopts it five times". Its committed baselines record fbw_blackhole_adopted_single_frame=0, loops_aborted=0, loops_compiled=1 on all three backends; ca9edf7 (#1096) moved them from 5 / 5 / 0. Header now states the recorded numbers and points at the `_declined` sibling, which records 5 / 5 / 0. blackhole_inlined_callee_local_after_escape.py opened "Guard for what an adopted multi-frame blackhole chain owes its inner levels". fbw_blackhole_adopted_multi_frame is 0 in all three of its baselines and was 0 before #1096 as well; the five adopts it used to take were single-frame, and today it takes none (0 / 0 / 2). Header now says so, names the eight fixtures' worth of corpus that does pin the multi-frame arm (getframe_inline_subwalk_multiframe, getframe_while_inlined_callee_subwalk et al., all nonzero on three backends), and points at the `_declined` sibling. No baseline was re-recorded; no Rust and no executable Python changed. Assisted-by: Claude * docs: separate the MIFrame perform_call builds from the frame a vref is taken of The previous commit's gate-triage note called `pyjitpl.py:2445-2476` a wrong citation and said `mod.rs` held the last copy. Both overreach. The range spans `newframe` (:2455-2476), which does build a fresh frame per inlined call -- an `MIFrame`, the tracer's register frame. `pyre-jit-trace/src/helpers.rs` cites it for exactly that and is correct; stripping it there would remove a right citation. The same sentence also lives in `inline_call.rs`, so `mod.rs` was never the only copy. What is actually wrong at the two vref sites is conflating that `MIFrame` with the app-level frame `ExecutionContext.enter` takes `jit.virtual_ref` of. State that at both, and say in gate-triage which of the two claims the range supports. Also: the emit note said `virtualref_boxes` is populated for every seeded level, but `entered_ec` adds a non-null concrete frame and a non-null `execution_context` on top of seeding -- necessary, not sufficient. And the crn fixture header's inserted paragraph left "Its effects are idempotent" pointing at the `_declined` sibling instead of the drive. Assisted-by: Claude * majit: lower the int/float list capacity read as arraylen_gc `list.int_capacity` / `list.float_capacity` emitted `getfield_gc_r(<strategy>_items.block)` followed by a struct `getfield_gc_i(ItemsBlock.capacity)`. Upstream's capacity read is `len(l.items)` on `l.items: Ptr(GcArray(ITEM))` (rpython/rtyper/lltypesystem/rlist.py:251 in `_ll_list_resize_hint`, rlist.py:286 in `_ll_list_resize_ge`), which the rtyper lowers to `getarraysize` and `jtransform.py:808 rewrite_op_getarraysize` rewrites to `arraylen_gc`. `list.obj_capacity` already emitted `ArrayLen`. Both arms now emit `OpKind::ArrayLen` on the backing block, matching the object-strategy arm. The two unit tests are renamed and assert the new op shape. Assisted-by: Claude * majit: narrow the PreviewShortState alignment claim to the two vectors that share an index space The struct doc and the publication comment both said the three published vectors are "index-aligned by construction". Only two of them are: `short_inputargs` and `short_inputarg_refs` get one entry per `add_short_input_arg`. `short_boxes` is a different population -- the surviving produced short ops, after `short_boxes_exported`'s `filter_map` drops every one whose `canonical_result` is constant -- so its length is unrelated to the other two and `short_boxes[i]` pairs with nothing. What one publication site does buy is that a reader cannot see one vector from this evaluation of the preview beside another that was never written; both comments now say that instead. The unroll.rs test helper restates the refs/inputargs length invariant as a `debug_assert_eq!`. The production publisher gets it from `create_short_inputarg_refs`, which asserts internally; a fixture builds the two vectors by hand and had no check between a short refs vector and the failed `Weak` upgrade it causes past the peel boundary. Assisted-by: Claude * docs: correct eight comments that assert a capability the code already has Each of these states an absolute negative -- "never emits", "never calls it", "cannot be added and go unnamed" -- that its own call site contradicts. - `residual_call.rs`: `vrefs_after_residual_call` is called by the walker, under the `is_may_force` gate mirroring `pyjitpl.py:2007`. Its loops are empty because no `jit.virtual_ref` producers exist, which is a fact about the vref list, not about the call site. - `branch.rs` `decode_side_other_target`: the fused `goto_if_not_<cmp>` forms do reach the walk dispatch, minted by `majit-translate`'s jtransform as `ExitSwitch::Fused` for the LLBC-lowered graphs. What is true is narrower: the sole caller passes a `PyJitCode`, built per Python CodeObject by pyre's own codewriter, and `ExitSwitch::Tuple` -- the only path to a fused goto there -- has no producer outside `flatten.rs`'s own unit test. A fused form would be declined, not mis-decoded. - `inline_call.rs` / `fbw_state.rs`: `callee_body_contains_raise` caps a raising callee at TWO multiframe levels, not at the top inline level, and the cross-frame unwind bridge is built. The measurement beside `effective_multiframe_depth` is what bounds it: two levels green, a third taking `selfrec_tail_exception_unwind` from 937 to 7408 guard failures. - `state.rs` (two sites) / `helpers.rs`: `perform_call` (`pyjitpl.py:2445-2449`) is `newframe` + `setup_call` + `raise ChangeFrame`, and `newframe` (`:2455-2476`) builds an `MIFrame` and nothing else. Upstream has no recording-time app-level frame to hand out at that point; it gets one from tracing the interpreter's own frame construction, which pyre does not do. - `state.rs` / `trace.rs`: the pointer to `perform_call (trace_opcode.rs)` is dead -- no such function there. Callee sym state is set by `inline_call.rs`'s `setup_call` port. - `diag.rs` `SPEC_FOLD_ROWS`: the table cannot become a complete census by adding rows. It names a fold by its function, and two shapes have no name to give: a fold whose emit is inlined into a `match` arm has no function, and a registry-dispatched fold grows by one entry with no new call site. Also: `specialize.rs` drops line numbers from an in-repo file reference, and `tupleobject.rs` records that the `w_tuple_new` interception is the sole reason the `_ff` layout has no producer -- so restoring the upstream shape also makes the walker's `ff` specialisation arm live. Assisted-by: Claude * fbw: print all fourteen fbw_diag slots from both readers and namespace the escape/force keys The two readers of the same counter array printed disjoint index sets: the native reader (pyre/pyrex) printed {1, 6..13} and the wasm host (pyre-wasm-runner) printed {0..5, 11..13}, so slots 0 and 2..5 were bumped on the native backends and readable only through the wasm export, and slots 6..10 the other way round. One key per tally slot is now declared beside the counters as `pyre_jit_trace::trace::fbw_diag::LABELS` (length `RING_BASE`, so rustc rejects an unnamed slot), re-exported as `pyre_jit::FBW_DIAG_LABELS`, and joined against `get(i)` by both readers into a single `[jit-stats] fbw_diag` line carrying the same keys in the same order. The runner mirrors the array positionally, as it already does for `MC_DIAG_LABELS`, since it links no pyre crate. The MIDBODY_LATCH doc said the native corpus reaches neither leg "so these say whether the wasm target does"; it now says that a nonzero native value is itself the news, which is why both readers print it. On wasm the tally line moves out of the PYRE_WASM_JIT_STATS block — check.py never sets that variable — into the MAJIT_STATS block, the gate the native reader prints under. The `[fbw-census]` ring stays where it was. The four gated keys (fbw_rolled_back_with_effects, fbw_store_journal_rollback_failed, fbw_blackhole_adopted_single_frame, fbw_blackhole_adopted_multi_frame) keep their spelling and move from the counter line onto that fbw_diag line; the single `pyre_fbw_diag` lookup still feeds the missing-export refusal. The two `subset/total` fractions become named keys, the hazardous subset spelled `fbw_midbody_latch_new_unjournaled` and `fbw_escape_plain_fallback_unclean`. The five bare keys portal_only, published_callee_only, portal_and_published_callee, by_portal and by_callee_only are renamed fbw_escape_portal_only, fbw_escape_published_callee_only, fbw_escape_portal_and_published_callee, fbw_force_by_portal and fbw_force_by_callee_only: check.py's `_jit_stats_merged` folds every `[jit-stats]` line into one flat map, in which an un-namespaced key is a collision hazard. No committed .jitstats baseline carries any of the five under either spelling, so nothing is orphaned by the rename. They are left out of JITSTATS_SNAPSHOT_FIELDS, i.e. deliberately ungated, and check.py now records why: they are workload counts with no healthy value and no measured polarity (the reason `bridges_compiled` sits in neither regression list), and listing one would make every baseline that lacks it compare 0 -> N and fail until re-recorded. That re-record is a decision to take deliberately, with a polarity in hand. Checked with `cargo check -p pyrex`, `cargo check -p pyre-wasm-runner` and `cargo fmt --check`. No pyre binary was built or run, and no .jitstats snapshot was re-recorded. Assisted-by: Claude * fbw: make the decline census process-wide, as its own doc already claimed `FBW_DECLINE_CENSUS` was a `thread_local!` while the comment above it called it a "Per-process census". pyre installs `_thread` (`pyre-interpreter/src/importing.rs`), so Python threads are real OS threads and each traces on its own; the dump therefore reported only whichever thread happened to print it and silently dropped every decline the others took. Now a `static Mutex<BTreeMap>` behind a `census_map()` accessor that recovers from poisoning -- a map of counters has no invariant a panicking writer can leave broken, and a diagnostic that goes silent after an unrelated panic is worse than one that keeps counting. The lock costs nothing at this rate: the map is touched only on the cold decline path, never on the hot trace path. Pinned by `the_decline_census_counts_a_record_from_another_thread`, shown to fail on the `thread_local!` storage first: assertion `left == right` failed: a decline recorded off-thread never reached the census left: 0 right: 1 No gate exposure: `fbw_census` appears in no check.py field, no pyrex path and no committed baseline, so this changes a diagnostic only. Assisted-by: Claude * fbw: pin the wasm runner's fbw_diag label mirror against the slot constants `pyre-wasm-runner` links no pyre crate, so it restates `pyre_jit_trace::trace::fbw_diag::LABELS` as a positional array. rustc length-checks each side against its own constant (`RING_BASE` / `FBW_SLOTS`), but neither compiler sees the spellings, so a rename drifts silently and every tally from the divergence onward is printed under the wrong key -- and check.py folds every `[jit-stats]` line into one flat map, so a wrong name is compared against the wrong baseline rather than reported as missing. Four checks, following `majit-metainterp/tests/mc_diag_mirror.rs`: the parser is validated against the compiler-enforced count before being used to diagnose drift, the two declared counts must agree, and the two arrays must agree entry by entry. A positive control injects both drift shapes into the real runner source in memory -- a rename, caught by the entry comparison, and a dropped last slot, which leaves every surviving entry correctly named and so can only be caught by the length check. Perturbing the real text rather than a fixture is what makes the control cover the anchors. A fifth check closes what a two-array diff structurally cannot see: `LABELS` shifting against the slot CONSTANTS moves both arrays together, renaming every tally on both backends at once. Each label is bound to its own constant, and the bound slots are required to be exactly `0..RING_BASE` so a new slot cannot go unbound. The bindings are written out rather than derived from the constant names because two of them break the mechanical reading: `ESCAPE_FORCE_BY_PORTAL` is `fbw_force_by_portal`, not `fbw_escape_force_by_portal`. Assisted-by: Claude * docs: name the third fold shape SPEC_FOLD_ROWS structurally cannot hold The table names a fold by its function. Two shapes with no name to give were already recorded; a sweep of `vable_ops.rs` found a third. An ELISION fold recognises a shape and emits nothing, so a census keyed on "what IR did this fold emit instead" has nothing to key on. Three arms are this, all guarded by `fbw_strict_fold_frame_reg`: a store to the current inline level's own unseeded portal frame is a virtual-field write, folded away with no SETFIELD_GC recorded. Their recognisers (`fbw_strict_fold_frame_reg`, `folded_store_is_observable_local`) are predicates -- they cannot emit, because the eliding is the arm. This is distinct from the functionless-replace shape already listed: `bool_box_truth_lookup`'s arm has no function but does write a result. Assisted-by: Claude * majit-translate: re-anchor pyre-side comment refs to symbols Replace `file.rs:NNN` citations in codewriter/ and annotator/ comments with the symbol that owns the cited code, verified by opening each target. Upstream `.py` line citations are untouched. String-literal occurrences (assertion and panic messages) are untouched. Also drop the internal tracking labels `Z2.5 Path C`, `Phase I3` and `F2 followup`, the filename-less `(line ~3273)` pointer, and replace insns.rs's "documented at the const-table site above" with the fact that byte 18 now houses `BC_GOTO_IF_NOT`. Four citations are left as-is because their target no longer exists: `build_flow.rs:215` (file deleted with the syn-AST front-end) in call.rs and codewriter.rs, and `parse.rs:314-318` (parse.rs shrank from ~2000 to 98 lines) twice in call.rs. Assisted-by: Claude * pyre-jit: re-anchor comment references from line numbers to symbols Replace every `file.rs:NNN` citation in pyre/pyre-jit comments with the symbol that lives at the cited location, or with the bare filename where the surrounding text already names the symbol. Upstream `.py` citations (rpython/pypy/lib-python) are untouched. Also replace directional cross-references ("see below", "see comment above", "see the deferral below") with the named symbol they point at, drop the internal "Slice α-2" and "Phase L2" markers, and drop stale self-file line refs ("line 1891", "line 2120-2122", "at line 1495"). Comment-only; no code, string literal or test data changed. Assisted-by: Claude * optimizeopt: re-anchor pyre-side comment refs to symbols Replace `file.rs:NNN` / `symbol:NNN` citations in optimizeopt comments with the symbol that lives there. Upstream `.py:NNN` citations are unchanged. - `propagate_from_pass_range:3336-3339` and `Optimizer::emit_operation:3524-3528` / `:3527` drop their line ranges; both symbols had moved (4582 / 4816). - `dispatch_emit:2631/2766` in heap.rs and virtualize.rs names no existing symbol; replaced with `emit_residual_call` / `handle_side_effects`. Strip internal tracking labels from comments: `Cat-2.2`, `Path A`, `Post-S0`, `S11`, `S7`, `S8`, `E5b`. GitHub references (`#9`, `#115`, `#160`, `#175`) and the `PYRE_S9_PROBE` env-knob name are kept. Make cross-reference pointers self-contained: "see comment above", "see doc comment above", "see the closure above", "see field doc", "same evidence as the args loop above", "for the reason given in the field loop above", "same rationale as raw fields above", "see the Virtual arm above", "the arms below", "the guard below" now state the load-bearing fact or name the owning symbol. Repair two sentences left dangling by previously stripped refs (`optimizeopt/mod.rs` setinfo_from_preamble, `virtualstate.rs` visit count). Comment-only: no code, string literal, or test data changed. Assisted-by: Claude * pyre-jit-trace: re-anchor pyre-side comment refs to symbol names Replace `<file>.rs:NNN` line citations in pyre-jit-trace comments with the symbol that lives at the cited location, after opening each target. Upstream `rpython/`, `pypy/` and `lib-python/` `.py:NNN` parity citations are left unchanged. Where the cited pyre-side location holds no nameable symbol, or the file/function no longer exists, only the `:NNN` is dropped and the filename kept. Strip internal session tracking labels (`B3`, `C3 S1`, `E1`, `G0`/`G1`/`G2`, `Epic G`, `gap 10 slice 2b`, `P2 drain`, `P3`, `Route C`, `Task 8`, `increment 2b`, `STEP 5`) from the comments that carried them; GitHub issue references (`#32`, `#73`, `#171`, `#203`, `#215`, `#62`/`#23`) are kept. Also make four cross-reference comments self-contained by stating the fact instead of pointing at another comment ("see the module preamble", "see above", "the `current`-frame pattern", "see `history.rs`"). Comment-only: no code, string literal or test data is modified. Assisted-by: Claude * docs: correct three comments refuted by their own call sites Each claimed a capability was missing; each is contradicted by the code it sits next to. `descr.rs`'s tag block says the Field tag is load-bearing for a synthetic `FieldIndexDescr` that unpacks offset/size/type/signed out of the index bits. That descriptor and its helpers were deleted — `majit-ir`'s descr module records the removal — and `VirtualizableFieldState.fields` is keyed by `FieldDescr::index_in_parent()` now (`info.py:203-206`). Nothing decodes the tag; what it still buys is disjoint index ranges so two descr kinds cannot collide on one `HeapCache` key. The `ptr_eq/rr>i` opcode-table row says the `b1 is b2` fast path is omitted, "same rationale as int comparisons". Both handlers implement it: `binop_ref_to_int_record` answers an identical operand pair out of `fastpath_same_boxes` without recording, and so does `binop_int_record`. The `raise` arm says resume-data capture is omitted, pointing at the `goto_if_not/iL` arm, which carries no such comment. The guard this arm emits calls `walker_capture_snapshot_for_last_guard(ctx, op.pc)` twelve lines below, which is `generate_guard`'s `resumepc=orgpc`. Assisted-by: Claude * descr: mark PyCode.co_firstlineno immutable, per _immutable_fields_ `pycode.py:95-106` lists `co_firstlineno` in `_immutable_fields_`; the PyCode descr group marked every field mutable because its spec builder hard-coded the flag. Give the builder the flag as a parameter and set it from the upstream list: only `co_firstlineno` changes. `co_name` and `hidden_applevel` are absent from that list and stay mutable — `w_name` is realized lazily by `w_code_name_obj` and does go null -> non-null after construction — and `code_ptr` is the raw body pointer with no upstream slot. The slot really is write-once: `box_code_constant_with_firstlineno` writes it onto an object `box_code_constant` has just boxed out of a fresh `Box`, so no caching lets a reader see it first, and `code.replace` reads it and builds a new code object rather than writing this one. No behaviour change is expected or observed. The only trace-side reader of a field descr's `is_immutable` is the replay-cleanliness rule in `fbw_state.rs`, which fires on a `setfield_gc` into a freshly allocated object, and traced Python never constructs a PyCode. `check.py --no-build --backend dynasm`: 441/441, no jitstats delta. Assisted-by: Claude * majit-metainterp: replace pyre-side line refs in comments with symbol names Comment-only change across `majit/majit-metainterp/src/` (excluding `src/optimizeopt/`) and `majit/majit-metainterp/tests/`. - Rewrite `<file>.rs:NNN` citations of pyre's own Rust sources to name the symbol that lives there, or drop the line number when the surrounding prose already names it. Line refs inside string literals and inside ```text panic transcripts are left untouched. - Upstream `.py:NNN` citations (rpython/, pypy/, lib-python/, lib_pypy/) are unchanged. - Strip session-local tracking tags (Slice X-D/X-G/X3-E/QQ-n/P3/T-final, Sub-slice B/C.x, F.n-orthodox, M2 Step n, Box Identity Phase E Step n, #19 Step n, Step 2e.2b, P1.5) from comment prose. - Replace "see above"/"see the header"/"same rationale as" pointers with the fact plus the symbol that holds the rest. Assisted-by: Claude * majit-translate: replace pyre-side comment line refs with symbol names Strip `file.rs:NNN` / `file.rs:NNN-MMM` line numbers from comments in majit/majit-translate/src (excluding codewriter/ and annotator/) and majit/majit-translate/tests, keeping the symbol name the comment already cited or naming the enclosing item where the citation had none. Upstream `.py` citations are untouched. Also: - qualify ambiguous bare `model.rs` references to `flowspace/model.rs` or `annotator/model.rs` where the named symbol resolves there - drop self-referential filename parentheticals in rclass.rs, rpbc.rs, rtyper.rs, mir.rs, flowspace_adapter.rs, cutover.rs and rbuiltin.rs - point flowspace_adapter.rs's exc_from_raise cross-reference at the "TODO: `Constant` SSA carrier shape" section that exists in that module preamble - remove the "slice A" / "Slice C" tracking labels from flowspace_adapter.rs and llinterp.rs Comment-only: no code, string literal or test data changed. Assisted-by: Claude * pyre-interpreter: replace pyre-side line-number comment refs with symbol names Rewrite `<file>.rs:NNN` citations in comments under pyre/pyre-interpreter to name the symbol at the cited location instead of a line number, or drop the line number where the symbol was already named. Citations against the pinned rustpython-compiler-core and rustpython-sre_engine snapshots (`oparg.rs`, `bytecode/instruction.rs`, `string.rs`, `engine.rs`) are left as they are. Also replace directional cross-references ("see above", "see below", "the note above", "as noted above") with the fact or the owning symbol, and drop the "B1" tracking prefix from a jit_fnaddr comment. Comment-only; no code, string literal or test data changed. Assisted-by: Claude * Re-anchor pyre-side comment references to symbols Replace `<file>.rs:NNN` line citations in comments across majit-backend-{dynasm,cranelift,wasm}, majit-macros, majit-ir, majit-gc and pyre-object with the file name alone, or with the symbol that the cited line's enclosing item defines where the cited location still matches the comment's claim. Upstream `rpython/`, `pypy/`, `lib-python/` and `lib_pypy/` `.py:NNN` citations are unchanged, as is the `compiler.rs:12884` reference inside the `bridge_cache_addrs` expect string. Symbols named where verified: `bh_call_r` / `bh_call_f` / `bh_call_v` default trait impls, `gc_rewriter`, `do_compile`, `emit_guard_exit`, `cranelift_realloc_frame`, `dynasm_typeid_subclass_range`, `generate_state_fields_jit_state`, `generate_trace_fn`, `handle_new`, `gen_malloc_nursery`, `gen_write_barrier`, `handle_write_barrier_setfield`, `do_collect_nursery`, `rescan_major_nonstack_roots_and_drain`, `register_active_hooks`, `CompiledLoopToken`, `next_op_can_accept_cc`, `AbstractVirtualPtrInfo`. Replace three cross-reference pointers with the fact they pointed at: the `write_float_at_mem` "see read sibling above", the `reg_write_audit` "see the module doc", and the wasm `stamp_and_publish_label_targets` "the comment below". Drop the internal tracking ids `S-11`, `Phase E.3+`, `Slice 80-G.7` and `Pre-A.2.3` from the comments they appeared in. Assisted-by: Claude * fbw: record the measured mechanism behind the loop-bearing blackhole handoff decline The `walk_abort_adopted` deny-list arm for `LoopBearingCalleeInlineUnsupported` carried a comment saying its second blocker was open and that dropping the arm produces wrong code, without naming a cause. Measured it on both failing fixtures and replaced that paragraph with the mechanism. `bhimpl_jit_merge_point` treats a frame that has a `nextblackholeinterp` as the recursive portal level: it takes `bhimpl_recursive_call_*`, parks the result in `tmpreg_*` and raises `LeaveFrame`. A multi-frame image stacks the callee above its caller, and this decline reports that the callee bears a loop, so the callee reaches its own loop-header merge point before any `*_return` and the caller below it receives `tmpreg_*` as the callee's return value. Both fixtures leave at that opcode with `ret_type=Ref`: `inline_subwalk_user_iterator` on `[run@1054, step@260]`, and `list_append_write_barrier_gc` on `[big_live_len_regrow@1191, churn@162]`. Comment only; no behaviour change. Assisted-by: Claude * fbw: name the caller-image refusal, the vref bracket's size, and the two qmut decline causes Three diagnostics, all gated on fbw_debug_abort_enabled, no behaviour change. capture_inline_parent_blackhole answered None from its three liveness-pass early returns without printing anything, so the downstream "parent.blackhole None (capture missing)" could not say which bank, which color, or whether the walk's shadow was merely too short. Added report_caller_image_decline and wired it into the int, ref and float sites. Each site changed from `...get(color).copied()?` to an explicit `let Some(..) = got else`, which separates an out-of-range color from one whose shadow holds a different concrete kind; the `?` conflated them. The vref bracket's two halves iterate virtualref_boxes and nothing reported its length, so its size was only ever restated from which call sites populate it. Print the pair count above vrefs_before_residual_call. The qmut flush leg printed one decline message for both WalkEndResume variants that can be unprovable. Split it: RewindUnproven means no opcode-entry sample was taken, a still-unprovable Rewind means the opcode had already applied an effect. Assisted-by: Claude * fbw: correct two docs that call the vref bracket's loops empty Both said the bracket's loops are empty because no jit.virtual_ref producers exist. A producer does exist and runs: walker_ec_enter takes a vref of every seeded callee frame through TraceCtx::opimpl_virtual_ref, paired with opimpl_virtual_ref_finish when the frame leaves. Measured with the [vref-bracket] report over 431 synth + 93 parity fixtures: 5487 bracket entries, 686 of them (12.5%) with at least one pair, 66 of the 316 emitting fixtures reaching a nonzero count, maximum 7 pairs. Assisted-by: Claude * docs: name the produced-view source PreviewShortState::short_boxes `produced_short_boxes_from_exported_boxes`'s header still cited `ctx.exported_short_boxes`, a field removed when the preview export was collapsed into `PreviewShortState`. Point it at the surviving field. Assisted-by: Claude * descr: state PyCode field purity per field in its spec test `pycode_field_descrs_share_parent_and_preserve_specs` asserted `!descr.is_always_pure()` for every PyCode field. Marking `co_firstlineno` immutable made that field answer true — `is_always_pure()` returns the `immutable` flag — so the test has been failing since that change; its commit verified with `check.py` only, which does not run crate unit tests. Add the expected purity to the per-field tuple and compare it, so each field states its own answer and a move in either direction fails. Also re-point one comment in optimizer.rs at `exported_short_boxes`, the name the local kept after the preview-export collapse. `cargo test --release -p pyre-jit-trace -p majit-metainterp --features dynasm`: rc=0. Assisted-by: Claude * fbw: answer a caller image's unstamped ref color instead of refusing the image `capture_inline_parent_blackhole`'s liveness pass demanded a `ConcreteValue::Ref` for every ref color live at the resume pc and returned `None` for the whole image otherwise. The innermost-frame fill `build_single_frame_miframe` (residual_call.rs) runs the same pass and answers the two ways that demand fails: * a live color whose register holds no box is skipped — a `-live-` set is the union over the paths INTO its coordinate, so a color can be live there and undefined on the path walked, and `_copy_data_from_miframe` (`blackhole.py:1711-1730`) likewise leaves a `None` box unset; * a color whose shadow is `ConcreteValue::Null`, the walker's untracked sentinel that `write_ref_reg` stamps for every recorded-but-unobserved result, is recovered through `TraceCtx::recover_ref_value`. Port both. The image is still refused when neither applies. Measured with a report added at the refusal site, over 431 synth + 93 parity fixtures (dynasm, darwin): 21 refusals across 10 fixtures, all bank `r` with a `Null` shadow, partitioning as 11 no-box and 10 recoverable and 0 neither. After the change the corpus reports none. The report itself stays, extended with the box and its recoverability, and the header's decline count is corrected: it cited the downstream `[s2-build-decline]` symbol, which prints only when a multi-frame build was attempted and so undercounted tenfold. `check.py --no-build`: dynasm 441/441, cranelift 441/441, wasm 434/434, no jitstats delta. `cargo test --release -p pyre-jit-trace -p majit-metainterp --features dynasm`: rc=0. Assisted-by: Claude * fbw: route the hazardous-inline deny through disable_noninlinable_function `fbw_abort_nested_unjournaled_residual` names the callee an abort is attributable to and denies it, but the deny wrote only the walker-local `FBW_HAZARDOUS_INLINE_DENY` thread-local, so the callee's JitCell never carried `JC_DONT_TRACE_HERE` and no warm-state reader saw it. It now also calls `disable_noninlinable_function` on `make_green_key(callee_code, 0)`, the function-entry key `inline_call.rs` already uses for that callee — the same answer `pyjitpl.py:2818-2828` gives for the callee `find_biggest_function` names. The consuming half — `warmstate.py:485-496`, where a `JC_DONT_TRACE_HERE` cell that has never seen a procedure token retraces at once instead of waiting out the counter — is already carried by `WarmEnterState::maybe_compile_decision`. Measured (dynasm): the three fbw witness fixtures now mint the denied callee's cell, cells 3 -> 4 on each. `get_stats` counts the `BaseJitCellState` enum rather than the flag, so its `dont_trace_here` reads 1 only on `wasm_ca_trampoline_decline`; on the two `foriter_exempt_*` fixtures the new cell has already moved on to tracing (tracing 0 -> 1) and the state no longer names the deny its flag still records. `list_append_write_barrier_gc` gains a compiled loop (loops_compiled 12 -> 13), re-recorded on all three backends; the wasm baseline is from a measured wasm run. Assisted-by: Claude * fbw: name which hazard clause denied the nested-residual inline `fbw_inline_callee_hazardous` fires on three clauses and returned only the callee's code key, so the `PYRE_LB_SITE=1` `[lb-arm]` line could say `hazard=true` and nothing more. It now returns the clause name alongside the key and the report prints it: `hazard=repeat`, `hazard=for-iter`, `hazard=self-recursive`, or `hazard=false`. The clauses are not equally tight. `repeat` and `self-recursive` name the frame that is actually recursing; `for-iter` is `code_has_for_iter`, which fires on any code object whose bytecode contains a `FOR_ITER` anywhere, whether or not an iterator is in flight at the decline point. Census over 441 synth + 83 parity fixtures: for-iter 8 fires / 6 fixtures, self-recursive 2 / 2, repeat 2 / 2. Only two of the six `for-iter` fixtures are the witnesses that clause documents. Same denial set as before — the three clauses are checked in the same order and return the same key. check.py --no-build --backend dynasm: 441/441. cargo test --release -p pyre-jit-trace: rc=0. Assisted-by: Claude * warmstate: drop the DontTraceHere state and count the denial off its flag `JC_DONT_TRACE_HERE` had two representations: the flag, and a `BaseJitCellState` variant. Every real decision already read the flag — `can_inline_callable`, `counter_tick_checked`, `should_start_dont_trace_here_trace`, and `should_remove_jitcell` — while `is_compiled` and `is_tracing` read the token and `JC_TRACING`. The state variant reached only `get_stats`, and the two answers disagreed: `disable_noninlinable_function` set the state only when `JC_TRACING` was clear, so a cell denied on its way into a trace carried the flag but never took the state, and the census counted zero denials on every fixture that reaches the fbw hazard arm. The flag is now the only representation. `get_stats` counts it directly and independently of the lifecycle state, which is what makes a denied-then-tracing cell visible; `is_dont_trace_here` reads it; the two `state == DontTraceHere` tests in `counter_would_fire` and `counter_tick` were unreachable behind the flag test on the line above and are gone. warmstate.py has no such state either: `JC_DONT_TRACE_HERE` is orthogonal to the lifecycle — a denied cell still traces, compiles, and is invalidated, and `warmstate.py:485-496` retraces it once its procedure token dies. So the abort paths now leave `BaseJitCellState::NotHot` and set the flag alone, which also collapses `abort_tracing`'s three branches into the single condition `abort_tracing_for_key` already used. cargo test --release -p majit-metainterp --features dynasm: rc=0. Assisted-by: Claude * fbw: record that narrowing the for-iter hazard clause is wrong code `fbw_inline_callee_hazardous`'s `for-iter` clause is deliberately loose — it fires on any callee whose bytecode contains a `FOR_ITER`, in flight or not — and the census this branch added shows it carrying 8 of the 32 declines across 441 synth + 83 parity fixtures for 2 witnesses. Narrowing it to "a consume already ran in this frame" is measurable and wrong. `FBW_FORITER_INFLIGHT` answers that question without the per-frame Python pc `InlineFrame` lacks, since its `Jit` entries carry the `jitcode_index` each consume ran in, and it does cut the clause to 3 fires with both witnesses still declining. But `foriter_exempt_shared_generator` then produces wrong output on all three backends, `inline_subwalk_user_iterator` regresses (loops_aborted 1 -> 5, fbw_rolled_back_with_effects 0 -> 5, loops_compiled 3 -> 2) and `list_append_write_barrier_gc` loses its compiled loop again (13 -> 12). The witness still declined, just at pc 533 instead of 261: inlining the residual is what carries the walk to the consume, so a test conditioned on the consume having happened is always one step late. The clause has to stay forward-looking, and a real narrowing needs FOR_ITER reachability from the frame's current position — which is where the missing per-frame pc actually bites. Assisted-by: Claude
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up of #18, addressing review comments about embedded x86-64 assemblies.
Self-review
Prompt & Model
Model: gpt-5.5
Prompt:
Answer
Summary by CodeRabbit
Refactor
Bug Fixes
Tests