Fix merge points in fannkuch loop - #34
Merged
youknowone merged 6 commits intoMay 13, 2026
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR introduces five coordinated enhancements to the JIT compilation system. Portal jit_merge_point emission is relaxed to allow multiple calls per dispatch instead of asserting on the first call only. Exception-guard suppression for primitive-lowered opcodes is implemented via a new per-opcode flag on MIFrame. Bridge optimization virtual-state handling is simplified by removing pre-force state capture. Resume numbering now correctly uses replacement OpRef types for virtual classification. Default retrace limits are reduced from 5 to 0. ChangesJIT System Enhancements for Portal Merge-Points, Exception Guards, and State Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-jit/src/jit/codewriter.rs (1)
4468-4604: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove the now-unreachable
loop_headerfallback.
is_portalis derived fromportal_jd_index.is_some()on Line 3016, so after Line 4469 theelse if let Some(jdindex) = portal_jd_indexarm can never run. Leaving it here makes the new contract harder to follow because it still looks like some loop headers may emitloop_headerinstead ofjit_merge_point.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-jit/src/jit/codewriter.rs` around lines 4468 - 4604, The code contains a dead "else if let Some(jdindex) = portal_jd_index" branch guarded by is_portal (which was set as portal_jd_index.is_some()), so remove the unreachable loop_header fallback: delete the entire else-if arm that builds loop_header_op and its GraphFlattener.emit_space_operation block, leaving only the is_portal branch that emits "jit_merge_point"; ensure no other code depends on the removed jdindex binding and run tests to confirm behavior.
🤖 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/resume.rs`:
- Around line 5096-5124: The test
test_number_boxes_uses_replacement_type_for_virtual_classification currently
uses SimpleBoxEnv whose is_virtual_ref and is_virtual_raw share the same backing
set, hiding regressions; change the test to use a small test-local BoxEnv double
(implementing the same trait used by ResumeDataLoopMemo) that overrides
is_virtual_ref and is_virtual_raw to return true for is_virtual_ref(target) and
false for is_virtual_raw(target), while keeping replacements/virtuals/types
entries as before; this will force _number_boxes (invoked via
ResumeDataLoopMemo::number) to take the ref-virtual branch and make wrong
type-selection observable. Ensure the new env still provides
replacements.insert, virtuals.insert, and types.insert behavior used earlier in
the test.
- Around line 4111-4118: The code currently skips calling opref.ty() when opref
!= raw_opref, risking wrong classification; change the box_type computation to
prefer the replacement OpRef's intrinsic type by always checking opref.ty()
first (falling back to snapshot_box.tp and then env.get_type(opref)) even in the
else branch—i.e., ensure the logic that computes box_type for both the opref ==
raw_opref and opref != raw_opref cases uses
opref.ty().or(snapshot_box.tp).unwrap_or_else(|| env.get_type(opref)) so typed
replacements are honored.
In `@majit/majit-metainterp/src/warmstate.rs`:
- Around line 272-273: The DEFAULT_RETRACE_LIMIT constant is incorrect (set to
0) and its comment misstates PyPy's default; update the constant
DEFAULT_RETRACE_LIMIT to 5 to match PyPy's retrace_limit default and adjust or
remove the comment referencing rlib/jit.py:595 so it accurately reflects the
intent (or note any deliberate deviation if you intend a non-PyPy default).
Ensure you change the value where DEFAULT_RETRACE_LIMIT is defined and update
the adjacent comment text to correctly state the source/default.
In `@pyre/pyre-jit-trace/src/state.rs`:
- Around line 1596-1600: Two MIFrame struct literals are missing the new
required field suppress_guard_no_exception_for_opcode which causes a
missing-field compile error; update each MIFrame construction (the literals
identified in the diff) to include suppress_guard_no_exception_for_opcode: false
so the struct literal matches the MIFrame definition and compiles. Ensure both
MIFrame instances that currently lack this field are updated.
---
Outside diff comments:
In `@pyre/pyre-jit/src/jit/codewriter.rs`:
- Around line 4468-4604: The code contains a dead "else if let Some(jdindex) =
portal_jd_index" branch guarded by is_portal (which was set as
portal_jd_index.is_some()), so remove the unreachable loop_header fallback:
delete the entire else-if arm that builds loop_header_op and its
GraphFlattener.emit_space_operation block, leaving only the is_portal branch
that emits "jit_merge_point"; ensure no other code depends on the removed
jdindex binding and run tests to confirm 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
Run ID: 362bc717-6552-4d2c-8c3f-d73284c7e24e
📒 Files selected for processing (8)
majit/majit-metainterp/src/jitcode/assembler.rsmajit/majit-metainterp/src/optimizeopt/optimizer.rsmajit/majit-metainterp/src/resume.rsmajit/majit-metainterp/src/warmstate.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/jit/codewriter.rs
This was referenced Jul 15, 2026
youknowone
added a commit
that referenced
this pull request
Jul 18, 2026
Add FBW_APPEND_PROMOTE_JOURNAL recording each list promoted Empty→typed during a speculative full-body walk, wired parallel to FBW_APPEND_JOURNAL through reset/commit/rollback and the GC root walker area. Rollback drains it after the length-rewind and calls w_list_clear to restore the Empty strategy + drop the typed backing block — the strategy switch + realloc the length journal alone cannot undo. No caller yet (fbw_append_promote_journal_push is #[allow(dead_code)] until the recognize gate admits the empty append); strict no-op with the gate off. Removable when single-executor tracing lands (gh#73/#34). Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 18, 2026
Add FBW_APPEND_PROMOTE_JOURNAL recording each list promoted Empty→typed during a speculative full-body walk, wired parallel to FBW_APPEND_JOURNAL through reset/commit/rollback and the GC root walker area. Rollback drains it after the length-rewind and calls w_list_clear to restore the Empty strategy + drop the typed backing block — the strategy switch + realloc the length journal alone cannot undo. No caller yet (fbw_append_promote_journal_push is #[allow(dead_code)] until the recognize gate admits the empty append); strict no-op with the gate off. Removable when single-executor tracing lands (gh#73/#34). Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 18, 2026
Add FBW_APPEND_PROMOTE_JOURNAL recording each list promoted Empty→typed during a speculative full-body walk, wired parallel to FBW_APPEND_JOURNAL through reset/commit/rollback and the GC root walker area. Rollback drains it after the length-rewind and calls w_list_clear to restore the Empty strategy + drop the typed backing block — the strategy switch + realloc the length journal alone cannot undo. No caller yet (fbw_append_promote_journal_push is #[allow(dead_code)] until the recognize gate admits the empty append); strict no-op with the gate off. Removable when single-executor tracing lands (gh#73/#34). Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 18, 2026
* jit: add PYRE_EMPTY_APPEND_VIRT gate scaffold (default-off) Add `empty_append_virt_enabled()` reading `PYRE_EMPTY_APPEND_VIRT` (default-off), mirroring `newlist_virt_enabled()`. The gate will admit the empty-list first append into the orthodox `w_list_append` fold in a later slice. No caller yet; `#[allow(dead_code)]` until wired. Assisted-by: Claude * jit: add dormant append-promote restore journal for FBW rollback Add FBW_APPEND_PROMOTE_JOURNAL recording each list promoted Empty→typed during a speculative full-body walk, wired parallel to FBW_APPEND_JOURNAL through reset/commit/rollback and the GC root walker area. Rollback drains it after the length-rewind and calls w_list_clear to restore the Empty strategy + drop the typed backing block — the strategy switch + realloc the length journal alone cannot undo. No caller yet (fbw_append_promote_journal_push is #[allow(dead_code)] until the recognize gate admits the empty append); strict no-op with the gate off. Removable when single-executor tracing lands (gh#73/#34). Assisted-by: Claude * jit: virtualize empty-list first append via Empty->typed promotion (gated) Admit the Empty-strategy first LIST_APPEND on the FBW walker under the PYRE_EMPTY_APPEND_VIRT gate (default off). recognize classifies the append by the value's type in switch_to_correct_strategy's int->float->object order; commit installs the typed backing block on the concrete receiver, emits the Empty->typed transition as inline heap IR mutating the existing wrapper, and journals the rewind to Empty for a non-commit walk. emit_promote_empty_list_inline returns the transition's NewArray block OpRef. commit stamps it with the concrete backing-block address (new w_list_items_block_ptr) so the append body sub-walk's `list.items_block.capacity` getfield folds via field_sanity_load to the concrete capacity (1); the spare-capacity `0 < capacity` branch then resolves instead of aborting with a symbolic GOTO_IF_NOT condition. The concrete strategy switch runs before the IR emit so the backing block exists when the block OpRef is stamped. Assisted-by: Claude * jit: enable empty-list first-append promotion by default Flip the PYRE_EMPTY_APPEND_VIRT gate to default-on (set PYRE_EMPTY_APPEND_VIRT=0 to restore the residual abort), matching the newlist_virt_enabled default-on convention. dynasm and cranelift both pass check.py 194/194 with the gate on. Assisted-by: Claude * jit: fold empty-append capacity via heapcache seed, drop concrete-stamp path Replace the NewArray-block concrete-address stamp with a heapcache capacity seed. emit_promote_empty_list_inline now seeds the new block's ItemsBlock.capacity getfield cache with the const (1) on the Integer and Float arms, so the append body sub-walk's spare-capacity `0 < capacity` check folds to the const instead of reading a symbolic getfield. This is the same shape the heapcache uses for a const-length new_array; the capacity is read through a getfield (not arraylen), so the field-index channel is seeded explicitly. Removes the emit_promote_empty_list_inline `Option<OpRef>` return, the commit-site reorder + set_opref_concrete stamp, and the now-unused w_list_items_block_ptr accessor. Object storage needs no seed (its capacity read resolves through list.items already). A/B verified both levers independently compile the empty-append loop; the seed is the smaller and matches the heapcache const-length model. dynasm+cranelift 194/194, list repros byte-identical under MAJIT_GC_STRESS. Assisted-by: Claude * jit: reseed vstack mirror across FOR_ITER-entry permutation; narrow LIST_APPEND admit to call-free bodies The py3.14 inlined comprehension emits a SWAP/BUILD_LIST/SWAP preamble before its FOR_ITER, lowered non-monotonically in jitcode: the JitCode-PC floor pivot maps a later jit_pc back to an earlier py_pc, so the walk re-visits those preamble py_pcs out of order (a backward py transition, then a forward re-walk). reconcile_vstack_at_boundary replayed the SWAP effect and reused a stale vstack_last_ref across that region, dropping the just-built list from the vstack_boxes mirror; the stack_sync overlay then copied the corrupt mirror into the resume snapshot, so a deopt from the comprehension body reconstructed a frame whose FOR_ITER iterator slot held a duplicated iterator and a dropped list ("not an iterator"). Add WalkContext.vstack_reorder_ceiling: when the previous opcode is a Swap/Copy and the py transition is backward (a codewriter layout artifact, never a real branch), reseed the whole mirror from the virtualizable shadow (VstackOpClass::ShadowReseed) until the walk advances past the py_pc it backed off from. Admit LIST_APPEND in for_iter_bodies_all_jit_safe only when its FOR_ITER body performs no CALL. A per-element user-frame call (a class ctor / user function) sets the FOR_ITER in-flight body-effect flag, and a mid-body abort then routes through fbw_foriter_inflight_take, which refuses delivery to avoid a double-apply and drops the trace-attempt iteration's item. Call-free comprehension bodies never enter a user frame and stay admissible; call-bearing bodies decline to interpretation. Assisted-by: Claude * jit: reload comprehension LIST_APPEND list receiver from its value-stack slot The codewriter lowered LIST_APPEND's list operand by cloning the peeked FlowValue from the compile-time stack. The inlined-comprehension accumulator lives on the operand stack, which is not loop-carried in registers (the jit_merge_point reds are [frame, ec]), so the preamble BUILD_LIST value is not a live register inside the resident loop. The jit_list_append residual then read an unbound register for its receiver and aborted ResidualCallArgUnbound, so the comprehension inner loop never compiled and the #171 empty-append fold was never reached. Reload the list receiver from its value-stack slot via getarrayitem_vable_r each iteration, the same per-iteration re-materialization the FOR_ITER iterator reload already uses. Call-free comprehension inner loops (`[j for j in range(n)]`) now compile with the append folded. A comprehension whose body enters a user frame (e.g. `[C(i) for i in ...]`) still declines via for_iter_bodies_all_jit_safe's call-free LIST_APPEND gate, since its in-flight FOR_ITER delivery is a separate concern. Assisted-by: Claude * jit: restrict LIST_APPEND FOR_ITER body admit to bare-accumulator shape A comprehension whose LIST_APPEND lands in a list Object-strategy (str/None/tuple/list/dict/f-string element) lowers the append to a Void setarrayitem_gc residual. The FBW body-effect gate marks that Void write as a body effect, so fbw_foriter_inflight_take refuses delivery of the in-flight FOR_ITER item on the trace-attempt abort and drops one iteration (len 999 vs 1000). int/float-strategy appends fold via the pure length-rewind journal and stay correct. Replace the body_has_call scan with a body_has_list_append scan: when a FOR_ITER body contains LIST_APPEND, admit only the bare-accumulator ops (LoadFast/StoreFast family, ListAppend, stack shuffles, backward jumps, ExtendedArg/Cache). Any value-producing op declines the body to the interpreter. Bodies without LIST_APPEND keep the existing predicate. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 19, 2026
This reverts commit 8a3bdd85b89a55552f8046ef4018b18903e3bdd1.
Admitting a call-bearing `LIST_APPEND` body reaches the in-flight delivery
gap often enough to lose an element. `[m.heappop(heap) for i in
range(size)]` aborts with `LoopBearingCalleeInlineUnsupported { pc: 40,
blackhole_required: true }` and the flush declines the multi-frame blackhole
handoff ("no outer caller resume pc"), so the legacy replay drops the
iteration the walk had already consumed: `test.test_heapq` raises
`IndexError: index out of range` out of `test_heapsort`, and
`extra_tests/parity_tests/for_iter_call_bearing_comprehension.py` builds 22
of 23 elements. Both are deterministic here and both pass on
`61dc91a5c1e`, so the widening owns them.
The commit says as much — it widens how often the gap is reached, and the
call scan it deleted was never a boundary the gap stayed behind. Closing
the gap is gh#73/#34; until then the scan is what keeps these shapes on the
interpreter. Declining to trace is semantically safe, so this is a
coverage reduction rather than a parity change.
What the commit bought comes back with it: seven synth fixtures compile
less again and their jitstats baselines return to the recorded values,
`mapdict_frozen_unboxing_fold`'s build phase goes back to 0.612s from
0.330s, and `exception_group_type` / `list_to_tuple_star` compile zero
loops again.
check.py dynasm 440/440.
Assisted-by: Claude
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This pull request makes several important changes to the JIT and meta-interpreter infrastructure, focusing on improving correctness, Python compatibility, and test coverage. The most notable updates include refining how virtual box types are determined during resume state construction, updating the default retrace limit for better alignment with upstream PyPy behavior, and enhancing test coverage for new and existing behaviors. Additionally, the handling of merge points in JIT code generation is made more robust, and a new field is added to the JIT frame state to support more accurate exception guard handling.
Resume state and virtual box handling:
ResumeDataLoopMemo::numberis updated to use the replacement box's type, matching PyPy's behavior and preventing incorrect virtual classification. A targeted test is added to verify this behavior.JIT configuration and merge point handling:
warmstate.rs, aligning with PyPy's default and ensuring immediate retry after the first cancelled unrolled compile. Associated comments are updated for clarity.JitCodeBuilderis relaxed to allow multiple merge points per portal jitcode, preserving only the first offset for schema validation and supporting PyPy's dispatch loop semantics.Test coverage and state struct enhancements:
suppress_guard_no_exception_for_opcode, is added toMIFrameto track when a Python opcode's exception guard should be suppressed due to primitive lowering. All relevant test instantiations are updated to set this field.Optimizer and virtual state handling:
Nonefor the virtual state in jump handling, simplifying the logic and better matching RPython semantics.Self-review
Prompt & Model
Model: gpt-5.5
Prompt:
Answer
Summary by CodeRabbit
Bug Fixes
Chores