majit: extract reusable JIT infrastructure from CEL - #1125
Merged
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 128 files, which is 28 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 ignored due to path filters (1)
📒 Files selected for processing (128)
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 |
youknowone
added a commit
that referenced
this pull request
Aug 11, 2026
`cargo fmt --check` is red on PR #1125 (job 93244977935) with 22 hunks. Local rustfmt 1.9.0-stable reproduces the CI output exactly, so the hunks are real rather than a toolchain skew. These ten files are the ones no concurrent work is touching; `majit/examples/i64env/src/main.rs`, `optimizeopt/virtualize.rs`, `virtualizable.rs` and `pyjitpl.rs` are being edited elsewhere and are left to their owners. Two of the hunks are import reorders that put the uppercase item first (`counter::{DEFAULT_SIZE, JitCounter}`, `llbc_fingerprint::{FreshnessMode, ..}`); the rest are line breaks. Applied by hand rather than by `cargo fmt`. The change is whitespace-only: over the whole diff the added and removed lines carry the identical token multiset. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 11, 2026
…orts The two `probe(...)` calls in `tests` take rustfmt's wrapping. `loop_header_at_zero` and `loop_header_at_three` take `#[rustfmt::skip]` instead: their `vec![]` is one bytecode instruction per source line with the operands beside the opcode, and rustfmt puts every element on its own line, which detaches each `// r1 = 1` from the row it annotates. `cargo fmt --all -- --check` reports no diff on the workspace after this. Assisted-by: Claude
youknowone
marked this pull request as ready for review
August 12, 2026 04:53
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 578f657). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![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: a66bcc6036
ℹ️ 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".
Comment on lines
+13408
to
+13409
| if is_builtin_shape { | ||
| return is_safe.then_some(FnPtrFamily::BuiltinWrapper); |
There was a problem hiding this comment.
When PYRE_FNPTR_INDIRECT=1 and an unsafe fn happens to have the BuiltinCodeFn signature, this early return produces None instead of Some(FnPtrFamily::Unknown). The dynamic-call lowering consequently falls back to the unregistered __dyn_call path rather than emitting the conservative IndirectCall { graphs: None }, losing the residual-call continuation and top effects that the new function-pointer path is intended to preserve; only selecting BuiltinWrapper should depend on is_safe.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d372155c27
ℹ️ 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".
| # without one, `cargo test -p majit` does not build. `cranelift` because this | ||
| # crate already carries `majit-backend-cranelift` as a dependency, so it adds | ||
| # nothing to the graph. Both CI legs pass `--no-default-features`. | ||
| default = ["cranelift"] |
There was a problem hiding this comment.
When the documented/required cargo test --workspace --features dynasm command is used, Cargo keeps default features enabled (cargo build --help describes --features as additive and provides the separate --no-default-features switch), so this default enables both backends. majit-metainterp/src/pyjitpl.rs:27-34 gives Cranelift precedence whenever both are present, meaning the supposed Dynasm validation silently runs against Cranelift and cargo test -p majit --features dynasm likewise selects the wrong backend. Make the backend selections mutually usable without relying on an undocumented --no-default-features, or update every required invocation accordingly.
AGENTS.md reference: AGENTS.md:L236-L238
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1971600200
ℹ️ 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".
Comment on lines
+712
to
+713
| let _ = REF_RETAIN.set(retain); | ||
| let _ = REF_RELEASE.set(release); |
There was a problem hiding this comment.
When two frontends concurrently call set_ref_resolver with different hook pairs, one thread can win REF_RETAIN.set while the other wins REF_RELEASE.set, violating the documented first-registration-wins contract. RetainedGreens would then retain objects through one ownership system and release them through another, potentially causing premature frees or memory corruption; store the pair in a single OnceLock<(RefRetainFn, RefReleaseFn)>.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0aef514ecb
ℹ️ 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".
| .map(|(i, expr)| { | ||
| let tag = green_type_tags.get(i).copied().flatten(); | ||
| emit_green_repr(expr, tag) | ||
| emit_green_repr(subst_target_for_pc(expr, pc, target), tag) |
There was a problem hiding this comment.
When can_enter_jit! receives a nontrivial target expression and the declared greens include the pc, this substitution evaluates the target once for the prepended key slot and again for the pc slot; the generated back_edge_structured call then evaluates it a third time as target_pc. An effectful or nondeterministic expression can therefore repeat side effects and produce a structured key whose two target-derived values disagree with the actual jump target. Bind the target expression to one temporary before building the key and reuse it for every slot and the call.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea62dc51a6
ℹ️ 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".
Comment on lines
+711
to
+713
| pub fn set_ref_resolver(retain: RefRetainFn, release: RefReleaseFn) { | ||
| let _ = REF_RETAIN.set(retain); | ||
| let _ = REF_RELEASE.set(release); |
There was a problem hiding this comment.
When two MAJIT frontends with different reference ownership systems run sequentially in one process, the second set_ref_resolver call is silently ignored by these process-global OnceLocks, so its RetainedGreens invoke the first frontend's retain/release functions on incompatible pointer values. This can corrupt ownership or memory even without concurrent registration; keep the hook pair on the frontend/JIT-driver owner and retain that pair with each stored key.
AGENTS.md reference: AGENTS.md:L189-L195
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b29f662f3
ℹ️ 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".
| green_key_raw, | ||
| green_key_values, | ||
| driver_descriptor, | ||
| green_key_values.map(|make_key| make_key()), |
There was a problem hiding this comment.
When a structured #[jit_interp] state uses the default JitState::code_ptr() == 0 (as the current macro examples do), with_typed_decision_key selects the hash-only warmstate path and creates a BaseJitCell with no comparekey. The declared key is only materialized here after StartTracing and placed in TraceCtx, so set_comparekey and the new RetainedGreens logic never run; even with retain hooks registered, a Ref green such as the program can be freed and have its address reused while the old compiled cell persists, making a different program enter the old loop. Feed the lazily built declared key into the warmstate installation path rather than using it only for tracing.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
Comment on lines
+1922
to
+1924
| let syn::ReturnType::Type(_, ty) = output else { | ||
| return None; | ||
| }; |
There was a problem hiding this comment.
For a #[jit_interp] portal with the ordinary unit return type, output is ReturnType::Default, so this returns None and the expansion emits no FINISH-latch drain. If compiled execution reaches a void FINISH, back_edge_internal records Some(Vec::new()) but also returns the back-edge PC; the generated portal then resumes there and can rerun completed iterations or duplicate side effects instead of returning. Unit needs its own drain that checks take_back_edge_finish() and emits return;.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
Separate reusable translator, metainterpreter, backend, bridge/resume, and virtualizable work from the CEL-specific branch while preserving interpreter semantics and backend coverage. Assisted-by: Codex
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 578f65720f
ℹ️ 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".
Comment on lines
+201
to
+202
| pub can_collect: bool, | ||
| pub can_invalidate: bool, |
There was a problem hiding this comment.
When a consumer uses the new Declared override with the normal CallControl translation path, rewrite_op_call extracts only d.extra_info.extraeffect and passes that to getcalldescr, which recomputes can_collect and can_invalidate from graph analysis instead of using these declared values. For a graphless external declared with can_collect: true, analysis can therefore emit a non-collecting residual call, omitting the GC handling that the declaration explicitly requires; can_invalidate: true is lost similarly. Pass the whole declared EffectInfo row through descriptor construction rather than reducing it to one enum.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
youknowone
added a commit
that referenced
this pull request
Aug 15, 2026
* majit-metainterp: add a hook that fires when a call enters compiled code
`JitHooks` carried six hooks and none of them fired at entry, so a
consumer asking whether a call ran compiled code had only proxies.
`on_compiled_entry` fires in `back_edge_internal` after every decline has
returned and immediately before
`run_compiled_detailed_with_values{,_at_dispatch_key}` — the point
`portal_rca_enabled()` already reports, but unconditional and counted.
The sibling entry sites in this file serve other front ends and do not
fire it; the doc comment says so.
Assisted-by: Claude
* majit: stop allocating per compiled-code entry
warmstate.py:387-398 execute_assembler enters compiled code by writing
the args into the jitframe and jumping; nothing per-entry is allocated
beyond the jitframe itself (llmodel.py:298). majit's entry path
(back_edge_internal -> run_compiled_detailed_with_values_at_dispatch_key)
allocated 19-20 times per call: Vec-returning JitState extraction,
extend_compiled_live_values growth, virtualizable flattening buffers, a
vable name clone, fail_arg_types/exit_types clones, and a defensive
inputs.to_vec in the cranelift executor.
- JitDriver gains EntryScratch (live values, raw, types, vable statics
and per-array buffers), taken by value around entry assembly and
returned at every exit; a missed return degrades to a fresh
allocation, never corruption.
- JitState gains *_into forms (extract_live_values_into,
extract_live_into, live_value_types_into,
export_virtualizable_boxes_into) whose defaults delegate to the
existing Vec-returning forms, so existing impls are untouched; the
jit_interp macro overrides the _into forms under its existing
emission condition.
- extend_compiled_live_values_into appends into the caller's buffer and
borrows the vable name; flatten_virtualizable_values_into likewise.
- The is_finish arm no longer builds the guard-failure exit description
(warmstate.py:406-419 fast path); exit_types is borrowed from the
descr; gc_ref/force_token slots move instead of clone.
- cranelift execute_with_inputs_at_dispatch_key takes Cow<[i64]>,
matching the cur_fail_descrs Cow beside it; only the external-JUMP
re-entry brings an owned vector.
Steady one-row entry drops 19/19/20 -> 9/9/9 allocations
(arith/policy/float, cel allocs_per_eval jit-steady rows; n=1000 rows
move identically). Kept with reasons: the jitframe and deadframe
(upstream allocates the frame; DeadFrame restructuring is separate),
CompileResult's values/typed_values (consumed by value on finish), the
16-byte compiled-meta clone (Arc-ifying CompiledEntry.meta is the
orthodox next step but touches ~15 sites).
Tracing and warmup paths keep their allocating shapes.
Assisted-by: Claude
* jit: promote the vable array index only on the standard branch; guard the array tid declarations
_opimpl_getarrayitem_vable (pyjitpl.py:1218-1230) decides
_nonstandard_virtualizable first; the non-standard branch reads through
getfield_gc_r + getarrayitem_gc_* with the index box untouched, and the
promote sits on the first line of _get_arrayitem_vable_index, reached
only by the standard branch. _opimpl_setarrayitem_vable is the same
shape. The walker handlers in vable_ops.rs hoisted
walker_promote_vable_array_index above that decision, so a non-standard
access with a non-constant index minted a GUARD_VALUE upstream does
not, over-specializing the trace on ordinary heap reads.
The hoist itself stays: the walker owns the MIFrameStack, so promoting
at the call site gives the guard a full-framestack snapshot, and
implement_guard_value's replace_box is the register-bank rewrite only
the walker can do. What moves is the decision: TraceCtx gains a public
nonstandard_virtualizable wrapper, and the four
vable_get/setarrayitem_*_indexed entry points forward to _checked legs
taking the decision as a parameter (existing callers unchanged). The
walker takes the decision, captures its guard window, and promotes
only on the standard leg. Regression test
a_nonstandard_vable_array_access_does_not_promote_the_index asserts no
GUARD_VALUE names the index on the non-standard branch and, as a
positive control, that the Step 4 PTR_EQ promote was minted; it fails
with the promote restored.
rlist.rs's two array tid cells stay process-global: the collector and
the typed-nursery hook are process-global singletons by design, and
upstream's instance owner (GcLLDescr init_array_descr writing the tid
into the ArrayDescr, gc.py:544-549) has no pyre counterpart at the
call site, which takes a bare tid. Both setters now go through
declare_array_gc_type_id, which debug-asserts a re-declaration carries
the same id, so a second host's differing slot fails at declaration
instead of surfacing as an unattributable mis-trace; the doc states the
single-host ownership and cites the upstream owner.
The six BC_*ARRAYITEM_VABLE_* arms in majit's own dispatch carry the
same pre-#1125 unconditional hoist; left for a follow-up now that the
_checked legs exist.
Assisted-by: Claude
* majit: count a tmp-only cell instead of entering it, and resolve entry decisions on the key's own cell
back_edge_internal tested has_compiled_loop and then unwrapped
get_compiled_meta; a compile_tmp_callback token has a body but no
frontend meta, so a driver that can mint one panicked at its own loop
header. The dispatch now binds the meta, so the fall-through to
maybe_start_tracing - upstream's 'attached by compile_tmp_callback().
count normally' arm (warmstate.py:471-478) - is structural.
has_compiled_loop stays as the first conjunct because invalidate_loop
leaves the meta in place, and meta-presence alone would enter an
invalidated loop. back_edge_or_run_compiled_internal and
can_enter_jit_keyed made the same decision more quietly (an absent meta
returned Abort, so the counter never ticked); both now use
has_runnable_compiled_loop. Tests mint a tmp-only cell via
attach_tmp_callback_to_interp and assert the back edge counts on.
Typed-key queries: get_procedure_token_for_key walks the collision
chain with the key comparator (warmstate.py:458-464 walks cell.next
testing comparekey before reading a token); has_compiled_loop_for_key /
has_runnable_compiled_loop_for_key on top. The entry branch of
back_edge_structured goes through entry_cell_has_compiled_code, which
takes the typed walk only when the bucket is actually chained - an
unchained bucket's head IS what the walk would find, and building the
GreenKey eagerly would add two allocations to every warm entry. The
u64 forms remain, documented as bucket-head reads. compiled_loops
itself is still a HashMap<u64,_> with no chain; documented as the
remaining collision surface.
CompiledEntry.meta, CompileResult.meta, RawCompileResult.meta and
RunResult's variants hold Arc<M>; get_compiled_meta returns
Option<&Arc<M>>. get_procedure_token hands back the cell's own object,
never a copy. The old clone was a 16-byte stack copy, so the cel
allocation meter reads 9/9/9 unchanged - this is a sharing-semantics
fix, not an allocation win.
The six BC_*ARRAYITEM_VABLE_* dispatch arms take the
nonstandard_virtualizable decision first and promote the index only on
the standard leg via the _checked legs, as the walker already does
(pyjitpl.py:1205/:1229/:1244). No metainterp-side fixture exists to pin
these six against a re-hoist; follow-up.
Assisted-by: Claude
* majit: stop writing FINISH values back into the interpreter state
back_edge_internal's FINISH arm called state.restore_values with the
FINISH descr's values, which are the portal's return value — one word
for an int portal — not the loop-carried state. A state with more than
one live field indexed past the end of that list and panicked. The
warmstate.py:405-419 execute_assembler fast path reads only the result
off the deadframe and touches no interpreter state on the way out. The
back_edge_finish latch is still set for callers that consume it.
The panic is only reachable once a driver outlives its calls with warm
bridges, which no per-call-driver fixture exercises; the fixture landing
next covers it.
Assisted-by: Claude
* majit: make the dynasm deadframe the jitframe itself
execute_token copied every jitframe slot into a Vec<i64>, staged
jf_guard_exc aside, and freed the frame chain before returning. Per
llmodel.py:240-250 and :323 the deadframe IS the jitframe: FrameData now
holds the chain (owning) or borrows the still-executing run's frame in
force (llmodel.py:280-284), decodes slots lazily via the descr's rd_locs
(_decode_pos, llmodel.py:422-424), reads jf_guard_exc off the frame
(llmodel.py:240-242), and frees the jf_forward chain on drop
(jitframe.py:139-145).
The frame is off the JF shadow stack for the whole window the frontend
reads it in, and majit has no conservative stack scan, so majit-gc gains
ActiveGcDeadFrameHooks — a fn-pointer table on the ActiveGcGuardHooks
pattern through which the backend publishes its live deadframes; the
collector walks them as a root source in the minor-collection root phase
and in the debug root enumeration. Documented in-code as a structural
adaptation.
Removes one of the measured per-entry allocations on dynasm (the
raw_values Vec); cranelift is untouched.
Assisted-by: Claude
* majit: meter heap allocations per compiled-code entry
New harness = false test with a counting global allocator: one driver
outlives 512 warmup + 256 measured calls, the window opens in the
existing set_on_compiled_entry hook, and the per-entry count is pinned
exactly — 10 on dynasm, 12 on cranelift. A short separate window
captures a backtrace per allocation and prints a per-site attribution
table, so the pin names which sites the count is made of.
The fixture takes &mut JitDriver as a parameter; a fixture that
constructs its driver inside the annotated function measures one call
per driver and cannot see this regime.
Assisted-by: Claude
* majit: cover the vable array promote order with a metainterp fixture
pyjitpl.py:1205/:1218-1230/:1236-1247 decide the nonstandard-
virtualizable branch before any promote, so a nonstandard vable's array
access must not mint a GUARD_VALUE on the index. The dispatch arms were
fixed earlier with no test because no metainterp MIFrame fixture
existed; this builds one — VirtualizableInfo, TraceCtx with
init_virtualizable_boxes, a JitCodeBuilder-emitted jitcode, and
JitCodeMachine::run_one_step — and asserts no guard names the index box
for BC_GETARRAYITEM_VABLE_I / BC_SETARRAYITEM_VABLE_I, with guards > 0
arming the assertion. Inverting the decision order in the six arms makes
it fail naming the index box; the inversion and result are recorded in
the doc comment.
Assisted-by: Claude
* majit: stop copying compiled-entry inputs and outputs on cranelift
execute_token_with_dispatch_key unwrapped the metainterp's &[Value] into
an owned Vec<i64> before the frame existed; llmodel.py:306-315 unwraps
each argument at the store into its frame slot and holds no second list.
A FrameInputs input source (Ints/OwnedInts/Values) now writes arguments
against the frame directly. JitExecResult::extract_outputs copied every
exit slot on every exit; its only consumers are the CALL_ASSEMBLER
sentinel (slot 0) and external-JUMP re-entry, so the copy is taken only
in those branches (llmodel.py:240-250 reads slots off the frame through
accessors). execute_token_ints_raw keeps the eager copy — returning the
raw slots is its contract. maybe_take_call_assembler_deadframe takes
&JitExecResult to read slot 0 without materialising the rest.
Cranelift installs no deadframe walker: its frames are GC objects whose
jf_gcref slot register_roots already roots for the deadframe window, and
a registered slot is rewritten when the frame moves where a walker's
address is not. Documented at register_active_hooks, including why the
single-slot hook must stay untouched for a co-compiled dynasm.
The per-entry allocation meter reads cranelift 12 -> 10; dynasm is
unchanged at 10, so the pinned constant collapses to one number.
Assisted-by: Claude
* majit: pin the entry deciding on one cell and executing off another
A deliberately-collided two-cell fixture (get_uhash folds
x = (x ^ hash_whatever(tp, v)) * MULT per warmstate.py:584-593, so the
collision is constructed with one xor, not searched): both cells carry
their own compiled token, entry_cell_has_compiled_code decides through
the comparekey-walked chained cell, and the runner reads the bucket
head's token — decided token #2, executed token #1.
warmstate.py:568-593 reaches a token only through the cell already
matched on greens + comparekey + get_uhash, and :483/:511 carries the
resolved procedure_token to the executor rather than re-looking-up by
key, so upstream has no second reader that could disagree.
The pin is #[ignore]d as an open defect: the runner also reads
compiled_loops, an IndexMap<u64, CompiledEntry<M>> with one slot per
hash and no chain, so two colliding keys cannot both file a meta there
and threading the token alone would split one artifact across two index
bases. Re-enabling the test is the acceptance criterion for moving the
compiled meta onto the cell.
Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 16, 2026
… the array tid declarations _opimpl_getarrayitem_vable (pyjitpl.py:1218-1230) decides _nonstandard_virtualizable first; the non-standard branch reads through getfield_gc_r + getarrayitem_gc_* with the index box untouched, and the promote sits on the first line of _get_arrayitem_vable_index, reached only by the standard branch. _opimpl_setarrayitem_vable is the same shape. The walker handlers in vable_ops.rs hoisted walker_promote_vable_array_index above that decision, so a non-standard access with a non-constant index minted a GUARD_VALUE upstream does not, over-specializing the trace on ordinary heap reads. The hoist itself stays: the walker owns the MIFrameStack, so promoting at the call site gives the guard a full-framestack snapshot, and implement_guard_value's replace_box is the register-bank rewrite only the walker can do. What moves is the decision: TraceCtx gains a public nonstandard_virtualizable wrapper, and the four vable_get/setarrayitem_*_indexed entry points forward to _checked legs taking the decision as a parameter (existing callers unchanged). The walker takes the decision, captures its guard window, and promotes only on the standard leg. Regression test a_nonstandard_vable_array_access_does_not_promote_the_index asserts no GUARD_VALUE names the index on the non-standard branch and, as a positive control, that the Step 4 PTR_EQ promote was minted; it fails with the promote restored. rlist.rs's two array tid cells stay process-global: the collector and the typed-nursery hook are process-global singletons by design, and upstream's instance owner (GcLLDescr init_array_descr writing the tid into the ArrayDescr, gc.py:544-549) has no pyre counterpart at the call site, which takes a bare tid. Both setters now go through declare_array_gc_type_id, which debug-asserts a re-declaration carries the same id, so a second host's differing slot fails at declaration instead of surfacing as an unattributable mis-trace; the doc states the single-host ownership and cites the upstream owner. The six BC_*ARRAYITEM_VABLE_* arms in majit's own dispatch carry the same pre-#1125 unconditional hoist; left for a follow-up now that the _checked legs exist. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 19, 2026
… the array tid declarations _opimpl_getarrayitem_vable (pyjitpl.py:1218-1230) decides _nonstandard_virtualizable first; the non-standard branch reads through getfield_gc_r + getarrayitem_gc_* with the index box untouched, and the promote sits on the first line of _get_arrayitem_vable_index, reached only by the standard branch. _opimpl_setarrayitem_vable is the same shape. The walker handlers in vable_ops.rs hoisted walker_promote_vable_array_index above that decision, so a non-standard access with a non-constant index minted a GUARD_VALUE upstream does not, over-specializing the trace on ordinary heap reads. The hoist itself stays: the walker owns the MIFrameStack, so promoting at the call site gives the guard a full-framestack snapshot, and implement_guard_value's replace_box is the register-bank rewrite only the walker can do. What moves is the decision: TraceCtx gains a public nonstandard_virtualizable wrapper, and the four vable_get/setarrayitem_*_indexed entry points forward to _checked legs taking the decision as a parameter (existing callers unchanged). The walker takes the decision, captures its guard window, and promotes only on the standard leg. Regression test a_nonstandard_vable_array_access_does_not_promote_the_index asserts no GUARD_VALUE names the index on the non-standard branch and, as a positive control, that the Step 4 PTR_EQ promote was minted; it fails with the promote restored. rlist.rs's two array tid cells stay process-global: the collector and the typed-nursery hook are process-global singletons by design, and upstream's instance owner (GcLLDescr init_array_descr writing the tid into the ArrayDescr, gc.py:544-549) has no pyre counterpart at the call site, which takes a bare tid. Both setters now go through declare_array_gc_type_id, which debug-asserts a re-declaration carries the same id, so a second host's differing slot fails at declaration instead of surfacing as an unattributable mis-trace; the doc states the single-host ownership and cites the upstream owner. The six BC_*ARRAYITEM_VABLE_* arms in majit's own dispatch carry the same pre-#1125 unconditional hoist; left for a follow-up now that the _checked legs exist. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 19, 2026
) * majit-metainterp: forward set_new_via_gc from JitDriver to the backend `set_new_via_gc` has existed as an inherent method on all three backends since it was added for aheui's nursery-backed nodes, and a repo-wide search finds no caller: `MetaInterp::backend_mut` is private, so no consumer of `JitDriver` could reach it, while its siblings `set_gc_allocator` and `set_vtable_offset` are both forwarded. Only the dynasm backend acts on the flag; the cranelift and wasm backends take it as a no-op. Assisted-by: Claude * majit-ir: stop SimpleInteriorFieldDescr's Debug from following its array back-reference SimpleArrayDescr.all_interiorfielddescrs and SimpleInteriorFieldDescr.array_descr hold each other strongly (the "CYCLE, accepted" row of this module's Arc cycle audit). Both ends derived Debug, so formatting either one recursed SimpleArrayDescr::fmt -> OnceLock -> Vec<DescrRef> -> SimpleInteriorFieldDescr::fmt -> SimpleArrayDescr::fmt until the stack ran out, for any {:?} reaching a struct-array descr. Replace the derive on SimpleInteriorFieldDescr with a hand-written Debug that renders array_descr as a cache_key/type_id summary. descr.py:420-421 InteriorFieldDescr.repr_of_descr prints only self.fielddescr.repr_of_descr() and never follows self.arraydescr. Add descr::tests::debug_of_a_struct_array_descr_terminates, which wires the cycle and formats both ends; with the traversing edge restored it aborts with a stack overflow. Assisted-by: Claude * majit: return a finish exit's result without building the guard-failure description warmstate.py:404-418 keeps a fast path ahead of the exception-shaped handling — "First, a fast path to avoid raising and immediately catching a DoneWithThisFrame exception" — that returns fail_descr.get_result(cpu, deadframe) and reads nothing else. The port gathered handle_fail's inputs unconditionally before the caller could see is_finish: the is_gc_ref_slot scan and its vector, force_token_slots().to_vec(), get_status(), descr_owning_jct(), get_savedata_ref() and grab_exc_value(), all discarded on every finish exit. execute_assembler_at_dispatch_key now splits on the descr the run ended on; the exit-slot decode is shared by both arms through decode_exit_slots, the finish arm returns immediately, and the layout builder keeps only its guard-only branch. The skipped reads are side-effect-free on a finish exit and jf_guard_exc is zero there — the frame is zeroed at allocation and the propagate-exception rewrite clears it explicitly. Assisted-by: Claude * majit-translate: derive the items-block element identities instead of naming pyre `regular_call_is_items_block_accessor` matched two whole `pyre_object::` paths while `graph_is_items_block_base_accessor` already matched the same two leaves by module-qualified suffix. The shared suffix is now `is_object_items_block_base_accessor`, called by both; brick 1 keeps the typed accessor as its own extra arm. `is_pyobjectref_items_ptr` compared the pointee class root against the literal `"PyObject"`. It is now `is_object_ref_items_ptr` and tests that `raw_ptr_pointee_class_root` answers at all, so the element type is read off the pointer rather than fixed. The inner `RawPtr` test is unchanged and is what still excludes a scalar `TypedItemsBlock` element. Both predicates accept the same set over pyre's closure: `fn items_block_items_base` and `fn items_block_items_ptr` are each defined exactly once, both in `pyre_object::object_array`, and their return type is `*mut PyObjectRef`. One test, beside the existing brick-1 gate test: every reference accessor brick 1 rewrites is one brick 3 recognises, the typed accessor is recognised only by brick 1, and neither gate is keyed on a crate. Assisted-by: Claude * majit-translate: compare the items-block accessor gates by path segment `is_object_items_block_base_accessor` and the typed arm of `graph_is_items_block_base_accessor` matched with `str::ends_with`, which on a `::`-joined path is a substring test rather than a path test: a module named `my_object_array` satisfies the `object_array::items_block_items_base` key. Both now compare through a new `path_ends_with_segments`, composed from the file's existing `path_eq_ignoring_raw` and `path_has_suffix_ignoring_raw`. Extends `the_two_bricks_agree_on_every_reference_items_base_accessor` with that shape, asserted negative, on both predicates, and records there that the test's pre-existing leaf-collision case is refused under `ends_with` as well and so did not cover the boundary. Assisted-by: Claude * majit-rlib: take the two GcArray type ids from the host instead of baking pyre's GC_INT_ARRAY_GC_TYPE_ID = 41 and GC_FLOAT_ARRAY_GC_TYPE_ID = 42 were the slots pyre's registration order happens to assign. A type id is a slot in one host's type registry and the collector indexes that registry with the word it reads back, so any other host of majit-rlib stamped a foreign index into its own items-block headers. Replace both with the setter/getter pair the crate already uses for the rbigint payload (set_rbigint_gc_type_id): an AtomicU32 plus set_gc_{int,float}_array_gc_type_id and a dont_look_inside reader. Undeclared is UNSET_GC_TYPE_ID = u32::MAX, not 0: TypeRegistry::register returns entries.len(), so 0 is a real slot. A const assert pins the sentinel outside 0..TypeRegistry::MAX_TYPES. Undeclared is refused rather than defaulted, in try_alloc_typed_items_block_nursery's GC branch — the point where the id enters a GC header. The std::alloc path this file documents for bare unit tests and the pre-init_gc_subsystem bootstrap keeps working, since no collector reads that word. pyre declares both from its own gc.register_type results in build_gc(), which runs before the allocator is installed. This drops the debug_assert_eq! that pinned 41/42; the registration order stays pinned by debug_assert_eq!(w_code_tid, W_CODE_GC_TYPE_ID = 43) on the next registration. Assisted-by: Claude * majit: route the class-word slot lookup through a SizeDescr accessor Seven consumers needed to know which field of a struct holds the class word, and each searched that struct's field list for a descr whose name is `w_class`. Add `SizeDescr::class_word_field()` and call it from all seven; the name test now appears in one default body, which a producer that declares the slot overrides. The default searches `gc_fielddescrs()` before `all_fielddescrs()`. A real layout lists the same `Arc` in both, so the order is unobservable there; it decides only for a descr that populates the two lists with different objects, and the GC list is what the byte-offset consumers searched before. `clear_gc_fields` matches the skipped slot by byte offset rather than by name. `w_class_store_is_covered_by_alloc` keeps its offset/field_size comparison, with a comment recording that a declared slot replaces the name test and nothing else. Assisted-by: Claude * majit: make the class-word header field a declared flag on the descr `FieldDescr::is_w_class()` tested the field's name, so every producer of a class-word descr had to spell the name a consumer would recognise. Default it to `false` and have each producer state the answer: `SimpleFieldDescr` stores `is_class_word`, seeded by `new_with_name`/`SimpleFieldDescrSpec` from the `"STRUCT.fieldname"` the codewriter supplies and settable directly through `with_class_word`; `PyreFieldDescr` stores it across its eleven construction sites, true only for `new_w_class_field_descr`. `false` is the default because RPython's field lists hold no header field at all (`heaptracker.py:66` drops `typeptr` before any descr exists), so a list entry means an ordinary value field unless its producer says otherwise. Add `FieldDescr::is_header_field()` — the class word or the typeptr — and use it at the three `virtualize.rs` sites that excluded both. Its doc records why the three remaining `is_w_class()` consumers cannot use it: each has already resolved a typeptr separately, so widening them would route a typeptr read into the class-word path. `name_is_class_word` is now the only reader of the name spelling, called by the two `SimpleFieldDescr` producers. Assisted-by: Claude * majit: answer the class word's slot position from all_fielddescrs only `class_word_field()` searches `gc_fielddescrs()` first, and `with_extra_gc_fielddescr` appends header edges that are absent from `all_fielddescrs()` by design — its doc states the invariant: "kept out of `all_fielddescrs` so the positional indexing above is unaffected". pyre seeds every object group's gc edges with the shared header descr, whose `index_in_parent` is 0. `OptVirtualize`'s class-word fold read `index_in_parent` off that accessor, so for a layout that declares no class word of its own it resolved slot 0 — the first value field — and forwarded a `Ref` onto an `Int`, tripping the `make_equal_to` Box.type invariant. Add `SizeDescr::class_word_index_in_parent()`, which consults `all_fielddescrs()` only, and read the fold's slot from it. Byte-offset consumers keep `class_word_field()`; the two lists differ, so one accessor cannot serve both. Assisted-by: Claude * majit-ir: reattach class_word_field's doc and note that it answers the first declaring entry `class_word_index_in_parent` was inserted between `class_word_field`'s doc comment and `class_word_field` itself, so the new accessor absorbed the whole block and the old one was left undocumented. The absorbed tail states "It searches `gc_fielddescrs()` before `all_fielddescrs()`", which is the opposite of what `class_word_index_in_parent` does. Move the block back onto `class_word_field` and add that the lookup answers with the first entry declaring `is_w_class()`, naming pyre's `Method` — which declares a payload field `w_class` beside the inherited header — as a layout that publishes two. Assisted-by: Claude * pyre-jit-trace: assert every object group's class word is the inherited PyObject header Neither accessor had a test that reads a non-`None` answer: the only assertion naming `class_word_index_in_parent` expects `None`, so a body returning a constant `None` passed the suite while silently disabling OptVirtualize's class-word fold rather than correcting it. Walk the 19 published size descrs and check both accessors resolve the field at `W_CLASS_OFFSET`, and that at least one group answers positionally. Violations are accumulated instead of asserted in place, so the failure names every offending group rather than whichever comes first. Two violations, both `Method`: it declares a payload field named `w_class` at `METHOD_W_CLASS_OFFSET` — documented at `method_w_class_descr` as distinct from the inherited header — which the group factory qualifies to `Method.w_class`, `name_is_class_word` accepts on the `.w_class` suffix, and which precedes the header row in both lists. `genop_new_with_vtable` reads the offset from that accessor. Pre-existing: the inline `all_fielddescrs().find(..)` that `class_word_index_in_parent` replaced selected the same field. The two are listed and compared by equality, so fixing `Method` fails the assertion and forces the list to be deleted. Assisted-by: Claude * majit-ir: let a layout declare its class word instead of inferring it from a field name `name_is_class_word` accepted `"w_class"` or any `".w_class"` suffix, and `build_object_descr_group_with_extra_gc_edges` qualifies every field name to `"{simple_name}.{field_key}"`. pyre's `Method` carries a payload field named `w_class` — the class the bound function was found on, which `method_w_class_descr` documents as distinct from the inherited header — so it qualified to `"Method.w_class"`, matched the suffix, and sat ahead of the header row. Both `class_word_field()` and `class_word_index_in_parent()` take the first entry declaring `is_w_class()`, so both answered with the payload field at `METHOD_W_CLASS_OFFSET` rather than the header at `W_CLASS_OFFSET`. Header rows are spelled inconsistently across groups (`PyObject.w_class`, `PyTraceback.w_class`, `W_BaseException.w_class`), which is the same shape as `Method.w_class`, so no name rule separates them. Add `SimpleFieldDescrSpec::is_class_word` and `GcCache::get_field_descr_declaring`, taking `declared_class_word: Option<bool>`; `get_field_descr` delegates with `None` so existing callers keep the inference. Rename `name_is_class_word` to `class_word_inferred_from_name` and document it as a fallback retained for the serialized-`BhDescr` path, which rebuilds a descr from a name with no layout in reach — `BhFieldSpec` carries no flag, so a declaration does not survive a blackhole round trip. pyre's group factory declares `is_class_word: offset == W_CLASS_OFFSET`, a layout invariant rather than a name rule. `EC_DESCR_GROUP` declares `false`: `ExecutionContext` is not a `PyObject` layout and its `EC_*` offsets share no origin with `W_CLASS_OFFSET`. `every_groups_class_word_is_the_inherited_pyobject_header_slot` now reports zero violations over all 19 published groups, so its two `Method` exceptions are removed. majit-ir 327/0, majit-metainterp 1500/0, majit-gc 254/0, pyre-jit-trace class-word tests 0 failed, `cargo fmt --all -- --check` rc=0. Assisted-by: Claude * jit: promote the vable array index only on the standard branch; guard the array tid declarations _opimpl_getarrayitem_vable (pyjitpl.py:1218-1230) decides _nonstandard_virtualizable first; the non-standard branch reads through getfield_gc_r + getarrayitem_gc_* with the index box untouched, and the promote sits on the first line of _get_arrayitem_vable_index, reached only by the standard branch. _opimpl_setarrayitem_vable is the same shape. The walker handlers in vable_ops.rs hoisted walker_promote_vable_array_index above that decision, so a non-standard access with a non-constant index minted a GUARD_VALUE upstream does not, over-specializing the trace on ordinary heap reads. The hoist itself stays: the walker owns the MIFrameStack, so promoting at the call site gives the guard a full-framestack snapshot, and implement_guard_value's replace_box is the register-bank rewrite only the walker can do. What moves is the decision: TraceCtx gains a public nonstandard_virtualizable wrapper, and the four vable_get/setarrayitem_*_indexed entry points forward to _checked legs taking the decision as a parameter (existing callers unchanged). The walker takes the decision, captures its guard window, and promotes only on the standard leg. Regression test a_nonstandard_vable_array_access_does_not_promote_the_index asserts no GUARD_VALUE names the index on the non-standard branch and, as a positive control, that the Step 4 PTR_EQ promote was minted; it fails with the promote restored. rlist.rs's two array tid cells stay process-global: the collector and the typed-nursery hook are process-global singletons by design, and upstream's instance owner (GcLLDescr init_array_descr writing the tid into the ArrayDescr, gc.py:544-549) has no pyre counterpart at the call site, which takes a bare tid. Both setters now go through declare_array_gc_type_id, which debug-asserts a re-declaration carries the same id, so a second host's differing slot fails at declaration instead of surfacing as an unattributable mis-trace; the doc states the single-host ownership and cites the upstream owner. The six BC_*ARRAYITEM_VABLE_* arms in majit's own dispatch carry the same pre-#1125 unconditional hoist; left for a follow-up now that the _checked legs exist. Assisted-by: Claude * jit: seed is_class_word in the PyCode descr group The rebase brought in PYCODE_DESCR_GROUP, whose SimpleFieldDescrSpec closure predates the producer-declared is_class_word field. The group lists only the four read-only payload fields, never the inherited PyObject header row, so no field in this list is the class word. Assisted-by: Claude * majit-backend-cranelift: dump the emitted code text under MAJIT_DUMP_CLIF Two dumps that agree on the IR can still disagree on the code: register allocation runs over the whole function, so a change confined to one block moves spills in the blocks that survive it, and only the emitted form shows that. Alongside the existing CLIF dump, print the body code size and the vcode disassembly for the same trace. Assisted-by: Claude * majit: report stale upstream citations, fix the ones it flags, and split the CI test builds `scripts/check-citation-drift.py` reads `file.py:line symbol` citations into the vendored RPython/PyPy sources and reports the ones whose line now belongs to a different definition. It partitions checkable from unmeasured citations so a zero cannot be read as complete coverage, and it exits nonzero only when its own population invariant fails. It is run by hand; nothing wires it into a workflow. The descr, embedded, info, warmstate and mir changes are citations it flagged. The workflow change is unrelated to the above: `cargo test` is split into a `--no-run` build step and a run step for each backend, so a compilation failure is reported separately from a test failure. Assisted-by: Claude * jit: promote vable array indices in the owning walker * docs: keep majit invariants anchored to symbols * pyre-jit-trace: name the class-word inference helper by its current symbol The assertion's doc cites `name_is_class_word`. That function was renamed to `class_word_inferred_from_name` by "majit-ir: let a layout declare its class word instead of inferring it from a field name", which is a later commit on this branch, and the cross-crate citation was not carried along. The described behaviour is unchanged: the helper is still `name == "w_class" || name.ends_with(".w_class")`. Comment only; no code 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.
Summary
This extracts reusable MAJIT infrastructure from the CEL branch so it can land and be tested independently of CEL-specific behavior.
Known boundary
Dynamic libffi-call specialization is not part of this extraction. The generic residual-call path preserves semantics until the translator has a dynamic call-descriptor and CIF parser suitable for specialization.
Self-review
Assisted-by: Codex.Validation
python3 scripts/extract-llbc.py majit-rlib pyre-object pyre-interpreter pyre-jitcargo check --workspace --features dynasmcargo test --workspace --features dynasmpython3 pyre/check.py --no-synthetic --no-cpython-suite