Repository navigation
jit: exempt list_write_barrier from the FOR_ITER body-effect gate (#389 item b) + wasm codegen fixes - #661
Conversation
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds nested-list comprehension JIT admission rules, extra virtual-root snapshot propagation through optimization and resume-data generation, idempotent list-barrier effect handling, and wasm safeguards for missing operand references. ChangesNested list virtualization and deoptimization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant TraceCapture
participant SnapshotMaps
participant Optimizer
participant ResumeDataVirtualAdder
TraceCapture->>SnapshotMaps: capture extra_virtual_roots
SnapshotMaps->>Optimizer: translate and store snapshot roots
Optimizer->>ResumeDataVirtualAdder: pass extra_virtual_roots
ResumeDataVirtualAdder->>ResumeDataVirtualAdder: seed virtual worklist and discover fields
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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: e9fc894b92
ℹ️ 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".
| let normalized = (**op).clone(); | ||
| let rt = normalized.result_type(); | ||
| if rt != majit_ir::Type::Void && normalized.pos.get().is_none() { | ||
| normalized | ||
| .pos | ||
| .set(majit_ir::OpRef::op_typed(num_inputs + op_idx as u32, rt)); |
There was a problem hiding this comment.
When the forced-materialization path leaves a producer unpositioned and a later op consumes it, this only stamps the fresh cloned Op. The cloned consumer still holds Operand::Op(Rc<original>), and Operand::to_opref() reads the original producer's pos, so it remains OpRef::NONE; wasm codegen can then resolve that normal operand as the new zero placeholder and use a null/zero value instead of the materialized result. Please mutate/rebind the producer identities (or rebuild operands to point at the normalized ops) before codegen.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit de53244). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e30d785d18
ℹ️ 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".
| || (!body_has_call | ||
| && (!body_has_nonempty_list_build || nested_list_fold_ok) | ||
| && matches!(body_instr, I::ListAppend { .. })); |
There was a problem hiding this comment.
When a comprehension element uses implicit dispatch such as x.__add__, a descriptor/property __get__, or x.__format__, the bytecode contains BinaryOp, LoadAttr, or FormatSimple rather than an explicit Call, so body_has_call remains false and the following LIST_APPEND is admitted. The residual-call body-effect logic already treats these as user-frame entries (pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs:4088-4091), so a later mid-body abort can still refuse the in-flight FOR_ITER delivery and drop the trace-attempt item—the same failure mode this gate avoids for explicit calls. Please include these MayForce implicit-call opcodes in the decline condition, or otherwise prove the operation is builtin-only before admitting LIST_APPEND.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9b35dcf9e
ℹ️ 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".
| I::Call { .. } | ||
| | I::CallKw { .. } | ||
| | I::CallFunctionEx | ||
| | I::CallIntrinsic1 { .. } => has_call = true, |
There was a problem hiding this comment.
When a comprehension appends a set or dict whose element/key type defines __hash__ or __eq__, the bytecode contains BUILD_SET/BUILD_MAP followed by LIST_APPEND but no explicit CALL, so this scan leaves body_has_call false and admits the append. Those builders route through checked dict/set construction (build_map_from_refs / build_set_from_refs), whose own comments note that hashing may run user __hash__ / __eq__; if a later walk aborts after that user frame, the in-flight FOR_ITER item can still take the existing refuse/drop path. Please classify non-empty BuildSet/BuildMap as call-bearing here unless the keys/elements are proven builtin-only.
Useful? React with 👍 / 👎.
bb17bd4 to
de2f808
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de2f808496
ℹ️ 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".
| || (!body_has_call | ||
| && (!body_has_nonempty_list_build || nested_list_fold_ok) | ||
| && matches!(body_instr, I::ListAppend { .. })); |
There was a problem hiding this comment.
When PYRE_NESTED_LIST_FOLD_VIRT=1, this condition admits non-empty nested-list elements like [[i] for ...], but the current tree still never populates GuardCaptureScope::sub_walk_extra_virtual_roots (the only references are the field/default and the snapshot reader). That means the append sub-walk's guard-fail resume still lacks the inner list backing block the comments describe, so enabling the gate reintroduces the null-OpRef deopt crash instead of making the shape safe; keep declining until the commit path actually supplies those roots.
Useful? React with 👍 / 👎.
| // non-reentrant Mutex. A parallel double-miss yields two equivalent holders | ||
| // and the later insert wins; each caller returns its own local holder, so | ||
| // the only cost is a leaked holder node. |
There was a problem hiding this comment.
In the parallel miss case this assumption is only true for the exact same key. If two threads add different attributes from the same self_node, both can observe the same cache_attrs().len() in new_cached_attribute_holder, creating sibling holders with duplicate order; find_branch_to_move_into uses order to decide when to stop reordering, so duplicate sibling orders can make later objects that need both attributes bounce between the two branches instead of reaching a canonical map. Reserve the order and insert while holding one critical section (or pass the reserved order into holder creation) so concurrent different-key misses cannot share an order.
Useful? React with 👍 / 👎.
…call-free Object-append comprehensions The #171 orthodox LIST_APPEND fold descends w_list_append; its Object-strategy arm stores a GC ref and runs list_write_barrier, a #[dont_look_inside] Void residual. The FBW body-effect gate classified that Void residual as a live-heap write, so an in-flight FOR_ITER item was refuse-dropped and an Object-strategy comprehension append ([(i,i) for ...], [None ...], ["s" ...], [{i:i} ...], [f"{i}" ...]) yielded one element short. - Add is_list_write_barrier(addr) in jit_fnaddr.rs, matching the registered barrier fnaddr, and exempt it from body_effect_candidate, fbw_mark_executed_nonpure_residual, and the executed-effect odometer in try_execute_residual_call_via_executor. The barrier still executes concretely. This mirrors RPython's COND_CALL_GC_WB, which pyjitpl never executes (executor.py:446) and the optimizer never treats as a side effect. - Widen for_iter_bodies_all_jit_safe to admit a call-free LIST_APPEND body (body_has_call replaces the bare-accumulator whitelist), but decline a non-empty nested BUILD_LIST element ([[i] for ...]): the fold virtualizes the inner list yet does not thread its backing block into the append commit sub-walk's guard-exit resume data. Empty [] and non-list containers are safe. - Add synth/comprehension_object_append_hot.py covering the five Object-strategy element kinds plus int. Assisted-by: Claude
… before codegen An inlined comprehension trace exposed two wasm-backend gaps that the dynasm and cranelift backends do not have (both compile the same trace): - The wasm backend handed the optimizer's ops straight to codegen without the position-normalization pass the native backends run (dynasm prepare_ops_for_compile, cranelift normalize_ops_for_codegen_simple). Add normalize_ops_for_codegen and call it in compile_loop / compile_bridge so every non-Void-result op left unpositioned by the force path gets a stamped OpRef before codegen. - emit_resolve indexed value_types with opref.raw() for a NONE operand (raw() == u32::MAX), panicking out of bounds when a Finish op carried a NONE fail-arg (a dead deopt slot). Spill a zero placeholder for a NONE operand; the blackhole never reads that slot back. Assisted-by: Claude
A non-empty nested BUILD_LIST element flowing into the #171 inlined-comprehension LIST_APPEND fold ([[i] for i in range(n)]) declines to the interpreter: the fold virtualizes the inner list, but its separately allocated backing block (NewArray / NewArrayClear) has no jitcode-liveness color, so collect_outer_active_boxes cannot root it in the append commit sub-walk's guard-exit resume data and a deopt resolves it to a null OpRef. Closing it needs the inner wrapper threaded into the guard snapshot as an extra virtual root, so the ported resume recursion (_visitor_walk_recursive / VArrayInfo) registers the nested virtual. Land the opt-in scaffold for that work, default-off so it is a strict no-op: - Add nested_list_fold_virt_enabled() (PYRE_NESTED_LIST_FOLD_VIRT, default off). - Add GuardCaptureScope::sub_walk_extra_virtual_roots, the dormant channel for walker-minted virtual roots that carry no outer-frame liveness color. Empty on every path today; the two struct-literal sites use ..default() so they are unaffected. - Gate the LIST_APPEND admit in for_iter_bodies_all_jit_safe on the flag: with it off the nested shape still declines via body_has_nonempty_list_build. - Add synth/nested_list_comprehension_hot.py, the acceptance repro. With the gate off it runs in the interpreter and prints the correct total; with the gate on it reaches the fold and aborts cleanly at UnfoldableListAppendResidualUnsupported (try_walker_orthodox_list_append_opcode declines the nested value) — the next slice's target. dynasm 227/227, cranelift 227/227, wasm 226/226. Assisted-by: Claude
…c-deopt miscompile The #171 orthodox LIST_APPEND fold records the Integer/Float unboxed spare-capacity fast path guarded by GuardTrue(length < arraylen(items)). When a comprehension result escapes the enclosing frame and a later append crosses the backing block's realloc boundary, the wasm backend mis-resumes that capacity-guard deopt: a frame-local list loses the reallocating append's iteration (one element short), and a kept operand-stack list resolves to NULL (TypeError: call failed). dynasm and cranelift resume the identical guard IR correctly, so this is a wasm-backend deopt/resume defect. Add wasm_unboxed_append_fold_declined() (PYRE_WASM_UNBOXED_APPEND_FOLD, default-on for wasm32, strict no-op on native, =1 forces the fold back on for root-fix bisecting) and gate the Integer/Float arms of orthodox_list_append_recognize on it — both the empty->typed first-append promotion and the steady spare-capacity branch. On wasm the unboxed append then runs as the residual jit_list_append (interpreter fallback, correct). The Object-storage arm is unaffected and stays folded. dynasm 227/227, cranelift 227/227, wasm 226/226. Assisted-by: Claude
…old, inert) Add a per-guard `extra_virtual_roots` channel that seeds the resume-data virtual worklist with virtual op-results reachable only as a field of another walker-minted virtual (no frame-liveness slot of their own). - `recorder::Snapshot` gains `extra_virtual_roots: Vec<OpRef>`; `capture_snapshot_for_last_guard_with_vable_vref` and the walker capture path write it. All other snapshot construction sites default empty. - `snapshot_map_from_trace_snapshots` flattens each recorder Snapshot's roots into a new position-keyed optimizer map `snapshot_extra_virtual_roots` (untyped SnapshotBox, 6-tuple return). New field added to OptContext, Optimizer, UnrollOptimizer and threaded through every snapshot_vref_boxes data-flow site: the max_snapshot next_pos chain, phase-move takes, phase1 clone, phase2 remap, bridge remap_snapshot_boxes, the five snapshot_map_from_trace_snapshots call sites, PreparedBridgeTrace, prepare_bridge_trace_for_optimizer, compile_bridge/compile_entry_bridge. Not added to collect_snapshot_const_ptr_slots (roots carry no inline ConstPtr gcrefs). - `ResumeDataVirtualAdder::finish` gains `extra_virtual_roots: &[OpRef]`; a seed loop after the liveboxes worklist seed registers each via register_virtual_box (UNASSIGNEDVIRTUAL) and pushes it into the virtual worklist, so the existing get_virtual_fields drain descends into it. store_final_boxes_in_guard reads the position map and passes the roots. Empty on every path until PYRE_NESTED_LIST_FOLD_VIRT populates the roots, so this is a no-op: dynasm 243/243, cranelift 243/243. Assisted-by: Claude
`nested_list_fold_virt_enabled` flips DEFAULT-OFF -> DEFAULT-ON, admitting a non-empty nested `BUILD_LIST` element (`[[i] for i in range(n)]`) into the `w_list_append` fold on all backends. With the trace-time single-executor forks retired, the inner list's separately allocated backing block is bound at every guard-exit deopt without an extra resume-data root, so the shape compiles bit-exact on dynasm / cranelift / wasm. check.py: dynasm 243/243, cranelift 243/243, wasm 242/242. The acceptance repro `bench/synth/nested_list_comprehension_hot.py` and a 5-shape stress match CPython 3.14 across all three backends; cranelift is ~27% faster gate-on. The wasm guest cannot read the env var, so the source default is the only way to exercise it there. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c2b54b5cb
ℹ️ 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".
| /// decline (native only — the wasm guest cannot read the env var). | ||
| pub fn nested_list_fold_virt_enabled() -> bool { | ||
| static ENABLED: std::sync::LazyLock<bool> = std::sync::LazyLock::new(|| { | ||
| std::env::var("PYRE_NESTED_LIST_FOLD_VIRT").map_or(true, |v| v != "0") |
There was a problem hiding this comment.
When PYRE_NESTED_LIST_FOLD_VIRT is unset, this map_or(true, ...) enables the nested-list append fold, so for_iter_bodies_all_jit_safe admits non-empty BUILD_LIST elements by default. Fresh evidence in the current tree is that sub_walk_extra_virtual_roots still has no writer (rg sub_walk_extra_virtual_roots only finds the field and snapshot reader), and the newly added nested-list test/benchmark still describe this as DEFAULT-OFF and expect the shape to decline; with the default on, normal runs can hit the unrooted inner-list backing-block deopt/null-OpRef path instead of staying in the interpreter.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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-backend-wasm/src/lib.rs`:
- Around line 1064-1073: Update normalize_ops_for_codegen so each original Op is
assigned its generated position before cloning, rather than setting
normalized.pos on the clone. Preserve the existing non-Void and missing-position
conditions, and ensure downstream Operand::Op references observe the updated
original Op position.
In `@pyre/bench/synth/nested_list_comprehension_hot.py`:
- Around line 1-13: Update the benchmark rationale around
nested_list_fold_virt_enabled() to match its actual default behavior, rather
than claiming PYRE_NESTED_LIST_FOLD_VIRT is default-off or that
for_iter_bodies_all_jit_safe declines the shape by default. Preserve the
benchmark’s requirement that it prints the same total across all backends when
the gate is enabled.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs`:
- Around line 3727-3741: Update the documentation for
sub_walk_extra_virtual_roots to remove the incorrect “DEFAULT-OFF” and “dormant”
claims, and describe its availability consistently with
nested_list_fold_virt_enabled(), whose default is enabled.
- Around line 6005-6049: Keep nested_list_fold_virt_enabled() default-on and
update all affected references: in
pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs:6005-6049 retain the new
default; update pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs:3727-3741 so
for_iter_nonempty_nested_list_comprehension_body_declines expects folding by
default or explicitly forces PYRE_NESTED_LIST_FOLD_VIRT=0; revise the
sub_walk_extra_virtual_roots documentation in
pyre/pyre-jit/src/eval.rs:10094-10114 and the benchmark header in
pyre/bench/synth/nested_list_comprehension_hot.py:1-13 to describe the
default-on behavior and opt-out.
🪄 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: 49f3edf6-3dda-4478-abfa-da07ab8b56e1
📒 Files selected for processing (20)
majit/majit-backend-wasm/src/codegen.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-metainterp/src/history.rsmajit/majit-metainterp/src/optimizeopt/mod.rsmajit/majit-metainterp/src/optimizeopt/optimizer.rsmajit/majit-metainterp/src/optimizeopt/unroll.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-metainterp/src/recorder.rsmajit/majit-metainterp/src/resume.rsmajit/majit-metainterp/tests/jit_driver_runtime_parity.rspyre/bench/synth/comprehension_object_append_hot.pypyre/bench/synth/nested_list_comprehension_hot.pypyre/pyre-interpreter/src/jit_fnaddr.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/eval.rs
| .map(|(op_idx, op)| { | ||
| let normalized = (**op).clone(); | ||
| let rt = normalized.result_type(); | ||
| if rt != majit_ir::Type::Void && normalized.pos.get().is_none() { | ||
| normalized | ||
| .pos | ||
| .set(majit_ir::OpRef::op_typed(num_inputs + op_idx as u32, rt)); | ||
| } | ||
| normalized | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Mutate the original Op's position before cloning it.
Because normalize_ops_for_codegen mutates the clone (normalized.pos.set(...)), the original Op's position remains NONE.
Later operations in the trace hold Operand::Op(OpRc) references that point to the original producer ops, not the clones in the returned Vec. When codegen resolves those operands, it will read pos from the original OpRc, yielding NONE and causing the exact out-of-bounds indexing crash this function was intended to prevent.
Mutate the original Op instead so that downstream consumers see the updated position.
🐛 Proposed fix
- let normalized = (**op).clone();
- let rt = normalized.result_type();
- if rt != majit_ir::Type::Void && normalized.pos.get().is_none() {
- normalized
- .pos
- .set(majit_ir::OpRef::op_typed(num_inputs + op_idx as u32, rt));
+ let rt = op.result_type();
+ if rt != majit_ir::Type::Void && op.pos.get().is_none() {
+ op.pos.set(majit_ir::OpRef::op_typed(num_inputs + op_idx as u32, rt));
}
- normalized
+ (**op).clone()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .map(|(op_idx, op)| { | |
| let normalized = (**op).clone(); | |
| let rt = normalized.result_type(); | |
| if rt != majit_ir::Type::Void && normalized.pos.get().is_none() { | |
| normalized | |
| .pos | |
| .set(majit_ir::OpRef::op_typed(num_inputs + op_idx as u32, rt)); | |
| } | |
| normalized | |
| }) | |
| .map(|(op_idx, op)| { | |
| let rt = op.result_type(); | |
| if rt != majit_ir::Type::Void && op.pos.get().is_none() { | |
| op.pos.set(majit_ir::OpRef::op_typed(num_inputs + op_idx as u32, rt)); | |
| } | |
| (**op).clone() | |
| }) |
🤖 Prompt for 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.
In `@majit/majit-backend-wasm/src/lib.rs` around lines 1064 - 1073, Update
normalize_ops_for_codegen so each original Op is assigned its generated position
before cloning, rather than setting normalized.pos on the clone. Preserve the
existing non-Void and missing-position conditions, and ensure downstream
Operand::Op references observe the updated original Op position.
| # An inlined list comprehension whose LIST_APPEND element is a non-empty nested | ||
| # list (`[[i] …]` / `[[i, i + 1] …]`). The #171 fold virtualizes the inner list, | ||
| # but its separately allocated backing block (NewArray / NewArrayClear) carries | ||
| # no jitcode-liveness color, so it is not rooted in the append commit sub-walk's | ||
| # guard-exit resume data and a deopt resolves it to a null OpRef. | ||
| # | ||
| # Held back behind the DEFAULT-OFF PYRE_NESTED_LIST_FOLD_VIRT gate: while the | ||
| # gate is off `for_iter_bodies_all_jit_safe` declines the shape, so this runs in | ||
| # the interpreter and prints the correct total. It is the acceptance repro for | ||
| # the recursive virtual-list-forcing rooting work — with the gate on it must | ||
| # still print the same total on all three backends (dynasm / cranelift / wasm) | ||
| # once the rooting lands. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Comment assumes the gate defaults off — see consolidated cross-file comment.
This benchmark's rationale ("Held back behind the DEFAULT-OFF PYRE_NESTED_LIST_FOLD_VIRT gate") assumes for_iter_bodies_all_jit_safe currently declines this shape by default, which contradicts nested_list_fold_virt_enabled()'s actual default. See the consolidated comment for details.
🤖 Prompt for 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.
In `@pyre/bench/synth/nested_list_comprehension_hot.py` around lines 1 - 13,
Update the benchmark rationale around nested_list_fold_virt_enabled() to match
its actual default behavior, rather than claiming PYRE_NESTED_LIST_FOLD_VIRT is
default-off or that for_iter_bodies_all_jit_safe declines the shape by default.
Preserve the benchmark’s requirement that it prints the same total across all
backends when the gate is enabled.
|
|
||
| /// Extra virtual roots to seed into the guard snapshot beyond the outer | ||
| /// Python frame's jitcode-liveness boxes — walker-minted virtual op-results | ||
| /// that carry no liveness color yet must survive a deopt as fail-arg roots. | ||
| /// The motivating case is the `w_list_append` commit sub-walk over a | ||
| /// non-empty nested list element (`[[i] for i in range(n)]`): the appended | ||
| /// inner-list wrapper is passed to the sub-walk as a register arg but is not | ||
| /// named by any live outer color, so without an explicit root the resume | ||
| /// recursion never descends into it and its backing block resolves to a null | ||
| /// `OpRef`. Each entry is the `(OpRef, Type)` of a root the snapshot builder | ||
| /// must append to the top frame's boxes so the ported | ||
| /// `_visitor_walk_recursive` registers the nested virtual (wrapper + backing | ||
| /// block) in resume data. Empty on every path today: only populated behind | ||
| /// the DEFAULT-OFF `PYRE_NESTED_LIST_FOLD_VIRT` gate, so this is dormant. | ||
| pub sub_walk_extra_virtual_roots: &'a [(OpRef, majit_ir::Type)], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Stale "DEFAULT-OFF" claim contradicts the gate's actual default.
The doc says extra virtual roots are "only populated behind the DEFAULT-OFF PYRE_NESTED_LIST_FOLD_VIRT gate, so this is dormant." However nested_list_fold_virt_enabled() (below, same file) is default-ON (map_or(true, ...)). This is part of a broader cross-file inconsistency — see the consolidated comment.
🤖 Prompt for 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.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs` around lines 3727 - 3741,
Update the documentation for sub_walk_extra_virtual_roots to remove the
incorrect “DEFAULT-OFF” and “dormant” claims, and describe its availability
consistently with nested_list_fold_virt_enabled(), whose default is enabled.
| /// `PYRE_NESTED_LIST_FOLD_VIRT` gate (read once) — admits a non-empty nested | ||
| /// `BUILD_LIST` element (`[[i] for i in range(n)]`) into the orthodox | ||
| /// `w_list_append` fold. The appended value is a virtualized inner list whose | ||
| /// separately allocated backing block (`NewArray` / `NewArrayClear`) carries no | ||
| /// jitcode-liveness slot; once the trace-time single-executor forks were retired | ||
| /// the append body no longer runs under a speculative-replay sub-walk, so the | ||
| /// backing block is bound at every guard-exit deopt and the shape compiles | ||
| /// bit-exact on dynasm / cranelift / wasm (comprehension-hot acceptance repro | ||
| /// `bench/synth/nested_list_comprehension_hot.py`). Default-on; set | ||
| /// `PYRE_NESTED_LIST_FOLD_VIRT=0` to fall back to the `for_iter_bodies_all_jit_safe` | ||
| /// decline (native only — the wasm guest cannot read the env var). | ||
| pub fn nested_list_fold_virt_enabled() -> bool { | ||
| static ENABLED: std::sync::LazyLock<bool> = std::sync::LazyLock::new(|| { | ||
| std::env::var("PYRE_NESTED_LIST_FOLD_VIRT").map_or(true, |v| v != "0") | ||
| }); | ||
| *ENABLED | ||
| } | ||
|
|
||
| /// `PYRE_WASM_UNBOXED_APPEND_FOLD` gate (read once) — admits the unboxed | ||
| /// (Integer/Float storage) `w_list_append` fold on the wasm backend. The fold | ||
| /// records the spare-capacity fast path guarded by `GuardTrue(length < | ||
| /// arraylen(items))`; when the comprehension result escapes the enclosing frame | ||
| /// and a later append crosses the backing block's realloc boundary, the wasm | ||
| /// backend mis-resumes that capacity-guard deopt — the reallocating append's | ||
| /// iteration is lost (list one element short) or, when the partially built list | ||
| /// is a kept operand-stack temp, the whole list resolves to NULL ("call | ||
| /// failed"). dynasm / cranelift resume the identical guard IR correctly, so this | ||
| /// is a wasm-backend deopt/resume defect. Until it is root-fixed, decline the | ||
| /// unboxed arm on wasm so the append runs as the plain residual `jit_list_append` | ||
| /// (interpreter fallback, correct if unaccelerated). DEFAULT-ON for wasm32 (the | ||
| /// backend that mis-resumes); a strict no-op on native (the fold always folds | ||
| /// there). Set `PYRE_WASM_UNBOXED_APPEND_FOLD=1` to force the fold back on for | ||
| /// bisecting the root fix. The Object-storage arm is unaffected — it resumes | ||
| /// correctly and stays folded. | ||
| fn wasm_unboxed_append_fold_declined() -> bool { | ||
| static DECLINED: std::sync::LazyLock<bool> = std::sync::LazyLock::new(|| { | ||
| if cfg!(target_arch = "wasm32") { | ||
| std::env::var("PYRE_WASM_UNBOXED_APPEND_FOLD").map_or(true, |v| v != "1") | ||
| } else { | ||
| false | ||
| } | ||
| }); | ||
| *DECLINED | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate every reference to the gate and its dependent field to determine
# the intended default and whether tests/CI override it.
rg -n "PYRE_NESTED_LIST_FOLD_VIRT" -C3
rg -n "sub_walk_extra_virtual_roots" -C3
rg -n "nested_list_fold_virt_enabled" -C3Repository: youknowone/pyre
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== gate definition ==\n'
sed -n '5998,6055p' pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
printf '\n== extra roots doc ==\n'
sed -n '3718,3750p' pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
printf '\n== eval test ==\n'
sed -n '10086,10120p' pyre/pyre-jit/src/eval.rs
printf '\n== benchmark header ==\n'
sed -n '1,20p' pyre/bench/synth/nested_list_comprehension_hot.py
printf '\n== all references ==\n'
rg -n "PYRE_NESTED_LIST_FOLD_VIRT|nested_list_fold_virt_enabled|sub_walk_extra_virtual_roots|for_iter_nonempty_nested_list_comprehension_body_declines|nested_list_comprehension_hot" pyre -C 2Repository: youknowone/pyre
Length of output: 12864
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== env override references ==\n'
rg -n "PYRE_NESTED_LIST_FOLD_VIRT=0|PYRE_NESTED_LIST_FOLD_VIRT=1|PYRE_NESTED_LIST_FOLD_VIRT" . -g '!target' -g '!node_modules' -C 2
printf '\n== gate call sites ==\n'
rg -n "nested_list_fold_virt_enabled\(" pyre -C 2
printf '\n== eval test neighbors ==\n'
sed -n '10060,10140p' pyre/pyre-jit/src/eval.rsRepository: youknowone/pyre
Length of output: 12178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== for_iter_bodies_all_jit_safe definition and callers ==\n'
rg -n "fn for_iter_bodies_all_jit_safe|for_iter_bodies_all_jit_safe\(" pyre/pyre-jit/src -C 3
printf '\n== nested_list_fold_virt_enabled callers ==\n'
rg -n "nested_list_fold_virt_enabled\(" pyre/pyre-jit-trace/src pyre/pyre-jit/src -C 3
printf '\n== gate-related shape classification ==\n'
rg -n "UnsupportedJitShape::CurrentFrameOnly|sub_walk_extra_virtual_roots|nested list" pyre/pyre-jit/src/eval.rs pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs -C 3Repository: youknowone/pyre
Length of output: 18264
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '4961,5040p' pyre/pyre-jit/src/eval.rsRepository: youknowone/pyre
Length of output: 4838
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '5040,5125p' pyre/pyre-jit/src/eval.rsRepository: youknowone/pyre
Length of output: 4413
PYRE_NESTED_LIST_FOLD_VIRT's default-on flip leaves the nested-list decline test and docs stale.
nested_list_fold_virt_enabled() now returns true by default, so for_iter_nonempty_nested_list_comprehension_body_declines will assert the opposite behavior unless the test environment forces PYRE_NESTED_LIST_FOLD_VIRT=0. Update that test, the sub_walk_extra_virtual_roots doc, and the benchmark header to the new default, or restore the old default if the decline is still intended.
📍 Affects 3 files
pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs#L6005-L6049(this comment)pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs#L3727-L3741pyre/pyre-jit/src/eval.rs#L10094-L10114pyre/bench/synth/nested_list_comprehension_hot.py#L1-L13
🤖 Prompt for 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.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs` around lines 6005 - 6049,
Keep nested_list_fold_virt_enabled() default-on and update all affected
references: in pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs:6005-6049 retain
the new default; update
pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs:3727-3741 so
for_iter_nonempty_nested_list_comprehension_body_declines expects folding by
default or explicitly forces PYRE_NESTED_LIST_FOLD_VIRT=0; revise the
sub_walk_extra_virtual_roots documentation in
pyre/pyre-jit/src/eval.rs:10094-10114 and the benchmark header in
pyre/bench/synth/nested_list_comprehension_hot.py:1-13 to describe the
default-on behavior and opt-out.
- rustfmt (mod.rs:6346, unroll.rs:956): reflow the extra_virtual_roots snapshot_get / std::mem::take to satisfy the CI rustfmt. - eval.rs test: with the nested-list fold gate default-on, a non-empty nested `BUILD_LIST` element is admitted, so the standalone `for_iter_nonempty_nested_list_comprehension_body_declines` assertion no longer holds. Fold its two shapes into `for_iter_value_producing_list_append_comprehension_body_is_jit_safe` (asserts admit + UnsupportedJitShape::None) and drop the decline test. Assisted-by: Claude
…tra_virtual_roots build break (#708) * comments: remove internal task/slice/gap tracking tags Session-invented tracking labels prefixed comments across 50 files: `task#50 phase-1`, `task #157`, `#73 S3.5`, `gh#73 S3.2`, `Parity #14 Slice C.4`, `Slice 7b`, `Slice C`, `sub-slice 4`, `micro-slice 3`, `gap-10`, `#203 gap-7`, `51d.1`, `M4`, `Phase G slice 2`. Each occurrence is replaced by the technical statement it prefixed; where the tag carried the sentence's subject the sentence is reworded. Dropped in the same lines: two claude memory-file paths cited as references (`item3_abstractstringrepr_epic_plan.md`, `project_issue73_architecture_walker_as_tracer_2026_05_28`), and the line numbers on file citations those comments carried. The `residual_call.rs` "Priority order for sub-slice 2 (widen)" paragraph is removed rather than reworded: it ordered work the same doc records as landed. Comment-only — no changed line is code. `cargo check --workspace --all-targets --features dynasm` passes; the pre-existing `vstack_mirror.rs` unreachable_patterns is the only warning. Assisted-by: Claude * majit-metainterp: initialize Snapshot::extra_virtual_roots at the guard-op multi-frame capture `capture_snapshot_for_last_guard_op_multi_frame_with_vable_vref` built a `recorder::Snapshot` without the `extra_virtual_roots` field, so majit-metainterp failed to compile with E0063 at `origin/main` `57b01d0e8bd`. The field was added by #661; this call site was not updated. Value and comment copied from the sibling `capture_snapshot_for_last_guard_multi_frame_with_vable_vref`, the same multi-frame path: the nested-list append fold resumes through the single-frame collapse, so no extra virtual roots reach here. Assisted-by: Claude * comments: remove AI-review labels, session plan/memory references, and remaining stage tags Second pass over the categories the first sweep did not cover. - `Codex P1` / `Codex P2` (+ `(PR #89)`, `(round 7/8/10)`) AI-review priority labels, 21 sites. The sentence each prefixed is kept. - References to files that exist only in a private session directory: `.claude/plans/*.md` (5), `memory/*.md` (6), and one `project_issue73_...` memory name. - Plan-stage identifiers: `S2.1`/`S2.2`/`S2.3`/`S2.4` "(wiggly-barto plan)", `S1.3`, `S0 spike`, `S1-S3`, `orth-9 step 4`, `Path B (B.6.7*)`, `[SPIKE-S0/FR]`, `A0-era`, `off-GC storage epic S2..S5`, `Task #85`/`#197`/`#333`, `#73 Slice-1`. Three changed lines are not comments, all string literals: the `S1.3` mentions in the `AnnotatorError` text and in an `expect_err` message in `rlib/jit.rs` / `extregistry.rs`, and a `memory/*.md` citation inside an `#[ignore = "..."]` reason string in `pyre-jit/src/eval.rs`. Every other changed line is a comment. `Finding #1`/`#2`/`#3` and `Option C` are kept: they are defined by the `#57` GitHub issue cited alongside them, so a reader can resolve them. `cargo check --workspace --all-targets --features dynasm` passes. Assisted-by: Claude
Summary
Fixes the Object-strategy inlined-comprehension
LIST_APPENDfold (#389 item b): a hotcomprehension whose appended element goes to the list's Object strategy
(
[(i,i) for …],[None …],["s" …],[{i:i} …],[f"{i}" …]) was yielding oneelement short under the JIT. Also fixes two wasm-backend codegen gaps the same trace
exposed.
Root cause (barrier body-effect misclassification)
The #171 orthodox
LIST_APPENDfold descendsw_list_append. Its Object-strategy armstores a GC ref and runs
list_write_barrier, a#[dont_look_inside]Void residual.The FBW body-effect gate treated that Void residual as a live-heap write, so the in-flight
FOR_ITERitem was refuse-dropped — one iteration lost. The Int/Float arms store unboxedscalars with no barrier, which is why only Object-strategy appends dropped.
This mirrors RPython, where the write barrier is
COND_CALL_GC_WB: pyjitpl never executesit (
executor.py:446), the optimizer never treats it as a side effect(
resoperation.py:1124), and it is inserted only in the backend rewrite pass(
rewrite.py:948). It therefore cannot be a trace-level body-effect.Changes
jit: exempt list_write_barrier from FOR_ITER body-effect gateis_list_write_barrier(addr)injit_fnaddr.rsmatches the registered barrier fnaddr.body_effect_candidate,fbw_mark_executed_nonpure_residual, and theexecuted-effect odometer in
try_execute_residual_call_via_executor. The barrier stillexecutes concretely and is still recorded in IR — only the FBW body-effect bookkeeping
is suppressed.
for_iter_bodies_all_jit_safeto admit a call-freeLIST_APPENDbody. Anon-empty nested
BUILD_LISTelement ([[i] for …]) is deliberately declined — thefold virtualizes the inner list but does not thread its backing block into the append
commit sub-walk's guard-exit resume data. Tracked as a follow-up in JIT: hot loops inside a with-block / comprehension never receive a JIT token (no-token cliff behind #373) #389 (see comment).
synth/comprehension_object_append_hot.py.majit-backend-wasm: normalize op positions and resolve NONE fail-argsnormalize_ops_for_codegen(mirroring dynasmprepare_ops_for_compile/cranelift
normalize_ops_for_codegen_simple) and call it incompile_loop/compile_bridge, so every non-Void-result op left unpositioned by the force path gets astamped
OpRefbefore codegen.emit_resolvespilledvalue_types[opref.raw()]for aNONEoperand(
raw() == u32::MAX), panicking OOB when aFinishop carried aNONEfail-arg (a deaddeopt slot). Spill a zero placeholder instead; the blackhole never reads that slot back.
Verification
check.py: dynasm 226/226, cranelift 226/226, wasm 225/225.cargo test -p pyre-jit for_iter17/17;cargo test -p pyre-interpreter is_list_write_barrier.total = 3000000).Assisted-by: Claude
Summary by CodeRabbit
New Features
Bug Fixes