Introduce CompileData parity markers and align with RPython - #52
Conversation
Add Rust structs mirroring RPython compile.py's CompileData hierarchy: CompileData, PreambleCompileData, SimpleCompileData, BridgeCompileData, and UnrolledLoopData. The structs are intentionally small input bundles so the sensitive compile path gains named parity boundaries without a broad control-flow rewrite. Thread the new bundles through the existing pyjitpl compile paths for preamble/unrolled loops, simple loops, bridge compilation, retrace, and finish compilation. Existing backend APIs and optimizer calls remain structurally unchanged; inputs such as trace ops, snapshots, inputargs, call_pure_results, and inline_short_preamble now flow through the corresponding parity marker where practical. Verification: cargo fmt; cargo check --features dynasm; cargo test --features dynasm; python3 pyre/check.py --backend dynasm (14/14 benchmarks passed).
Keep the new CompileData structs honest about what they model: they are Rust input bundles for the flattened pyjitpl optimizer flow, not a separate reimplementation of RPython's optimize_trace dispatch. Document that adaptation explicitly. Thread the RPython constructor payloads through the Rust bundles instead of using placeholder state. The compile data now carries enable_opts, UnrolledLoopData requires the real JitCellToken and ExportedState, compile_retrace uses the tracing session's call_pure_results and existing loop token, and compile_bridge receives the bridge trace's call_pure_results, runtime boxes, and resume storage. This removes the bogus None/empty HashMap/empty runtime-box cases found in the static parity review while preserving the existing flattened optimizer entry points.
BridgeCompileData.runtime_boxes should mirror RPython compile.py:1056-1060: the runtime boxes passed into optimize_bridge are the live boxes from the bridge-closing JUMP, before trace.get_iter() allocates fresh input boxes for optimization. The previous Rust plumbing built runtime_boxes from prepared.inputargs. Those are the fresh TraceIterator input boxes, not the original live_arg_boxes that RPython threads through BridgeCompileData and unroll.py:183-184. That was currently dormant because optimize_bridge still derives its active runtime boxes from the pre-opt JUMP args, but it made the new CompileData bundle structurally misleading and unsafe for future readers. Read the closing JUMP args before shadowing bridge_ops with the prepared trace, and pass that vector into BridgeCompileData instead. If the path has no terminal JUMP, keep the existing empty fallback. Verification: cargo fmt --check; cargo check --features dynasm; cargo test --features dynasm; python3 pyre/check.py --backend dynasm (14/14 passed).
Keep CompileData itself limited to the shared trace payload, matching RPython's base class, and store call_pure_results on each concrete CompileData variant instead. Construct BridgeCompileData from the original bridge trace and runtime boxes before the Rust TraceIterator preparation step, so the wrapper mirrors compile.py while the explicit preparation remains the unroll.py trace.get_iter adaptation. Look up the retrace loop token before taking call_pure_results or closing/consuming the active trace, matching compile.py's get_procedure_token/assert ordering and avoiding trace mutation on a missing token.
RPython compile_retrace resolves the existing loop_jitcell_token before it records the closing JUMP for the retrace. Pyre had moved that lookup inside the block that had already taken self.tracing, so a rare missing-token path returned false after draining the active trace context. Move the token lookup to the start of compile_retrace, before partial_trace, exported_state, retracing_from, or tracing are consumed. If the token is absent, the retrace state remains intact instead of turning a pre-JUMP failure into a destructive cancellation. Keep the CompileData payload structs in place. They still document the RPython compile.py data-shape adaptation, while this commit fixes the concrete parity regression without deleting that structural marker. Verification: cargo check -p majit-metainterp --features dynasm; cargo test -p majit-metainterp --features dynasm.
RPython's BridgeCompileData keeps runtime_boxes separate from the trace iterator and passes them to UnrollOptimizer.optimize_bridge. The Rust bridge path was constructing BridgeCompileData but then discarded that field, leaving Optimizer::optimize_bridge to reuse the prepared bridge JUMP args for both fallback jump emission and virtual-state guard generation. Pass runtime_boxes explicitly into Optimizer::optimize_bridge. Keep the prepared pre-optimization JUMP args for send_extra_operation fallback jumps, but use the original runtime boxes when calling jump_to_existing_trace so guard generation follows unroll.py's runtime_boxes contract. Also thread the same runtime box payload through the entry-bridge path, where Pyre has a flattened compile_entry_bridge adaptation instead of the exact RPython CompileData object path.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR introduces compile-data bundle types to encapsulate trace metadata and propagates them through the compilation pipeline. New struct types ( ChangesCompile Data Bundling and Pipeline Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@majit/majit-metainterp/src/pyjitpl/mod.rs`:
- Around line 5518-5531: The code duplicates construction of a compile bundle
(SimpleCompileData::new(...) plus deriving trace_snapshots and trace_ops) in
multiple places; extract that sequence into a single helper (e.g.,
build_simple_compile_data or make_simple_compile_views) that takes the shared
inputs (trace, &call_pure_results, enable_opts from
self.warm_state.get_enable_opts()) and returns the SimpleCompileData and the
derived trace_snapshots and trace_ops (or at least the snapshots and operations
vectors). Replace the direct calls to SimpleCompileData::new,
simple_data.base.snapshots().to_vec(), and
simple_data.base.operations().to_vec() with calls to this helper in both places
(including where ctx.constants.refresh_from_gc() / into_inner_with_types() is
used) so both paths use the identical assembly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a559a674-26b8-4860-857e-4c7420742d69
📒 Files selected for processing (3)
majit/majit-metainterp/src/compile.rsmajit/majit-metainterp/src/optimizeopt/optimizer.rsmajit/majit-metainterp/src/pyjitpl/mod.rs
Extract the repeated SimpleCompileData setup in pyjitpl into make_simple_compile_views. Both finish compilation and compile_simple_loop now use the same helper to assemble the SimpleCompileData plus the derived trace snapshots and operation vector. This keeps the compile.py parity marker wiring consistent across the two simple compile paths while leaving constant-pool refresh and optimizer behavior unchanged. Verification: cargo fmt; cargo check --features dynasm; cargo test --features dynasm.
Adds `.claude/skills/cel-unboxed-values/SKILL.md`, the design of record for task #52: one CEL bytecode VM as `Program::execute`, traced by majit-translate, over a `#[repr(C)]` header-first `Arc`-free class family. Force-added: `.claude` is in `.gitignore`, matching how `codex-review`, `merge`, `parity` and `rebase` are tracked. Records, among others: - four shapes that do not lower and must not be re-derived — a function-pointer vtable struct, `virtualizables: []` on a stack VM, a raw `*mut CelRef` slot array, and `Result<CelRef, CelError>` inside the traced VM (the `?`-to-exception-link rewrite is keyed on `PyError`); - CEL errors carried out-of-band and host builtins compiled to integer opcodes, both consequences of those two; - a majit change budget M1..M8, with M4 (`guard_class`) off the critical path and M8 (a user-registered-function residual path) newly on it; - write-barrier sites, the root set, and the `Value: !Send` break that follows from cel having no GIL; - the P0.d run of 2026-08-07: assertions 1 and 3 hold, assertion 2 holds once the class-static address is a constant — which pins M1 as its sole gate — and assertion 4 is not checkable below the codewriter. Assisted-by: Claude
`compiles >= 1` could not be tightened while the count was not the fixture's own: before #52 the probe read `compiled 2` in 10 of 10 runs, because the counters are process-wide and the two result-only tests entered the JIT without taking PROBE_LOCK. With #52 landed the count is deterministic -- 8 consecutive full-suite runs read `compiled 1` -- so the inequality can become an equality. That also makes the assertion gate the contamination #52 closed: a future test entering the JIT inside the probe's window reads 2 and fires here, where previously any count >= 1 passed and the ratio below was computed from whichever trace arrived last. The ratio and ops_before figures are unchanged (4805 -> 5, ratio 961): both fixtures run away to trace_limit and fold identically, which is why the contamination was invisible to every assertion in the file. Assisted-by: Claude
…nd residual fnaddr shims Move `StringBuilderBox`/`StringPieceBox` and the `rbuilder_runtime` module (`ll_new`, `ll_grow_by`, `ll_grow_and_append`, `ll_append`, `ll_append_char`, `ll_getlength`, `ll_shrink_final`, `ll_fold_pieces`, `ll_build`, plus the #52 semantics tests) from `pyre-jit::eval` to `pyre_object::rbuilder`. Move the raw low-level-string helpers `bh_lowlevel_chars_offset`, `bh_read_lowlevel_string`, `bh_write_lowlevel_char` to `pyre_object::lowlevel_string` beside their existing `bh_alloc`/`bh_free`/`bh_len` siblings. The relocated crate is reachable from `pyre-interpreter`, where the fnaddr registry lives; `pyre-jit::eval` was not. Add `pub extern "C"` shims `jit_ll_append_res0(builder, ll_str)` and `jit_ll_append_res_slice(builder, ll_str, start, end)` delegating to `ll_append` with `item_size = 1`, and register them in `jit_trace_fnaddrs` under the un-prefixed leaf keys `ll_append_res0` / `ll_append_res_slice`. With the fnaddr bound, `guess_call_kind`'s `dont_look_inside` residual arm (the append helper recognizer) resolves these callsites. `build_gc`'s builder/piece tid registration now reads the `pyre_object::rbuilder` offset/size consts instead of `size_of`/`offset_of` on the moved structs, so `pyre-jit::eval` no longer references the struct types. Assisted-by: Claude
…nd residual fnaddr shims Move `StringBuilderBox`/`StringPieceBox` and the `rbuilder_runtime` module (`ll_new`, `ll_grow_by`, `ll_grow_and_append`, `ll_append`, `ll_append_char`, `ll_getlength`, `ll_shrink_final`, `ll_fold_pieces`, `ll_build`, plus the #52 semantics tests) from `pyre-jit::eval` to `pyre_object::rbuilder`. Move the raw low-level-string helpers `bh_lowlevel_chars_offset`, `bh_read_lowlevel_string`, `bh_write_lowlevel_char` to `pyre_object::lowlevel_string` beside their existing `bh_alloc`/`bh_free`/`bh_len` siblings. The relocated crate is reachable from `pyre-interpreter`, where the fnaddr registry lives; `pyre-jit::eval` was not. Add `pub extern "C"` shims `jit_ll_append_res0(builder, ll_str)` and `jit_ll_append_res_slice(builder, ll_str, start, end)` delegating to `ll_append` with `item_size = 1`, and register them in `jit_trace_fnaddrs` under the un-prefixed leaf keys `ll_append_res0` / `ll_append_res_slice`. With the fnaddr bound, `guess_call_kind`'s `dont_look_inside` residual arm (the append helper recognizer) resolves these callsites. `build_gc`'s builder/piece tid registration now reads the `pyre_object::rbuilder` offset/size consts instead of `size_of`/`offset_of` on the moved structs, so `pyre-jit::eval` no longer references the struct types. Assisted-by: Claude
…nd native fnaddr (#1447) * majit rbuilder: residualize dont_look_inside append helpers when a native fnaddr is bound guess_call_kind classifies a call to ll_append_res0 / ll_append_res_slice as Residual once a function_fnaddrs entry is registered for the path, matching the @jit.dont_look_inside decoration on those helpers in rbuilder.py. The append / append_slice jit arms route through these general residuals; residualizing keeps the grow/malloc body out of the trace. Gated on the function_fnaddrs entry: without a bound native address the symbolic-fallback address is not callable, so the call stays Regular and executes the generated jitcode. This is the codewriter half; a later change registers the runtime helper fnaddr to activate it. Assisted-by: Claude * majit rbuilder: relocate builder runtime to pyre-object; bind ll_append residual fnaddr shims Move `StringBuilderBox`/`StringPieceBox` and the `rbuilder_runtime` module (`ll_new`, `ll_grow_by`, `ll_grow_and_append`, `ll_append`, `ll_append_char`, `ll_getlength`, `ll_shrink_final`, `ll_fold_pieces`, `ll_build`, plus the #52 semantics tests) from `pyre-jit::eval` to `pyre_object::rbuilder`. Move the raw low-level-string helpers `bh_lowlevel_chars_offset`, `bh_read_lowlevel_string`, `bh_write_lowlevel_char` to `pyre_object::lowlevel_string` beside their existing `bh_alloc`/`bh_free`/`bh_len` siblings. The relocated crate is reachable from `pyre-interpreter`, where the fnaddr registry lives; `pyre-jit::eval` was not. Add `pub extern "C"` shims `jit_ll_append_res0(builder, ll_str)` and `jit_ll_append_res_slice(builder, ll_str, start, end)` delegating to `ll_append` with `item_size = 1`, and register them in `jit_trace_fnaddrs` under the un-prefixed leaf keys `ll_append_res0` / `ll_append_res_slice`. With the fnaddr bound, `guess_call_kind`'s `dont_look_inside` residual arm (the append helper recognizer) resolves these callsites. `build_gc`'s builder/piece tid registration now reads the `pyre_object::rbuilder` offset/size consts instead of `size_of`/`offset_of` on the moved structs, so `pyre-jit::eval` no longer references the struct types. Assisted-by: Claude * majit rbuilder: UnicodeBuilder append_slice arm; shrink_array selects width by GC tid UnicodeBuilderRepr::rtype_method gains the append_slice arm StringBuilderRepr already had, dispatching the shared width-generic rtype_builder_append_slice with the UNICODE piece/pointer lltypes and mallocunicode/copyunicodecontent. Adds a unicodebuilder_rtype_method_append_slice_emits_direct_call test mirroring the STR one (asserts the UNICODE callee tree mints mallocunicode). jit_ll_shrink_array splits into a width-parametric shrink_lowlevel_array core and a shrink_array_width helper that reads the buffer's GC type id (lowlevel_unicode tid -> item_size 4, else STR width 1), so a single residual target serves both builder widths without a jtransform change. A raw-fallback buffer has no GC owner and keeps STR width. Adds shrink tests for both widths. Assisted-by: Claude * rbuilder: add pending end-to-end StringBuilder fixture (join/repeat/concat/replace) pyre/bench/synth/_pending/rbuilder_string_build_hot.py drives the builder-mode string paths (unicodeobject.rs Wtf8Buf accumulators lowered via graph_has_builder_accumulator to ll_new/ll_append*/ll_build) from four hot loops with deterministic integer output (join/repeat/concat/replace). Kept in _pending/ — run_synthetic_suite globs *.py non-recursively, so the gate skips it — until its jitstats snapshots and max-pypy-ratio are recorded and tuned on a charon-capable host and it is promoted to pyre/bench/synth/. Verified deterministic on CPython (1800000/420000/256000/960000). Assisted-by: Claude * majit rbuilder: port ll_grow_by growth ovfchecks; abort on overflow Native `ll_grow_by` computed the growth size with plain `i64` arithmetic, which wraps on overflow in release and would allocate an undersized buffer. Factor the three `ovfcheck`s of `rbuilder.py ll_grow_by` into `grow_by_sizes` using `checked_add`; on overflow the caller aborts with a message instead of wrapping. `raise MemoryError` propagation and the JIT graph-builder's `int_add_ovf` exception edge remain unported. Add a `grow_by_sizes` unit test covering the 64-round-up and each of the three overflow points. Assisted-by: Claude * majit rbuilder: model grow_by/grow_and_append ovfcheck exception edges ll_grow_by: split the three growth ovfchecks into one block each so each int_add_ovf is its block's trailing op; the OverflowError edge lands in a shared handler that raises MemoryError via the graph exceptblock. ll_grow_and_append: split the fast path into block_fast_check (the trailing ovfcheck) and block_fast_body; the OverflowError edge routes to the slow path (`except OverflowError: pass`). Add close_ovfcheck_block to attach the c_last_exception exit shape flatten insert_exits requires for a block containing an _ovf op (2-3 exits switched on last-exception). Update the two graph-structure tests for the multi-block layout. Assisted-by: Claude * majit: preserve exception-link extravars in opname-spine lowering; harden grow_by test jtransform_opname::lower_graph rebuilt each exit link with Link::new_mixed, which leaves last_exception/last_exc_value None. A canraise block's OverflowError exit (as close_ovfcheck_block emits for ll_grow_by/ ll_grow_and_append) then reaches flatten's make_exception_link with None extravars, which asserts they are Some and unwraps them for an empty-target handler. Copy the flowspace link's extravars onto the model link. Add lower_graph_preserves_ovfcheck_exception_extravars covering a trailing int_add_ovf block with an OverflowError edge. build_ll_grow_by_ovf_arith_mallocs_piece_and_relinks_buffer asserted only op-name counts and the MemoryError edge; add block_d data-flow assertions: the mallocfn size is the same value written to current_end, total_size is a distinct value, and current_pos is 0. Assisted-by: Claude * majit rbuilder: thread StringBuilder with_capacity size through the newstringbuilder marker The builder-mode ctor marker (front `mir.rs`) and its flowspace_adapter lowering both emitted `newstringbuilder` with no operands, so `rtype_builder_new` could only take the `nb_args() == 0` branch and called `ll_new(INIT_SIZE)`. Forward the ctor operands verbatim: `new()` carries none and `with_capacity(n)` carries the size `n`, so the operand count mirrors `len(hop.args_v)` in `AbstractStringBuilderRepr.rtyper_new` and a requested capacity threads `ll_new(n)`. Add the size-carrying cases: a flowspace_adapter test that the marker forwards its operand to `newstringbuilder`, and an rtyper test that `rtyper_new` with a size operand emits `direct_call ll_new` carrying the requested size rather than the default. Assisted-by: Claude
Summary
This pull request introduces a set of new data structures to standardize and clarify the data passed into the JIT compilation pipeline, closely modeling RPython's
CompileDataand its subclasses. It refactors how trace and runtime information (such as snapshots, input arguments, and optimization options) are bundled and passed throughout the compilation and optimization process. The changes improve maintainability and parity with the original RPython design, and ensure that all relevant context is consistently available to downstream consumers.Key changes include:
Introduction of CompileData Bundles:
CompileData,PreambleCompileData,SimpleCompileData,BridgeCompileData, andUnrolledLoopData) incompile.rsto encapsulate trace, runtime, and optimization state, mirroring RPython's object model for JIT compilation inputs. These bundles are now used to pass data to optimizers and other compilation stages, replacing ad-hoc argument lists.Refactoring Compilation Call Sites:
pyjitpl/mod.rsto construct and use the newCompileDatabundles, ensuring that trace operations, snapshots, input arguments, and optimization options are accessed via these unified interfaces. This includes retrace, simple compilation, and bridge compilation paths.call_pure_resultsand other relevant state from the appropriateCompileDatabundle, ensuring consistency.Runtime Boxes and Bridge Compilation:
runtime_boxes(the original live argument boxes from the closing JUMP) are passed separately and consistently into optimizer and bridge compilation routines, matching RPython's model and fixing previous inconsistencies.Miscellaneous Improvements:
call_pure_results,enable_opts, andsnapshots) is cloned or moved from the context at the correct time to avoid accidental mutation or loss of data.These changes bring the codebase closer to RPython's design, reduce code duplication, and make the flow of compilation data more explicit and maintainable.
Self-review
Prompt & Model
Model: gpt-5.5
Prompt:
Answer
Summary by CodeRabbit