jit: pc_map read-cutover B–D + vestigial py_pc cleanup; begin backxlat reentry-inverse deletion (jitcode-blackhole Slice 1) - #709
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
✨ 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit dbb2fb0). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
cd6b272 to
ba3cb65
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afdf9e452d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1907,12 +1907,6 @@ pub use crate::liveness::{LiveVars, liveness_for}; | |||
| pub struct PyreJitState { | |||
| #[vable(frame)] | |||
| pub frame: usize, | |||
There was a problem hiding this comment.
Removing resume_pc here leaves pyre/pyre-jit-trace/tests/multi_frame_restore_supported.rs still constructing PyreJitState { frame: 0, resume_pc: None }, so the required cargo test --features dynasm path will fail to compile that integration test with an unknown-field error before any tests run. Update the integration test (or keep a compatibility field) in the same change so the required test suite can build.
AGENTS.md reference: AGENTS.md:L200-L200
Useful? React with 👍 / 👎.
afdf9e4 to
493e626
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 493e626e32
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| depth_after_residual_pred_by_jit_pc | ||
| .push((pos, static_depth.get(ft_rc).copied())); |
There was a problem hiding this comment.
When two Python PCs share the same first_jit_pc_by_py_pc offset, this new depth twin keeps both duplicate keys and later sorts them unstably, unlike the floor table just above that collapses duplicate offsets with the later py winning. depth_after_residual_for_jitcode_pc(op.pc) exact-searches by that offset, so an after-residual guard whose call starts at a shared offset can read the earlier py's fallthrough depth instead of the same py that python_pc_for_jitcode_pc would resolve, sizing the vstack/snapshot to the wrong stack depth; publish this table with the same last-wins deduplication before binary search.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
The operand-stack mirror's exception-handler entry (`vstack_enter_exception_handler`) and the resume-frame coordinate (`vstack_coordinate_for_jitcode_pc`) read the value-stack depth by inverting a jitcode pc through `vstack_containing_py_pc` and indexing the py_pc-keyed static-liveness `depth_at_py_pc`. `vstack_containing_py_pc` is a pure floor lookup over `py_floor_by_jit_pc` — no block-head marker precedence, no trivia — so the existing marker-baking depth twins cannot reproduce it. Build a new floor-only twin `depth_containing_by_jit_pc` at `finalize_jitcode`, sharing `py_floor_by_jit_pc`'s keys exactly with each value the static-liveness depth at that segment's py, and read it through `PyJitCode::depth_containing_for_jitcode_pc`. A `partition_point` floor pivot identical to `floor_segment_for_jitcode_pc` makes the twin equal `depth_at_py_pc[vstack_containing_py_pc(jit_pc)]` (including the out-of-range -> 0 collapse) for every jit_pc. Both seams gate on `depth_containing_populated()`; skeleton / fixture installs (empty twin) keep the raw py_pc-keyed read, so the result is byte-identical. `PYRE_PCMAP_CONTAINING_AUDIT` asserts twin == raw at both seams; off in production. Assisted-by: Claude
The inline-caller-frame resume (`compute_inline_caller_frame`, `compute_nested_inline_caller_frame`) and the self-recursive call-assembler vstack rebuild (`try_walker_call_assembler_self_recursive`) read the value-stack depth at the after-residual-call fallthrough pc by inverting a jitcode pc to a python pc and indexing the py_pc-keyed static-liveness `depth_at_py_pc`. Build a jitcode-pc-keyed twin `depth_after_residual_by_jit_pc` at `finalize_jitcode`, extending the existing `result_color_after_residual` construction loops: for each block-head / op-start offset, bake the static-liveness depth at `semantic_fallthrough_pc(py)` for the raw resolving py (no trivia skip). Read it through `PyJitCode::depth_after_residual_for_jitcode_pc`, resolved with the same exact-marker / predecessor-op-start tiers as `after_residual_marker_for_jitcode_pc`, so it equals `depth_at_py_pc[semantic_fallthrough_pc(python_pc_for_jitcode_pc(jit_pc))]` by construction. The three seams gate on `depth_after_residual_populated()` and keep the raw py_pc-keyed read as the skeleton / fixture fallback, so the result is byte-identical. `PYRE_PCMAP_AFTERRESIDUAL_AUDIT` asserts twin == raw at each seam; off in production. The branched multi-frame vsd read (`walker_capture_multi_frame_inline_snapshot`) is left for a follow-up. Assisted-by: Claude
`walker_capture_multi_frame_inline_snapshot` published the outermost caller's `valuestackdepth` scalar by inverting the paused-parent resume coordinate to a python pc (`resolve_parent_resume_py_pc`) and indexing the py_pc-keyed static-liveness `depth_at_py_pc`. Read the depth off the jitcode-pc twins instead, branching on the parent resume coordinate: `Backxlat` reads `depth_trivia_for_jitcode_pc`, `CallFallthrough` reads `depth_after_residual_for_jitcode_pc`. The raw read resolves the py against `outer.jitcode_index` but indexes the caller frame's own liveness, so the twin cut is gated on `outer.jitcode_index == jc.index`; a cross-jitcode parent keeps the raw read. The `None` depth arm stays `valuestackdepth`, so the twin's `Option` is pattern-matched rather than defaulted. Byte-identical: `PYRE_PCMAP_AFTERRESIDUAL_AUDIT` asserts twin == raw at the seam. Completes the after-residual / multiframe depth read-cutover; only the py-only params and deliberate `None`-arm fallbacks remain py_pc-keyed. Assisted-by: Claude
`step_vstack_mirror` and `seed_vstack_mirror` published the operand-stack mirror depth by inverting a jitcode pc to a python pc (`vstack_step_py_pc` / `vstack_initial_py_pc`) and indexing the py_pc-keyed static-liveness `depth_at_py_pc`. Read the depth off the jitcode-pc twins instead. `step_vstack_mirror` reads the floor-only `depth_containing_for_jitcode_pc`: its depth is consumed only when the walk crosses an opcode boundary, where `vstack_step_py_pc` returns the floor segment py, so the floor twin reproduces the raw read (the block-head-marker branch returns the current py and the value is dead). `seed_vstack_mirror` mirrors `vstack_initial_py_pc`'s branch: the floor twin for a normal entry, and a new exact-match block-head depth twin `depth_block_head_by_jit_pc` (same keys as `block_head_py_by_jit_pc`, same static-liveness source) for a permuted FOR_ITER entry, falling back to the floor when the start pc is not a marker offset. Both seams keep the raw read when the twin is unpopulated (skeleton / fixture). Byte-identical: `PYRE_PCMAP_CONTAINING_AUDIT` asserts twin == raw at each seam. Retires the last py_pc-keyed `depth_at_py_pc` reads in the vstack mirror. Assisted-by: Claude
`collect_outer_active_boxes` carried two Python-pc parameters that no longer key any table: `entry_py_pc` fed only a diagnostic panic string, and `guard_py_pc: Option<u32>` was read only as a presence flag (its `Some(gpc)` binding was never used — the body resolves everything through `carried_jitcode_pc`). Every live table read is already keyed by `carried_jitcode_pc` / `entry_jitcode_pc`. Drop `entry_py_pc` (the panic string now reports `entry_jitcode_pc`) and replace `guard_py_pc` with `guard_present: bool`. The four entry-capture call sites pass `false`; the guard-capture site passes `guard_py_pc.is_some()`. Two now-unused `backxlat_py_pc` inversions that existed only to compute the dropped argument — `root_py_pc` in `compute_bridge_root_parent_frame` and `call_site_py_pc` in `try_walker_inline_resolved_user_call` — are removed. Byte-identical: no table index changes. Assisted-by: Claude
Two `python_pc_for_jitcode_pc` inversions remained whose result no longer keyed any table — each only fed a diagnostic string or a presence flag. - `CalleeAbortPermanentHit.marker_py_pc` (trace.rs) was computed in `loop_inlines_abort_permanent_callee` solely to print `marker_py=` in the `fbw_debug_abort_enabled()`-gated decline log. Drop the field and the inversion; the log now reports only `abort_permanent_jit_pc` (`marker_jit_pc`). - `guard_py_pc: Option<u32>` in `walker_capture_snapshot_for_last_guard_impl` (resume_snapshot.rs) was read only as `.is_some()` (presence) and as `liveness_py_pc = guard_py_pc.unwrap_or(py_pc)`. Its former consumers now key on the guard's own jitcode pc: the #124 kept-stack overlay resolves through `scope.branch_guard_jitcode_pc`, and the entry window through `entry_jitcode_pc`. Branch guards and the `after_residual_call` family (the only other `liveness_py_pc` consumer) never coincide, so `liveness_py_pc` collapses to the merge `py_pc`. Replace the inversion with `has_branch_guard: bool`. Byte-identical: no table index or resume-coordinate changes. Assisted-by: Claude
`rebuild_typed_from_rd_numb` derived an outer-frame Python pc via the `backxlat_py_pc` jitcode→py inverse and stored it into `PyreJitState.resume_pc`. The field had no readers repo-wide — its doc comment's `restore_guard_failure_values` consumer was never wired. Delete the field, both writers, its initializers, and the now-dead backxlat computation; `rebuild_typed_from_rd_numb` drops the `Option<usize>` element from its return tuple. This removes one of the two `backxlat_py_pc` call sites; the surviving one is the multi-frame branch of `build_resumed_frames`. Assisted-by: Claude
493e626 to
dbb2fb0
Compare
pc_map read-cutover — slices B–D + vestigial py_pc cleanup
Continues the #366 layer-3 / #73 work of retiring the surviving py_pc-keyed
depth_at_py_pc()reads in the JIT tracer, cutting each seam over to thejitcode-pc-keyed "twin" tables built at
finalize_jitcode. With these slices,every
depth_at_py_pcread in the tracer is retired and the remainingvestigial py_pc plumbing is removed.
Depth-read cutover
Each twin reproduces the py_pc-derived depth by construction (same static
depth_at_py_pcsource, same keys), verified byte-identical per slice via aPYRE_PCMAP_*_AUDITassert-equality gate + audit-OFF byte-identity before thisrebase, and re-verified green on the current base.
188ad44): containing-opcode depth (sites Enhance dict lookup caching and propagate extra descriptors #8/Refactor register file to color-indexed mapping and elide warnings #17) onto a newfloor-only twin
depth_containing_by_jit_pc.vstack_containing_py_pcis apure floor segment (no block-head marker, no trivia) that the marker-baking
twins cannot reproduce.
fe1bd28): after-residual fallthrough depth (build(deps): bump the actions group across 1 directory with 4 updates #12/Fix list_setslice.py bench #13/Fix blackhole failure handling and update documentation references #15) ontodepth_after_residual_{marker,pred}_by_jit_pc, extending the existingresult_color_after_residual_*construction loops.f0dde46): the multi-frame-inline resume vsd depth (Eliminate trivially-true guards after force_box resolution #14),branching on
ParentResumeCoord(Backxlat→trivia twin, CallFallthrough→after-residual twin), gated on
outer.jitcode_index == jc.index.9aac63b): the vstack-mirror seed/step depths (Optimize info transfer in replace_op for AbstractInfo handling #6/Increase stack size for build.rs to prevent overflow on Windows #7). Optimize info transfer in replace_op for AbstractInfo handling #6 reusesthe floor twin; Increase stack size for build.rs to prevent overflow on Windows #7's permuted-FOR_ITER-entry arm reads a block-head marker, so
it needed a new exact-match
depth_block_head_by_jit_pc.Vestigial py_pc plumbing removal
1502d8a: dropcollect_outer_active_boxes's deadentry_py_pc(panic-string only) and
guard_py_pc: Option<u32>→guard_present: boolparams, plus two now-unused
backxlat_py_pcinversions.98334a0: drop the last twopython_pc_for_jitcode_pcinversions —CalleeAbortPermanentHit.marker_py_pc(debug-log only) andguard_py_pc→has_branch_guardin the guard-snapshot capture.After these, no byte-identical-removable py_pc plumbing remains: every surviving
py_pc use is load-bearing or the structural
backxlat_py_pcinterpreter-reentryinverse (pyre's resume interp runs Python bytecode, so it needs the jitcode→py
inverse at re-entry — a difference from PyPy's blackhole, which runs jitcode
directly). Deleting that inverse is a separate "jitcode-blackhole" epic (make the
resume/reentry interp itself run jitcode), out of scope here.
jitcode-blackhole epic — Slice 1
With the read-cutover done, the only remaining py_pc use is the structural
backxlat_py_pcjitcode→py reentry inverse. pyre already has a working,PyPy-orthodox jitcode-blackhole (
majit-metainterp/src/blackhole.rs+resume::blackhole_from_resumedata, jitcode-coordinate, no backxlat); theinverse survives only via the bridge-trace resume path. There are exactly two
backxlat_py_pcreentry consumers, and the first is dead:afdf9e4: delete the write-onlyPyreJitState.resume_pcfield.rebuild_typed_from_rd_numbderived an outer-frame py pc viabackxlat_py_pcand stored it into
resume_pc, which had zero readers repo-wide (its doccomment's
restore_guard_failure_valuesconsumer was never wired). Deletes thefield, both writers, all initializers, and the dead computation — dropping the
Option<usize>from the function's return tuple. Removes one of the twobackxlat_py_pccall sites. (PyPy stores no such field — the resume pc is alocal threaded forward, never persisted or computed backward.)
Verification
cargo check -p pyre-jit-trace --features dynasm: clean (only the pre-existingvstack_mirror.rs:168unreachable-pattern warning).python3 pyre/check.py --backend dynasm,cranelift: dynasm 246/246 +cranelift 246/246.