jit: heal nested for-range degenerate loop; virtualize range GET_ITER/FOR_ITER - #683
Merged
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
WalkthroughThe PR adds walker-native specialization for exact ChangesRange iterator JIT flow
Bridge merge-point resumption
WebAssembly finish exception handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant JITCodeWriter
participant ResidualCallDispatcher
participant WalkerSpecializer
participant RangeIterator
JITCodeWriter->>ResidualCallDispatcher: emit GetIter helper metadata
ResidualCallDispatcher->>WalkerSpecializer: specialize exact W_Range
WalkerSpecializer->>RangeIterator: allocate and initialize iterator state
RangeIterator-->>ResidualCallDispatcher: return iterator result
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f01aaf23c3
ℹ️ 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".
| /// `get_iter(obj)` — the GET_ITER residual (`iter(obj)`). The full-body | ||
| /// walker recognises exact machine-word `range` objects and emits the | ||
| /// virtual `W_IntRangeIterator` allocation shape directly. | ||
| GetIter, |
There was a problem hiding this comment.
PyreHelperKind is repr(u8), and the enum's tail comment explicitly warns that inserting helpers in the middle changes discriminants consumed by serialized/stable helper metadata. Placing GetIter before StoreDeref shifts the numeric tags for StoreDeref, ListAppendValue, CallFunctionEx, and later helpers; any reused serialized JitCode/descriptor metadata generated with the old tags can then be decoded as the wrong helper, causing the walker to miss body-effect handling or run an unrelated specialization. Add the new helper at the tail or pin explicit discriminants so existing tag values stay stable.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 115b01b). 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
https://github.com/youknowone/pyre/blob/478831592664259fe6a563aaca47e4ff6f9053da/pyre-jit-trace/src/jitcode_dispatch/specialize.rs#L4508-L4512
When tagged_int::CAN_BE_TAGGED is flipped on, pyre_object::is_int() accepts tagged-immediate small ints, so range(10) bounds pass this predicate even though start_obj, step_obj, and length_obj are not heap W_IntObjects. The specialization then emits GuardClass/GetfieldGcI(intval) for those operands, which will dereference the tagged word as a pointer in compiled code; either decline tagged bounds here or emit the tagged-int unbox path before using int_intval_descr().
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79cb8200e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+920
to
+922
| ( | ||
| "W_Range.step", | ||
| RANGE_STEP_OFFSET, |
There was a problem hiding this comment.
When this new W_Range SizeDescr is used, the field list jumps from start to step, but W_Range also has a stop: PyObjectRef field (pyre/pyre-object/src/functional.rs:685-689). build_object_descr_group_with_def_path derives gc_fielddescrs() from only the supplied Type::Ref fields and publishes this SizeDescr under W_RANGE_GC_TYPE_ID, so traced/materialized range objects described by it will not report the stop edge for GC clearing/scanning or field-descr lookup. Add a RANGE_STOP_OFFSET entry between start and step so the runtime descriptor matches the actual object layout.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-ir/src/effectinfo.rs`:
- Around line 634-637: Move the GetIter variant to the append-only tail of the
effect enum, preserving all existing variant discriminants used by serialized
and stable helper metadata. Update the adjacent tail comment to describe the new
GetIter placement while leaving the existing variants’ order unchanged.
In `@pyre/pyre-jit-trace/src/descr.rs`:
- Around line 905-943: Update RANGE_DESCR_GROUP’s W_Range.start, W_Range.step,
and W_Range.length field descriptors to use std::mem::size_of::<PyObjectRef>()
instead of the hard-coded width 8, preserving the existing Type::Ref and
descriptor metadata.
🪄 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: 353e1ed4-fd50-4bd4-ae43-4866a1bbb8f0
📒 Files selected for processing (11)
majit/majit-ir/src/effectinfo.rsmajit/majit-metainterp/src/optimizeopt/unroll.rspyre/pyre-interpreter/src/module/_collections/mod.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/codewriter.rspyre/pyre-object/src/functional.rs
Comment on lines
+905
to
+943
| static RANGE_DESCR_GROUP: LazyLock<PyreObjectDescrGroup> = LazyLock::new(|| { | ||
| build_object_descr_group_with_def_path( | ||
| std::mem::size_of::<W_Range>(), | ||
| pyre_object::functional::W_RANGE_GC_TYPE_ID, | ||
| &pyre_object::functional::RANGE_TYPE as *const _ as usize, | ||
| &[ | ||
| ( | ||
| "W_Range.start", | ||
| RANGE_START_OFFSET, | ||
| 8, | ||
| Type::Ref, | ||
| false, | ||
| true, | ||
| false, | ||
| ), | ||
| ( | ||
| "W_Range.step", | ||
| RANGE_STEP_OFFSET, | ||
| 8, | ||
| Type::Ref, | ||
| false, | ||
| true, | ||
| false, | ||
| ), | ||
| ( | ||
| "W_Range.length", | ||
| RANGE_LENGTH_OFFSET, | ||
| 8, | ||
| Type::Ref, | ||
| false, | ||
| true, | ||
| false, | ||
| ), | ||
| ], | ||
| "W_Range", | ||
| "functional::W_Range", | ||
| ) | ||
| }); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'field_size|PyreFieldDescr|wasm32|wasm' pyre majitRepository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the W_Range descriptor and nearby type definitions.
sed -n '850,980p' pyre/pyre-jit-trace/src/descr.rs
printf '\n---\n'
# Find the W_Range struct definition and PyObjectRef alias.
rg -n 'struct W_Range|type PyObjectRef|pub struct PyObjectRef|enum PyObjectRef' pyre/pyre-interpreter/src pyre/pyre-jit-trace/src
printf '\n---\n'
# Inspect how field_size is interpreted in the JIT/runtime.
sed -n '9738,9845p' pyre/pyre-jit-trace/src/state.rs
printf '\n---\n'
# Inspect any explicit pointer-size handling around descriptor field sizes.
rg -n 'size_of::<PyObjectRef>|pointer_size|field_size: std::mem::size_of::<usize>|field_size: 8' pyre/pyre-jit-trace/src pyre/pyre-interpreter/srcRepository: youknowone/pyre
Length of output: 10762
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrower probe: look for W_Range fields and any target-layout annotations.
ast-grep outline pyre/pyre-interpreter/src pyre/pyre-jit-trace/src --match W_RangeRepository: youknowone/pyre
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focused search for the exact descriptor group and related comments.
rg -n -A6 -B6 'RANGE_DESCR_GROUP|W_Range\.start|W_Range\.step|W_Range\.length' pyre/pyre-jit-trace/src/descr.rsRepository: youknowone/pyre
Length of output: 2360
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the PyObjectRef definition and any target-width assumptions.
rg -n -A4 -B4 'pub type PyObjectRef|struct PyObjectRef|enum PyObjectRef|type PyObjectRef' pyre
printf '\n---\n'
# Inspect the interpreter-side type alias/definition if found.
for f in $(rg -l 'pub type PyObjectRef|struct PyObjectRef|enum PyObjectRef|type PyObjectRef' pyre); do
echo "FILE: $f"
sed -n '1,120p' "$f" | rg -n 'PyObjectRef|PyObject|usize|u64|u32|NonZero|Tagged'
echo
done
printf '\n---\n'
# Check the descriptor API for any notes on Ref field sizing.
sed -n '1,140p' pyre/pyre-jit-trace/src/descr.rsRepository: youknowone/pyre
Length of output: 9102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect references to the exact field-size convention for Ref fields.
rg -n -A3 -B3 'Type::Ref|field_size.*Ref|wrapped PyObjectRef|pointer_size\(\)|size_of::<PyObjectRef>' pyre/pyre-jit-trace/src pyre/pyre-interpreter/src pyre/pyre-object/srcRepository: youknowone/pyre
Length of output: 50372
Use pointer width for W_Range ref fields.
PyObjectRef is pointer-sized, so hard-coding 8 makes this descriptor wrong on wasm32 and can corrupt field/GC handling. Use std::mem::size_of::<PyObjectRef>() for start, step, and length.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-jit-trace/src/descr.rs` around lines 905 - 943, Update
RANGE_DESCR_GROUP’s W_Range.start, W_Range.step, and W_Range.length field
descriptors to use std::mem::size_of::<PyObjectRef>() instead of the hard-coded
width 8, preserving the existing Type::Ref and descriptor metadata.
Owner
Author
|
Pushed an additional independent commit (`119c6405270`): route CALL_ASSEMBLER callee ExitFrameWithException exits through the deopt helper — a wasm-only miscompile where a self-recursive — commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/2020a85c55959045c0e940e6ed8ca171575c8f4f/pyre-jit-trace/src/jitcode_dispatch/specialize.rs#L4506
When a trace is warmed with a fitting stop and later reused for a range whose stop is a non-machine long but whose start/step/length still pass the emitted checks (for example an empty descending range(0, huge, -1)), w_range_iter would choose W_LongRangeIterator because w_range_fields_i64 includes stop, but this specialization ignores _stop_obj at runtime and materializes W_IntRangeIterator. If the iterator escapes or its type/reduce behavior is observed, the compiled trace diverges from the interpreter; please load/guard stop or decline unless it is proven machine-int too.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1787
to
1791
| descr.is_finish | ||
| && !failguard::meta_descr_is_exit_frame_with_exception(&descr.meta_descr) | ||
| }) | ||
| .map(|descr| descr.fail_index) | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
When the only FINISH on a compiled CALL_ASSEMBLER target is ExitFrameWithException (or there is no DoneWithThisFrame), this new filter leaves no clean match but the following .unwrap_or(0) still publishes fail index 0 as the clean finish. The wasm CA arm treats fi == loop_finish_fi as a direct return from F'[1], so an exception finish or guard assigned fail_index 0 bypasses wasm_ca_resume_deopt and is still misrouted; use the existing WASM_CA_FINISH_FI_UNKNOWN sentinel here, and similarly for the bridge finish selector, when no clean finish exists.
Useful? React with 👍 / 👎.
youknowone
force-pushed
the
wasm-jit
branch
2 times, most recently
from
July 20, 2026 23:20
8cd76e7 to
e769b34
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
majit/majit-ir/src/effectinfo.rs (1)
634-637: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAppend
GetIterinstead of inserting it mid-enum.The enum’s own invariant says that inserting a variant changes discriminants consumed by serialized/stable helper metadata. Add
GetIterat the append-only tail so all existing helper discriminants remain unchanged, and update the adjacent tail comment accordingly.♻️ Proposed fix
- /// `get_iter(obj)` — the GET_ITER residual (`iter(obj)`). The full-body - /// walker recognises exact machine-word `range` objects and emits the - /// virtual `W_IntRangeIterator` allocation shape directly. - GetIter, /// `store_deref_value(cell, value)` — the STORE_DEREF residual ... LoadMethodSelf, + /// `get_iter(obj)` — the GET_ITER residual (`iter(obj)`). The full-body + /// walker recognises exact machine-word `range` objects and emits the + /// virtual `W_IntRangeIterator` allocation shape directly. + GetIter, }🤖 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-ir/src/effectinfo.rs` around lines 634 - 637, Move the GetIter variant from its current position to the append-only tail of the effect enum, preserving all existing variant discriminants used by serialized or stable helper metadata. Update the adjacent tail comment to document GetIter’s new position while retaining its existing description.pyre/pyre-jit-trace/src/descr.rs (1)
905-943: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse pointer width for
W_Rangeref fields.
PyObjectRefis pointer-sized, so hard-coding8makes this descriptor wrong on wasm32 and can corrupt field/GC handling. Usestd::mem::size_of::<PyObjectRef>()(orstd::mem::size_of::<usize>()) forstart,step, andlength.♻️ Proposed fix
( "W_Range.start", RANGE_START_OFFSET, - 8, + std::mem::size_of::<usize>(), Type::Ref, false, true, false, ), ( "W_Range.step", RANGE_STEP_OFFSET, - 8, + std::mem::size_of::<usize>(), Type::Ref, false, true, false, ), ( "W_Range.length", RANGE_LENGTH_OFFSET, - 8, + std::mem::size_of::<usize>(), Type::Ref, false, true, 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 `@pyre/pyre-jit-trace/src/descr.rs` around lines 905 - 943, Update the size argument for the W_Range.start, W_Range.step, and W_Range.length entries in RANGE_DESCR_GROUP to use the pointer-sized PyObjectRef or usize size instead of the hard-coded 8, preserving the existing descriptor metadata.
🤖 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 `@pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs`:
- Around line 532-546: Re-extract the LLBC artifacts, including the Charon
.ullbc files, with scripts/extract-llbc.py before rebuilding the rtyper prepass
affected by bridge_entry_merge_pc. Then run the complete benchmark suite across
all 8 benchmarks and verify there are no regressions.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs`:
- Around line 4508-4569: Verify the implementations of w_range_fields_i64 and
w_range_length_i64 and W_Range field construction to confirm whether start,
step, and length can contain tagged immediates when tagged_int::CAN_BE_TAGGED is
enabled. If they can, update the range specialization’s initial validation
before emitting GuardClass and getfield_gc_i calls to decline any tagged-int
field, matching sibling specializations; otherwise document the confirmed
invariant in the review without changing the guards.
In `@pyre/pyre-jit/src/call_jit.rs`:
- Line 3398: Update the CALL_ASSEMBLER result handling around
backend.get_ref_value to use get_int_value for retrieving the return value,
preserving full 64-bit Int results on wasm32 while remaining compatible with
zero-extended 32-bit Ref pointers.
---
Duplicate comments:
In `@majit/majit-ir/src/effectinfo.rs`:
- Around line 634-637: Move the GetIter variant from its current position to the
append-only tail of the effect enum, preserving all existing variant
discriminants used by serialized or stable helper metadata. Update the adjacent
tail comment to document GetIter’s new position while retaining its existing
description.
In `@pyre/pyre-jit-trace/src/descr.rs`:
- Around line 905-943: Update the size argument for the W_Range.start,
W_Range.step, and W_Range.length entries in RANGE_DESCR_GROUP to use the
pointer-sized PyObjectRef or usize size instead of the hard-coded 8, preserving
the existing descriptor metadata.
🪄 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: 30dfb434-195a-4734-8592-ff970d75d5ad
📒 Files selected for processing (14)
majit/majit-backend-wasm/src/codegen.rsmajit/majit-backend-wasm/src/failguard.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-ir/src/effectinfo.rsmajit/majit-metainterp/src/optimizeopt/unroll.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/unpack_state.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/jit/codewriter.rspyre/pyre-object/src/functional.rs
Comment on lines
+4508
to
+4569
| if !pyre_object::is_int(start_obj) | ||
| || pyre_object::is_bool(start_obj) | ||
| || !pyre_object::is_int(step_obj) | ||
| || pyre_object::is_bool(step_obj) | ||
| || !pyre_object::is_int(length_obj) | ||
| || pyre_object::is_bool(length_obj) | ||
| { | ||
| return Ok(None); | ||
| } | ||
| let Some((start, _stop, step)) = pyre_object::functional::w_range_fields_i64(range_obj) | ||
| else { | ||
| return Ok(None); | ||
| }; | ||
| let Some(length) = pyre_object::functional::w_range_length_i64(range_obj) else { | ||
| return Ok(None); | ||
| }; | ||
| let one_past_i128 = start as i128 + length as i128 * step as i128; | ||
| let Ok(one_past) = i64::try_from(one_past_i128) else { | ||
| return Ok(None); | ||
| }; | ||
| let Some(mul) = length.checked_mul(step) else { | ||
| return Ok(None); | ||
| }; | ||
| let Some(one_past_checked) = start.checked_add(mul) else { | ||
| return Ok(None); | ||
| }; | ||
| debug_assert_eq!(one_past_checked, one_past); | ||
| (start, step, length, mul, one_past) | ||
| }; | ||
|
|
||
| let range_type_addr = &pyre_object::functional::RANGE_TYPE as *const _ as i64; | ||
| if !range_op.is_constant() && !ctx.trace_ctx.heap_cache().is_class_known(range_op) { | ||
| let range_type_const = ctx.trace_ctx.const_int(range_type_addr); | ||
| ctx.trace_ctx | ||
| .record_guard(OpCode::GuardClass, &[range_op, range_type_const], 0); | ||
| walker_capture_snapshot_for_last_guard(ctx, op_pc)?; | ||
| } | ||
| ctx.trace_ctx | ||
| .heap_cache_mut() | ||
| .class_now_known(range_op, range_type_addr); | ||
|
|
||
| let int_type_addr = &pyre_object::pyobject::INT_TYPE as *const _ as i64; | ||
| let int_type_const = ctx.trace_ctx.const_int(int_type_addr); | ||
|
|
||
| let start_r = crate::state::opimpl_getfield_gc_r( | ||
| ctx.trace_ctx, | ||
| range_op, | ||
| crate::descr::range_start_descr(), | ||
| ); | ||
| if !ctx.trace_ctx.heap_cache().is_class_known(start_r) { | ||
| ctx.trace_ctx | ||
| .record_guard(OpCode::GuardClass, &[start_r, int_type_const], 0); | ||
| walker_capture_snapshot_for_last_guard(ctx, op_pc)?; | ||
| ctx.trace_ctx | ||
| .heap_cache_mut() | ||
| .class_now_known(start_r, int_type_addr); | ||
| } | ||
| let start_i = crate::state::opimpl_getfield_gc_i( | ||
| ctx.trace_ctx, | ||
| start_r, | ||
| crate::descr::int_intval_descr(), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Missing tagged-immediate decline for range int fields.
The gate only checks is_int(start_obj) && !is_bool(start_obj) (and likewise for step/length), but is_int also returns true for a tagged-immediate int when tagged_int::CAN_BE_TAGGED. A tagged field would pass this gate and then be misread: the emitted GuardClass INT_TYPE on start_r/step_r/length_r plus getfield_gc_i(int_intval_descr()) assume a heap W_IntObject header/intval slot, which a tagged pointer does not have. Every sibling specialization here declines tagged immediates first (e.g. try_walker_specialize_newtuple Line 1846, try_walker_specialize_newlist Line 1695, try_walker_trace_exception_new Line 3784).
Please confirm W_Range fields can never be tagged immediates in a CAN_BE_TAGGED build; otherwise add the same tagged-int decline before emitting the field-read guards. As per coding guidelines: "The generated JIT must preserve the interpreter's semantics."
#!/bin/bash
# Do W_Range start/stop/step/length fields ever hold tagged-int immediates?
# Inspect w_range_fields / w_range_fields_i64 / w_range_length_i64 and CAN_BE_TAGGED usage.
fd -t f 'functional.rs' pyre/pyre-object/src | xargs -I{} sh -c 'echo "== {} =="; sed -n "1,60p" {}'
ast-grep run --pattern 'pub fn w_range_fields_i64($$$) { $$$ }' --lang rust $(fd -t f 'functional.rs' pyre/pyre-object/src)
rg -nP 'CAN_BE_TAGGED|is_tagged_int|w_range_(fields|length)' pyre/pyre-object/src/functional.rs -C2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs` around lines 4508 -
4569, Verify the implementations of w_range_fields_i64 and w_range_length_i64
and W_Range field construction to confirm whether start, step, and length can
contain tagged immediates when tagged_int::CAN_BE_TAGGED is enabled. If they
can, update the range specialization’s initial validation before emitting
GuardClass and getfield_gc_i calls to decline any tagged-int field, matching
sibling specializations; otherwise document the confirmed invariant in the
review without changing the guards.
| .expect("CA deopt: get_latest_descr_arc returned a non-FailDescr Descr"); | ||
| if descr.is_finish() { | ||
| Outcome::Finished(backend.get_ref_value(&frame, 0).as_usize() as i64) | ||
| let result = backend.get_ref_value(&frame, 0).as_usize() as i64; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Prevent 64-bit truncation of Int returns.
Reading the result using get_ref_value(...).as_usize() as i64 truncates 64-bit Int returns to 32 bits on wasm32. Since CALL_ASSEMBLER bridges support both Int and Ref returns, you should use get_int_value to safely retrieve the 64-bit value without truncation. This correctly preserves 64-bit integers while remaining perfectly safe for 32-bit pointers (which are already zero-extended to i64 in raw_values).
🐛 Proposed fix
- let result = backend.get_ref_value(&frame, 0).as_usize() as i64;
+ let result = backend.get_int_value(&frame, 0);📝 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.
Suggested change
| let result = backend.get_ref_value(&frame, 0).as_usize() as i64; | |
| let result = backend.get_int_value(&frame, 0); |
🤖 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/call_jit.rs` at line 3398, Update the CALL_ASSEMBLER result
handling around backend.get_ref_value to use get_int_value for retrieving the
return value, preserving full 64-bit Int results on wasm32 while remaining
compatible with zero-extended 32-bit Ref pointers.
Replace the range iterator pointer mask with a snapshot-backed continuation guard so a locally consumed item stays a removable virtual New. Take the exhaustion edge before the continuation guard, returning the loop-exit null for the trailing GuardNonnull, so a nested inner loop run to completion does not record a GuardTrue against a concretely-false condition. Force the appended value through the ptr->int->ptr identity in orthodox_list_append_commit so the inline w_list_append sub-walk unboxes the live per-iteration item instead of a loop-carried trace-entry scalar. Assisted-by: Claude Assisted-by: Codex
`for _ in range(N): for x in range(P): total += x` stormed instead of terminating for OUTER >= ~50: a guard-failure bridge closed a 0-progress no-op `[GetfieldRawI; Jump->start:0]` and re-entered it forever. Three changes, correct only together: - optimizeopt/unroll.rs: assemble_peeled_trace_with_jump_args no longer filters base label args through the backend-constants map. import_state already drops LEVEL_CONSTANT virtual-state slots at make_inputargs time; re-deriving the slot set post-hoc drops a still-live loop-carried box that a phase-2 guard postprocess const-forwarded (the exhaust guard's `remaining`), desyncing the label from the positionally-aligned jump and orphaning the head guard's operand. - jitcode_dispatch: FbwWalkMode.bridge_entry_merge_pc carries a guard-failure bridge's own resume python-pc, seeded at the top-level bridge walk; the jit_merge_point arm skips exactly the first crossing that lands on it. A guard's resume coordinate lies past the dispatch-top jit_merge_point (generate_guard resumepc=orgpc), so an RPython bridge never re-crosses the loop-header merge point at position zero; the walker resumes at the opcode boundary and would otherwise close immediately with an empty body. - range GET_ITER virtualization: GET_ITER on an exact `range` emits the virtual W_IntRangeIterator allocation shape. Adds the GetIter helper kind, W_Range field descriptors/offsets, and is_exact_w_range. nfr sweep 25/25; check.py dynasm 225/225, cranelift 225/225, wasm 224/224. Assisted-by: Claude
…hrough the deopt helper `is_finish` is true for both DoneWithThisFrame and ExitFrameWithException FINISH exits. The self-recursive CALL_ASSEMBLER arm picked its "clean callee finish" `fail_index` (`bridge_finish_fi` in codegen, `loop_finish_fi` in the published dispatch metadata) as the first `is_finish` guard, which could be an ExitFrameWithException. The guest arm then short-circuited that exit to output slot 0, banking the raised exception object as the recursive-call return value. Exclude ExitFrameWithException exits from both finish-index selectors via `meta_descr_is_exit_frame_with_exception`, so an exception finish routes to `wasm_ca_resume_deopt`. Handle it there with a new `FinishedException` outcome that publishes the exception via `store_jit_exception` and returns garbage so the caller's GUARD_NO_EXCEPTION fires — parity with the outer Finished arm and `handle_blackhole_result`'s ExitFrameWithExceptionRef arm. Fixes synth/selfrec_tail_exception_unwind on wasm (was `int + ZeroDivisionError` TypeError, now 4250). wasm/dynasm/cranelift synthetic suites 230/230. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 13, 2026
`orthodox_list_append_commit` recorded an identity CastPtrToInt -> CastIntToPtr pair on the appended value before descending the `w_list_append` sub-walk, which made the value's pointer identity observable and so materialized an otherwise non-escaping virtual. The pair came in with the range FOR_ITER virtualization (#683) to stop the sub-walk unboxing a loop-carried trace-entry scalar. The forwarding it emulated is already there: `trace_box_int` / `trace_box_float` stamp `class_now_known` and cache the payload field's current SSA box at the boxing site, and `getfield_gc_i_pureornot` returns that cached box on a hit, so the descended unbox reads this iteration's payload with the value left virtual — the same forwarding `OptVirtualize.optimize_GETFIELD_GC_I` performs in rpython/jit/metainterp/optimizeopt/virtualize.py. The two `set_opref_concrete` calls went with the pair; they stamped the OpRefs the casts created, and the incoming `value_op` already carries its concrete Ref. Measured on bench/synth/list_pop_append, dynasm, GC-rewritten steady-state loop body: 32 ops -> 22, one CallMallocNursery(32) and the four stores initializing that box -> none, 8 guards -> 6 (the GuardClass and w_class GuardValue applied to the freshly created box fold away once the value keeps the class the boxing site already stamped). Reverting the change puts the allocation back. bench/synth/list_append_write_barrier_gc reports guard_failures 938 -> 941 on all three backends with loops_compiled, bridges_compiled and loops_aborted unchanged; an in-place control arm at the previous base reads 938 before the change and 941 after. Its Object-strategy appends store the value itself, so the box is materialized at the store either way and only the forcing point moves. Baselines re-recorded. Assisted-by: Claude Assisted-by: Codex
youknowone
added a commit
that referenced
this pull request
Aug 14, 2026
`orthodox_list_append_commit` recorded an identity CastPtrToInt -> CastIntToPtr pair on the appended value before descending the `w_list_append` sub-walk, which made the value's pointer identity observable and so materialized an otherwise non-escaping virtual. The pair came in with the range FOR_ITER virtualization (#683) to stop the sub-walk unboxing a loop-carried trace-entry scalar. The forwarding it emulated is already there: `trace_box_int` / `trace_box_float` stamp `class_now_known` and cache the payload field's current SSA box at the boxing site, and `getfield_gc_i_pureornot` returns that cached box on a hit, so the descended unbox reads this iteration's payload with the value left virtual — the same forwarding `OptVirtualize.optimize_GETFIELD_GC_I` performs in rpython/jit/metainterp/optimizeopt/virtualize.py. The two `set_opref_concrete` calls went with the pair; they stamped the OpRefs the casts created, and the incoming `value_op` already carries its concrete Ref. Measured on bench/synth/list_pop_append, dynasm, GC-rewritten steady-state loop body: 32 ops -> 22, one CallMallocNursery(32) and the four stores initializing that box -> none, 8 guards -> 6 (the GuardClass and w_class GuardValue applied to the freshly created box fold away once the value keeps the class the boxing site already stamped). Reverting the change puts the allocation back. bench/synth/list_append_write_barrier_gc reports guard_failures 938 -> 941 on all three backends with loops_compiled, bridges_compiled and loops_aborted unchanged; an in-place control arm at the previous base reads 938 before the change and 941 after. Its Object-strategy appends store the value itself, so the box is materialized at the store either way and only the forcing point moves. Baselines re-recorded. Assisted-by: Claude Assisted-by: Codex
youknowone
added a commit
that referenced
this pull request
Aug 14, 2026
`orthodox_list_append_commit` recorded an identity CastPtrToInt -> CastIntToPtr pair on the appended value before descending the `w_list_append` sub-walk, which made the value's pointer identity observable and so materialized an otherwise non-escaping virtual. The pair came in with the range FOR_ITER virtualization (#683) to stop the sub-walk unboxing a loop-carried trace-entry scalar. The forwarding it emulated is already there: `trace_box_int` / `trace_box_float` stamp `class_now_known` and cache the payload field's current SSA box at the boxing site, and `getfield_gc_i_pureornot` returns that cached box on a hit, so the descended unbox reads this iteration's payload with the value left virtual — the same forwarding `OptVirtualize.optimize_GETFIELD_GC_I` performs in rpython/jit/metainterp/optimizeopt/virtualize.py. The two `set_opref_concrete` calls went with the pair; they stamped the OpRefs the casts created, and the incoming `value_op` already carries its concrete Ref. Measured on bench/synth/list_pop_append, dynasm, GC-rewritten steady-state loop body: 32 ops -> 22, one CallMallocNursery(32) and the four stores initializing that box -> none, 8 guards -> 6 (the GuardClass and w_class GuardValue applied to the freshly created box fold away once the value keeps the class the boxing site already stamped). Reverting the change puts the allocation back. bench/synth/list_append_write_barrier_gc reports guard_failures 938 -> 941 on all three backends with loops_compiled, bridges_compiled and loops_aborted unchanged; an in-place control arm at the previous base reads 938 before the change and 941 after. Its Object-strategy appends store the value itself, so the box is materialized at the store either way and only the forcing point moves. Baselines re-recorded. Assisted-by: Claude Assisted-by: Codex
youknowone
added a commit
that referenced
this pull request
Aug 15, 2026
`orthodox_list_append_commit` recorded an identity CastPtrToInt -> CastIntToPtr pair on the appended value before descending the `w_list_append` sub-walk, which made the value's pointer identity observable and so materialized an otherwise non-escaping virtual. The pair came in with the range FOR_ITER virtualization (#683) to stop the sub-walk unboxing a loop-carried trace-entry scalar. The forwarding it emulated is already there: `trace_box_int` / `trace_box_float` stamp `class_now_known` and cache the payload field's current SSA box at the boxing site, and `getfield_gc_i_pureornot` returns that cached box on a hit, so the descended unbox reads this iteration's payload with the value left virtual — the same forwarding `OptVirtualize.optimize_GETFIELD_GC_I` performs in rpython/jit/metainterp/optimizeopt/virtualize.py. The two `set_opref_concrete` calls went with the pair; they stamped the OpRefs the casts created, and the incoming `value_op` already carries its concrete Ref. Measured on bench/synth/list_pop_append, dynasm, GC-rewritten steady-state loop body: 32 ops -> 22, one CallMallocNursery(32) and the four stores initializing that box -> none, 8 guards -> 6 (the GuardClass and w_class GuardValue applied to the freshly created box fold away once the value keeps the class the boxing site already stamped). Reverting the change puts the allocation back. bench/synth/list_append_write_barrier_gc reports guard_failures 938 -> 941 on all three backends with loops_compiled, bridges_compiled and loops_aborted unchanged; an in-place control arm at the previous base reads 938 before the change and 941 after. Its Object-strategy appends store the value itself, so the box is materialized at the store either way and only the forcing point moves. Baselines re-recorded. Assisted-by: Claude Assisted-by: Codex
youknowone
added a commit
that referenced
this pull request
Aug 15, 2026
`orthodox_list_append_commit` recorded an identity CastPtrToInt -> CastIntToPtr pair on the appended value before descending the `w_list_append` sub-walk, which made the value's pointer identity observable and so materialized an otherwise non-escaping virtual. The pair came in with the range FOR_ITER virtualization (#683) to stop the sub-walk unboxing a loop-carried trace-entry scalar. The forwarding it emulated is already there: `trace_box_int` / `trace_box_float` stamp `class_now_known` and cache the payload field's current SSA box at the boxing site, and `getfield_gc_i_pureornot` returns that cached box on a hit, so the descended unbox reads this iteration's payload with the value left virtual — the same forwarding `OptVirtualize.optimize_GETFIELD_GC_I` performs in rpython/jit/metainterp/optimizeopt/virtualize.py. The two `set_opref_concrete` calls went with the pair; they stamped the OpRefs the casts created, and the incoming `value_op` already carries its concrete Ref. Measured on bench/synth/list_pop_append, dynasm, GC-rewritten steady-state loop body: 32 ops -> 22, one CallMallocNursery(32) and the four stores initializing that box -> none, 8 guards -> 6 (the GuardClass and w_class GuardValue applied to the freshly created box fold away once the value keeps the class the boxing site already stamped). Reverting the change puts the allocation back. bench/synth/list_append_write_barrier_gc reports guard_failures 938 -> 941 on all three backends with loops_compiled, bridges_compiled and loops_aborted unchanged; an in-place control arm at the previous base reads 938 before the change and 941 after. Its Object-strategy appends store the value itself, so the box is materialized at the store either way and only the forcing point moves. Baselines re-recorded. Assisted-by: Claude Assisted-by: Codex
youknowone
added a commit
that referenced
this pull request
Aug 15, 2026
`orthodox_list_append_commit` recorded an identity CastPtrToInt -> CastIntToPtr pair on the appended value before descending the `w_list_append` sub-walk, which made the value's pointer identity observable and so materialized an otherwise non-escaping virtual. The pair came in with the range FOR_ITER virtualization (#683) to stop the sub-walk unboxing a loop-carried trace-entry scalar. The forwarding it emulated is already there: `trace_box_int` / `trace_box_float` stamp `class_now_known` and cache the payload field's current SSA box at the boxing site, and `getfield_gc_i_pureornot` returns that cached box on a hit, so the descended unbox reads this iteration's payload with the value left virtual — the same forwarding `OptVirtualize.optimize_GETFIELD_GC_I` performs in rpython/jit/metainterp/optimizeopt/virtualize.py. The two `set_opref_concrete` calls went with the pair; they stamped the OpRefs the casts created, and the incoming `value_op` already carries its concrete Ref. Measured on bench/synth/list_pop_append, dynasm, GC-rewritten steady-state loop body: 32 ops -> 22, one CallMallocNursery(32) and the four stores initializing that box -> none, 8 guards -> 6 (the GuardClass and w_class GuardValue applied to the freshly created box fold away once the value keeps the class the boxing site already stamped). Reverting the change puts the allocation back. bench/synth/list_append_write_barrier_gc reports guard_failures 938 -> 941 on all three backends with loops_compiled, bridges_compiled and loops_aborted unchanged; an in-place control arm at the previous base reads 938 before the change and 941 after. Its Object-strategy appends store the value itself, so the box is materialized at the store either way and only the forcing point moves. Baselines re-recorded. Assisted-by: Claude Assisted-by: Codex
youknowone
added a commit
that referenced
this pull request
Aug 15, 2026
`orthodox_list_append_commit` recorded an identity CastPtrToInt -> CastIntToPtr pair on the appended value before descending the `w_list_append` sub-walk, which made the value's pointer identity observable and so materialized an otherwise non-escaping virtual. The pair came in with the range FOR_ITER virtualization (#683) to stop the sub-walk unboxing a loop-carried trace-entry scalar. The forwarding it emulated is already there: `trace_box_int` / `trace_box_float` stamp `class_now_known` and cache the payload field's current SSA box at the boxing site, and `getfield_gc_i_pureornot` returns that cached box on a hit, so the descended unbox reads this iteration's payload with the value left virtual — the same forwarding `OptVirtualize.optimize_GETFIELD_GC_I` performs in rpython/jit/metainterp/optimizeopt/virtualize.py. The two `set_opref_concrete` calls went with the pair; they stamped the OpRefs the casts created, and the incoming `value_op` already carries its concrete Ref. Measured on bench/synth/list_pop_append, dynasm, GC-rewritten steady-state loop body: 32 ops -> 22, one CallMallocNursery(32) and the four stores initializing that box -> none, 8 guards -> 6 (the GuardClass and w_class GuardValue applied to the freshly created box fold away once the value keeps the class the boxing site already stamped). Reverting the change puts the allocation back. bench/synth/list_append_write_barrier_gc reports guard_failures 938 -> 941 on all three backends with loops_compiled, bridges_compiled and loops_aborted unchanged; an in-place control arm at the previous base reads 938 before the change and 941 after. Its Object-strategy appends store the value itself, so the box is materialized at the store either way and only the forcing point moves. Baselines re-recorded. Assisted-by: Claude Assisted-by: Codex
youknowone
added a commit
that referenced
this pull request
Aug 15, 2026
`orthodox_list_append_commit` recorded an identity CastPtrToInt -> CastIntToPtr pair on the appended value before descending the `w_list_append` sub-walk, which made the value's pointer identity observable and so materialized an otherwise non-escaping virtual. The pair came in with the range FOR_ITER virtualization (#683) to stop the sub-walk unboxing a loop-carried trace-entry scalar. The forwarding it emulated is already there: `trace_box_int` / `trace_box_float` stamp `class_now_known` and cache the payload field's current SSA box at the boxing site, and `getfield_gc_i_pureornot` returns that cached box on a hit, so the descended unbox reads this iteration's payload with the value left virtual — the same forwarding `OptVirtualize.optimize_GETFIELD_GC_I` performs in rpython/jit/metainterp/optimizeopt/virtualize.py. The two `set_opref_concrete` calls went with the pair; they stamped the OpRefs the casts created, and the incoming `value_op` already carries its concrete Ref. Measured on bench/synth/list_pop_append, dynasm, GC-rewritten steady-state loop body: 32 ops -> 22, one CallMallocNursery(32) and the four stores initializing that box -> none, 8 guards -> 6 (the GuardClass and w_class GuardValue applied to the freshly created box fold away once the value keeps the class the boxing site already stamped). Reverting the change puts the allocation back. bench/synth/list_append_write_barrier_gc reports guard_failures 938 -> 941 on all three backends with loops_compiled, bridges_compiled and loops_aborted unchanged; an in-place control arm at the previous base reads 938 before the change and 941 after. Its Object-strategy appends store the value itself, so the box is materialized at the store either way and only the forcing point moves. Baselines re-recorded. Assisted-by: Claude Assisted-by: Codex
youknowone
added a commit
that referenced
this pull request
Aug 15, 2026
* jit: stop forcing the appended value's box in the list-append fold `orthodox_list_append_commit` recorded an identity CastPtrToInt -> CastIntToPtr pair on the appended value before descending the `w_list_append` sub-walk, which made the value's pointer identity observable and so materialized an otherwise non-escaping virtual. The pair came in with the range FOR_ITER virtualization (#683) to stop the sub-walk unboxing a loop-carried trace-entry scalar. The forwarding it emulated is already there: `trace_box_int` / `trace_box_float` stamp `class_now_known` and cache the payload field's current SSA box at the boxing site, and `getfield_gc_i_pureornot` returns that cached box on a hit, so the descended unbox reads this iteration's payload with the value left virtual — the same forwarding `OptVirtualize.optimize_GETFIELD_GC_I` performs in rpython/jit/metainterp/optimizeopt/virtualize.py. The two `set_opref_concrete` calls went with the pair; they stamped the OpRefs the casts created, and the incoming `value_op` already carries its concrete Ref. Measured on bench/synth/list_pop_append, dynasm, GC-rewritten steady-state loop body: 32 ops -> 22, one CallMallocNursery(32) and the four stores initializing that box -> none, 8 guards -> 6 (the GuardClass and w_class GuardValue applied to the freshly created box fold away once the value keeps the class the boxing site already stamped). Reverting the change puts the allocation back. bench/synth/list_append_write_barrier_gc reports guard_failures 938 -> 941 on all three backends with loops_compiled, bridges_compiled and loops_aborted unchanged; an in-place control arm at the previous base reads 938 before the change and 941 after. Its Object-strategy appends store the value itself, so the box is materialized at the store either way and only the forcing point moves. Baselines re-recorded. Assisted-by: Claude Assisted-by: Codex * jit: preserve box identity across short preambles * jit: lazily load frozen indirect call targets * jit: load the build-time descr pool per index instead of as one table `descrs.bin` was one `bincode::serialize(&Vec<BhDescr>)`, and the blackhole builder's `setup_descrs` took the deserialized slice, so constructing the builder materialized all 4897 entries. `size_of::<BhDescr>()` is 552 bytes and the per-entry `String`/`Vec`/`HashMap` payloads sit behind that, so the 1.78 MB artefact expanded to 10.7 MB of retained heap. A run names 7 to 33 of those entries. Serialize each entry independently and add `descrs_index.bin` carrying the byte offsets, matching the `jitcodes.bin` / `jitcodes_index.bin` pair. `descrs` on the builder and on each blackhole frame becomes `&'static dyn DescrTable`; `blackhole.py:102-103` only ever indexes the list, so the interface is unchanged. Entries materialize on the index that names them and are leaked for `&'static`, as the sibling jitcode table already does. `rehydrate_build_descr_raw_sets` keeps its ordering and still visits every entry, but through `load_descr_uncached`, which drops each one after use: visiting the pool no longer implies retaining it. `descr_ref_at` calls the rehydration `Once` before resolving so the container groups are published before `make_descr_from_bh` reads the gccache. `DescrTable::get` takes `&'static self`. Every holder is already a `&'static dyn DescrTable`, and it lets the slice impl return a `&'static BhDescr` without widening a borrow the type system never checked. Adds `PYRE_DESCR_DEMAND`, which tallies the distinct pool indices a run resolves, and an ignored `descr_startup_rss_decomposition` measurement. Release RSS, same-run A/B against the parent binary: `pass` 74.6 -> 52.4 MB, int loop 87.2 -> 62.1, list_pop_append 92.9 -> 66.6, call_loop_local 89.7 -> 63.6. Trace shape is unchanged: loops_compiled, bridges_compiled and guard_failures are identical on both binaries for exception_traceback_loop_forms, inline_chain_depth_typeflip and check_exc_match_invalid_class, and descr_set_absent / ambiguous / stale_absent stay zero. Assisted-by: Claude * descr: mint the ExecutionContext group as a non-GC-managed struct `EC_DESCR_GROUP` used `make_simple_descr_group`, which hardcodes the GC-managed, headered shape. `ExecutionContext` is a plain Rust struct — `EC_SIZE` is `size_of::<ExecutionContext>()` and the field offsets come from `offset_of!` — so it carries no type-id word at `ref - GcHeader::SIZE`. `StructPtrInfo.make_guards` gates `GUARD_GC_TYPE` on `is_gc_managed() && !headerless()`, so the group emitted `GUARD_GC_TYPE(ec, 0)`: a guard reading the word before the EC allocation and comparing it against the group's own `type_id 0`. It failed on every loop re-entry once the exported short-preamble state began carrying a `StructPtrInfo` for the EC pointer. Mint through `make_simple_descr_group_with_flags` with `is_gc_managed = false`. On dynasm, `check_exc_match_invalid_class`, `type_immutable_reject` and `exception_value_op_caught` return to their recorded jitstats (`check_exc_match_invalid_class` guard_failures 201 -> 1), and no `GuardGcType` remains in the compiled loop. Assisted-by: Claude * optimizeopt: skip GUARD_GC_TYPE when the descr names no type id `StructPtrInfo.make_guards` / `ArrayPtrInfo.make_guards` read `descr.type_id()` and emit `GUARD_GC_TYPE` against it. A serialized `BhDescr::Array` that carries neither a `gc_type_id` nor a cache key resolves to 0 through `BhDescr::resolve_gc_tid`, because the runtime array type ids are handed out by `gc.register_type` at interpreter startup and the build-time analyzer cannot see them. The guard was then emitted as `GUARD_GC_TYPE(x, 0)`. 0 is not an absent value at runtime — it is the `rclass.OBJECT` root header — so the guard is wrong in both directions: it fails on every object with a real header, and passes on a plain `object`, certifying a layout the optimizer never named. The tid allocator starts at 1, so a 0 on the descr means no identity was ever assigned. Gate both arms on `type_id() != 0`. `GUARD_GC_TYPE` installs no info in the optimizer (`rewrite.rs` passes it through or removes it on a constant), so the skip costs only the runtime re-check. On dynasm this returns `list_pop_append` (guard_failures 201 -> 1), `minmax_key_rooting` (205 -> 5) and `listcomp_hot` (470 -> 239, bridges 2 -> 1) to their recorded jitstats, with `list_pop_append` still answering `5 0` and the `from_opref` rotation-loop reproducers still silent. Assisted-by: Claude * majit: bind a rebuilt short preamble's inputarg domain for the next retrace `ExtendedShortPreambleBuilder::setup` seeded `phase1_to_inputarg` only from each entry's `arg_mapping`, so an op whose argument was the original loop's `InputArgRef` — not a mapped Label position — had no binding. Seed the map positionally from `short_preamble.inputargs` first, and record the remapped domain in a new `ShortPreamble::phase1_inputargs` so a preamble rebuilt by an active builder can be re-bound by the next one. `jump_to_preamble` seeds the same domain from the live builder's label args. Heap replay in `OptContext` now emits `preamble_op.arg(0)` / `.arg(1)` rather than routing them through `dep_or_materialize`, which collapsed the `preamble_op` and `source_op` receiver identities and produced guards on an exporting-phase box. `resolve_arg` is still called to decide whether the operands are bindable at all. `retrace_outer_loop_type_flip` goes from `loops_aborted=2 retraces_compiled=0` back to its recorded `loops_aborted=0 retraces_compiled=1 bridges_compiled=1 guard_failures=201`; six other synth fixtures return to their recorded jit-stats. Assisted-by: Claude * bench: re-record the synth jit-stats this branch moves Each counter below was attributed against an `origin/main` (d5ae680) control arm built in place from the branch's touched-file list, on both the dynasm and cranelift backends. Only fixtures where the control reproduces the committed baseline exactly — that is, where the delta is this branch's — are re-recorded here. No badness field moved in any of them. exc_mixed_classes_bridge_flavor, exception_bridge_traceback_head loops_compiled 2 -> 1, bridges_compiled 4 -> 3, guard_failures 802 -> 601. Reverting `jit: preserve box identity across short preambles` reproduces 4/802/2, so that commit accounts for the whole delta. The dropped loop and bridge are not declines: FIRED=3, cb_entered=3, bridges_compiled=3 with cb_invalidloop, cb_arity_giveup, ceb_*, retrace_bailed, wct_declined and cl_hct_giveup all zero on both backends. A guard site that used to fail 201 extra times is gone, so its bridge is never requested. Both fixtures still print their pinned expected output. inline_chain_depth_typeflip guard_failures 3681 -> 3702 list_append_write_barrier_gc guard_failures 1345 -> 1348 bound_method_builtin_fold guard_failures 458 -> 459 (cranelift only) Structure is unchanged — loops_compiled and bridges_compiled hold. The first reproduces 3702 across three runs against the control's 3681 across two. `list_append_write_barrier_gc` prints the same five lines as CPython. Left alone deliberately: `gc_bug_bridge_flavor_traceback_names` (+3) and `exception_escape_hot_callee_tb_node_once` (loops_compiled 16 -> 15) reproduce identically on the control, and `sre_pattern_methods` / `sre_wasm_min` are byte-identical between branch and control. Those baselines are stale against main, not against this branch. The `.wasm.jitstats` files are untouched because no wasm arm was measured; wasm counters are not a copy of dynasm's (`inline_chain_depth_typeflip` records 3820 there, not 3681). Assisted-by: Claude * bench: restore the three small jit-stats deltas to their committed values The previous commit re-recorded five fixtures. Three of them are being put back: `inline_chain_depth_typeflip`, `list_append_write_barrier_gc` and `bound_method_builtin_fold` (cranelift). Their deltas were +21, +3 and +1 guard_failures with loops_compiled and bridges_compiled unchanged, which is the profile of warmup-table drift rather than a codegen change: `make_green_key` builds the JitCell uhash from the pycode heap address, so which counter cells cohabit a bucket — and therefore which units reach their trace threshold — depends on total prior allocation, i.e. on every byte of the binary. The decisive evidence is that these counters are not a single number across platforms. For `inline_chain_depth_typeflip` the windows leg of run 31723002466 compared against `bridges_compiled=19, guard_failures=3818` while the shared file records 18/3681, and the macOS leg did not flag the fixture at all. Writing a number measured from one local darwin binary into a baseline shared by every platform would trade a row that passes on macOS for one that does not. That is the same standard already applied to `gc_bug_bridge_flavor_traceback_ names`, `exception_escape_hot_callee_tb_node_once` and the `sre_*` pair, which were left untouched for the same reason. `exc_mixed_classes_bridge_flavor` and `exception_bridge_traceback_head` keep their new values. Those are structural — a whole loop and a whole bridge — they were attributed to a single commit by reverting it, and CI measured exactly the same transition (`loops_compiled 2 -> 1, bridges_compiled 4 -> 3, guard_failures 802 -> 601`) on its own binary. Assisted-by: Claude * gate-triage: register PYRE_DESCR_DEMAND, and re-record four CI-confirmed benches `every_live_pyre_gate_has_a_gate_triage_entry` failed on all three cargo-test legs: `PYRE_DESCR_DEMAND`, added with the per-index descr pool loader, reads the environment but had no row in pyre/gate-triage.md. It is a default-OFF measurement probe with no ON behaviour to graduate, so it joins §5's diagnostics bucket with a note that it retires with the demand counter itself. The jit-stats re-records are the four benches CI observed at exactly the values measured here, which is the corroboration the previous commit was missing when it put three of them back: inline_chain_depth_typeflip guard_failures 3681 -> 3702 list_append_write_barrier_gc guard_failures 1345 -> 1348 inheritance_dispatch bridges_compiled 3 -> 4, guard_failures 601 -> 801 nested_loop_gate_switch bridges_compiled 6 -> 7, guard_failures 1796 -> 1900 The macOS leg of run 31796630818 printed those transitions verbatim, so they are a property of the tree rather than of one local binary. The last two are a compile-set effect, not codegen. Saved arms bisect them to `majit: bind a rebuilt short preamble's inputarg domain for the next retrace`: the arm carrying every other commit reproduces 3/601 and 6/1796. For `inheritance_dispatch` the GC-rewritten steady loop is identical across the two arms — 40 ops, same opcodes in the same order, differing only in SSA numbering and in heap addresses embedded as GuardClass/GuardValue immediates — so the extra bridge is an extra compiled unit, not a changed loop body. Still not re-recorded, because the value measured here is not the value CI reports: `str_fstring` (cranelift) and `bound_method_builtin_fold` (cranelift) pass locally against their darwin baselines. Assisted-by: Claude * optimizeopt: resolve a layout guard's runtime tid, or decline the short preamble `StructPtrInfo`/`ArrayPtrInfo::make_guards` read the descr's stamped `type_id()` and skipped `GUARD_GC_TYPE` when it was 0. The skip removed the only layout check on that short-preamble entry, so a loop could be re-entered with a different GC representation while the hoisted accesses kept the original descr's element interpretation. 0 is never a legitimate stamp — the allocator starts at 1 — but it is a live header value (the `rclass.OBJECT` root), so guarding on it is wrong in both directions and skipping it is unsound. Resolve the dense tid from the structural `cache_key` through `gc_cache`, the same route `resolve_gc_tid` takes, and decline through `signal_invalid_loop` when even that fails. `make_guards` returns `bool`; `collect_use_box_guards` returns `Option`. Resolved array tids are stamped back through `set_type_id`. Struct tids are not: `SizeDescr` has no shared-reference setter. Assisted-by: Claude * optimizeopt: assert import_state's source/target on box identity `unroll.py:496 assert source is not target` compares Box identity. The port compared `OpRef` positions, which the surrounding code expects to coincide — that is why it forwards to the carried `Rc` instead of re-materializing by position — so the assertion fired in debug builds. Compare the resolved `Operand`s, whose `PartialEq` is `Rc::ptr_eq`. Also record why the neighbouring short-preamble seed zips two lists of different lengths: the builder's Label domain and the body's jump args agree only on their common prefix. Requiring equal arities takes `retrace_outer_loop_type_flip` to `loops_aborted` 0 -> 2, `retraces_compiled` 1 -> 0, `guard_failures` 201 -> 590 on both backends. Assisted-by: Claude * jit: keep the first index for a folded runtime fnaddr Identical-code folding can map several build-time addresses onto one runtime address, which the `FNADDR_CORRESPONDENCE` note already describes, so the `assert!` on a duplicate insert aborted the process on a legitimate layout. Keep the first index instead. `indirect_target_lookup_decodes_only_the_matched_jitcode` compared `JitCode.fnaddr`, a build address, against a runtime-address map key; translate before comparing. Its cell-count assertions are absolute because `load_jitcode_cells` leaks a fresh slice per thread, so the `spawn` is the isolation — say so at the test. Record why `frozen_indirectcall_dict` stays on the thread-local state: it is what gives repeated lookups one `JitCode` object, and the jitcode arena it derives from is per-thread, so a process-wide map would hand one thread a body minted from another thread's family. Assisted-by: Claude * bench: re-record the six wasm jit-stats baselines this branch moves The recorded wasm values encoded a wasm-vs-dynasm divergence that no longer exists. Against the dynasm baselines checked in beside them, the values CI observes on wasm now match exactly for five of the six — exc_mixed_classes_bridge_flavor and exception_bridge_traceback_head at 1/3/601, inheritance_dispatch at 1/4/801, list_append_write_barrier_gc at 12/5/1348, nested_loop_gate_switch at 2/7/1900 — and inline_chain_depth_typeflip agrees on loops and bridges (6/18) while its guard_failures reads 3745. The direction differs per fixture, always toward dynasm, so this is convergence rather than drift. Two ubuntu CI runs (31805976746 and 31817779249) report identical numbers for every one of the six, and no fixture header forbids re-recording. Only loops_compiled, bridges_compiled and guard_failures are rewritten; no badness field moved. Assisted-by: Claude * descr: mint the four PyCode field descrs as one group `PYCODE_CODE_PTR_FIELD_DESCR`, `PYCODE_W_NAME_FIELD_DESCR`, `PYCODE_CO_FIRSTLINENO_FIELD_DESCR` and `PYCODE_HIDDEN_APPLEVEL_FIELD_DESCR` were standalone `PyreFieldDescr`s carrying `parent_descr: None`, but all four are handed to `GetfieldGc*`. `ensure_ptr_info_arg0` reads `descr.get_parent_descr()` whenever arg0 has no pointer info yet (`optimizer.py:478`) and panicked there: `getframe_root_loop_force_blackhole_crn_nonidempotent` aborted on all three backends. The same `parent_descr: None` is present on the base revision; this branch reached the path. Mint the four through `make_simple_descr_group_with_flags`, so each field's `parent_descr` is the owning SizeDescr and `index_in_parent` is its offset-sorted slot. Offsets, field sizes, field types, signedness, mutability and names are unchanged. The group carries `W_CODE_GC_TYPE_ID` with `is_gc_managed = true`. The unkeyed factory publishes only into the JIT descriptor snapshot — `register_external_size` appends to `_cache_size_order` and never writes `_cache_size[key]`, which is what `resolve_struct_tid` reads — so the collector's `TypeInfo` table stays solely owned by `eval::initialize_gc`, and `StructPtrInfo::make_guards` can emit `GUARD_GC_TYPE(code, 43)` against the header `gc.register_type` already stamps. Assisted-by: Claude * majit: log the pre-optimization trace under jit-log-noopt `compile.py:49-50 CompileData.optimize_trace` calls `logger_noopt.log_loop_from_trace(self.trace)`, which `logger.py:15-24` wraps in a `jit-log-noopt` section headed by the traced op count. pyre emitted only `jit-log-opt-loop` / `jit-log-opt-bridge`, so no section showed the trace as the optimizer receives it. Emit the section at the optimizer entry in `compile_loop`, alongside the existing `[jit-diag] entering optimizer` line. Assisted-by: Claude * majit: cache ordinary heap fields read off a virtualizable receiver `PtrInfo::Virtualizable(VirtualizableFieldState)` had no arm in any field accessor: `setfield` and `clear_field` fell through to `_ => {}`, `getfield` and `has_preamble_field` to `_ => None` / `false`, and `set_preamble_field`'s catch-all re-seated the whole PtrInfo as an `InstancePtrInfo`, dropping the tracked virtualizable state. `ensure_ptr_info_arg0` also lists the variant among the kinds it returns unchanged, so it is never upgraded to an info that can hold fields. Every ordinary heap field written to a virtualizable receiver was therefore discarded and every later read of it missed. `info.py` has no virtualizable-specific subclass — the hierarchy ends at `InstancePtrInfo` / `StructPtrInfo` — so upstream a virtualizable frame carries a plain `InstancePtrInfo` and `optimizer.py:484 init_fields` gives each slot a home in the one `_fields` list the heap cache consults. Add `heap_fields` to `VirtualizableFieldState`, keyed by `FieldDescr::index_in_parent`, and give the five accessors their arm. It cannot share the existing `fields` vec, which is indexed in `VirtualizableInfo::static_fields` order. `clear_field` is what `CachedField::invalidate` clears through, so without that arm a cached value would survive a call. PyFrame is the virtualizable, so `inline_helper` traced six unfolded `getfield_gc_r(p0, PyFrame.execution_context)` off one frame. Because those receivers were distinct, the frame push/pop `topframeref` stores landed in different slots and never coalesced; an emitted store of a virtual VRef forces it, which is where the `NewWithVtable(VRefSizeDescr)` and the per-enter/leave `ForceToken` came from. Measured on `pyre/bench/inline_helper.py`, dynasm, `PYRE_NO_UNROLL=1` compiled loop: 74 -> 52 ops, execution_context loads 6 -> 1, topframeref 10 -> 1, NewWithVtable 4 -> 2. Peeled: 125 -> 79 ops, execution_context 12 -> 1, topframeref 20 -> 1, NewWithVtable 4 -> 0. Output, loops_compiled, bridges_compiled and guard_failures unchanged. Wall clock, min of 9 against pypy 7.3.20: dynasm 1.80x -> 1.35x, cranelift 2.94x -> 1.37x. Assisted-by: Claude * bench: re-record twelve synthetic jitstats baselines Twenty-four files, twelve fixtures across dynasm and cranelift. Every moved counter was attributed against an in-place control arm built from the same base with a8c159480ef reverted. Control and HEAD agree, both differ from the recorded baseline, so the move came from the base rather than from a8c159480ef: comprehension_object_append_hot bridges 18->14, guards 3610->2810 comprehension_param_range_call_flush bridges 3->2, guards 600->400 const_arg_call_resume bridges 9->6, guards 1804->1204 foriter_setadd_call_consuming_body bridges 22->21, guards 3980->3780 list_append_write_barrier_gc bridges 5->4, guards 1348->1152 nested_list_comprehension_hot bridges 6->4, guards 1202->802 recursive_forced_frame_kept_stack bridges 5->4, guards 1000->800, fbw_rolled_back_with_effects 0->1 listcomp_hot guards 239->220 Those eight were last recorded at #1086, #1166 and 7cb84760d5b. Moved by a8c159480ef. Timings are direct min-of-N runs of the two binaries, dynasm then cranelift: generator_tree_recursion guards +/-1 (dynasm 2952->2951, cranelift 2951->2952); -3.3% / -4.4% exc_mixed_classes_bridge_flavor loops 1->2, bridges 3->4, guards 601->802; -25.5% / -22.7% at N=6000000 inline_chain_depth_typeflip bridges 18->19, guards 3702->3819; -17.8% / -24.8% exception_bridge_traceback_head loops 1->2, bridges 3->4, guards 601->802; +6.4% / +3.6% at N=600000 exception_bridge_traceback_head is the only fixture that got slower. It carries the same counter movement as exc_mixed_classes_bridge_flavor, which gets 25% faster, and differs from it only by reading e.__traceback__.tb_frame.f_code.co_name in the handler. Output is byte-identical to the control for all twelve fixtures on both backends, rc=0. retraces_compiled=0 is written into the files that lacked the key. Assisted-by: Claude * bench: re-record eleven synthetic wasm jitstats baselines Measured from a wasm run of `pyre/check.py --snapshot --backend wasm` on the rebased tree. The dynasm and cranelift halves of these fixtures were recorded in the previous commit; the wasm halves were not, and the linux leg is the only one that runs the wasm backend. The wasm counters are not a copy of the other two backends'. Two fixtures read differently there: comprehension_object_append_hot bridges 18->17, guards 3610->3410 (dynasm/cranelift: 18->14, 3610->2810) inline_chain_depth_typeflip guards 3745->3818 (dynasm/cranelift: 3702->3819) The remaining nine move as their dynasm and cranelift counterparts do. Assisted-by: Claude * bench: correct three synthetic wasm jitstats baselines arith_int_bool bridges 10 -> 11, guards 2211 -> 2307 comprehension_object_append_hot bridges 17 -> 14, guards 3410 -> 2810 short_circuit_value_kept_stack bridges 12 -> 11, guards 2510 -> 2201 Each value is what the ubuntu leg observed on run 31879590864 and what a local wasm run now reads, so the two agree exactly. comprehension_object_append_hot was recorded at 17/3410 one commit ago. That reading came from a wasm run whose wasmtime `.cwasm` module cache had not been rebuilt for the tree under test, so it measured an older module; the shared baseline it produced disagreed with every other backend on the same host. A wasm re-record is only valid against a freshly built module. arith_int_bool and short_circuit_value_kept_stack were not touched by this branch. Their counters moved under #1231, which keys the applied write-barrier set through SameAs forwarding. Assisted-by: Claude
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
wasm-jit correctness fixes. The first three commits fix a hang/miscompile of nested
for _ in range(N): for x in range(P): total += x, which stormed (a 0-progress infinitebridge) for any OUTER ≳ 50. The fourth is an independent wasm self-recursive
CALL_ASSEMBLERexception-unwind fix.virtualize range FOR_ITER items with guarded exhaustion
Replaces the range-iterator pointer mask with a snapshot-backed continuation guard so a locally consumed item stays a removable virtual
New. Takes the exhaustion edge before the continuation guard (returns the loop-exit null for the trailingGuardNonnull), so a nested inner loop run to completion does not record aGuardTrueagainst a concretely-false condition.heal nested for-range degenerate loop; virtualize range GET_ITER
optimizeopt/unroll.rs:assemble_peeled_trace_with_jump_argsno longer filters base label args through the backend-constants map.import_statealready dropsLEVEL_CONSTANTvirtual-state slots atmake_inputargstime; re-deriving the slot set post-hoc drops a still-live loop-carried box that a phase-2 guard postprocess const-forwarded (the exhaust guard'sremaining), desyncing the label from the positionally-aligned jump and orphaning the head guard's operand.jitcode_dispatch:FbwWalkMode.bridge_entry_merge_pccarries a guard-failure bridge's own resume python-pc; thejit_merge_pointarm skips exactly the first crossing that lands on it. A guard's resume coordinate lies past the dispatch-topjit_merge_point(generate_guard resumepc=orgpc), so an RPython bridge never re-crosses the loop-header merge point at position zero; the walker resumes at the opcode boundary and would otherwise close a 0-op no-op bridge → storm.GET_ITERon an exactrangeemits the virtualW_IntRangeIteratorallocation shape (newGetIterhelper kind,W_Rangefield descriptors/offsets,is_exact_w_range).Net effect: a Loop + healing Bridge sharing one target token, matching PyPy's structure for this nested loop.
register _collections deque iterators as GC roots
Adds
_deque_iterator/_deque_reverse_iteratorto the immortalregister_pyre_class_offsetsblock so their manageddequechild is traced.route CALL_ASSEMBLER callee ExitFrameWithException exits through the deopt helper
FailDescr::is_finish()is true for bothDoneWithThisFrameandExitFrameWithExceptionFINISH exits. The self-recursive
CALL_ASSEMBLERarm picked its "clean callee finish"fail_index(bridge_finish_fiin codegen,loop_finish_fiin the published dispatchmetadata) as the first
is_finishguard, which could be anExitFrameWithException. Theguest arm then short-circuited that exit to output slot 0, banking the raised exception
object as the recursive-call return value (surfaced as
int + <exc>). Native has no CA arm(it calls the compiled portal directly), so its callee exception flows through the normal
portal-return →
GUARD_NO_EXCEPTIONchannel and never hits thisis_finish-classifiedfail-index set — the miscompile was wasm-only.
Fix: exclude
ExitFrameWithExceptionfrom both finish-index selectors(
meta_descr_is_exit_frame_with_exception) so an exception finish routes towasm_ca_resume_deopt; a newFinishedExceptionoutcome there publishes the exception viastore_jit_exceptionand returns garbage so the caller'sGUARD_NO_EXCEPTIONfires — paritywith the outer Finished arm and
handle_blackhole_result'sExitFrameWithExceptionRefarm.Only exception finishes route to the host, so the #501 CA fast-path for non-raising recursion
is preserved. Fixes
synth/selfrec_tail_exception_unwind(wasint + ZeroDivisionErrorTypeError, now 4250).
Verification
nfr.pysweep 25/25 (primary10 200 → 9000, regressionsv1_single_for/nfr_const/nfr_var, P×OUTER sweep {3,7,10,11,13,17,100}×{200,700,2000})check.py --synthetic-only: dynasm 230/230, cranelift 230/230, wasm 230/230🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
rangeiteration and iterator creation for faster execution.Bug Fixes