jit: read a deref-last projection as a value, and put fast2locals on the virtualizable protocol - #1358
Merged
Conversation
`build_rvalue`'s `Rvalue::Ref` and `Rvalue::RawPtr` arms called every `PlaceKind::Projection` an address-of. `&(*p).f` names the field's address; `&*(*p).f` reads the pointer the field holds and dereferences it. Both are projections, so both were marked. `mark_place_address_of` sets `taken_by_address` on the descriptor of the last op the projection emitted, which is the `getfield`. `rewrite_op_getfield` folds `suppresses_virtualizable()` into `fresh_virtualizable`, and the `vable_array_vars` insert is gated on `!fresh_virtualizable`, so a marked read is never registered: it stays an ordinary `getarrayitem_gc` against the heap array, which is written back only at `sync_virtualizable_before_jit`, `sync_virtualizable_after_jit` and `sync_virtualizable_after_guard_failure`. `locals_w!` expands to `&*$frame.locals_cells_stack_w`, the deref-last shape, and it has 44 call sites across `pyframe.rs`, `eval.rs` and `builtins.rs`. `place_ref_is_address_of` spells the `Deref` test as `resolve_place` and `emit_projection_write` already spell it, applied one level out. `address_of_the_vable_array_slot_is_marked_not_a_read` keeps the `addr_of_mut!` shape, whose outermost step is a field, on the marked side. Assisted-by: Claude
A subscript evaluates its receiver before its index expression, so `locals_w!(self)[self.valuestackdepth - 1 - depth]` emits the `locals_cells_stack_w` read first and the subtraction's overflow check after it. That check branches, so the array is defined in one block and consumed in another. Lowering `peek_at` shows it directly: the `FieldRead` is in bb0, the two `sub`s split bb0 from bb4 from bb5, and the `ArrayRead` is in bb5, with the array carried as a link argument the whole way. `_check_no_vable_array` rejects that — `Escaped via: link argument` — and the rejection is a `panic!` inside `generate_into`, which `pyre-jit-trace`'s `build.rs` calls with no `catch_unwind`, so it is a build failure rather than a degraded trace. The panic message names this cause first: "indexing with an index not known non-negative". `peekvalue_maybe_none` and `settopvalue` already compute the index into a local before the subscript, which is also how `pyframe.py:479-484` spells it, and lowering the former puts its `FieldRead` and `ArrayRead` in one block. Six sites did not: `peek`, `peek_at`, `peekvalues` (`base + idx`) and the three reads in `with_except_start`. The slice bounds check does not split a block — the front folds it into the `ArrayRead` — only the arithmetic does. Assisted-by: Claude
`pyframe.py:572` decorates `fast2locals` `@jit.unroll_safe`. `look_inside_graph` cancels `contains_loop` for a graph carrying the hint (`policy.py:61-62`), so the slot loop no longer keeps the codewriter out. Assisted-by: Claude
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughThe MIR translator now distinguishes address-taking projections from value reads. The interpreter also hoists stack-index calculations before virtualizable array reads and marks ChangesMIR address classification
JIT stack access
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to This PR changes JIT handling of frame-local reads and enables optimized frame synchronization paths. The package-specific verification passes, but the required dynasm-enabled repository checks are still outstanding, so merge readiness is moderate until those checks pass or the owner explicitly accepts the gap. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 11925bcec7
ℹ️ 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".
| /// allocates a fresh dict (pyframe.py:557 `self.space.newdict(instance=True)`) | ||
| /// and caches it, so `locals() is locals()` holds. Errors propagate. | ||
| /// | ||
| /// `@jit.unroll_safe` (`pyframe.py:572`) cancels `contains_loop` in the |
There was a problem hiding this comment.
In the checked-in pypy/interpreter/pyframe.py, line 572 is the cell.get() call, not the @jit.unroll_safe decorator; the decorator belongs to PyFrame.fast2locals and is currently at line 539. Replace this stale line reference with a symbol-based citation such as pypy/interpreter/pyframe.py PyFrame.fast2locals, as required by the root AGENTS.md, so the comment points to the evidence it claims.
AGENTS.md reference: AGENTS.md:L188-L191
Useful? React with 👍 / 👎.
youknowone
added a commit
that referenced
this pull request
Aug 20, 2026
…e three #1358 follow-ups (#1374) * jit: name a virtualizable config whose array lengths were never patched in `to_optimizer_config` builds `VirtualizableConfig` with `array_lengths: vec![]` and relies on its caller to fill them in: `MetaInterp::current_virtualizable_optimizer_config` assigns `ctx.virtualizable_array_lengths()` one line later, beside the identical patch of `vable_input_offset`. That sibling field documents the convention on itself; `array_lengths` did not. A length is not a property of the shape — upstream reads `len(lst)` off the live object in every `virtualizable.py` accessor and stores it nowhere. `VirtualizableTracker::init` zips `array_field_offsets` with `array_lengths`, so a config that declares an array field and carries no length runs that loop zero times, leaves `state.arrays` empty, and turns every later `tracked_array_element` into a miss that reads as "this trace had no array elements". The `debug_assert!` names that state instead of absorbing it. Both escapes in the assertion are load-bearing: the state-field macro JIT sets `track_array_elements = false` and carries its elements through the live `virtualizable_boxes` shadow, and a virtualizable with no array field has nothing to seed. `array_tracking_config_without_lengths_is_named_not_absorbed` has to build the state by hand, which is itself the statement that no production path produces it: both writers of `TraceCtx::virtualizable_boxes` set the lengths in the same statement, `state.rs seed_virtualizable_boxes` passes `vec![array_len]` on the portal and bridge paths, and `optimizer_vable_config_matches_registered_virtualizable_when_boxes_active` already pins the patched result. Assisted-by: Claude * interpreter: record why _unpackiterable_known_length_jitlook carries no unroll_safe The doc comment quotes upstream's `@jit.unroll_safe` along with the body it ports, which reads as an unfinished port. It is not one. Upstream reaches that body two ways and hints only one of them: `unpackiterable` goes through `_unpackiterable_known_length`, which is `@jit.dont_look_inside` ("the JIT stopped looking inside already"), while `unpackiterable_unroll` calls it directly with an UNPACK_SEQUENCE oparg as `expected_length`. pyre has neither `unpackiterable_unroll` nor the shim, so `unpackiterable` is this body's only caller — the one upstream fences off. Being loopy and unhinted the graph is rejected by `look_inside_graph` and stays a residual call, which is the boundary the shim buys upstream. Carrying the attribute alone would open that path, with `expected_length` — a red argument on one graph ~40 callers share — as the unroll bound. Restoring the split needs more than the attribute: `#[majit_macros::dont_look_inside]` registers a helper call descriptor, and `helper_call_kind_for_type` answers `Unsupported` for this signature's `Result<Vec<PyObjectRef>, PyError>`, so the shim needs an `extern "C" fn(..) -> i64` publication first. `test_unroll_safe_inventory` asserts the harvested `unroll_safe` set is a subset of a reviewed list, plus a named negative for this body. Subset rather than equality because a developer's `build/llbc` is routinely older than the source and can only under-report, which must not red; `builtins:: leading_non_null_count` is the positive control, and its absence skips the test loudly rather than passing on an artefact too old to say anything. Assisted-by: Claude * jit: carry the iterator element type into the next fold `iter_next_item_type` answered `Int` for a container produced by `front::range_iter`'s `range()` builtin and `Ref` for every other one. The `iter` op carries the iterator, not the container's item type, and a slice of non-GC items is spelled exactly like a slice of references — `is_concrete_iter_constructor` collapses `Vec<T>`, `[T; N]` and `Box<[T]>` onto the same `core::slice::…::iter`. So the container alone could not separate them. `charon-corpus`'s `branch_loop_sum(slice: &[i64], ..)` folds `for &v in slice`, and its `i64` element was typed as a GC reference. `result_ty` is not a hint the rtyper overrules: `resolve_call_result_kind` consults `concretetype` only when `result_ty` is `Unknown`, and `authoritative_result_types` stamps the derived kind back over it, so the answer here outranks the rtyper for every graph that gets a JitCode. The recording site already reads a callee's `Result` payload for `result_exc_call_results`; `next_call_results` now carries the `Option<T>` payload the same way, with the `&` a slice iterator adds peeled off by `strip_ty_wrappers`. `Ref(Some(root))` normalises back to `Ref(None)` so every GC-element graph that folds today stamps a byte-identical `result_ty`, and the range arm answers before the recorded type is consulted, because `rrange.py ll_rangenext_*` returns `Signed` whatever the Rust range spells. An unreadable `Option` shape falls back to `Ref(None)`, the answer the fold assumed unconditionally before. `branch_loop_sum_next_yields_an_int_element` fails on the previous behaviour with `left: [Ref(None)], right: [Int]`. Assisted-by: Claude * jit: port arraylen_vable to the codewriter `rewrite_op_getarraysize` (`jtransform.py:808-817`) is the third consumer of `vable_array_vars`, alongside `rewrite_op_getarrayitem` and `rewrite_op_setarrayitem`. The codewriter had the other two and answered a `len()` over a virtualizable array with a plain `arraylen_gc` on the raw array pointer. Adds `OpKind::VableArrayLen`, the `rewrite_op_getarraysize` arm, and the assembler encoding for the `arraylen_vable/rdd>i` key that `insns.rs`, `blackhole.rs`, `opimpl_arraylen_vable` and `bhimpl_arraylen_vable` already carried. The macro lowering (`majit-macros` `lower_vable_array_len`) emitted the instruction; the codewriter path did not. Assisted-by: Claude * jit: drop the getfield a virtualizable array field read produced `rewrite_op_getfield`'s `except VirtualizableArrayField:` handler ends in `return []` (`jtransform.py:848-857`): registering the base in `vable_array_vars` is the whole rewrite. The port kept the op, so a `getfield_gc_r` of the array pointer stayed in the jitcode and fell through to the immutability-rank rewrite below. All three consumers now answer against the vable base, so the read has no user left. Assisted-by: Claude * jit: fix indentation and the descr-pair citation on the arraylen_vable arms Two of the new match arms landed at the wrong column; `cargo fmt` leaves them alone because it bails on the enclosing `match` in both files. The descr-pair comments named `expect_matching_vable_array_descrs`, which is `pyre-jit`'s assembler. The runtime that decodes the emitted `arraylen_vable/rdd>i` is `MIFrame::vable_array_index_pair_at`. Assisted-by: Claude * jit: address the #1374 review — gate the vable arms, and peel one reference `rewrite_op_getfield` runs whether or not `lower_virtualizable` is set, because the quasi-immutable tail below it does not depend on virtualizable lowering. Its two virtualizable arms do. With `vable_arrays` set and the flag off, the array arm registered a base no consumer would read and dropped a read those consumers still referenced, leaving regalloc an undefined variable. `strip_ty_wrappers` peels `Ref` repeatedly, so an iterator over `&[&i64]` recorded its `Option<&&i64>` payload as `Int` and put a pointer in the integer register bank. `iterator_payload_element` peels the one reference the iterator adds and leaves the element's own. Also: require `array_lengths.len() == array_field_offsets.len()` rather than non-emptiness, since the zip truncates a short vector silently; pin the `arraylen_vable/rdd>i` wire shape and its descr pair; and fail the unroll_safe inventory when two harvested paths share a leaf, which is the assumption it matches on. Assisted-by: Claude * jit: read the iterator ADT before peeling, and catch vable array escapes by any route `iterator_payload_element` peeled one reference off every `next()` payload. A slice iterator adds that reference, but the by-value iterators `is_concrete_iter_constructor` admits do not: `alloc::vec::into_iter::IntoIter` and `core::array::iter::IntoIter` yield `Option<T>`, so the payload is the element already. `Vec<&i64>` and `[&i64; N]` therefore recorded their `&i64` element as `Int` and put a pointer in the integer register bank -- the mirror image of the `&[&i64]` defect the peel was added for. The `next()` receiver names the iterator ADT; peel only for `core::slice::iter::Iter` / `IterMut`. `slice_of_refs_sum` and `array_of_refs_sum` carry both shapes in the corpus. Peeling unconditionally fails the first, never peeling fails `branch_loop_sum_next_yields_an_int_element`; no fixed answer passes both. `check_no_vable_array` enumerated four operand positions. Registering a variable in `vable_array_vars` drops the `getfield` that defined it, and nothing prunes dead operations between `transform` and regalloc, so any operand a kept operation still names is a variable used and never defined. A fifth route scans every operand of every operation the block kept; it reports last and least precisely, and it exists because the four are an enumeration. `_handle_list_call` carries no `vable_array_vars` check and is owed none: upstream splits on `resizable`, putting the check on the `do_fixed_list_*` arms whose receiver is a `GcArray`, and every spelling pyre ports is of the resizable family with a `W_ListObject` receiver. Also: decode the assembled bytes in the `arraylen_vable` wire-shape test rather than only the descr pool order; exercise `lower_virtualizable = false` on the scalar-field arm as well as the array arm; and run the unroll_safe leaf-collision check after the CONTROL guard, so a stale artefact is skipped rather than judged. Assisted-by: Claude * majit: cover the untested virtualizable abort paths `VableArrayIndexNotConcrete` and `GuardSnapshotVableUntyped` had no tests. Neither fires on the synth corpus (0 across the 374 fixtures that trace, where `VableEscapedDuringResidualCall` takes 123). - `array_vable_handlers_with_unpinned_index_surface_index_not_concrete` drives `getarrayitem_vable_i` / `setarrayitem_vable_i` with a seeded vable ref and an index register holding no concrete value. - `an_untyped_virtualizable_box_is_not_snapshot_buildable` pins `TraceCtx::vable_snapshot_buildable` over an absent box list, an all-typed list, and an untyped entry in each of the two positions `build_vable_snapshot_boxes` reads separately. - `build_vable_snapshot_boxes_panics_on_an_untyped_{identity,entry}` pin the two `.expect()` calls that predicate keeps unreachable. Assisted-by: Claude * majit: drop the optimizer's virtualizable array-element seeding Both loop-close arms carry the tracer's live `virtualizable_boxes` shadow into the JUMP as `[reds..., virtualizable_boxes[..-1]]`: the macro state-field JIT through `JitState::collect_jump_args_with_boxes`, PyFrame through `jitcode_dispatch::append_virtualizable_boxes`. PyFrame reaches the second only — nothing under `pyre/` produces `TraceAction::CloseLoop`, so its `collect_jump_args_with_boxes` override is not called in production; note that where the override is defined. `elements_carried_via_shadow` classified PyFrame as not shadow-carried and kept `track_array_elements` on for it, so `VirtualizableTracker::init` seeded element state from the trace-entry input args. Remove that seeding, along with `VirtualizableConfig::track_array_elements`, `::array_lengths` and the length patch in `current_virtualizable_optimizer_config`. The standard-path read answers from the shadow and records no op (`vable_getarrayitem_*_checked`, pyjitpl.py:1170-1184), and the tracer updates the shadow through `set_virtualizable_entry_at` without recording one, so a seeded element box had nothing to fold against and could go stale. Measured before removal: check.py dynasm 434/434, zero jit-stats counters moved. Replace the three tests that pinned the removed length assertion with one that pins what `ensure_setup` still owes — the identity `PtrInfo::Virtualizable` install — and state in the tracker's doc which parts remain and what retiring them would require. Also check `set_virtualizable_entry_at`'s documented precondition against `virtualizable_slot_type` instead of only stating it: a non-Ref value in a Ref slot decodes to NULL through `value_as_ref_bits`. 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
1 participant
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.
&*(*p).fwas classified as taking the address of(*p).f. It reads thevalue that field holds. Every
locals_w!read of the frame's virtualizablearray has that shape, so the classification kept all of them out of the
virtualizable protocol — and that is what stopped
@jit.unroll_safefrombeing portable onto
PyFrame::fast2locals.The defect
build_rvalue'sRvalue::RefandRvalue::RawPtrarms testedmatches!(&place.kind, PlaceKind::Projection(..)). That is true for both&(*p).fand&*(*p).f; only the first names a field's address.The mark is not inert.
mark_place_address_ofsetstaken_by_addresson thedescriptor of the last op the projection emitted — the
getfield.rewrite_op_getfieldfoldssuppresses_virtualizable()intofresh_virtualizable, and thevable_array_varsinsert is gated on!fresh_virtualizable:So a wrongly marked read is never registered. It stays an ordinary
getarrayitem_gcagainst the heap array — not a slower equivalent of theprotocol, a stale read: the heap array is synchronised only at
sync_virtualizable_before_jit,sync_virtualizable_after_jitandsync_virtualizable_after_guard_failure, andVirtualizableInfo::to_optimizer_configpassesarray_lengths: vec![], sothe optimizer's const-index backstop is unseeded too.
locals_w!expands to&*$frame.locals_cells_stack_w. It has 44 callsites across
pyframe.rs(27),eval.rs(15) andbuiltins.rs(2).The fix
place_ref_is_address_ofreturns false when the outermost projection elementis
Deref. The test is spelled the wayresolve_placeandemit_projection_writealready spell it, applied one level out;mir.rshasfive other sites with the identical
PlaceKind::Projection(_, ProjectionElem::Atom(s)) if s == "Deref"match.It is deliberately narrow.
addr_of_mut!(frame.locals_cells_stack_w)isField-last and stays marked —
address_of_the_vable_array_slot_is_marked_not_a_readpins exactly that, including its own anti-vacuity guard.
@jit.unroll_safeonfast2localsUpstream
pyframe.py:572decorates it.look_inside_graphcancelscontains_loopfor a graph carrying the hint (policy.py:61-62), so the slotloop stops keeping the codewriter out.
Ordering is load-bearing and this PR respects it. The hint is the arming
switch for the defect, not a consumer of it: it admits
fast2localsinto thejitcode population, which is what makes the wrongly-classified reads reachable.
Ported alone it is either dead or latently wrong. The defect fix is the first
commit here; the port is the second.
fresh_virtualizableis not an escape hatch for a reader that trips_check_no_vable_array. Upstream'sis_virtualizable_getsetreturns False onthat flag before
raise res, so hinting a reader removes the access from theprotocol rather than fixing it — i.e. it silences the build by shipping the
stale read.
rlib/jit.py:90documents it as "virtualizable was justallocated", and upstream's only production use is
PyFrame.__init__.The escape the fix exposed, and why the second commit exists
Applying the first commit alone breaks the build. That is not a
speculation —
cargo check -p pyre-jit-tracedied with:Registering 44 previously-suppressed reads arms
_check_no_vable_arrayforevery graph holding one, and
peek_atwas the first to trip it.Lowering it says why, and the contrast with a sibling isolates the cause:
A subscript evaluates its receiver before its index expression, so
locals_w!(self)[self.valuestackdepth - 1 - depth]emits the array readfirst and the two subtractions' overflow checks after it; each check branches,
and the array rides the links.
peekvalue_maybe_nonecomputes the index intoa local first, which is also how
pyframe.py:479-484spells it, and its readand use land in one block. The slice bounds check is not the problem — the
front folds it into the
ArrayRead, which is exactly what that contrastshows.
Six sites spelled the arithmetic inside the brackets; the other twenty-four
already hoisted it. Loop-shaped readers (
clear_stack_above,peekvalues,restore_resume_state_from,build_snapshot_frame) are residualized bycontains_loopand never reach the check — which is the same reason theunroll_safehint is the arming switch forfast2locals.Verification
cargo check -p pyre-jit-traceis the authoritative gate:build.rscallsgenerate_intowith nocatch_unwind, so an escaping graph is a hard buildfailure rather than a degraded trace, and CI runs it via
cargo test --all.With all three commits applied, against a freshly extracted and stamped set of
all four artefacts (
--checkclean,window writes: 0 candidate(s)):_check_no_vable_arraypanic;fast2localsis among them — the port takes effect;peek_atandpeekstill in the population, now without escaping.Tests, all green:
a_deref_last_projection_reads_a_value_rather_than_naming_an_addresspinsthe classification on five constructed shapes with no LLBC, so it runs in
ordinary CI.
test_vable_array_len.rs, includingaddress_of_the_vable_array_slot_is_marked_not_a_read—addr_of_mut!isField-last and must stay marked, so this is the over-reach guard for commit
1 — and
every_vable_array_read_in_fast2locals_is_consumed_in_its_block,which covers exactly the graph commit 3 admits.
test_fast2locals_codewriter.rs, both tests.Follow-ups (not in this PR)
VirtualizableInfo::to_optimizer_configpassesarray_lengths: vec![],leaving
optimize_getarrayitem_gc'stracked_array_elementunseeded.baseobjspace.rs:13921quotes upstream_unpackiterable_known_length_jitlookwith its
@jit.unroll_safein a doc comment, but carries no attribute.front/iter_next.rs::is_iter_op_segmentsadmits non-GC slices (&[usize],&[u8]); narrowing it needs the container's element type.Summary by CodeRabbit
Bug Fixes
Performance and Stability