jit: collapse the DONT_TRACE_HERE duplication and route the fbw hazard deny through the warm state - #1364
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 216 files, which is 116 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (216)
You can disable this status message by setting the 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: a5dc1c2db9
ℹ️ 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".
| // `GetfieldGcR` chain back to a value the walk did observe | ||
| // and rereads the field. | ||
| Some(o) => match ctx.trace_ctx.recover_ref_value(o, 8) { | ||
| Some(majit_ir::Value::Ref(gc)) => gc.0 as pyre_object::PyObjectRef, |
There was a problem hiding this comment.
When a paused inline caller has a live reference whose shadow is ConcreteValue::Null and whose OpRef was stamped Value::Ref(GcRef::NO_CONCRETE)—for example after a failed materialized-array fold—recover_ref_value returns that sentinel immediately through concrete_of_opref. This new arm accepts every Value::Ref and writes usize::MAX - 1 into the caller's blackhole frame, so resumption can dereference an impossible PyObjectRef instead of declining the image. Reject GcRef::NO_CONCRETE here, as concrete_ref_for_opref and the other concrete-reference consumers do.
AGENTS.md reference: AGENTS.md:L19-L23
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit c9ed5a6). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptationsNone. |
`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
…tate 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
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
…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
`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
…s 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
…y 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
…e 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
…imed
`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
…stants `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
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
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
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
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
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
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
`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
… 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
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
…bol 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
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
…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
…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
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
`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
`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
…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
…ction `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_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
…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_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
youknowone
added a commit
that referenced
this pull request
Aug 20, 2026
…ecord why filtering it downstream is not the follow-up (#1380) * majit: stop recover_ref_value answering the NO_CONCRETE sentinel as a value `recover_ref_value` returned whatever `concrete_of_opref` held, including `Value::Ref(GcRef::NO_CONCRETE)` -- the stamp `heapcache_ops` writes over a load the walk could not replay. The two blackhole-frame ref fills that reach this function directly match on the `Value::Ref` variant alone, so `usize::MAX - 1` was written into a resumed frame's ref bank as if it were an object address: `capture_inline_parent_blackhole` (jitcode_dispatch/resume_snapshot.rs) and `build_single_frame_miframe` (jitcode_dispatch/residual_call.rs). It now answers unresolved, which is what those sites already do for a color whose register holds no box: the image is declined and the outer flush falls back to the legacy replay. The receiver guard in the same function checked NULL and the all-ones tombstone by hand and missed the sentinel; it now calls `live_gc_ptr`, the one place that names all three. Sites that read the recorder's stamp table first and only then fall back to this function are NOT covered, because `Some(_).or_else(f)` never calls `f`: `fill_trace_too_long_register_banks` and the two `vable_ops` resolvers still see the sentinel. The test fails on the unfixed function (returns `Some(Ref(NO_CONCRETE))`) and carries a positive control so the assertion cannot pass by the stamp failing to land. Reported by Codex review on #1364. Assisted-by: Claude * majit: record at lookup_opref_concrete why filtering the NO_CONCRETE stamp is not the fix The recorder's stamp table reports `Value::Ref(GcRef::NO_CONCRETE)` verbatim -- `heapcache_ops`' materialized-array walk writes it over a load it could not replay. `recover_ref_value` rejects the sentinel; this method does not, because "never stamped" and "stamped unresolved" are different states and only the caller knows which one it can act on. Three sites pair the two stamp-first, so `Some(_).or_else(f)` short-circuits ahead of that guard: `vable_value_concrete`, `current_inline_vable_target` and `fill_trace_too_long_register_banks`. Two more accept the sentinel through a `value.0 != 0` arm, which excludes NULL but not `usize::MAX - 1`. Filtering it at those sites was proposed and does not hold: each one ends in a fallback tail, so rejecting the sentinel does not decline the image, it promotes `from_register` / `from_shadow` / `live_vable_frame_addr` -- a different concrete address landing in a blackhole frame's ref bank or, through `store_live_frame_array_slot`, in a live frame's locals array behind a write barrier. `vable_ops.rs` already records that re-reading the shadow it would promote can return a stale concrete value from a prior loop iteration. Comment only; no behaviour change. Assisted-by: Claude * majit: name what the finish_and_compile loops_compiled bump counts `exception_bridge_traceback_head` reports `loops_compiled=2` with a single `[jit] compiled loop at key=` line, which reads as a double count. It is not. Five sites bump the counter, one per `Backend::compile_loop` caller, and only `compile_loop_body` logs that line. The second increment on that fixture is this site, reached only from the `bridge.is_none()` arm of `compile_finish_from_active_session`, and upstream counts a root FINISH trace with no LABEL as a loop as well: `ResumeFromInterpDescr.compile_and_attach` (compile.py:1006-1017) mints its own JitCellToken and calls `send_loop_to_backend(.., "entry bridge", ..)`. The backend-side `total_compiled_loops`, bumped at the `model.py:297` point, also reads 2 there -- two CompiledLoopTokens, two green keys. Comment only; no behaviour change. 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.
31 commits on top of
origin/main. Two behaviour changes, the rest is diagnostics, spec tests, and comment re-anchoring.Behaviour
warmstate: one representation forJC_DONT_TRACE_HERE. pyre kept the denial in both a flags bit and aBaseJitCellStatevariant, and the two disagreed:disable_noninlinable_functionset the state only whenJC_TRACINGwas clear, so a cell denied on its way into a trace carried the flag but vanished fromget_stats. Every real decision already read the flag (can_inline_callable,counter_tick_checked,should_start_dont_trace_here_trace,should_remove_jitcell), andis_compiled/is_tracingread the token andJC_TRACING, so the variant reached only the census. The variant is deleted; the census counts the flag independently of the lifecycle state. This is also warmstate.py's shape — the flag is orthogonal to the lifecycle, andwarmstate.py:485-496retraces a denied cell once its procedure token dies. The abort paths now leaveNotHotand set the flag alone, which collapsesabort_tracing's three branches into the single condition its typed twin already used.fbw: the hazardous-inline deny reaches the warm state.fbw_abort_nested_unjournaled_residualnames the callee an abort is attributable to, but the deny wrote only a walker-local thread-local, so the callee's JitCell never carriedJC_DONT_TRACE_HERE. It now also callsdisable_noninlinable_functionon the function-entry keyinline_call.rsalready uses for that callee — the answerpyjitpl.py:2818-2828gives for the calleefind_biggest_functionnames.majit: lower the int/float list capacity read asarraylen_gc, and publish the preview short-preamble export as onePreviewShortStateinstead of a five-part side channel.descr:PyCode.co_firstlinenois immutable, per_immutable_fields_.Diagnostics
fbw_diagslots print from both readers, keyed by a declaredLABELStable whose length isRING_BASEso rustc rejects an unnamed slot; the wasm runner's positional mirror is pinned by a test with a positive control. The wasm tally moved out of thePYRE_WASM_JIT_STATSblock, whichcheck.pynever sets — the wasm half had been invisible on every gated run.FBW_DECLINE_CENSUSis process-wide, as its own doc already claimed — it wasthread_local!while pyre exposes real OS threads, so the dump reported only the printing thread. Pinned by a test shown to fail on the unfixed storage first.fbw_inline_callee_hazardousreports which of its three clauses fired, so a corpus census can say how much each carries.Measured refutations, written into the code
Several of this branch's investigations ended by refuting their own premise. Each verdict is recorded where the next reader will hit it:
for-iterhazard clause must stay forward-looking. It looks over-broad — it denies any callee whose bytecode contains aFOR_ITERanywhere — and it can be narrowed to "a consume already ran in this frame" without the per-frame Python pcInlineFramelacks, by askingFBW_FORITER_INFLIGHT. The census then looks right: clause fires 8 → 3 over 441 synth + 83 parity fixtures, both witnesses still declining. The gate disagrees —foriter_exempt_shared_generatorproduces wrong output on all three backends, plus jitstats regressions on two more fixtures. Inlining the residual is what carries the walk to the consume, so a decline conditioned on the consume having happened is one step late: the witness still declined, at pc 533 instead of 261. Written into the clause's doc comment.SPEC_FOLD_ROWSnow names the third fold shape it structurally cannot hold — 73 is a floor, not a total.bhimpl_jit_merge_pointtreats a frame with anextblackholeinterpas the recursive portal level (blackhole.py:1066-1093), which is right upstream but wrong for a multi-frame image whose callee bears a loop.Comment re-anchoring
Seven commits replace pyre-side line-number references in comments with symbol names, across
majit-metainterp,majit-translate,optimizeopt,pyre-jit,pyre-jit-traceandpyre-interpreter. Upstreamrpython//pypy/line numbers are left alone — those are stable and expected. Line numbers into our own tree go stale on every edit, which is exactly how the stale comments above got there.Verification
pyre/check.pyafter a full LLBC re-extract on the rebased base: dynasm 442/442, cranelift 442/442, wasm 435/435.🤖 Generated with Claude Code
https://claude.ai/code/session_01TBNBmnBveAQFbqRtrvQVyQ