majit: make the retrace path work — resumekey survival, same_greenkey merge points, quasi-immut deps, front-entry split - #960
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR expands raw-store lowering, changes resumed merge-point handling, adds bridge-decline state and diagnostics, preserves optimizer metadata across retraces, stabilizes compiled-loop entry selection, and supports guard-attached retrace compilation. ChangesJIT continuation and lowering
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 3357a2e). 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
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9d5e9ba2e
ℹ️ 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".
| // as the previous segment left it, so the snapshot is taken | ||
| // here rather than once outside — `FrameBox` owns a shadow | ||
| // stack root guard and cannot be cloned. | ||
| let trace_frame = bridge_frame_root.frame().snapshot_for_tracing(); |
There was a problem hiding this comment.
When the first partial-retrace header appends a merge point, the driver immediately invokes this closure again, but this snapshot advances only if take_walk_end_flush_committed() succeeds below. A walk containing an unjournaled residual call or an unresolved frame slot explicitly leaves that flag false, so the next segment starts from the original guard frame while TraceCtx and sym already represent the end of the previous segment. Such bridge retraces can therefore record guards and operations against mismatched concrete values; abort/replay the trace when the end state cannot be committed instead of re-entering with this stale frame.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
| .map(|descr| descr.index()); | ||
| jump_descr.is_some() && jump_descr == label_descr | ||
| }; | ||
| if !closes_on_own_label { |
There was a problem hiding this comment.
When unroll optimization takes its normal jump_to_preamble outcome—such as when short-preamble inlining is disabled, only the preamble target exists, virtual-state matching falls back, or the retrace limit is exhausted—the closing JUMP intentionally targets an earlier label rather than this trace's last LABEL. This check consequently cancels every such retrace, leaving hot polymorphic guards to abandon JIT execution instead of attaching the valid preamble fallback that unroll.py:198-242 and compile.py:392-393 compile upstream; the stated function-per-trace backend limitation must be fixed rather than used to reject that supported path.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majit/majit-macros/src/jit_interp/jitcode_lower/lower_stmt.rs (1)
1825-1836: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject raw-store arguments that are not int bindings.
lower_raw_store_stmtemits its operands withRegister::int, and the existing tests only cover int inputs. If a generated caller or rewritten intrinsics passBindingKind::ReforBindingKind::Float, the lowering will pick the wrong register bank. Add binding-kind checks and negative lowering tests for ref/float raw-store arguments.🤖 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-macros/src/jit_interp/jitcode_lower/lower_stmt.rs` around lines 1825 - 1836, Update lower_raw_store_stmt to validate that the base, effective address, and value arguments have BindingKind::Int before emitting Register::int operands; reject BindingKind::Ref and BindingKind::Float inputs instead of lowering them. Add negative lowering tests covering ref and float raw-store arguments while preserving the existing int-input behavior.Source: Coding guidelines
🤖 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/optimizeopt/shortpreamble.rs`:
- Around line 2690-2704: Filter exported result-side IDs to only those whose
operations were emitted during setup, excluding unresolved ops skipped by the
resolvable check. Update ShortPreambleBuilder’s used_boxes and
short_preamble_jump exports, plus ExtendedShortPreambleBuilder’s
short_preamble_jump and short_jump_args setup/export, so inline replay never
receives IDs absent from the emitted-op mapping.
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 7985-7991: Recompute compiled.front_entry_index unconditionally
after replacing compiled.front_target_tokens in the shown unroll_opt update
block. Remove the None-only guard and match the behavior used by compile_loop
and compile_retrace when publishing a new token list, ensuring the index always
corresponds to the replacement list.
In `@majit/majit-metainterp/src/pyjitpl/dispatch.rs`:
- Around line 4743-4767: Initialize ctx.compiled_key_for_greens_fn whenever a
TraceCtx is created in the trace-start paths, including primary entry, bridge
entry, and retrace entry. Reuse the appropriate compiled-greens key lookup
function for each path so the has_targets check can evaluate the current merge
point’s greens before auto-stamping.
---
Outside diff comments:
In `@majit/majit-macros/src/jit_interp/jitcode_lower/lower_stmt.rs`:
- Around line 1825-1836: Update lower_raw_store_stmt to validate that the base,
effective address, and value arguments have BindingKind::Int before emitting
Register::int operands; reject BindingKind::Ref and BindingKind::Float inputs
instead of lowering them. Add negative lowering tests covering ref and float
raw-store arguments while preserving the existing int-input behavior.
🪄 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 Plus
Run ID: e77cf631-2499-4a4f-8904-1a4fab1c63ac
📒 Files selected for processing (14)
majit/majit-macros/src/jit_interp/jitcode_lower/lower_stmt.rsmajit/majit-macros/src/jit_interp/jitcode_lower/lower_value.rsmajit/majit-macros/src/jit_interp/mod.rsmajit/majit-metainterp/src/compile.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/lib.rsmajit/majit-metainterp/src/optimizeopt/mod.rsmajit/majit-metainterp/src/optimizeopt/shortpreamble.rsmajit/majit-metainterp/src/optimizeopt/unroll.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-metainterp/src/trace_ctx.rspyre/pyre-jit/src/call_jit.rspyre/pyre-wasm-runner/src/main.rs
| // pyjitpl.py:1553-1554 keys `ptoken` on `greenboxes` | ||
| // — the greens of the merge point being visited RIGHT | ||
| // NOW, not the trace's own header. Keyed on the fixed | ||
| // `ctx.green_key` instead, the stamp re-arms at every | ||
| // merge point the trace reaches once the START key has | ||
| // compiled targets, so `reached_loop_header` runs on | ||
| // body merge points that upstream returns from at 1554 | ||
| // without recording anything. Every consequence of that | ||
| // follows: a GUARD_FUTURE_CONDITION per body merge | ||
| // point (upstream emits it only inside | ||
| // reached_loop_header, 2993), a close attempt per body | ||
| // merge point, and — once `retrace_needed` has armed | ||
| // `partial_trace` — a close storm that leaves no room | ||
| // for the one extra iteration a retrace has to trace. | ||
| let has_targets = ctx | ||
| .has_compiled_targets_fn | ||
| .compiled_key_for_greens_fn | ||
| .as_ref() | ||
| .map(|f| f(ctx.green_key)) | ||
| .unwrap_or(false); | ||
| .and_then(|f| { | ||
| f(&( | ||
| mp_green_ints.clone(), | ||
| mp_green_refs.clone(), | ||
| mp_green_floats.clone(), | ||
| )) | ||
| }) | ||
| .is_some(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm compiled_key_for_greens_fn is installed at every trace-start path.
set -euo pipefail
echo "== field definition =="
rg -n 'compiled_key_for_greens_fn' --type=rust -C3
echo "== assignment sites (installs) =="
rg -n 'compiled_key_for_greens_fn\s*=' --type=rust -C5Repository: youknowone/pyre
Length of output: 279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "repo files matching dispatch.rs:"
git ls-files | rg '(^|/)dispatch\.rs$' || true
echo
echo "tracking status/stat:"
git status --short | sed -n '1,40p' || true
git diff --stat || true
echo
echo "locate TraceCtx / related names:"
rg -n 'struct TraceCtx|compiled_key_for_greens_fn|portal_call_depth_fn|has_compiled_targets_fn|TraceCtx\s*\{|fn new\b|new\s*\(' --type=rust -C3 | sed -n '1,220p'Repository: youknowone/pyre
Length of output: 350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file sizes =="
wc -l majit/majit-metainterp/src/pyjitpl/dispatch.rs majit/majit-macros/src/jit_interp/jitcode_lower/dispatch.rs || true
echo
echo "== exact identifiers =="
rg -n 'compiled_key_for_greens_fn|portal_call_depth_fn|TraceCtx|struct TraceCtx|has_compiled_targets_fn' majit/majit-metainterp/src/pyjitpl/dispatch.rs majit/majit-macros/src/jit_interp/jitcode_lower/dispatch.rs -C3 || true
echo
echo "== focused sections =="
sed -n '1,220p' majit/majit-metainterp/src/pyjitpl/dispatch.rs
sed -n '4700,4785p' majit/majit-metainterp/src/pyjitpl/dispatch.rsRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search all tracked files for compiled_KEY identifiers =="
rg -n 'compiled_key_for_greens_fn|compiled.*greens|compiled_key|has_compiled_targets_fn|portal_call_depth_fn' -C2 || true
echo
echo "== Search all tracked files for TraceCtx constructor names =="
rg -n 'impl TraceCtx|struct TraceCtx|TraceCtx\s*::|TraceCtx<' majit --type=rust -C3 | sed -n '1,260p' || true
echo
echo "== Search all tracked files for MetaInterp setup/tracing functions =="
rg -n 'setup_tracing|force_start_tracing|start_bridge_tracing|start_trace|retrace|TraceCtx::new|TraceCtx::for_test' -C3 --iglob '*.rs' | sed -n '1,260p' || trueRepository: youknowone/pyre
Length of output: 213
Wire in compiled_key_for_greens_fn before using it for auto-stamping.
The auto-stamp eligibility now checks ctx.compiled_key_for_greens_fn.as_ref().and_then(...). Assign ctx.compiled_key_for_greens_fn in the trace-start paths that create TraceCtx, including primary entry, bridge entry, and retrace entry, so has_targets can evaluate the current merge point’s greens.
🤖 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-metainterp/src/pyjitpl/dispatch.rs` around lines 4743 - 4767,
Initialize ctx.compiled_key_for_greens_fn whenever a TraceCtx is created in the
trace-start paths, including primary entry, bridge entry, and retrace entry.
Reuse the appropriate compiled-greens key lookup function for each path so the
has_targets check can evaluate the current merge point’s greens before
auto-stamping.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76923209f8
ℹ️ 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 header_pc = ctx.close_header_pc(); | ||
| ctx.add_merge_point(key, green_boxes, header_pc); |
There was a problem hiding this comment.
When a guard bridge resumes at a different PC from the loop header it targets, start_bridge_tracing leaves ctx.header_pc at the guard's resume_pc, while this new path registers the merge point under close_header_pc(). On the next visit the preliminary scan finds that entry, but compile_retrace still calls get_merge_point_at(green_key, ctx.header_pc), so retrace_cut is None; it then optimizes the full recording and appends it to the already-saved partial trace, duplicating the pre-cut bridge prefix and potentially compiling incorrect operations. Preserve the selected header through compile_retrace or perform its lookup using the same close-header key used here.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9948baf069
ℹ️ 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, | ||
| loop_jitcell_token, |
There was a problem hiding this comment.
When a guard bridge from loop A closes into compiled loop B and optimize_bridge requests a retrace, compile_bridge selects B through cell_token_key, but retrace_needed persists only origin A; compile_retrace therefore reloads A's JitCellToken and target list and passes them here. The imported ExportedState was built against B's virtual states, so combining it with A can abort a valid retrace or build and charge the specialization under the wrong loop. Carry B's target key in the partial-retrace state and use it for the optimizer and target registry while retaining A's resumekey solely for attaching the backend artifact to the source guard.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
| // (`compile.py:356`'s `get_procedure_token(greenkey)`), which | ||
| // is what `pyjitpl.py:3923 has_compiled_targets` reads. | ||
| for target_token in &unroll_opt.target_tokens { | ||
| target_token.set_original_jitcell_token_number(source_jct.number); |
There was a problem hiding this comment.
When the source guard belongs to a retired JitCellToken after the same green key has been recompiled, unroll_opt.target_tokens contains clones of the current entry's pre-existing targets as well as the newly emitted target. Because each clone shares its LoopTargetDescr Arc, this assignment rewrites all current labels to claim ownership by the retired source token; later JUMPs to those labels are then classified as cross-token and their keepalive/redirect bookkeeping points at the old token. Upstream propagate_original_jitcell_token changes only LABELs present in the newly compiled trace, so restrict this update to the newly emitted label targets rather than the entire target-token scan list.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majit/majit-metainterp/src/optimizeopt/shortpreamble.rs (1)
2716-2719: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear all exported inline state before returning
false.This branch clears only
shortandshort_results. A prior setup can leaveextra_same_as,short_preamble_jump,used_boxes,short_jump_args, andphase1_to_inputargpopulated. A fallback or reuse path can then consume stale aliases or mapping keys from an inline thatsetupdeclined.Reset these fields to the same base state that the success path establishes before returning
false.Based on library context,
setupmust clear partial inline state when it returnsfalse.Proposed fix
self.short.clear(); self.short_results.clear(); + self.extra_same_as = self.base_extra_same_as.clone(); + self.short_preamble_jump.clear(); + self.used_boxes.clear(); + self.short_jump_args.clear(); + self.phase1_to_inputarg.clear(); self.label_args = label_args.to_vec(); return false;🤖 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-metainterp/src/optimizeopt/shortpreamble.rs` around lines 2716 - 2719, In the failure branch of the setup logic containing self.short.clear(), reset all exported inline state before returning false: extra_same_as, short_preamble_jump, used_boxes, short_jump_args, and phase1_to_inputarg, alongside short and short_results. Match the base-state initialization established by the successful setup path, while preserving the label_args assignment and false return.
🤖 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/jitdriver.rs`:
- Around line 3184-3202: Define the keep-tracing flag contract across both close
paths: at majit/majit-metainterp/src/jitdriver.rs:3184-3202, ensure
take_keep_tracing_after_close() is latched only for the append-only live-context
outcome, or re-check self.meta.is_tracing() before continue for any other
outcome; at majit/majit-metainterp/src/jitdriver.rs:3379-3389, make
CloseLoopWithArgs consume the flag like its CloseLoop counterpart or clear it
before self.sym = None to prevent leakage into the next session.
---
Outside diff comments:
In `@majit/majit-metainterp/src/optimizeopt/shortpreamble.rs`:
- Around line 2716-2719: In the failure branch of the setup logic containing
self.short.clear(), reset all exported inline state before returning false:
extra_same_as, short_preamble_jump, used_boxes, short_jump_args, and
phase1_to_inputarg, alongside short and short_results. Match the base-state
initialization established by the successful setup path, while preserving the
label_args assignment and false return.
🪄 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 Plus
Run ID: 4f5c898a-3738-4355-a44b-515a8dd9dec6
📒 Files selected for processing (3)
majit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/optimizeopt/shortpreamble.rsmajit/majit-metainterp/src/pyjitpl.rs
| if self.meta.take_keep_tracing_after_close() { | ||
| // The walk-final handoff staged at the top of this arm | ||
| // describes a trace that ENDED. Discard it: publishing it | ||
| // would make the `jit_merge_point!` expansion write the | ||
| // walk state into native `state` and resume the guest at | ||
| // the merge-point pc, which is exactly the instruction the | ||
| // next walk segment still has to record. | ||
| self.meta.single_pass_outcome = None; | ||
| self.meta.single_pass_scalar_values = None; | ||
| self.meta.single_pass_virt_array_values = None; | ||
| // pyjitpl.py:1577 `self.pc = saved_pc` — the resumed walk | ||
| // re-enters at the merge point's own guest pc, so the | ||
| // merge-point op must fall through once instead of | ||
| // closing again with nothing recorded in between. | ||
| if let Some(ctx) = self.meta.trace_ctx() { | ||
| ctx.merge_point_resumed = true; | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Define the keep-tracing flag contract across both close arms. The reworked merge-point loop reads take_keep_tracing_after_close() at one site only, and the loop top does not re-check that the session is live. The shared root cause is an undefined contract for which close outcomes may latch the flag and which arms may consume it.
majit/majit-metainterp/src/jitdriver.rs#L3184-L3202: confirm the flag is latched only on the append-only outcome, in which nothing compiled and the ctx stays live; if it can also be latched after thevalidate_closeabort at Line 3173 or after theCompiled/Abortedoutcomes, re-checkself.meta.is_tracing()before thecontinue.majit/majit-metainterp/src/jitdriver.rs#L3379-L3389: consume the flag inCloseLoopWithArgsthe way theCloseLooptwin does, or clear it explicitly beforeself.sym = Noneso it cannot leak into the next session.
📍 Affects 1 file
majit/majit-metainterp/src/jitdriver.rs#L3184-L3202(this comment)majit/majit-metainterp/src/jitdriver.rs#L3379-L3389
🤖 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-metainterp/src/jitdriver.rs` around lines 3184 - 3202, Define the
keep-tracing flag contract across both close paths: at
majit/majit-metainterp/src/jitdriver.rs:3184-3202, ensure
take_keep_tracing_after_close() is latched only for the append-only live-context
outcome, or re-check self.meta.is_tracing() before continue for any other
outcome; at majit/majit-metainterp/src/jitdriver.rs:3379-3389, make
CloseLoopWithArgs consume the flag like its CloseLoop counterpart or clear it
before self.sym = None to prevent leakage into the next session.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c1ec828a5
ℹ️ 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".
| }) | ||
| .unwrap_or(false); | ||
| if position_matches { | ||
| .and_then(|ctx| ctx.get_merge_point_at(ctx.green_key, ctx.close_header_pc())) |
There was a problem hiding this comment.
When a JitDriver has additional scalar or reference greens and a bridge revisits the same guest PC with one of those greens changed, this lookup still returns the previously appended merge point: ctx.green_key remains the trace's (code, pc) key and close_header_pc() extracts only the first integer green, although ctx.close_greens contains the complete tuple. The retrace is then compiled from an exported state belonging to a different green configuration, rather than taking upstream's no-same_greenkey append path, which can attach a specialization with assumptions from the wrong state. Compare the complete close-green tuple when selecting the merge point.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbd2ac490c
ℹ️ 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".
| state.short_box_producer_roots = short_box_producer_roots; | ||
| // unroll.py:234: carry deps collected while optimizing the exported | ||
| // preamble state so compile.py:384-390 can merge them on retrace. | ||
| state.quasi_immutable_deps = optimizer.quasi_immutable_deps.clone(); |
There was a problem hiding this comment.
When a bridge requests a retrace and GC moves a dependency object before the next iteration compiles, this stores its address only as a raw u64. ExportedState::root_all_gcrefs and walk_const_ptr_refs_mut do not visit quasi_immutable_deps, so refresh_from_gc leaves that copy stale even though other ConstPtr copies are updated. compile_retrace later publishes the stale address through last_quasi_immutable_deps, and register_quasi_immutable_deps unsafely casts it back to a module strategy or type object, which can miss invalidation or access invalid memory. Store the dependency as a traced GcRef and include it in the exported-state root/refresh walk.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c8018fa42
ℹ️ 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".
| // has to keep it — and it must re-enter the walk rather than | ||
| // let the native dispatch loop run the merge point's own | ||
| // instruction unrecorded (see the loop header above). | ||
| if self.meta.take_keep_tracing_after_close() { |
There was a problem hiding this comment.
When a production Pyre guard bridge requests a retrace, its first header revisit is emitted as TraceAction::CloseLoopWithArgs (pyre/pyre-jit-trace/src/trace.rs:5014-5035), and compile_loop registers the new merge point and returns Cancelled. This continuation check exists only in the preceding bare CloseLoop arm; the CloseLoopWithArgs arm instead reaches its unconditional self.sym = None, so the next merge point aborts rather than recording the required extra iteration and compiling the retrace. Fresh evidence relative to the earlier re-entry discussion is that the production mapper explicitly constructs CloseLoopWithArgs; share this keep-tracing handling with that arm.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
majit/majit-macros/src/jit_interp/jitcode_lower/lower_stmt.rs (1)
1803-1840: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate all raw-store operand kinds before emitting
RawStore.
lower_value_exprcan return a Ref or Float binding. Lines 1833-1835 force every binding into the Int register bank. The generatedraw_store_ican then read an unrelated Int register value and differ from interpreter execution.Require
BindingKind::Intforbase,ea, andval. ReturnNonewhen an operand has another kind. Add a regression test that verifies Ref and Float operands do not lower.Proposed fix
let base = self.lower_value_expr(&call.args[0])?; let ea = self.lower_value_expr(&call.args[1])?; let val = self.lower_value_expr(&call.args[2])?; + if !matches!( + (base.kind, ea.kind, val.kind), + (BindingKind::Int, BindingKind::Int, BindingKind::Int) + ) { + return None; + } let (base_reg, ea_reg, val_reg) = (base.reg, ea.reg, val.reg);As per coding guidelines, “The generated JIT must preserve interpreter semantics.”
🤖 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-macros/src/jit_interp/jitcode_lower/lower_stmt.rs` around lines 1803 - 1840, Update lower_raw_store_stmt to validate that the bindings returned for base, ea, and val are all BindingKind::Int before constructing RawStore; return None if any operand is a Ref or Float binding, and only then extract their integer registers. Add a regression test confirming raw-store calls with Ref and Float operands are rejected and do not lower.Source: Coding guidelines
majit/majit-metainterp/src/trace_ctx.rs (1)
2691-2720: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftTrack
virtualizable_heap_ptrforwarding separately from the identity slot.
standard_virtualizable_ptr()documents a reachable divergence:virtualizable_heap_ptris the synchronization target, while the root-portal-seed case bakes that pointer at asnapshot_for_tracingcopy and keeps the identity alive on the compiled-frame object.walk_virtualizable_value_refs()currently overwritesvirtualizable_heap_ptrwith the trailing identity Ref on every GC walk, so synchronizing virtualizable box writes can target the identity/live frame object instead of the synchronization target. Track the identity’s forwarding in a separate field and update only the sync target when the identity itself changes.🤖 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-metainterp/src/trace_ctx.rs` around lines 2691 - 2720, Update walk_virtualizable_value_refs and the surrounding trace context state so forwarding of the trailing identity Ref is tracked separately from virtualizable_heap_ptr. Preserve virtualizable_heap_ptr as the synchronization target used by standard_virtualizable_ptr, and only update that target when its own forwarding changes rather than assigning the identity’s address on every GC walk.
♻️ Duplicate comments (1)
majit/majit-metainterp/src/pyjitpl.rs (1)
8035-8041: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecompute
front_entry_indexwhenfront_target_tokensis replaced.Line 8036 replaces the whole token list. Line 8037 only fills
front_entry_indexwhen it isNone, so a retrace attached to a source guard keeps the index computed for the previous list.Two failures follow:
- The stale index can address a preamble target in the new list.
selected_front_target_tokenthen names the preamble LABEL as the Cranelift front door.- The stale index can exceed the new list length.
selected_front_target_tokenreturnsNone, sofront_target_dispatch_keyandfront_target_inputarg_typesfall back and the host loses the direct LABEL entry.
compile_loop_body(Line 6675) andcompile_retrace(Line 7821) recompute the index whenever they publish a new token list. Match that here.🐛 Proposed fix
if !unroll_opt.target_tokens.is_empty() { compiled.front_target_tokens = unroll_opt.target_tokens.clone(); - if compiled.front_entry_index.is_none() { - compiled.front_entry_index = - Self::front_entry_index_for(&compiled.front_target_tokens); - } + compiled.front_entry_index = + Self::front_entry_index_for(&compiled.front_target_tokens); }🤖 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-metainterp/src/pyjitpl.rs` around lines 8035 - 8041, In the token-list replacement block of the retrace flow, update front_entry_index unconditionally after assigning compiled.front_target_tokens, rather than only when it is None. Match the recomputation behavior used by compile_loop_body and compile_retrace, ensuring the index is derived from the newly cloned target-token list.
🤖 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/optimizeopt/mod.rs`:
- Around line 2133-2136: Update the input-argument resolution branch around
inputarg_refs so it validates ia.tp against self.inputarg_type(opref).or_else(||
opref.ty()) in addition to matching the index; return the bound operand only
when both checks pass, otherwise return None. Add a regression test covering a
Ref input at index 0 with an Int placeholder.
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 9155-9160: In the unit-test section for pyjitpl, add coverage for
front_entry_index_for covering a mixed list (returns the first non-preamble
index), an all-preamble non-empty list (returns index 0), and an empty list
(returns None). Construct the smallest TargetToken fixtures needed and assert
each expected result.
- Around line 6837-6866: Handle the failed registration in
register_retrace_merge_point instead of silently returning when any jump_args
element lacks a type. Propagate a declined-registration result to the caller at
the retrace close path so it clears partial_trace and retracing_from before
returning CompileOutcome::Cancelled; preserve successful merge-point
registration and keep_tracing_after_close behavior.
- Around line 1289-1296: Reset keep_tracing_after_close during trace teardown so
its value cannot leak into later compilations. Update the relevant
abort_trace_live, clear_trace_session, and finish_trace_live paths, or consume
and clear the flag before the driver closes a trace, while preserving the
existing compile_loop_body and Cancelled handling.
In `@majit/majit-metainterp/src/trace_ctx.rs`:
- Around line 2169-2211: Call TraceCtx::adopt_normalized_virtualizable_elements
from the normalization flow described in its doc comment, immediately after
remove_consts_and_duplicates updates the element block and before merge-point
registration or the closing JUMP consumes it. Pass the normalized elements slice
covering virtualizable_boxes[..virtualizable_boxes.len() - 1], ensuring the
helper’s length assertion is satisfied and later readers use the updated boxes.
---
Outside diff comments:
In `@majit/majit-macros/src/jit_interp/jitcode_lower/lower_stmt.rs`:
- Around line 1803-1840: Update lower_raw_store_stmt to validate that the
bindings returned for base, ea, and val are all BindingKind::Int before
constructing RawStore; return None if any operand is a Ref or Float binding, and
only then extract their integer registers. Add a regression test confirming
raw-store calls with Ref and Float operands are rejected and do not lower.
In `@majit/majit-metainterp/src/trace_ctx.rs`:
- Around line 2691-2720: Update walk_virtualizable_value_refs and the
surrounding trace context state so forwarding of the trailing identity Ref is
tracked separately from virtualizable_heap_ptr. Preserve virtualizable_heap_ptr
as the synchronization target used by standard_virtualizable_ptr, and only
update that target when its own forwarding changes rather than assigning the
identity’s address on every GC walk.
---
Duplicate comments:
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 8035-8041: In the token-list replacement block of the retrace
flow, update front_entry_index unconditionally after assigning
compiled.front_target_tokens, rather than only when it is None. Match the
recomputation behavior used by compile_loop_body and compile_retrace, ensuring
the index is derived from the newly cloned target-token list.
🪄 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 Plus
Run ID: 5fb945fa-d0dc-4efb-b087-bab66ca59b5b
📒 Files selected for processing (14)
majit/majit-macros/src/jit_interp/jitcode_lower/lower_stmt.rsmajit/majit-macros/src/jit_interp/jitcode_lower/lower_value.rsmajit/majit-macros/src/jit_interp/mod.rsmajit/majit-metainterp/src/compile.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/lib.rsmajit/majit-metainterp/src/optimizeopt/mod.rsmajit/majit-metainterp/src/optimizeopt/shortpreamble.rsmajit/majit-metainterp/src/optimizeopt/unroll.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-metainterp/src/trace_ctx.rspyre/pyre-jit/src/call_jit.rspyre/pyre-wasm-runner/src/main.rs
| if let Some(ia) = self.inputarg_refs.get(idx) { | ||
| return Some(Operand::from_bound_inputarg(ia)); | ||
| if ia.index as usize == idx { | ||
| return Some(Operand::from_bound_inputarg(ia)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Reject input-argument bindings with the wrong type.
The index check is insufficient. InputArg::new_int(0) passes for OpRef::input_arg_ref(0) because both indices are 0. The resolver can then return an Int operand for a Ref position.
Compare ia.tp with self.inputarg_type(opref).or_else(|| opref.ty()) before returning. Otherwise return None.
Proposed fix
+ let expected_type = self.inputarg_type(opref).or_else(|| opref.ty());
if let Some(ia) = self.inputarg_refs.get(idx) {
- if ia.index as usize == idx {
+ if ia.index as usize == idx && expected_type == Some(ia.tp) {
return Some(Operand::from_bound_inputarg(ia));
}
}Add a regression test for a Ref input at index 0 with an Int placeholder.
As per coding guidelines, the generated JIT must preserve interpreter semantics.
📝 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.
| if let Some(ia) = self.inputarg_refs.get(idx) { | |
| return Some(Operand::from_bound_inputarg(ia)); | |
| if ia.index as usize == idx { | |
| return Some(Operand::from_bound_inputarg(ia)); | |
| } | |
| let expected_type = self.inputarg_type(opref).or_else(|| opref.ty()); | |
| if let Some(ia) = self.inputarg_refs.get(idx) { | |
| if ia.index as usize == idx && expected_type == Some(ia.tp) { | |
| return Some(Operand::from_bound_inputarg(ia)); | |
| } |
🤖 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-metainterp/src/optimizeopt/mod.rs` around lines 2133 - 2136,
Update the input-argument resolution branch around inputarg_refs so it validates
ia.tp against self.inputarg_type(opref).or_else(|| opref.ty()) in addition to
matching the index; return the bound operand only when both checks pass,
otherwise return None. Add a regression test covering a Ref input at index 0
with an Int placeholder.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majit/majit-metainterp/src/pyjitpl.rs (1)
7609-7615: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestrict foreign-target rejection to the backend that cannot compile it.
Lines 7609-7615 reject every retrace whose closing
JUMPtargets an existing preamble label. The preceding code states that this shape is valid upstream and that Cranelift resolves it correctly as an external exit. The unconditional return disables valid Cranelift retraces.Gate this decline to the affected backend, or make wasm resolve JUMPs by descriptor before allowing the shared path.
🤖 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-metainterp/src/pyjitpl.rs` around lines 7609 - 7615, Update the retrace validation around closes_on_own_label so foreign-target rejection applies only to the backend that cannot compile external exits, rather than unconditionally returning false. Preserve acceptance of existing preamble-label targets for Cranelift, or resolve those JUMPs by descriptor in the wasm-specific path before reaching this shared check.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 7609-7615: Update the retrace validation around
closes_on_own_label so foreign-target rejection applies only to the backend that
cannot compile external exits, rather than unconditionally returning false.
Preserve acceptance of existing preamble-label targets for Cranelift, or resolve
those JUMPs by descriptor in the wasm-specific path before reaching this shared
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6e456ac7-2786-4664-9022-b7b0b9136258
📒 Files selected for processing (1)
majit/majit-metainterp/src/pyjitpl.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73aa96b84f
ℹ️ 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".
| // procedure token this retrace specializes, the same one | ||
| // `unroll_opt.retraced_count` was seeded from. | ||
| loop_jitcell_token.set_retraced_count(unroll_opt.retraced_count); | ||
| self.last_quasi_immutable_deps = quasi_immutable_deps; |
There was a problem hiding this comment.
When a guard-originated retrace compiles successfully but the current bridge walk cannot commit its end state, trace_and_compile_from_bridge returns BridgeResolution::ResumeBlackhole even though the backend artifact is already attached. The general guard caller registers last_quasi_immutable_deps for the other successful resolutions but leaves ResumeBlackhole empty (pyre/pyre-jit/src/eval.rs:7946-7984), and the CALL_ASSEMBLER callers do not register them either. Thus the dependencies stored here can remain unregistered, allowing a later module/type mutation to execute the retrace with stale optimized constants; register them as soon as the artifact is attached rather than tying registration to how the current failed iteration resumes.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 765d0d7cc8
ℹ️ 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".
| self.compiled_loops.insert( | ||
| green_key, |
There was a problem hiding this comment.
When compile_retrace has no guard resumekey (the ResumeFromInterpDescr entry-bridge path), this installs a new front-door loop and returns success but never assigns last_compiled_key, which was reset at trace start. JitDriver::compile_and_record_loop records loop_header_greens and single_pass_compiled_key only when last_compiled_key() is Some (jitdriver.rs:2447-2468), so this retraced entry is omitted from compiled_key_for_greens; subsequent guards reaching the same header are then treated as having no compiled target and cannot close into it. Set last_compiled_key here as the normal successful compile path does at pyjitpl.rs:6744.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
199c2d0 to
6467172
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64671720ef
ℹ️ 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 fvc = self.active_frame_value_count_fn(); | ||
| if let Some(compiled) = self.compiled_loops.get_mut(&green_key) { |
There was a problem hiding this comment.
When a guard in loop A closes into loop B, green_key here is B's retargeted key, while record_loop_or_bridge(&source_jct, ...) stamps every internal guard with A's ownership. Even though the target token now correctly follows B, inserting the resulting trace and exit layouts into compiled_loops[B] makes descriptor-based readers—which resolve through A's source_jct.green_key()—miss bridge_trace_id, so a subsequent failure inside the retrace has no resume metadata. Store the artifact metadata under the source loop while retaining B only for target-token and retrace-count bookkeeping.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
| op.opcode | ||
| ); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
When this path skips a pure producer with an unresolvable Phase-1 argument, it also leaves the producer absent from short_results; a later force_box can therefore make ExtendedShortPreambleBuilder::use_box re-add that producer using its weaker, non-recursive dependency check. Replay then encounters the still-unmapped argument and signals InvalidLoop, after which jump_to_existing_trace persists the polluted builder back onto the target token before observing the failure, causing later bridges to repeat the same invalid inline instead of retaining the valid filtered preamble. Track skipped producers or apply the same resolvability test in use_box.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
32afb89 to
027580c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 027580c73a
ℹ️ 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".
| | Self::UnsupportedOpname { .. } | ||
| | Self::OrthodoxSubWalkTraceUnsupported { .. } | ||
| | Self::UnfoldableListAppendResidualUnsupported { .. } | ||
| | Self::MayForceNullRefArgUnsupported { .. } |
There was a problem hiding this comment.
When a CALL_MAY_FORCE argument has been stamped with a stale raw NULL, check_may_force_null_ref_args raises MayForceNullRefArgUnsupported specifically because the real live Ref is unavailable; the bridge-seeding code at state.rs:9840-9847 and state.rs:10031-10042 explicitly identifies this as an unmaterialized hole. Classifying that error as a complete image lets walk() latch it and the default WalkAbort path restart the same residual call in the blackhole with the NULL bank value, rather than taking the legacy replay that reconstructs the live argument, which can dereference NULL or execute the call with the wrong operand. Keep this error out of the allow-list unless the actual live Ref is recovered first.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
…name
`lower_raw_store_stmt` accepted only `majit_raw_store_i64` and minted
`add_raw_int_array_descr(8)`, which is unsigned. It now recognizes the
full `majit_raw_store_{i,u}{8,16,32,64}` table and mints
`add_raw_int_array_descr_signed(item_size, is_signed)`, mirroring the
load-side table at `lower_value.rs:471-505`.
`jtransform.py:1156-1163 rewrite_op_raw_store` builds the descr from the
stored value's own type, so width and sign come off the descr.
All three backends and the blackhole already read `item_size` from the
descr (`compiler.rs:12822`, `aarch64/assembler.rs:2900`,
`codegen.rs:2739`, `runner.rs:3363`); the store path does not read
signedness, so the i64 arm changing from unsigned to signed writes the
same bytes.
Assisted-by: Claude
…boxes once `start_retrace_from_guard` minted the bridge recorder's inputargs from types alone, and `compile_bridge_trace` rebuilt them the same way, so their concrete value slots were empty. resume.py:1267-1282 `load_box_from_cpu` constructs the frontend box as `IntFrontendOp(num, cpu.get_int_value(deadframe, num))`; `closing_jump_runtime_boxes` reads those values to build `runtime_boxes`, which virtualstate.py:493-498 consults. Stamp both sites from `fail_values` / `recorder.inputargs()`. `reached_loop_header` (pyjitpl.py:2974-2989) builds `live_arg_boxes` once and hands the same list to the bridge JUMP, the merge-point scan and `current_merge_points.append`. The CloseLoop arm built and normalized it twice; `remove_consts_and_duplicates` records a SAME_AS per rewrite, so the bridge fall-through path appended a second set after `compile_trace` had sampled `potential_retrace_position`. Hoist the construction above the bridge branch. `compile_retrace` never set `Optimizer::trace_inputargs`, so the Phase 2 TraceIterator seeded an empty cache (opencoder.py:264-267) and `_get` panicked on the first body operand referring to a cut inputarg. `resolve_to_operand` returned `inputarg_refs[idx]` without checking that the slot belongs to `idx`; slots grown by `resize_with(.., new_int(0))` hold an index-0 Int placeholder, so an out-of-range Ref position resolved to an Int box at position 0. Records in `compile_loop_body` why the merge-point scan stays a two-way test: after a cancelled close the walk emits a loop-close action at every pc it steps to, so upstream's third branch (append and keep tracing) does not terminate. pyre/check.py: dynasm 352/352, cranelift 352/352, wasm 348/348. Assisted-by: Claude
… port the partial-trace gate and the merge-point append pyjitpl.py:1553-1554 reads `ptoken = self.get_procedure_token(greenboxes)` where `greenboxes` are the greens of the merge point being visited. `dispatch.rs` keyed that lookup on the fixed `ctx.green_key` instead, so once the trace-start key had compiled targets the stamp re-armed at every merge point the trace reached and `reached_loop_header` ran on body merge points that upstream returns from at 1554. pyjitpl.py:3003 `if not self.partial_trace:` gates the bridge attempt; `jitdriver.rs` computed the value into `_has_partial_trace` and discarded it. pyjitpl.py:3018-3060 has three outcomes, not two: a same-greenkey merge point at `retracing_from` retraces, one elsewhere aborts, and NO same-greenkey entry falls through to `current_merge_points.append` with the history still live. `compile_loop_body` collapsed that to a two-way test and compiled the retrace in the same call, so the retrace body was `Label; Jump`. It now registers the merge point through `register_retrace_merge_point` and returns `Cancelled`; `MetaInterp::keep_tracing_after_close` tells the driver to keep `self.sym` so the walk records the next iteration, and the retrace closes on the following header visit. With CEL_RETRACE_LIMIT=5 on the cel poison reproducer the close census goes from one CloseLoop per element to 8 at pc=45 plus 1 at pc=27, and `compile_retrace` cuts a 28-op body instead of an empty one. Assisted-by: Claude
…whole inline shortpreamble.py:128-139 `PureOp.add_op_to_short` (and its HeapOp / LoopInvariantOp siblings) returns None as soon as one arg's `produce_arg` returns None, and `add_op_to_short` (:311-341) then leaves THAT ONE op out of `produced_short_boxes`. Anything that depended on it drops in turn because its own `produce_arg` now misses. Upstream therefore reaches the inline with the filtering already done, which is why `ExtendedShortPreambleBuilder.setup` (:460-463) is three field assignments and cannot fail. `ExtendedShortPreambleBuilder::setup` cleared `short`/`short_results` and returned false on the first unresolvable arg, and the caller turned that into `invalid_loop`. Pyre's short boxes are exported against one label-arg set and replayed against another — a retrace runs no Phase 1 and imports the partial trace's exported state — so an arg that was produce-able at export can be unresolvable at setup. Skip that op and keep going; `short_results` already carries the transitive drop. On the cel poison reproducer with CEL_RETRACE_LIMIT=5 exactly one op is dropped (IntAdd at IntOp(241), whose arg InputArgInt(222) is not a body label arg), `jump_to_existing_trace` reports `jumped=true, invalid_loop=false` instead of falling back to jump_to_preamble, and the compiled retrace terminates instead of looping forever. Assisted-by: Claude
…handing the guest back pyjitpl.py:1570-1578 calls `reached_loop_header` with `self.pc = orgpc` and, when it returns without raising (the `current_merge_points.append` path at :3058-3060), restores `self.pc = saved_pc` and keeps interpreting under the recorder. This driver returned from `merge_point` instead, and the `jit_merge_point!` expansion's `#pc = __sp_pc; continue;` handed the guest to the native dispatch loop, which executed the merge point's own instruction unrecorded. `merge_point` now loops on that path: it discards the walk-final handoff staged at the top of the CloseLoop arm, sets `TraceCtx::merge_point_resumed`, and re-enters the walk. The flag is read-and-clear; the dispatch merge-point arm consumes it and falls through once, and the macro wrapper's header-revisit fast path peeks at it so the resumed segment records the instruction rather than closing again with nothing between. `merge_point` and its delegate `jit_merge_point_keyed` take `FnMut`. pyre's bridge closure moved a non-Clone `FrameBox` in, so it snapshots the rooted frame per entry. Assisted-by: Claude
…race_inner The comment said the values on the recorder's inputargs are the ones `start_retrace_from_guard` seeded from the deadframe. That seeding was removed in 5c1ec82, so the citation names a channel that no longer exists. The values that reach this copy are written by the frontend through `TraceCtx::try_set_opref_concrete`, once per box the resume rebuild resurrects. Also record what `compile_bridge`'s `PendingBridgeRd` zip actually is: `prd.liveboxes` is built from every bridge inputarg (pyjitpl.rs:11581) and zipped positionally with the raw `fail_values`, so it is not the live-filtered form `bridgeopt.py:126` is cited for -- that line is a length assert. It also runs after `closing_jump_runtime_boxes` reads the inputargs, which is why the removed trace-start stamp changed `runtime_boxes` and this one does not. Comment only; no code change. Assisted-by: Claude
…instead of assembling an uncut trace
`compile_retrace` re-derives the merge point with
`ctx.get_merge_point_at(green_key, ctx.header_pc)` and, on a miss, fell through
to `else { trace }` and assembled the trace uncut. `combined_ops` still prepends
`partial.ops`, which covers [0, retracing_from), to a body optimized from
position 0, and `root_inputargs` still comes from `partial.inputargs` while the
body references the recorder namespace.
compile.py:347 `trace = metainterp.history.trace.cut_trace_from(start,
inputargs)` is unconditional: `start` is read once at the caller's single
merge-point selection (pyjitpl.py:3019) and handed straight down, so there is no
upstream "merge point not found" state for the fallback to mirror.
Nothing downstream rejects the uncut shape.
`normalize_root_loop_entry_contract` (pyjitpl.rs:747-796) compares LABEL against
JUMP arity within the same optimized output -- both minted by the same peel --
and returns `root_inputargs` untouched. The closing-JUMP cancel
(`closes_on_own_label`, pyjitpl.rs:7581) rejects only foreign-target closes,
which a self-close is not. 65aaf8d records a wrong-cut retrace passing both
gates, attaching, and executing with divergent checksums.
The caller reads `false` as pyjitpl.py:3004 "creation of the loop was
cancelled", which is the outcome it already handles for every other decline in
this function.
The re-lookup itself should not exist; the caller selected this merge point
under `close_header_pc()` and this reads `header_pc`. Threading the selected
merge point down is tracked separately, as is the `mp.position._pos > 0`
conjunct, which has no upstream counterpart -- pyjitpl.py:2902 seeds
`current_merge_points = [(original_boxes, (0, 0, 0, 0, 0))]`, so position 0 is a
valid selection upstream.
cargo test -p majit-metainterp --release: 1515 passed, 0 failed.
pyre/check.py --backend dynasm: failure set byte-identical to the parent commit
(16 entries). `retracing_from` is armed only by `retrace_needed`, gated on
`retrace_limit = 0` (rlib/jit.py:595), so pyre does not reach this path.
Assisted-by: Claude
`compile_retrace` consumes `self.tracing`, so every `false` it returns after
that point reaches the caller's `if self.tracing.is_none()` arm. That arm
returned `CompileOutcome::Aborted` without touching the session, unlike the two
sibling abort arms beside it which run `clear_retrace_state` +
`warm_state.abort_tracing` + `clear_trace_session`.
The driver's `Aborted` arm calls `abort_trace(false)`, and `abort_trace_live`'s
whole body sits inside `if let Some(ctx) = self.tracing.take()` — already `None`
here, so all of it is skipped and nothing calls `clear_trace_session`. That
leaves `active_trace_session` and `bridge_info` set (pyjitpl.rs:2992-2993 are
their only clear sites) and `profiler_tracing_active` true, because
`leave_profiler_tracing` runs only from `clear_trace_session`. The next trace
start calls `enter_profiler_tracing`, whose `assert!` on that flag is a release
assert, so the process aborts on a later trace rather than here.
The arm now mirrors its siblings. `green_key` is captured at the call site
before `compile_retrace` takes the ctx.
`retracing_from` is also now required rather than degraded: `compile.py:341-347`
takes `start` as a parameter and has no `compile_retrace` without one, so the
`else { trace }` arm is unreachable and can be deleted when the merge-point
selection is threaded down from the caller.
The arm predates this branch; the preceding commit made it the standard exit for
a merge-point re-lookup miss.
cargo test -p majit-metainterp --release: 1515 passed, 0 failed.
pyre/check.py --backend dynasm: failure set byte-identical to the parent commit
(16 entries).
Assisted-by: Claude
…_boxes The write-back skipped `adopt_normalized_virtualizable_elements` whenever the element count exceeded `live_arg_boxes.len()`. Upstream has no such state: `pyjitpl.py:2985-2988` normalizes `self.virtualizable_boxes` in place and then appends it, so every later reader sees the rewrite. Skipping leaves the trace ctx's copy unnormalized, and a later cut mints a LABEL whose collapsed slots are missing -- the artifact 7e305fc added this write-back to prevent. `adopt_normalized_virtualizable_elements` (trace_ctx.rs:2203) already asserts the same invariant from the other side. The invariant holds by construction: `collect_jump_args_with_boxes` appends the element block to the loop-carried list, and `remove_consts_and_duplicates_untyped` takes `&mut [OpRef]`, so it substitutes SameAs ops in place and cannot shorten it -- matching `pyjitpl.py:2958-2964`, which assigns `boxes[i]` rather than removing. The `n > 0` short-circuit is kept: it skips a call that would copy zero elements. cargo test -p majit-metainterp --release: 1515 passed, 0 failed. pyre/check.py --backend dynasm: the assert does not fire on any of the 366 fixtures, and the failure set is byte-identical to the parent commit (16 entries). Assisted-by: Claude
…f mirror `capture_frame_stack_for_publish` reads `locals_cells_stack_w`, which a root walk never writes: `setarrayitem_vable_via_metainterp` mirrors into a concrete frame only through `current_inline_vable_target`, i.e. only for a frame an INLINE level owns. Measured at the single adopt in `list_length_hint_validate`, the slot array reads `[0]` while the walker's mirror holds the live operand. Resolve the stack from `WalkContext::vstack_boxes` instead (`capture_frame_stack_from_mirror`), keyed on `vstack_cur_pypc`. Convert a walk abort into a blackhole resume only for the classes whose MIFrame image is complete (`DispatchError::leaves_complete_image`). Every RPython `SwitchToBlackhole` is a stop-tracing decision taken with the registers bound, which is why `_copy_data_from_miframe` (`blackhole.py:1713-1730`) copies the banks unconditionally and has no failing path. The walker resolves registers lazily against the live trace context, so a second family exists where the abort IS the report that a value is unavailable (`RegisterReadUnbound`, `*NotConcrete`); converting one resumes on the hole the walker refused to read, and broke `list_length_hint_validate` and `raise_reg_unbound_jitstress`. `WalkEndCommitLeg::WalkAbort` carries the leg and `PYRE_WALKABORT_OFF` disables it. 353 synth fixtures produce identical output with the leg on and off. Assisted-by: Claude
…side for the qmut leg The walk-abort mirror stack is resolved to concrete refs when the abort latches and then held in a plain `Vec` until the walk-end adopter publishes it. That is the same collector-invisible window as the latched MIFrame Ref bank beside it, which `fbw_store_journal_walk_roots` already forwards, so forward the mirror slots there too. `flush_with_latched_stack` does not need this because it roots its stack (`push_resume_ref_roots`) for the duration of one flush call; this image outlives the call that builds it. `ForceQuasiImmutable` is the counterpart of `pyjitpl.py:1116` `raise SwitchToBlackhole(Counters.ABORT_FORCE_QUASIIMMUT)`, taken with the registers bound after `do_force_quasi_immutable` ran, so classify it in `leaves_complete_image`. Exclude it at the general adopt site instead: it owns `flush_qmut_abort_state`, which resumes AT the forcing opcode rather than finishing the frame past it. Assisted-by: Claude
…against the current corpus `cargo fmt --all -- --check` reported the `publishes_root_stack` binding. The gate comment cited `raise_reg_unbound_jitstress`, which no longer reproduces: #973 refuses the frame when a live color has no concrete, and that covers the failure the comment described. Re-measured with the classification removed, 351 of 353 synth fixtures are unchanged and two break — `list_length_hint_validate` (its one `RegisterReadUnbound` walk, pc=456 reg=3 bank=r, underflows the operand stack) and `getframe_while_subwalk_decline_shapes` (an inline-escape shape whose caller banks are incomplete, dying on an unwired blackhole opcode; the fixture exists to pin that decline). Assisted-by: Claude
…sh Ref The catch-landing exception-value push allocated a fresh Ref Variable with no defining op. Regalloc treats a producerless Ref as dead-until-first-use and coalesces it onto a body colour (a local live across the `try`). On a guard-failure bridge that re-walks the handler, that body colour's def sits in the skipped prefix, so the pushed value is read unbound (RegisterReadUnbound), aborting the walk. Reuse the landing's own last_exception value Variable (the edge pair set by exception_landing_state, whose colour generate_last_exc writes) as the pushed value. It is re-derived on any bridge and interferes with the surrounding live ranges, so it earns a distinct colour. Same fix as the PUSH_EXC_INFO last_exc_value re-read. Restores list_length_hint_validate to its recorded jit-stats baseline (loops_aborted 34->14, guard_failures 4923->828); raise_reg_unbound_jitstress unchanged. dynasm synthetic gate: 352 passed, only exception_args_virtual red. Assisted-by: Claude
`compile_bridge_trace` copied each recorder inputarg's concrete value onto the freshly minted `InputArg`. The channel that writes those values is not resume-walked: pyre's bridge setup stamps `try_set_opref_concrete` over every Ref input arg positionally, `Value::Ref(0)` included, while resume.py:1267-1282 `load_box_from_cpu` constructs a box only for a live resume entry. A dead Ref slot therefore reached `closing_jump_runtime_boxes` as a constant NULL and the NULL folded into the trace as an operand: the dynasm backend emitted `mov x0, #0` for the `jit_next` FOR_ITER residual, so `baseobjspace::next` dereferenced NULL. Measured on dynasm / macOS aarch64 with `lib-python/3/test/test_strftime.py` under `MAJIT_STRICT=1 MAJIT_STATS=1`: 3/27 runs SIGSEGV with the copy, 1/120 without it, `loops_compiled` 21 and `bridges_compiled` 21 -> 20. pyre/check.py: dynasm 370/370, cranelift 370/370, wasm 365/366 (the remaining `synth/pickle_terminal_raise_resume` `loops_aborted 54 -> 59` failure is unchanged from before this commit). Assisted-by: Claude
…ch moved `check.py --snapshot-diff` reported 53 rows, all `jit-stats diff` and none `snapshot output diff`, so no fixture's output changed. Recorded with `check.py --snapshot`; only `.jitstats` files moved. Direction of the 109 field moves, against the gate groups in check.py: 93 in the improving direction, 0 rises in a badness field, 0 falls in `loops_compiled`, and 16 rises in `guard_failures` — all inside the `base + max(base // 4, 2)` band the field is gated on. Twelve of the sixteen `guard_failures` rises accompany a `loops_compiled` rise or a `loops_aborted` fall in the same fixture, i.e. more of the frame compiles so more guards exist to fail: `attr_cache_invalidation` 802 -> 1002 with `loops_compiled` 1 -> 2, `exception_reentry_guard_finally_residual` 2044 -> 2261 with `loops_aborted` 5 -> 0 and `loops_compiled` 2 -> 3, `goto_if_not_same_box` 0 -> 2 with `loops_compiled` 0 -> 2, `blackhole_inlined_callee_local_after_escape` 0 -> 1 with `loops_aborted` 10 -> 5. The remaining four are +1 or +2 on cranelift. `loops_aborted` summed over the 53 recorded files: 120 -> 56. The wasm baselines are unchanged; main re-recorded them in #996. Assisted-by: Claude
…idual call The six compare/bitwise specializations boxed their result with `emit_trace_bool_value_from_truth`, which records a `CallR(jit_bool_value_from_truth)` carrying `EffectInfo::new(CannotRaise, None)`. That effect set is not pure, so no optimizer pass removes the call even when the box is dead; on `pyre/bench/raise_catch_loop.py` two survived per iteration. `baseobjspace.py:896-900 newbool` returns one of the prebuilt `boolobject.py:79-80` singletons, and `pyjitpl.py:511-534` records that branch as `GUARD_TRUE`/`GUARD_FALSE` plus a promoted constant (`pyjitpl.py:525-526 replace_box`). `walker_newbool_guarded` emits that shape: the directional guard over the raw truth, `w_bool_from(observed)` as a Const ref, and `bool_box_truth_record` against the promoted Int so the following `is_true` folds. The residual box stays as the fallback. The new emission is restricted to the shape the forward JitCode lookahead already recognizes, so it is split out of `compare_box_provably_dead`: `classify_compare_box_use` returns `CompareBoxUse::FeedsBranchOnly` for conditions 1-3 (single reader, that reader is the `is_true` residual, scan terminates at `goto_if_not`) and carries condition 4 (`dst_reg`'s color dead at both arms) as a field. `compare_box_provably_dead` keeps requiring both; the guarded emission requires only the shape. `raise_catch_loop` traces 64 ops -> 44 after opt with no `CallR`, and its recorded `loops_compiled=1 bridges_compiled=1 loops_aborted=0 guard_failures=201` is unchanged. check.py --snapshot-diff: 370/370 dynasm, 370/370 cranelift, 366/366 wasm, jit-stats diff clean. Assisted-by: Claude
… the exec floor `check.py` floors each side's startup-subtracted exec at `EXEC_TIME_FLOOR_S` (0.005s off Windows, check.py:112). Measured startup-subtracted pypy exec was 0.0018s for `exc_in_loop_divzero_continue` (N=120000) and 0.0025s for `finally_bare_raise` (N=2000000), so both ratio gates were dividing by the floor. The ratios they reported, 4.7x and 4.3x on dynasm, were 12.8x and 8.4x against real work, and a startup sample landing high moved the reported number without any change in pyre's own time — the wandering single-OS red that check.py:104-110 already describes for the startup half of the same divisor. N goes to 6000000 and 32000000. pypy exec is then 0.036s and 0.032s, and the ratios are 2.1x/2.5x and 1.8x/2.7x (dynasm/cranelift) against gates of 20 and 17; both fall rather than rise with size, so the compiled tier amortizes the added work. No gate number is changed. Cost: cpython 0.73s/1.13s, pyre 0.13s/0.12s, wasm 0.17s/0.12s. `exc_in_loop_divzero_continue`'s recorded jitstats move with the workload: guard_failures 360 -> 603 and bridges_compiled 0 -> 3 for 50x the iterations (sub-linear, i.e. the compiled loop holds), with loops_compiled=3, loops_aborted=0, internal_compile_panics=0 and descr_set_* unchanged. `finally_bare_raise`'s baseline is unchanged. Assisted-by: Claude
…nder the inline diag The builtin-helper inline path returned Ok(None) silently at each of its four screens. Print which one fired, the callable's type, and the BuiltinCode.func address, so a builtin missing from the `builtin_wrapper_indirect_graphs` PBC family is distinguishable from a non-Function callable. Assisted-by: Claude
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
majit/majit-metainterp/src/pyjitpl/dispatch.rs (3)
4708-4731: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThe one-shot latch can be consumed by a later merge point.
Two early returns share the same visit slot. The inline-depth guard at Line 4708 returns before
take_merge_point_resumed()runs, so it does not consume the latch. When a resumed walk re-enters at a guest pc whose merge point is skipped by that guard, the latch stays armed. The next merge point the walk reaches — a different pc, possibly the real header — then consumes the latch and returns without evaluating the close ladder. That skips one legitimate close attempt per resumed walk.Decide which visit owns the latch. If the latch must cover the exact resumed instruction, consume it before the inline-depth guard, or key it on the resumed pc so an unrelated merge point cannot clear it.
🐛 Option: consume the latch before the inline-depth guard
+ // The latch belongs to the visit the walk was resumed at, so + // consume it before any other early return can claim that visit. + if ctx.take_merge_point_resumed() { + return TraceAction::Continue; + } if ctx.inline_depth() > 0 && self.seen_loop_header_for_jdindex < 0 && !no_loop_header { return TraceAction::Continue; } - if ctx.take_merge_point_resumed() { - return TraceAction::Continue; - }🤖 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-metainterp/src/pyjitpl/dispatch.rs` around lines 4708 - 4731, Update the resumed-merge-point handling around take_merge_point_resumed so the latch is consumed by the exact resumed visit before the inline-depth guard can return. Ensure the guard cannot leave the latch armed for a later, unrelated merge point, while preserving the existing TraceAction::Continue behavior.
9039-9061: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend coverage to the int-result and void-result typed helpers.
The two tests cover
call_float_functionin both argument orders.call_int_function_typedandcall_void_function_typeduse the samearg_classes_from_typesprojection and are the paths that regress silently: an Int- or Ref-returning callee with aFloatparameter is exactly the case the untypedcall_int_functioncannot serve, and no test pins it.Add one test for
call_int_function_typedwith an interleaved(Float, Int)signature, and one forcall_void_function_typedthat records the received float through a static.💚 Sketch
extern "C" fn sum_int_from_float(x: f64, k: i64) -> i64 { (x as i64) + k } #[test] fn typed_int_call_places_float_argument_in_fp_register() { let result = call_int_function_typed( sum_int_from_float as *const (), &[2.5_f64.to_bits() as i64, 3], &[Type::Float, Type::Int], ); assert_eq!(result, 5, "sum_int_from_float(2.5, 3) == 5"); }🤖 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-metainterp/src/pyjitpl/dispatch.rs` around lines 9039 - 9061, Extend the dispatch tests near interleaved_int_after_float_dispatches by adding a call_int_function_typed case with a (Float, Int) signature and an integer-returning extern function, asserting the expected result. Add a call_void_function_typed case with a float parameter that records the received value through a static, then assert the recorded value to verify float arguments reach the correct register.
8574-8584: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake the argument-coverage check release-active.
The doc comment states that an undescribed slot travels as a machine word, and that a
Floatslot in that position is exactly the mis-placement this projection prevents. Thedebug_assert!at Line 8574 disappears in release builds, and theNonearm at Line 8584 then maps the undescribed slot toArgClass::Int. A float argument would be read from an integer register, which produces a wrong value silently and can corrupt the call frame.These functions are
pub, so a caller outside this module can supply a shortarg_types. Use a release-activeassert!for the coverage contract, matching the deliberateassert!strength used for the cannot-raise contract at Line 1972.🛡️ Proposed fix
- debug_assert!( + assert!( arg_types.len() >= args_len, "typed call: calldescr describes {} argument types for {args_len} positional \ arguments; an undescribed Float slot would travel in an integer register", arg_types.len() ); let mut classes = [majit_backend::call_stub::ArgClass::Int; MAX_HOST_CALL_ARITY]; for (i, slot) in classes.iter_mut().enumerate().take(args_len) { *slot = match arg_types.get(i) { Some(Type::Float) => majit_backend::call_stub::ArgClass::Float, - Some(Type::Int | Type::Ref) | None => majit_backend::call_stub::ArgClass::Int, + Some(Type::Int | Type::Ref) => majit_backend::call_stub::ArgClass::Int, + // Unreachable: the assertion above pins full coverage. + None => unreachable!("typed call: missing argument class at slot {i}"),🤖 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-metainterp/src/pyjitpl/dispatch.rs` around lines 8574 - 8584, Replace the debug-only coverage check in the typed-call argument classification flow with a release-active assert, preserving the existing condition and diagnostic message. Update the check immediately before the classes initialization so short arg_types from external callers are rejected before the None fallback in the classification loop.
♻️ Duplicate comments (3)
majit/majit-metainterp/src/pyjitpl.rs (2)
6902-6931: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReport the declined registration to the caller.
register_retrace_merge_pointreturns(). It declines in two cases: no active trace context (Line 6903) and an untypedjump_argselement (Line 6910). On both paths it registers no merge point and leaveskeep_tracing_after_closeatfalse.The caller at Lines 5626-5632 returns
CompileOutcome::Cancelledfor every outcome.Cancelledpreservesself.tracing,partial_traceandretracing_from. The next close reaches the same lookup at Line 5624, finds no merge point again, and declines again. The retrace never completes and the retrace state never clears.Return the decline to the caller so it can clear the retrace state.
🐛 Proposed fix
- fn register_retrace_merge_point(&mut self, jump_args: &[OpRef]) { + fn register_retrace_merge_point(&mut self, jump_args: &[OpRef]) -> bool { let Some(ctx) = self.tracing.as_mut() else { - return; + return false; }; let green_boxes: Vec<crate::trace_ctx::GreenBox> = jump_args .iter() .filter_map(|&op| op.ty().map(|ty| crate::trace_ctx::GreenBox::new(op, ty))) .collect(); if green_boxes.len() != jump_args.len() { - return; + return false; }Tail of the same function:
self.keep_tracing_after_close = true; + true }Call site at Lines 5626-5632:
if merge_position.is_none() { - self.register_retrace_merge_point(jump_args); + if !self.register_retrace_merge_point(jump_args) { + self.clear_retrace_state(); + } return CompileOutcome::Cancelled; }🤖 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-metainterp/src/pyjitpl.rs` around lines 6902 - 6931, Update register_retrace_merge_point to return whether registration succeeded, returning failure when tracing is absent or any jump_args element is untyped and success after add_merge_point completes. At its caller near the CompileOutcome::Cancelled path, check this result and clear the retrace state before returning when registration is declined, while preserving the existing successful registration behavior.
8131-8137: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRecompute
front_entry_indexwhen the token list is replaced.Line 8132 replaces the whole
front_target_tokenslist. Line 8133 keeps the previous index when it is alreadySome(_), which is the normal state for an entry installed bycompile_loop. The retained index then addresses a different list.Two failure modes follow:
- The retained index can select a preamble target.
selected_front_target_tokenthen names the preamble LABEL as the host entry, which the field doc at Line 816 excludes.- The retained index can exceed the new list length.
selected_front_target_tokenreturnsNone, sofront_target_dispatch_keyandfront_target_inputarg_typeslose the direct LABEL entry.Every other publish site recomputes the index unconditionally (Lines 6740, 7917, 9101, 11483, 11941). The
None-only guard at Line 6610 is safe only because it sits insideif compiled.front_target_tokens.is_empty(). This site has no such precondition.🐛 Proposed fix
if !unroll_opt.target_tokens.is_empty() { compiled.front_target_tokens = unroll_opt.target_tokens.clone(); - if compiled.front_entry_index.is_none() { - compiled.front_entry_index = - Self::front_entry_index_for(&compiled.front_target_tokens); - } + compiled.front_entry_index = + Self::front_entry_index_for(&compiled.front_target_tokens); }🤖 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-metainterp/src/pyjitpl.rs` around lines 8131 - 8137, In the unroll optimization publish block, update the `front_entry_index` assignment after replacing `compiled.front_target_tokens` so it is recomputed unconditionally from the new token list via `Self::front_entry_index_for`, rather than preserving an existing index. Keep the existing non-empty token guard and token-list replacement behavior unchanged.majit/majit-metainterp/src/optimizeopt/shortpreamble.rs (1)
2685-2793: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
used_boxesis not filtered for ops dropped by the new per-op resolution logic.The per-op loop in
setup()now drops any op whose args are unresolvable, unless the op is a guard, an overflow producer, or a producer of the short preamble's own JUMP arg (short_preamble.jump_args.contains(...)). Those three cases decline the whole inline to avoid a dangling reference.
self.used_boxes = short_preamble.used_boxes.clone();(Line 2834) has no equivalent protection. It copies the pre-dropused_boxeslist unconditionally. If a dropped op's result position is referenced inused_boxesbut not injump_args, the exportedShortPreamble(viabuild_short_preamble_struct()) ends up with aused_boxesentry that has no corresponding op inself.short.The previous review on this function named
used_boxesexplicitly as one of the fields requiring this filtering, alongsideshort_jump_args. The current fix protectsshort_jump_args/jump_argsbut appears to leaveused_boxesexposed to the same class of stale-reference risk.Confirm whether a stale
used_boxesentry can reach a strict lookup downstream, or only the lenientmaterialize_operand_atpath (which would silently carry an uncomputed value into a label/jump slot instead of panicking).#!/bin/bash # Trace ExtendedShortPreambleBuilder.used_boxes consumers to check for # stale-reference exposure after the per-op drop in setup(). set -euo pipefail FILE="majit/majit-metainterp/src/optimizeopt/shortpreamble.rs" UNROLL="majit/majit-metainterp/src/optimizeopt/unroll.rs" echo "== used_boxes field writes/reads in shortpreamble.rs ==" rg -n -C3 '\bused_boxes\b' "$FILE" echo "== used_boxes consumption in unroll.rs (sp.used_boxes / extra_label_args) ==" rg -n -C5 '\bused_boxes\b' "$UNROLL"🤖 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-metainterp/src/optimizeopt/shortpreamble.rs` around lines 2685 - 2793, Update ExtendedShortPreambleBuilder::setup so used_boxes is filtered alongside short_jump_args when unresolved pure producers are dropped; retain only entries whose producing operations remain in self.short, while preserving whole-inline decline for guards, overflow producers, and jump-argument producers. Trace used_boxes consumers in build_short_preamble_struct and unroll handling to ensure stale entries cannot reach strict lookups or materialize_operand_at, and adjust the stored list before returning success.
🤖 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-macros/src/jit_interp/mod.rs`:
- Around line 1348-1364: Regenerate the affected Charon .ullbc artifacts before
the annotator/rtyper prepass: update majit/majit-macros/src/jit_interp/mod.rs
lines 1348-1364, majit/majit-metainterp/src/trace_ctx.rs lines 404-413, and
majit/majit-metainterp/src/compile.rs lines 5308-5364 as applicable, then run
scripts/install-charon.py, scripts/extract-llbc.py, and PYRE_RTYPER_VERBOSE=1
cargo build --release -p pyre-jit-trace to re-extract them.
In `@majit/majit-metainterp/src/compile.rs`:
- Around line 5347-5364: Update compile_retrace to use ctx.close_header_pc()
instead of ctx.header_pc when calling get_merge_point_at and when reporting the
diagnostic header value, so both retrace selection and validation use the same
close-loop header.
In `@majit/majit-metainterp/src/jitdriver.rs`:
- Around line 3712-3741: Ensure every session-ending path in the relevant
MetaInterp flow clears bridge_attempt_declined together with
bridge_body_start_op_count. Add a shared clear_bridge_session_latches helper
near clear_tracing_session_state, call it before meta.clear_trace_session() in
CompileTrace, Compiled outcomes, pending-success returns, Finish, SegmentedLoop,
bail-out arms, Abort, and AbortPermanent, and replace the existing individual
resets with this helper.
---
Outside diff comments:
In `@majit/majit-metainterp/src/pyjitpl/dispatch.rs`:
- Around line 4708-4731: Update the resumed-merge-point handling around
take_merge_point_resumed so the latch is consumed by the exact resumed visit
before the inline-depth guard can return. Ensure the guard cannot leave the
latch armed for a later, unrelated merge point, while preserving the existing
TraceAction::Continue behavior.
- Around line 9039-9061: Extend the dispatch tests near
interleaved_int_after_float_dispatches by adding a call_int_function_typed case
with a (Float, Int) signature and an integer-returning extern function,
asserting the expected result. Add a call_void_function_typed case with a float
parameter that records the received value through a static, then assert the
recorded value to verify float arguments reach the correct register.
- Around line 8574-8584: Replace the debug-only coverage check in the typed-call
argument classification flow with a release-active assert, preserving the
existing condition and diagnostic message. Update the check immediately before
the classes initialization so short arg_types from external callers are rejected
before the None fallback in the classification loop.
---
Duplicate comments:
In `@majit/majit-metainterp/src/optimizeopt/shortpreamble.rs`:
- Around line 2685-2793: Update ExtendedShortPreambleBuilder::setup so
used_boxes is filtered alongside short_jump_args when unresolved pure producers
are dropped; retain only entries whose producing operations remain in
self.short, while preserving whole-inline decline for guards, overflow
producers, and jump-argument producers. Trace used_boxes consumers in
build_short_preamble_struct and unroll handling to ensure stale entries cannot
reach strict lookups or materialize_operand_at, and adjust the stored list
before returning success.
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 6902-6931: Update register_retrace_merge_point to return whether
registration succeeded, returning failure when tracing is absent or any
jump_args element is untyped and success after add_merge_point completes. At its
caller near the CompileOutcome::Cancelled path, check this result and clear the
retrace state before returning when registration is declined, while preserving
the existing successful registration behavior.
- Around line 8131-8137: In the unroll optimization publish block, update the
`front_entry_index` assignment after replacing `compiled.front_target_tokens` so
it is recomputed unconditionally from the new token list via
`Self::front_entry_index_for`, rather than preserving an existing index. Keep
the existing non-empty token guard and token-list replacement behavior
unchanged.
🪄 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 Plus
Run ID: 404044be-27de-486d-bd92-16afed7dcb6a
📒 Files selected for processing (32)
majit/majit-macros/src/jit_interp/jitcode_lower/lower_stmt.rsmajit/majit-macros/src/jit_interp/jitcode_lower/lower_value.rsmajit/majit-macros/src/jit_interp/mod.rsmajit/majit-metainterp/src/compile.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/lib.rsmajit/majit-metainterp/src/optimizeopt/mod.rsmajit/majit-metainterp/src/optimizeopt/shortpreamble.rsmajit/majit-metainterp/src/optimizeopt/unroll.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-metainterp/src/trace_ctx.rspyre/bench/synth/attr_cache_invalidation.cranelift.jitstatspyre/bench/synth/blackhole_inlined_callee_local_after_escape.cranelift.jitstatspyre/bench/synth/closure_per_call.cranelift.jitstatspyre/bench/synth/comprehension_object_append_hot.cranelift.jitstatspyre/bench/synth/const_arg_call_resume.cranelift.jitstatspyre/bench/synth/exc_caught_in_callee_return_loop.cranelift.jitstatspyre/bench/synth/exc_in_loop_divzero_continue.cranelift.jitstatspyre/bench/synth/exc_in_loop_divzero_continue.dynasm.jitstatspyre/bench/synth/exc_in_loop_divzero_continue.pypyre/bench/synth/exception_escape_inlined_midframe_tb_node.cranelift.jitstatspyre/bench/synth/exception_inline_callee_tb_frames.cranelift.jitstatspyre/bench/synth/exception_loop_warmup.cranelift.jitstatspyre/bench/synth/exception_reentry_guard_finally_residual.cranelift.jitstatspyre/bench/synth/exception_traceback_frame_lineno.cranelift.jitstatspyre/bench/synth/exception_traceback_lineno_chain.cranelift.jitstatspyre/bench/synth/exception_try_call_inlined_callee_raise.cranelift.jitstatspyre/bench/synth/finally_bare_raise.pypyre/bench/synth/gc_bug_bridge_flavor_traceback_names.cranelift.jitstatspyre/bench/synth/goto_if_not_same_box.cranelift.jitstatspyre/bench/synth/inline_gate_operand_provenance.cranelift.jitstats
| // pyjitpl.py:1577 `self.pc = saved_pc`: a merge point whose | ||
| // `reached_loop_header` returned without closing (the | ||
| // `current_merge_points.append` path, :3058-3060) resumes the | ||
| // walk at the SAME guest pc. This closure is re-run for that, | ||
| // so the header-revisit fast path below — which exists to close | ||
| // a trace that came back round to its header — must not fire on | ||
| // the resumption: it would return CloseLoop before the walk ran | ||
| // a single step, and the retrace would compile an empty body. | ||
| // | ||
| // PEEK only. The dispatch merge-point arm is the consumer | ||
| // (`take_merge_point_resumed`); taking it here would let the | ||
| // resumed walk's own first merge-point visit close instantly. | ||
| let __resumed = __meta | ||
| .trace_ctx() | ||
| .map(|__ctx| __ctx.merge_point_resumed) | ||
| .unwrap_or(false); | ||
| if !__resumed && __sym.trace_started && __pc == __sym.loop_header_pc() { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root="$(git rev-parse --show-toplevel)"
cd "$root"
printf 'Repository root: %s\n' "$root"
git status --short
git worktree list
printf '\nTracked Charon artifacts:\n'
fd -HI -t f -e ullbc . || true
printf '\nDocumented Charon and prepass commands:\n'
rg -n -i \
-g 'Cargo.toml' -g '*.md' -g '*.sh' -g '*.yml' -g '*.yaml' \
'charon|\.ullbc|annotator|rtyper' . || trueRepository: youknowone/pyre
Length of output: 21698
Regenerate the affected Charon .ullbc artifacts before rebuilding the prepass.
Changing the generated JIT merge wrapper, trace session state, or retrace header selection requires re-extracting the corresponding Charon .ullbc files. Run the extraction (scripts/install-charon.py plus scripts/extract-llbc.py / PYRE_RTYPER_VERBOSE=1 cargo build --release -p pyre-jit-trace) before this change reaches the annotator/rtyper prepass.
📍 Affects 3 files
majit/majit-macros/src/jit_interp/mod.rs#L1348-L1364(this comment)majit/majit-metainterp/src/trace_ctx.rs#L404-L413majit/majit-metainterp/src/compile.rs#L5308-L5364
🤖 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-macros/src/jit_interp/mod.rs` around lines 1348 - 1364,
Regenerate the affected Charon .ullbc artifacts before the annotator/rtyper
prepass: update majit/majit-macros/src/jit_interp/mod.rs lines 1348-1364,
majit/majit-metainterp/src/trace_ctx.rs lines 404-413, and
majit/majit-metainterp/src/compile.rs lines 5308-5364 as applicable, then run
scripts/install-charon.py, scripts/extract-llbc.py, and PYRE_RTYPER_VERBOSE=1
cargo build --release -p pyre-jit-trace to re-extract them.
Source: Coding guidelines
| /// `pyjitpl.py:3021 same_greenkey(original_boxes, live_arg_boxes, | ||
| /// num_green_args)` — the loop header the trace is closing ON, read from | ||
| /// the greens it closes WITH. | ||
| /// | ||
| /// `close_greens` is `live_arg_boxes[:num_green_args]` for the close the | ||
| /// walk just performed; its first green is the guest pc, which is what | ||
| /// [`MergePoint::header_pc`] records. `header_pc` is the header the walk | ||
| /// last registered and a close on a different loop leaves it stale, so | ||
| /// merge-point matching has to go through here rather than read it | ||
| /// directly. Falls back to `header_pc` when the walk recorded no close | ||
| /// greens (nothing to compare against, so the session's own header is | ||
| /// the only candidate). | ||
| pub fn close_header_pc(&self) -> usize { | ||
| self.close_greens | ||
| .as_ref() | ||
| .and_then(|greens| greens.0.first().copied()) | ||
| .map_or(self.header_pc, |pc| pc as usize) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use close_header_pc() when cutting the retrace.
compile_loop_body selects retrace_pos with ctx.close_header_pc(). Downstream compile_retrace still calls get_merge_point_at(green_key, ctx.header_pc). A cross-loop close therefore selects a merge point and then rejects it because ctx.header_pc is stale.
Use ctx.close_header_pc() for the compile_retrace lookup and its diagnostic header value.
Proposed fix
- let header_pc = ctx.header_pc;
+ let header_pc = ctx.close_header_pc();As per coding guidelines: “Preserve interpreter semantics exactly in the generated JIT.”
🤖 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-metainterp/src/compile.rs` around lines 5347 - 5364, Update
compile_retrace to use ctx.close_header_pc() instead of ctx.header_pc when
calling get_merge_point_at and when reporting the diagnostic header value, so
both retrace selection and validation use the same close-loop header.
Source: Coding guidelines
| self.meta.abort_trace_live(false); | ||
| // `pyjitpl.py:2785-2789` counts every SwitchToBlackhole abort, | ||
| // including a bridge whose greenkey is None. `Decline` is a | ||
| // pyre pre-trace coverage fallback, not a traced abort, so it | ||
| // deliberately remains outside `aborted_tracing` accounting. | ||
| if !matches!(action, TraceAction::Decline) { | ||
| self.meta.aborted_tracing(reason_int); | ||
| } | ||
| self.sym = None; | ||
| self.bridge_body_start_op_count = None; | ||
| self.bridge_attempt_declined = false; | ||
| self.meta.clear_trace_session(); | ||
| } | ||
| self.meta.abort_trace_live(false); | ||
| // `pyjitpl.py:2785-2789` counts every SwitchToBlackhole abort, | ||
| // including a bridge whose greenkey is None. `Decline` is a | ||
| // pyre pre-trace coverage fallback, not a traced abort, so it | ||
| // deliberately remains outside `aborted_tracing` accounting. | ||
| if !matches!(action, TraceAction::Decline) { | ||
| self.meta.aborted_tracing(reason_int); | ||
| } | ||
| self.sym = None; | ||
| self.bridge_body_start_op_count = None; | ||
| self.meta.clear_trace_session(); | ||
| } | ||
| TraceAction::AbortPermanent => { | ||
| if self.meta.bridge_info().is_some() { | ||
| crate::debug::log_one("jit-abort", "AbortPermanent during bridge tracing"); | ||
| TraceAction::AbortPermanent => { | ||
| if self.meta.bridge_info().is_some() { | ||
| crate::debug::log_one("jit-abort", "AbortPermanent during bridge tracing"); | ||
| } | ||
| if self.bridge_attempt_declined { | ||
| crate::mc_diag_bump(52); // abort_after_declined | ||
| } | ||
| self.meta.abort_trace(true); | ||
| self.sym = None; | ||
| // The session ends here, so the latch must not outlive it. The | ||
| // sibling `bridge_body_start_op_count` is cleared on the | ||
| // Abort/Decline arm and in `clear_tracing_session_state` but | ||
| // not here, which is why this arm needs its own reset rather | ||
| // than a shared teardown. | ||
| self.bridge_attempt_declined = false; | ||
| self.meta.clear_trace_session(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Reset bridge_attempt_declined on every session-ending arm, not only on the abort arms. The comment at Lines 3734-3738 states the contract: the latch must not outlive its session. Only three arms honor it — Abort/Decline (Line 3722), AbortPermanent (Line 3739), and clear_tracing_session_state (Line 3771).
Every other arm that ends the session calls self.meta.clear_trace_session() directly and leaves the latch set: CompileTrace (Lines 2783-2787), the Compiled outcomes at Lines 3038-3051 and 3279-3293, the compile_trace_success_pending return at Lines 3345-3349, Finish (Line 3487), SegmentedLoop (Lines 3529-3531), and the bail-outs at Lines 3103-3113, 3126-3134, 3327-3331 and 3350-3359.
A stale true is masked today because start_bridge_tracing resets it at Line 6103, and that is the only path in this file that installs bridge_info. The masking therefore rests on a single call site while MetaInterp::set_bridge_trace_info and send_bridge_to_backend are both reachable independently. If any future path installs bridge_info without start_bridge_tracing, the gate at Lines 2931-2935 and 3245-3249 skips the whole bridge attempt with no diagnostic.
Clear the latch in one shared teardown next to bridge_body_start_op_count so each arm cannot omit it.
♻️ Proposed shared teardown
Add a small helper next to clear_tracing_session_state:
/// Both bridge latches belong to the session that set them, so every arm
/// that ends a session must drop them together.
#[inline]
fn clear_bridge_session_latches(&mut self) {
self.bridge_body_start_op_count = None;
self.bridge_attempt_declined = false;
}Then call it from each session-ending arm before clear_trace_session(), and reuse it in the two existing sites:
self.sym = None;
- self.bridge_body_start_op_count = None;
- self.bridge_attempt_declined = false;
+ self.clear_bridge_session_latches();
self.meta.clear_trace_session(); self.meta.abort_trace(true);
self.sym = None;
- // The session ends here, so the latch must not outlive it. The
- // sibling `bridge_body_start_op_count` is cleared on the
- // Abort/Decline arm and in `clear_tracing_session_state` but
- // not here, which is why this arm needs its own reset rather
- // than a shared teardown.
- self.bridge_attempt_declined = false;
+ self.clear_bridge_session_latches();
self.meta.clear_trace_session();🤖 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-metainterp/src/jitdriver.rs` around lines 3712 - 3741, Ensure
every session-ending path in the relevant MetaInterp flow clears
bridge_attempt_declined together with bridge_body_start_op_count. Add a shared
clear_bridge_session_latches helper near clear_tracing_session_state, call it
before meta.clear_trace_session() in CompileTrace, Compiled outcomes,
pending-success returns, Finish, SegmentedLoop, bail-out arms, Abort, and
AbortPermanent, and replace the existing individual resets with this helper.
check.py fails on origin/main across all three OS runners. Three recorded
baselines do not match what any platform measures.
exception_try_call_inlined_callee_raise.cranelift
loops_compiled 3 -> 2, guard_failures 402 -> 201
exception_try_call_inlined_callee_raise.wasm
loops_compiled 3 -> 2, loops_aborted 1 -> 0, guard_failures 821 -> 201
exc_in_loop_divzero_continue.wasm
bridges_compiled 0 -> 3, guard_failures 360 -> 603
The first two record the values every runner reads. The cranelift file
returns to the content #1003 replaced, and the wasm file, whose fields
#1009 first recorded, converges on that same content. Measured over
repeated runs of pyre-cranelift and pyre-wasm-runner, identical each time,
and CI reads 2 on ubuntu, macos and windows alike.
The third records a bridge count that rose from zero. #960 compiles
bridges for this fixture where none were compiled before; the new bridges
carry their own guards, so guard_failures rises with them. loops_compiled
stays at 3, so no loop was lost.
check.py --synthetic-only: cranelift 356/356, wasm 355/355.
The CPython suite gate is red for an unrelated reason and is untouched
here: test.test_asyncio went PASS -> TIMEOUT starting at #1003 and is a
behaviour change, not a recorded value.
Assisted-by: Claude
…1011) check.py fails on origin/main across all three OS runners. Three recorded baselines do not match what any platform measures. exception_try_call_inlined_callee_raise.cranelift loops_compiled 3 -> 2, guard_failures 402 -> 201 exception_try_call_inlined_callee_raise.wasm loops_compiled 3 -> 2, loops_aborted 1 -> 0, guard_failures 821 -> 201 exc_in_loop_divzero_continue.wasm bridges_compiled 0 -> 3, guard_failures 360 -> 603 The first two record the values every runner reads. The cranelift file returns to the content #1003 replaced, and the wasm file, whose fields #1009 first recorded, converges on that same content. Measured over repeated runs of pyre-cranelift and pyre-wasm-runner, identical each time, and CI reads 2 on ubuntu, macos and windows alike. The third records a bridge count that rose from zero. #960 compiles bridges for this fixture where none were compiled before; the new bridges carry their own guards, so guard_failures rises with them. loops_compiled stays at 3, so no loop was lost. check.py --synthetic-only: cranelift 356/356, wasm 355/355. The CPython suite gate is red for an unrelated reason and is untouched here: test.test_asyncio went PASS -> TIMEOUT starting at #1003 and is a behaviour change, not a recorded value. Assisted-by: Claude
…ee converged jitstats baselines (#1014) * jit: restrict the LOAD_LOCALS residual to portal jitcodes In a non-portal callee `frame_var` aliases the outermost frame, the same aliasing the LoadGlobal namespace split documents, where it resolved the caller's `names` table. Threading the callee's own frame is not available as a remedy: an inlined callee has no materialised frame at all (`frame_ptr == 0`, `portal_frame_reg` unseeded), which is what inlining a virtualizable means. The two opcodes differ in whether the receiver's identity can change the answer, so they no longer share one arm: `load_locals` returns that frame's own `w_locals` via get_or_create_w_locals, so an aliased receiver is a wrong value that no guard can catch, and there is no frame-free way to compute it. It keeps the residual in portal jitcodes and declines otherwise. `load_build_class` reads frame.get_builtin(), which with honor__builtins__ false is space.builtin for every frame (baseobjspace::frame_builtin_obj), so the answer does not depend on which frame asks. It stays unrestricted. That is a property of the flag rather than of this arm, so a const assertion on HONOR_BUILTINS now fails the build, naming this site, if the flag is ever flipped. Restricting both instead was measured and rejected: it cost slots_class_var_conflict 0 -> 5, list_length_hint_validate guard_failures 1 -> 13427, and two fixtures' loops_compiled. Splitting them costs none of that — every fixture the residual improved keeps its value. check.py: dynasm 371/371, cranelift 371/371, wasm 366 passed. The one wasm failure in that run, exception_try_call_inlined_callee_raise loops_compiled 3 -> 2, is inherited: that fixture's bytecode contains neither opcode, and the same failure appears with the rejected variant above. Assisted-by: Claude * bench: record the cranelift and wasm jitstats converged onto by #1003 and #960 Three baselines moved when those two landed. Each is a backend converging onto values the other backends already recorded, not a loss, so each is recorded rather than investigated as a regression. exception_try_call_inlined_callee_raise: cranelift and wasm both drop loops_compiled 3 -> 2 and guard_failures to 201, which is what the dynasm baseline already held. All three backends now carry byte-identical numbers for this fixture (bridges_compiled=1 guard_failures=201 loops_aborted=0 loops_compiled=2), and wasm additionally loses its extra loops_aborted=1. The declining walk reports the same census reason on dynasm and wasm, P2Drain::CompileRootRaise=1. exc_in_loop_divzero_continue: wasm was the outlier at bridges_compiled=0, guard_failures=360 against dynasm's and cranelift's 3 and 603, and now matches both. The guard_failures rise is a consequence of compiling those three bridges rather than none, so it accompanies more compiled coverage, not less. Both fixtures' output is byte-identical to python3.14 on every backend. check.py at this tree: dynasm 371/371, cranelift 371/371, wasm 367/367. Assisted-by: Claude
RPython's retrace path (
compile.py:341-394 compile_retrace) is dormant inproduction —
rpython/rlib/jit.py:595setsPARAMETERS['retrace_limit'] = 0.majit's port had never been exercised end to end. Driving it through the
in-repo cel-jit tier with a temporary
retrace_limitknob crashed, and once itstopped crashing it silently produced wrong answers. This branch takes it from
"SEGV" to "correct, and faster than not retracing at all".
What was wrong
The resumekey was destroyed mid-session.
MetaInterp::bridge_infoismajit's
self.resumekey. Upstream writes it exactly twice, both at sessionstart (
pyjitpl.py:2905ResumeFromInterpDescr,pyjitpl.py:2939= resumedescr), and never clears it —compile.py:392-393 resumekey.compile_and_attachdispatches on it at the very end. majit clearedit on
RetraceNeeded, which turned every guard-originated retrace into aResumeFromInterpDescrfront-door install instead ofResumeGuardDescr::compile_and_attach. That was the SEGV.The same field was also cleared on four fall-through-to-
compile_looppaths,to express a session phase. That destroys the resumekey class as a side
effect, and four later readers dispatch on that class —
compile_retrace(front door vs
attach_retrace_to_source_guard, i.e. the same defect again),compile_finish_from_active_session(pyjitpl.py:3216/3242), theentry-bridge routing in
jitcode_dispatch, and the closing JUMP's arg typing.The phase now has its own latch.
Merge points were matched on the wrong key.
pyjitpl.py:3021same_greenkey(original_boxes, live_arg_boxes, num_green_args)compares amerge point's greens against the greens the trace is closing with. majit
looked them up by
ctx.header_pc— the header the walk last registered — andregistered new ones under the same field. A trace closing on a different loop
than the one the session last registered leaves that pc stale, so the scan
matched a merge point whose greens differ from the close and retraced it, where
upstream's
continuefalls out of the scan and appends(
pyjitpl.py:3059-3060). That was the wrong answer: under one green key therepro registered headers at guest pc 45 and 89 and closed the pc-89 one with
close_greens = ([27], ...), and the retrace grown there attached to the pc-45loop's guard.
The preamble's quasi-immutable deps were dropped.
compile.py:384-390merges
loop_info.quasi_immutable_depsand thenstart_state.quasi_immutable_deps; majit wrote only the first half becauseExportedStatehad no such field.One list carried two jobs.
CompiledEntry::front_target_tokenswas bothupstream's
jitcell_token.target_tokens(scanned atunroll.py:198/239/277/325,appended at
:297) and a majit-only selector for the LABEL the Cranelift hostenters through. Writing a retrace's labels back into the scan list could
therefore move the front door to a
label_block_idbelonging to the retrace'sCranelift function while entering the parent's.
Results
cel-jit
nums.map(y, y * 2)with a short-list-first poison,retrace_limit=5against a
retrace_limit=0control, across nine row counts(16 40 64 80 100 128 160 200 2000):
phase3 checksum=232— row 0 right, every later row emitted a zero-length listThe whole win is the first retrace; limits 1 through 5 are all ~29.9 ns/row.
Also in this branch
same_greenkeyparity for the loop-header stamp, thepyjitpl.py:3003if not self.partial_trace:gate on theCloseLoopWithArgsarm, and themerge-point append.
guest back.
inline — except when the dropped op's result is one the short preamble's own
JUMP carries, which has no per-op answer and keeps the old whole-inline
decline.
review notes below: the reason originally given for this was wrong, and the
check is narrower than upstream.
live_arg_boxesbuilt once, aspyjitpl.py:2974-2989does, instead of onceper consumer — normalizing twice appends a second set of
SAME_ASrewrites.compile_bridgealready does it, zipping
liveboxeswithfrontend_boxes(
bridgeopt.py:126).421d070b8dahad added a second, positional stamp instart_retrace_from_guardthat filtered on nothing butType::Void, so slotsthe resume data never resurrected got values too. A dead Ref slot then read
back as the constant NULL,
virtualstate.py:400-405found it equal to atarget label's
LEVEL_CONSTANT, and appended aGUARD_VALUEpinning it — aguard on a value nothing keeps stable.
resume.py:1267-1282 load_box_from_cpuruns once per live box while
rebuild_from_resumedatawalks the resume data,not once per
fail_arg_typesslot. Reverted in9948baf069c; measured onsynth/defaults_reassigned_midloop(dynasm), it was 4 extraGUARD_VALUEs,two of them on
ptr(0x0), andguard_failures 404 → 812,bridges_compiled 2 → 4,loops_compiled 3 → 4.majit-macros: raw-store width and signedness read from the intrinsic name.MC_DIAGgrows to 38 slots.ct_compile_bridge_falsegives the dominantDeclinedproducer a tally it never had, and three slots make the declined /no-targets close path observable.
Verification
cargo test -p majit-metainterp --release— 1515 passed, 0 failedtarget/release/pyre-dynasm(pyre/check.pyprints child stderr only forfailing runs, so it cannot census this):
bridge_declined_close=3,bridge_no_targets_close=7,ct_compile_bridge_false=6,abort_after_declined=0— the latched paths arelive, so the calls this replaces were not dead code.
⚠
pyre/check.pyis red on this branch, and it is red without this workCorrection. An earlier revision of this section claimed a byte-identical
failure set against the base. That claim was wrong, and CI caught it: the
pyre/check.pyjobs on ubuntu and macOS each reported 8 entries thatmain's own CI run at the identical merge-base SHA (f3bddb31f28) does nothave —
guard_failuresroughly doubling on five fixtures across dynasm andcranelift. The local A/B missed it because the baseline was keyed on
FAIL <backend> <fixture>: those five fixtures were already in the failure listat the base for other reasons, so a fixture-name diff could not see the failure
reason change.
Bisected to
421d070b8da, then to a single hunk inside it, and fixed in9948baf069c— see "Also in this branch" below. All five fixtures now reproducetheir committed
.jitstatsbaselines exactly on both backends.Re-run on CI at
9948baf069cand compared tomain's own run at the merge-base,keyed on (fixture, backend, reason), the branch adds nothing on any host:
macOS and windows are set-identical to
main. ubuntu is a strict subset —synth/exception_multi_handler_warmup(dynasm + cranelift) fails onmainandon the earlier branch tip but not on this run, while the same fixture still
fails in a local gate at this commit. That fixture is in the known-unstable
exception_*family, so read it as flake, not as an improvement this branchearned.
The base itself fails 17 dynasm / 19 cranelift / 11 wasm — a 47-entry set this
branch neither adds to nor removes from. It splits into:
.jitstatsbaseline at all:
math_log_trig_hot,tuple_unpack_array_backed_hot,inline_freevar_after_mayforce,pypy_dict_primitives_nonbinding,raise_reg_unbound_jitstress,hot_loop_exit_then_class_stmt.guard_failuresroughly doubling onseveral fixtures, and
loops_aborted 0 -> Non the exception andsrefamilies.
These need a separate fix on
main; this PR should be read as "no regressionagainst its base", not as "gate green".
Perf-ratio entries on a loaded machine are not attributable without an isolated
re-run: an earlier gate reported
cranelift synth/kwargs_positional_only7.6x > 6x and
cranelift synth/type_metatype_data_descr6.7x > 5x while asecond
check.pyran on the same host (load average 15 → 44). pypy'sdenominator on those fixtures is 0.01s, so a few ms of scheduler noise clears
the gate; re-run alone they are 2.7x and 2.8x.
The base moved after that measurement
A peer rebased this branch onto
05abdbb694c(#845, "stdlib: expandcompatibility foundations"), so the three-host table above was measured against
the old base
f3bddb31f28and needs a CI re-run to stand.#845is not a neutral base for this gate: alongside the stdlib work it carriesjit: authoritative reraise, resume anchors and GC rootsandjit: skip in-frame exception delivery when the frame stack was never written. A localdynasm/cranelift/wasm gate at the new base shows 11 entries the old-base
comparison does not have. Building
05abdbb694c's ownpyre-dynasmin aseparate worktree and running the same fixtures attributes them:
main@ 05abdbbsynth/float_div_zero_caught_loop#845synth/mutate_then_raise_caught#845synth/inline_subwalk_mutating_residual#845synth/inline_subwalk_property_mutates#845synth/pickle_terminal_raise_resumeThe four
guard_failuresentries reproduce identically onmain's ownbinary, so their committed
.jitstatsbaselines are simply stale with respectto
#845. The remaining new entries are fixtures#845added without recordingbaselines.
⚠
synth/pickle_terminal_raise_resumeis a build-layout lottery — do not A/B it#845added this fixture with no committed baselines. It crashes(
IndexError: pop from empty listinpickle.pop_mark) only with the JIT on(
PYRE_JIT=0is clean), and it is not attributable to either side:fbd2ac490cffbd2ac490cf(same source)5c1ec828a5c05abdbb694cDeterministic within a build, flips across builds of identical source. A full
git bisect runnamedfbd2ac490cf"first bad commit", which is provablywrong — that commit's entire diff sits inside
if !resolvableinExtendedShortPreambleBuilder::setup, and that branch fires 0 times on arun that did crash (checked with
MAJIT_LOG=1on). Neither branch-guilt normain-innocence is established. Tracked separately; it needs alayout-independent detector, not another A/B.
Two corrections to earlier claims in this PR
5c1ec828a5c's commit message describes the wrong mechanism. It says thesurviving stamp in
compile_bridgeis the live-filtered form. It is not:prd.liveboxesis built from every bridge inputarg (pyjitpl.rs:11581)and zipped positionally with the raw
fail_values, andbridgeopt.py:126isonly a length assert. What actually fixed the
GUARD_VALUEstorm isordering —
closing_jump_runtime_boxes(:11639) reads the inputargsbefore that zip (
:11707), so with the trace-start stamp gone the dead Refslots are no longer constified into
runtime_boxes. The measured result andthe CI table stand; the explanation did not.
3c8018fa42frecords both factsin-code and corrects a comment that still pointed at the deleted channel.
The "census: 0 / 371 fixtures" in
fbd2ac490cf's commit message isuntrustworthy. The recipe used
timeout 300 ./target/release/pyre-dynasm …and
timeoutdoes not exist on macOS, so the pipeline ran nothing andrgcounted 0 everywhere.
Re-measured, and the number holds: 0 / 371. Not with
MAJIT_LOG=1—that gate emits ~4.5 MB of stderr per fixture and one fixture
(
foriter_call_resume_drops_iteration) burned 34 min wall / 33 min CPUwithout finishing. Instead a temporary ungated
eprintln!was put at thetop of the
if !resolvableblock, the corpus was run at normal speed with noMAJIT_LOG, and the probe was reverted. Positive control:rg -a -c <probe> target/release/pyre-dynasm= 1, so the 0 means "neverfired", not "never instrumented". The reachability argument is independent
and unaffected either way:
setupsees a non-coincident arg set only on aretrace close, which
retrace_limit = 0keeps dormant.Review triage
Every finding from the Codex parity review and the inline comments was checked
against the source. Four survived, four did not.
Acted on:
ExtendedShortPreambleBuilder::setup— a dropped op whose result the shortpreamble's own JUMP carries leaves
unroll.rs:3920'sexpect("mapping missing jump_arg")reachable. The whole-inline decline isrestored for that case only. (CodeRabbit,
shortpreamble.rs:2704.)BridgeCompileResult::RetraceNeededarms now set the attempt latch.Removing
take_bridge_info()kept the resumekey class, which is the point,but it also left the bridge gate open on the edge where
compile_bridgeexports no state and
partial_tracetherefore never arms.compile_retrace's closing-JUMP comment claimed the backend cannot emit ajump_to_preambleJUMP. That is false — see fix x86 fib_loop: emit LEA for IntAdd; preserve obj in guards #33.cargo fmt:8cc2aaf54c2wrapped a body inloop { … break; }withoutre-indenting it.
Not acted on — the claim does not survive reading the source:
front_entry_indexgoes stale whenfront_target_tokensis replaced.Raised independently by both reviewers.
unroll_opt.target_tokensopens as aclone of
compiled.front_target_tokens(pyjitpl.rs:7367) and only everpushes (unroll.rs:1517); the oneinsert(0, …)(
unroll.rs:501-510) is guarded byfirst().virtual_state.is_none(), whichholds for every non-empty prior list.
rg 'front_target_tokens\s*\.\s*(push| insert|remove|clear|retain|swap|sort|truncate)' majit/→ 0 hits. The retainedindex names the same
TargetToken, and overrun is impossible becausenew_len >= prior_len. The claimed disagreement withcompact_label_values_for_selected_targetdoes not exist either: its onlycall site (
pyjitpl.rs:6431) and the cached index (pyjitpl.rs:6654) readthe same unmutated local through the same predicate.
same_greenkeybecause it does notcompare
MergePoint.green_boxes.green_boxesdoes not hold the greens —every producer stores the const-stripped red loop-carried list
(
jitdriver.rs:2856-2870,dispatch.rs:5197-5215), whileclose_greensisthree vectors of green constants. The proposed comparison would compare
different universes. The greens are already compared element-wise on the
close side (
dispatch.rs:4959-4964), and the predicate itself is byte-identical to
origin/main— this commit only swappedctx.header_pcforctx.close_header_pc(), which is strictly more discriminating.resume_pc. The re-entrycontinueis in the bareTraceAction::CloseLooparm. pyre'strace_bytecodemaps every loop close toCloseLoopWithArgs(
trace.rs:5014-5033) and never emits bareCloseLoop, so the closure runsat most once and there is no second segment.
compiled_key_for_greens_fnis used without being wired in. All threeproduction installs assign it before any bytecode is traced
(
pyjitpl.rs:4184,:4470, andjitdriver.rs:6273fromstart_bridge_tracing). The one unwired constructor is test-only, and bothconsumers use
.as_ref().and_then(…)and degrade conservatively.Filed rather than fixed here:
fix x86 fib_loop: emit LEA for IntAdd; preserve obj in guards #33 —
compile_retracedeclines everyjump_to_preambleretrace. This isa real gap: upstream compiles that outcome (both
unroll.py:156/171sitesreturn a full
UnrollInfo), majit ports the retargeting(
unroll.rs:1831-1878), and Cranelift lowers the foreign-descr JUMP as anexternal exit. But the check is load-bearing for wasm today, for a reason the
old comment did not state:
codegen.rs:1983dispatches onhas_loop, not onwhether the descr resolves, so a retrace's preamble JUMP takes the local arm
and
find_label_args' trailing-label fallback silently makes it a back-edgeto this trace's own label. Relaxing the check alone would trade a performance
decline for a wasm miscompile.
has_loopalso decides whether the LABELbecomes the wasm
loopheader, so the backend fix touches every wasm traceand needs its own gate run.
Split descr identity between metainterp and backend #36 —
generate_guards'(Constant, _)arm catches a virtual incomingstate, where
virtualstate.py:392-394raisesVirtualStatesCantMatch('comparing a constant against something that is a virtual')first. Real parity gap, but inert on the corpus: adding therejection while bisecting the regression above left
defaults_reassigned_midloopat 812, so none of its extraGUARD_VALUEswere against a virtual. Kept out of the fix commit to keep it single-variable.
Fix SSA REPR parity gaps #37 — whether the bridge recorder's inputargs need deadframe values at
trace-open time at all, and if so which slots are live there. That is the
salvageable half of the reverted hunk; it needs the cel-jit retrace ladder to
justify it and the five
check.pyfixtures to price it, and neither wasmeasured for the original. The
CEL_RETRACE_LIMIT/ phase3 poison harness isnot in the current working copy and has to be reconstructed first.
Fix merge points in fannkuch loop #34 — the
!attempt_declinedconjunct on the two bridge-close gatesdeviates from
pyjitpl.py:3003, which retries at later loop headers. The gateanswer is byte-identical to
origin/main(which reached the same suppressionthrough
take_bridge_info()), and dropping the conjunct without a companionshort-circuit would re-run the full optimize+lower at every later header on a
terminally-declining backend.
Fix x86 fib_recursive #39 — a regression this branch introduced.
df845164792moved themerge-point selection (
pyjitpl.rs:5577) and the registration (:6855) toctx.close_header_pc()but leftcompile_retrace's own re-lookup (:7305)on
ctx.header_pc; onorigin/mainall three usedheader_pc, so a callermatch implied a
compile_retracematch. For a bridge the two keys differ byconstruction (parent loop header vs the bridge's
resume_pc), so there-lookup misses and the trace is never cut. Upstream reads
startonce(
pyjitpl.py:3019) andcompile.py:347 cut_trace_fromis unconditional.73aa96b84f9turns the resultingelse { trace }fallback into a harddecline so the shape can no longer be assembled, but the key mismatch itself
needs the selection threaded down. This is why the branch's own goal — a
retrace that works end to end — is not actually reached.
Implement list slice assignment JIT path with BUILD_SLICE tracing #40 —
attach_retrace_to_source_guardbinds the artifact andrecord_loop_or_bridgetosource_jctwhile filing the guard metadata(
traces.insert,build_guard_metadata,set_next_header_pc,caller_recovery_layout,previous_tokens) underctx.green_key.compile.py:797-811has one identity. pyre retargets the green key mid-close(
pyre-jit-trace/src/trace.rs:5023-5027), so a bridge closing on an innerheader files under
key(inner)while every reader resolveskey(L)— thenext bridge then traces with no resume data. Note the fix is not "use
source_jct.green_key()everywhere": theretraced_countcharge andrecord_target_tokencorrectly follow the entered loop(
unroll.py:213-215,:290-297).Unify backend descr identity via DescrRef #41 —
ExtendedShortPreambleBuilder::use_boxis a second writer intoself.shortwith a strictly weaker predicate thansetup's (one level deep,different membership sets, no guard/jump-arg test), so it can re-add exactly
the op
setupdropped. Not a miscompile — the replay then signalsInvalidLoop— but the write-back atunroll.rs:3511persists the resultonto the token for every later bridge, which upstream cannot reach because it
raises instead of deferring. Same dormancy as the rest.
Wire portal red args through the regular liveness/inputarg path #42 —
synth/pickle_terminal_raise_resume, the build-layout lotterydescribed above.
Refuted along the way — recorded so they are not re-chased
virtualstate.pyVirtualStateConstructor.create_statecaches by box andVirtualState.enumreturns early when
position != -1, so upstream collapses duplicate boxesand drops constants in the LABEL exactly as majit does.
GuardTrue(Int(1))in the retrace preamble is cosmetic. Theguard args are plain
OpRefs; theInt(1)came fromformat_traceresolving through the const map. It is the
make_constantupstream alsoperforms after
emit(rewrite.py:163-183).front_target_tokenswrite-back inattach_retrace_to_source_guardis not involved in the miscompile.
is_bridge_tracing()conjoined withself.sym.is_some()was tried anddropped. Both its consumers want a live session rather than the resumekey
class, but
symis not that signal:start_bridge_tracingsetsbridge_infoand runsrebuild_from_resumedatabeforeself.sym = Some(sym), so the protection window opens whilesymis stillNone. The comment on the accessor records this.🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Diagnostics