Repository navigation
jit: walker-native range FOR_ITER-next specialization with polymorphic demotion - #545
Conversation
|
Warning Review limit reached
Next review available in: 34 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 (6)
WalkthroughAdds walker-native specialization for ChangesRange FOR_ITER specialization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FOR_ITER dispatch
participant Walker specialization
participant W_IntRangeIterator
participant Trace failure tracking
participant JIT driver
FOR_ITER dispatch->>Walker specialization: specialize ForIterNext
Walker specialization->>W_IntRangeIterator: update current and remaining
W_IntRangeIterator-->>FOR_ITER dispatch: boxed item or NULL
FOR_ITER dispatch->>Trace failure tracking: record specialized guard failure
Trace failure tracking->>JIT driver: invalidate and remove compiled loop
JIT driver-->>FOR_ITER dispatch: resume in blackhole
Possibly related PRs
Suggested reviewers: 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 0fb62c0). 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)None. 4. Structural adaptations
|
![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: e3f69bf2f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let specialized = | ||
| RANGE_FORITER_SPECIALIZED_GUARDS.with(|s| s.borrow().contains(&(green_key, fail_index))); | ||
| if specialized { | ||
| fbw_decline(green_key); |
There was a problem hiding this comment.
When this fires for a polymorphic FOR_ITER, it puts the green key into FBW_DECLINED_KEYS; however trace_bytecode checks that set before calling full_body_walk_trace and otherwise returns TraceAction::Abort through the retired trait path (trace.rs lines 621-651). That means a site that first traced as range and later sees a list/generator never gets the intended retrace with the generic ForIterNext residual after invalidation; every hot retry just skips the walker and aborts. This needs a separate range-demotion set that only makes try_walker_specialize_for_iter_next return None.
Useful? React with 👍 / 👎.
e3f69bf to
d08bbf1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d08bbf170f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let guard_fail_index = ctx.trace_ctx.num_guards() as u32; | ||
| let type_const = ctx.trace_ctx.const_int(range_iter_type_addr); | ||
| ctx.trace_ctx | ||
| .record_guard(OpCode::GuardClass, &[iter_op, type_const], 0); |
There was a problem hiding this comment.
This captures the range guard's ordinal from the recorder before optimizeopt runs, but handle_fail compares it with the backend fail_index assigned later by build_guard_metadata over the optimized compiled_ops. When any earlier guard is removed or folded (for example a heapcached class/null guard), the range guard's runtime index shifts, so a polymorphic list/seq iterator will miss the demotion path, or an unrelated guard at the old ordinal can be treated as the range guard. The demotion marker needs to be tied to a stable guard descriptor/trace identity or filled after guard indices are finalized.
Useful? React with 👍 / 👎.
310fa58 to
08858cd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08858cd792
ℹ️ 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".
| merge_point_pcs() | ||
| .filter(|&pc| pc >= entry) | ||
| .min() | ||
| .or_else(|| merge_point_pcs().filter(|&pc| pc <= entry).max()) |
There was a problem hiding this comment.
When the hot counter fires at an inner loop of a for inside a while, this forward-only selection now seeds the inner merge point and lets that nested header compile. The previous code explicitly detected this case and forced a decline because nested-loop bridge closure still miscompiles; that limitation is still documented in jitcode_dispatch.rs:24325-24338, and this commit does not change that close path. Please keep the nested-header exception until that bridge bug is fixed, otherwise these nested loops can produce wrong results instead of staying interpreted.
Useful? React with 👍 / 👎.
08858cd to
e47188a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e47188a880
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let ei = call_descr.get_extra_info(); | ||
| let provably_side_effect_free = ei.check_is_elidable() | ||
| || ei.extraeffect == majit_ir::ExtraEffect::LoopInvariant | ||
| || ei.pyre_helper == majit_ir::PyreHelperKind::ForIterNext; |
There was a problem hiding this comment.
When a callee is inlined while the caller has an in-flight FOR_ITER, guard failure resumes by re-executing the whole callee from the caller's CALL boundary. Treating PyreHelperKind::ForIterNext as side-effect-free lets a helper that advances an iterator supplied by its caller (for example, returning the first item from an iterator) pass this precheck; the sub-walk has already consumed one element, and an outer abort/delivery path can then replay the call and consume a second one. Please only allow this when the iterator is proven fresh/local, otherwise keep the residual/non-inline path.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyre/pyre-jit-trace/src/trace.rs (1)
637-680: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM on the merge-point selection rewrite; consider a regression test for sibling/nested loops.
The new "first merge point at/after
entry, fall back to last one at/before" logic and its rationale (mirroringopimpl_jit_merge_point, sibling loops forward-selecting their own header) read soundly, and it's correctly parameterized onentry(unlikeloop_body_has_abort_permanentbelow). No existing test appears to exercise a function with two independent loops going through this path specifically; given the removedheader_entry_is_nestedhelper it replaces was presumably handling exactly that case, a small regression test would give confidence here.🤖 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/trace.rs` around lines 637 - 680, The merge-point selection in loop_header_merge_point_regs lacks regression coverage for sibling and nested loops. Add a focused test exercising multiple independent loops, including a nested loop, through this path and verify each loop selects its own governing merge point and produces the expected register lists; preserve existing behavior for straight-line entries and body_resume selection.pyre/pyre-jit/src/eval.rs (1)
1921-1926: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd destructors for
W_ModuleDictObjectandW_SetObject
W_ModuleDictObjectownsdstorage,mstrategy, andobject_storage, andW_SetObjectownsitems; without a destructor on GC reclamation, those native allocations leak.🤖 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/eval.rs` around lines 1921 - 1926, Register destructors for W_ModuleDictObject and W_SetObject so their owned native allocations are released during GC reclamation. Update the type registration around w_module_dict_tid to use the destructor-capable TypeInfo constructor, and make the corresponding W_SetObject registration invoke cleanup for dstorage, mstrategy, object_storage, and items through the appropriate existing destruction mechanism.
🤖 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/trace.rs`:
- Around line 2286-2337: Update loop_body_has_abort_permanent to use start_pc
when selecting the governing jit_merge_point and back-edge, rather than the
first merge point in the entire jitcode. Restrict the abort_permanent scan and
loop boundary calculations to the loop body associated with start_pc, while
preserving the existing exception-table and full-scan behavior.
- Around line 96-157: Scope RANGE_FORITER_SPECIALIZED_GUARDS to the current
trace so a retrace that omits the range guard cannot reuse a stale fail ordinal.
Update record_range_foriter_specialization and range_foriter_guard_failed, plus
their callers as needed, to include and match a trace identifier or explicitly
clear entries when retracing; preserve demotion only for the exact guard failure
from the active trace.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/trace.rs`:
- Around line 637-680: The merge-point selection in loop_header_merge_point_regs
lacks regression coverage for sibling and nested loops. Add a focused test
exercising multiple independent loops, including a nested loop, through this
path and verify each loop selects its own governing merge point and produces the
expected register lists; preserve existing behavior for straight-line entries
and body_resume selection.
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 1921-1926: Register destructors for W_ModuleDictObject and
W_SetObject so their owned native allocations are released during GC
reclamation. Update the type registration around w_module_dict_tid to use the
destructor-capable TypeInfo constructor, and make the corresponding W_SetObject
registration invoke cleanup for dstorage, mstrategy, object_storage, and items
through the appropriate existing destruction mechanism.
🪄 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: a6748391-41ef-4a88-b634-04ab5154df0a
📒 Files selected for processing (4)
majit/majit-metainterp/src/jitdriver.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/eval.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyre/pyre-jit-trace/src/trace.rs (1)
637-680: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM on the merge-point selection rewrite; consider a regression test for sibling/nested loops.
The new "first merge point at/after
entry, fall back to last one at/before" logic and its rationale (mirroringopimpl_jit_merge_point, sibling loops forward-selecting their own header) read soundly, and it's correctly parameterized onentry(unlikeloop_body_has_abort_permanentbelow). No existing test appears to exercise a function with two independent loops going through this path specifically; given the removedheader_entry_is_nestedhelper it replaces was presumably handling exactly that case, a small regression test would give confidence here.🤖 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/trace.rs` around lines 637 - 680, The merge-point selection in loop_header_merge_point_regs lacks regression coverage for sibling and nested loops. Add a focused test exercising multiple independent loops, including a nested loop, through this path and verify each loop selects its own governing merge point and produces the expected register lists; preserve existing behavior for straight-line entries and body_resume selection.pyre/pyre-jit/src/eval.rs (1)
1921-1926: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd destructors for
W_ModuleDictObjectandW_SetObject
W_ModuleDictObjectownsdstorage,mstrategy, andobject_storage, andW_SetObjectownsitems; without a destructor on GC reclamation, those native allocations leak.🤖 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/eval.rs` around lines 1921 - 1926, Register destructors for W_ModuleDictObject and W_SetObject so their owned native allocations are released during GC reclamation. Update the type registration around w_module_dict_tid to use the destructor-capable TypeInfo constructor, and make the corresponding W_SetObject registration invoke cleanup for dstorage, mstrategy, object_storage, and items through the appropriate existing destruction mechanism.
🤖 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/trace.rs`:
- Around line 2286-2337: Update loop_body_has_abort_permanent to use start_pc
when selecting the governing jit_merge_point and back-edge, rather than the
first merge point in the entire jitcode. Restrict the abort_permanent scan and
loop boundary calculations to the loop body associated with start_pc, while
preserving the existing exception-table and full-scan behavior.
- Around line 96-157: Scope RANGE_FORITER_SPECIALIZED_GUARDS to the current
trace so a retrace that omits the range guard cannot reuse a stale fail ordinal.
Update record_range_foriter_specialization and range_foriter_guard_failed, plus
their callers as needed, to include and match a trace identifier or explicitly
clear entries when retracing; preserve demotion only for the exact guard failure
from the active trace.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/trace.rs`:
- Around line 637-680: The merge-point selection in loop_header_merge_point_regs
lacks regression coverage for sibling and nested loops. Add a focused test
exercising multiple independent loops, including a nested loop, through this
path and verify each loop selects its own governing merge point and produces the
expected register lists; preserve existing behavior for straight-line entries
and body_resume selection.
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 1921-1926: Register destructors for W_ModuleDictObject and
W_SetObject so their owned native allocations are released during GC
reclamation. Update the type registration around w_module_dict_tid to use the
destructor-capable TypeInfo constructor, and make the corresponding W_SetObject
registration invoke cleanup for dstorage, mstrategy, object_storage, and items
through the appropriate existing destruction mechanism.
🪄 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: a6748391-41ef-4a88-b634-04ab5154df0a
📒 Files selected for processing (4)
majit/majit-metainterp/src/jitdriver.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/eval.rs
🛑 Comments failed to post (2)
pyre/pyre-jit-trace/src/trace.rs (2)
96-157: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n 'RANGE_FORITER_SPECIALIZED_GUARDS|RANGE_FORITER_DEMOTED|range_foriter_guard_failed|record_range_foriter_specialization|inline_subwalk' pyre/pyre-jit-trace/src/trace.rs pyre/pyre-jit-trace/src/jitcode_dispatch.rs pyre/pyre-jit/src/eval.rs rg -n 'fn fail_index_per_trace|fail_index_per_trace' majit/majit-metainterp/src/compile.rsRepository: youknowone/pyre
Length of output: 7663
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant trace helpers and their call sites. sed -n '2200,2365p' pyre/pyre-jit-trace/src/trace.rs printf '\n---\n' sed -n '20290,20420p' pyre/pyre-jit-trace/src/jitcode_dispatch.rs printf '\n---\n' sed -n '5180,5225p' pyre/pyre-jit/src/eval.rs printf '\n---\n' sed -n '1,240p' pyre/pyre-jit-trace/src/trace.rsRepository: youknowone/pyre
Length of output: 26890
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '2280,2355p' pyre/pyre-jit-trace/src/trace.rsRepository: youknowone/pyre
Length of output: 3962
🏁 Script executed:
#!/bin/bash set -euo pipefail # Check how trace_id and fail_index are used in the fail path and whether # fail indices are reset/reassigned per trace. rg -n 'trace_id|fail_index_per_trace|set_fail_index_per_trace|record_range_foriter_specialization|range_foriter_guard_failed' \ pyre/pyre-jit/src/eval.rs \ pyre/pyre-jit-trace/src/trace.rs \ majit/majit-metainterp/src/compile.rs \ majit/majit-metainterp/src/*.rs printf '\n---\n' sed -n '5190,5225p' pyre/pyre-jit/src/eval.rs printf '\n---\n' sed -n '3128,3175p' majit/majit-metainterp/src/compile.rs printf '\n---\n' sed -n '3418,3438p' majit/majit-metainterp/src/compile.rsRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect how fail_index_per_trace is allocated and how retraces/bridges # are keyed in the JIT driver. sed -n '360,455p' majit/majit-metainterp/src/compile.rs printf '\n---\n' sed -n '3010,3095p' majit/majit-metainterp/src/jitdriver.rs printf '\n---\n' sed -n '11390,11440p' majit/majit-metainterp/src/pyjitpl.rs printf '\n---\n' sed -n '9600,9665p' majit/majit-metainterp/src/pyjitpl.rsRepository: youknowone/pyre
Length of output: 13566
🏁 Script executed:
#!/bin/bash set -euo pipefail # Check whether there is any per-trace reset/cleanup for the range FOR_ITER maps. rg -n 'RANGE_FORITER_SPECIALIZED_GUARDS|RANGE_FORITER_DEMOTED|remove_compiled_loop|invalidate_loop|start_retrace_from_guard|record_range_foriter_specialization|range_foriter_guard_failed' \ pyre/pyre-jit-trace/src/trace.rs \ majit/majit-metainterp/src/jitdriver.rs \ majit/majit-metainterp/src/pyjitpl.rs printf '\n---\n' sed -n '100,165p' pyre/pyre-jit-trace/src/trace.rs printf '\n---\n' sed -n '4460,4510p' majit/majit-metainterp/src/jitdriver.rsRepository: youknowone/pyre
Length of output: 9541
Scope the range-FOR_ITER demotion map by trace
RANGE_FORITER_SPECIALIZED_GUARDSis keyed only bygreen_key, so a retrace that doesn’t re-emit this guard can leave a stale ordinal behind and demote the wrong loop on a later failure. Key it by(green_key, trace_id)or clear the entry when retracing.🤖 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/trace.rs` around lines 96 - 157, Scope RANGE_FORITER_SPECIALIZED_GUARDS to the current trace so a retrace that omits the range guard cannot reuse a stale fail ordinal. Update record_range_foriter_specialization and range_foriter_guard_failed, plus their callers as needed, to include and match a trace identifier or explicitly clear entries when retracing; preserve demotion only for the exact guard failure from the active trace.
2286-2337: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n 'fn loop_body_has_abort_permanent' -A 45 pyre/pyre-jit-trace/src/trace.rs rg -n 'fn loop_header_merge_point_regs' -A 35 pyre/pyre-jit-trace/src/trace.rs rg -n 'loop_body_has_abort_permanent\(' pyre/pyre-jit-trace/src/trace.rs rg -n 'fn start_pc_is_loop_header' -A 25 pyre/pyre-jit-trace/src/trace.rsRepository: youknowone/pyre
Length of output: 6695
🏁 Script executed:
#!/bin/bash sed -n '520,720p' pyre/pyre-jit-trace/src/trace.rs printf '\n---\n' sed -n '2280,2365p' pyre/pyre-jit-trace/src/trace.rs printf '\n---\n' rg -n 'abort_permanent|loop_body_has_abort_permanent|loop_header_merge_point_regs|start_pc_is_loop_header|same_greenkey|current_merge_points' pyre/pyre-jit-trace/src/trace.rs -A 4 -B 4 printf '\n--- tests ---\n' rg -n 'abort_permanent|loop_body_has_abort_permanent|loop_header_merge_point_regs|start_pc_is_loop_header' pyre/pyre-jit-trace/src -A 3 -B 3Repository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash sed -n '2530,2605p' pyre/pyre-jit-trace/src/trace.rs printf '\n---\n' rg -n 'trace_bytecode\(' pyre/pyre-jit-trace/src -A 3 -B 3 printf '\n---\n' rg -n 'body_resume|loop_header_merge_point_regs\(' pyre/pyre-jit-trace/src -A 5 -B 5Repository: youknowone/pyre
Length of output: 8803
Anchor the abort-permanent scan to
start_pc.
loop_body_has_abort_permanentcurrently picks the firstjit_merge_pointin the whole jitcode, so sibling or nested loops in the sameCodeObjectcan be scanned against the wrong body. Usestart_pcto select the governing merge point/back-edge for the loop being traced.🤖 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/trace.rs` around lines 2286 - 2337, Update loop_body_has_abort_permanent to use start_pc when selecting the governing jit_merge_point and back-edge, rather than the first merge point in the entire jitcode. Restrict the abort_permanent scan and loop boundary calculations to the loop body associated with start_pc, while preserving the existing exception-table and full-scan behavior.
…c demotion Replace the opaque jit_range_iter_next_or_null residual (EffectInfo::MOST_GENERAL, w_int_new-boxing every iteration) with inline getfield/IntAdd/SetfieldGc IR for range iterators, mirroring W_IntRangeIterator.next. The produced item stays boxed (Phase 1); the cursor advance is forward-delivered through fbw_foriter_inflight_capture, not journaled, so abort reconciliation is unchanged. Non-range iterators keep the residual. A GuardClass(RANGE_ITER) at the FOR_ITER loop header storms when the site is later driven by a non-range iterator: the guard fails on every re-entry and the bridge cannot close. A monomorphic range loop never fails its own class guard, so the first failure of that guard witnesses polymorphism. On that failure handle_fail invalidates and removes the compiled loop and demotes the FOR_ITER green key. Demotion records the key in a separate RANGE_FORITER_DEMOTED set rather than fbw_decline: the full-body walk still runs on retrace and compiles the generic polymorphic-tolerant residual — only try_walker_specialize_for_iter_next declines the demoted site — so a demoted polymorphic site compiles one generic loop instead of going interpret-only. The range class guard's per-trace ordinal is tracked per green key in a green_key -> fail_index map that a retrace overwrites in place, so a non-range body guard that happens to share a stale ordinal cannot be conflated with the range guard and spuriously demote a monomorphic range site. Function-scoped `for i in range(30_000_000): s += i` drops from 0.47s to 0.14s. Assisted-by: Claude
Narrow the FOR_ITER-inflight inline decline to callees whose bodies can replay without mutating live heap. Pure-leaf callees now inline, while live-heap stores and non-pure residuals remain declined. PYRE_FBW_FORITER_INLINE=0 restores the former blanket decline. Assisted-by: Claude
The specialized Add-tag exemption holds only when every callee argument is int or float, which lets the walker replace the generic residual with pure IntAddOvf or FloatAdd. Non-numeric operands fell through to an impure residual and abort-stormed; require numeric arguments before admitting the exemption. Assisted-by: Claude
…e vsd The guard-failure blackhole ContinueRunningNormally handoff was open-coded at four sites (three in eval.rs — execute_assembler, bound_reached, try_function_entry_jit — and one in call_jit.rs handle_blackhole_result, the CALL_ASSEMBLER resume for both backends). Each set the frame's last_instr to the merge-point next_instr but left valuestackdepth at the value the blackhole resume wrote: the operand depth at the failing guard's own py_pc. When the merge point differs from the guard pc — a mid-loop guard resuming at the loop header — that depth over-counts, and the loop header's super-instruction pushes overflow the frame at its peak stack use (PyFrame::push). test_strtod crashed flakily in _randbelow_with_getrandbits this way. Extract correct_resume_vsd(frame, resume_pc): re-derive valuestackdepth from the resume pc via depth_based_vsd_for_wcode and null the operand slots above it, mirroring the bridge path's existing correction. apply_blackhole_crn_handoff sets last_instr then applies it; route all three eval.rs CRN arms through it and call correct_resume_vsd from the call_jit.rs CALL_ASSEMBLER arm, so the resume coordinate and its operand depth stay consistent on every leg. Add PyFrame::clear_stack_above for the shared fresh-frame parity clear. Assisted-by: Claude
e47188a to
a69d1d0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a69d1d0188
ℹ️ 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".
| if !args_all_numeric | ||
| || !matches!( | ||
| d.key, | ||
| "residual_call_ir_r/iIRd>r" | "residual_call_ir_i/iIRd>i" | "residual_call_ir_v/iIRd" | ||
| ) |
There was a problem hiding this comment.
When the callee is invoked from an in-flight FOR_ITER body with numeric arguments but its Add residual actually uses non-argument refs (for example a global/user object whose __add__ mutates state), this predicate returns true just because args_all_numeric is true. The sub-walk can then execute the generic BinaryOp residual with user side effects, and a later abort or guard replay from the caller's CALL boundary reruns the callee and duplicates those effects; please prove the residual call's R-list operands are the numeric arguments or concrete int/float objects before treating it as replay-safe.
Useful? React with 👍 / 👎.
…c demotion (#545) * jit: walker-native range FOR_ITER-next specialization with polymorphic demotion Replace the opaque jit_range_iter_next_or_null residual (EffectInfo::MOST_GENERAL, w_int_new-boxing every iteration) with inline getfield/IntAdd/SetfieldGc IR for range iterators, mirroring W_IntRangeIterator.next. The produced item stays boxed (Phase 1); the cursor advance is forward-delivered through fbw_foriter_inflight_capture, not journaled, so abort reconciliation is unchanged. Non-range iterators keep the residual. A GuardClass(RANGE_ITER) at the FOR_ITER loop header storms when the site is later driven by a non-range iterator: the guard fails on every re-entry and the bridge cannot close. A monomorphic range loop never fails its own class guard, so the first failure of that guard witnesses polymorphism. On that failure handle_fail invalidates and removes the compiled loop and demotes the FOR_ITER green key. Demotion records the key in a separate RANGE_FORITER_DEMOTED set rather than fbw_decline: the full-body walk still runs on retrace and compiles the generic polymorphic-tolerant residual — only try_walker_specialize_for_iter_next declines the demoted site — so a demoted polymorphic site compiles one generic loop instead of going interpret-only. The range class guard's per-trace ordinal is tracked per green key in a green_key -> fail_index map that a retrace overwrites in place, so a non-range body guard that happens to share a stale ordinal cannot be conflated with the range guard and spuriously demote a monomorphic range site. Function-scoped `for i in range(30_000_000): s += i` drops from 0.47s to 0.14s. Assisted-by: Claude * jit: inline side-effect-free callees inside FOR_ITER bodies Narrow the FOR_ITER-inflight inline decline to callees whose bodies can replay without mutating live heap. Pure-leaf callees now inline, while live-heap stores and non-pure residuals remain declined. PYRE_FBW_FORITER_INLINE=0 restores the former blanket decline. Assisted-by: Claude * jit: gate FOR_ITER add-tag inline exemption on numeric operands The specialized Add-tag exemption holds only when every callee argument is int or float, which lets the walker replace the generic residual with pure IntAddOvf or FloatAdd. Non-numeric operands fell through to an impure residual and abort-stormed; require numeric arguments before admitting the exemption. Assisted-by: Claude * jit: unify blackhole ContinueRunningNormally handoff, re-derive resume vsd The guard-failure blackhole ContinueRunningNormally handoff was open-coded at four sites (three in eval.rs — execute_assembler, bound_reached, try_function_entry_jit — and one in call_jit.rs handle_blackhole_result, the CALL_ASSEMBLER resume for both backends). Each set the frame's last_instr to the merge-point next_instr but left valuestackdepth at the value the blackhole resume wrote: the operand depth at the failing guard's own py_pc. When the merge point differs from the guard pc — a mid-loop guard resuming at the loop header — that depth over-counts, and the loop header's super-instruction pushes overflow the frame at its peak stack use (PyFrame::push). test_strtod crashed flakily in _randbelow_with_getrandbits this way. Extract correct_resume_vsd(frame, resume_pc): re-derive valuestackdepth from the resume pc via depth_based_vsd_for_wcode and null the operand slots above it, mirroring the bridge path's existing correction. apply_blackhole_crn_handoff sets last_instr then applies it; route all three eval.rs CRN arms through it and call correct_resume_vsd from the call_jit.rs CALL_ASSEMBLER arm, so the resume coordinate and its operand depth stay consistent on every leg. Add PyFrame::clear_stack_above for the shared fresh-frame parity clear. Assisted-by: Claude
The
single-walkerline, containing a single commit on top ofmain.jit: walker-native range FOR_ITER-next specialization with polymorphic demotion
jit_range_iter_next_or_nullresidual (EffectInfo::MOST_GENERAL,w_int_new-boxing every iteration) with inlinegetfield/IntAdd/SetfieldGcIR for range iterators, mirroringW_IntRangeIterator.next. The produced item stays boxed (Phase 1); the cursor advance is forward-delivered throughfbw_foriter_inflight_capture, not journaled, so abort reconciliation is unchanged. Non-range iterators keep the residual.GuardClass(RANGE_ITER)at the FOR_ITER loop header: when the site is later driven by a non-range iterator the guard storms and the bridge cannot close. Since a monomorphic range loop never fails its own class guard, the first failure of that guard witnesses polymorphism.handle_failinvalidates the loop and declines the FOR_ITER green key (fbw_decline), retracing as the polymorphic-tolerant residual; the specialization declines a demoted site on retrace. Keyed on(green_key, fail_index)so an unrelated body guard in the same loop is not conflated.for i in range(30_000_000): s += idrops from 0.47s to 0.14s.Touches
pyre/pyre-jit-trace/src/jitcode_dispatch.rs,pyre/pyre-jit-trace/src/trace.rs, andpyre/pyre-jit/src/eval.rs.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
FOR_ITERloops over integer ranges with faster iterator advancement and correct loop-exit handling.Bug Fixes
FOR_ITERspecialization fails, invalidating and removing stale compiled loop data to prevent repeated mis-speculation.Refactor