majit: #25 de-conflate frame-input shape from true-portal; always-portal flip (default ON)#457
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 (5)
✨ 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: 1fbd3ecdf4
ℹ️ 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 frame_inputs = if is_true_portal || always_portal { | ||
| FrameInputs::Portal |
There was a problem hiding this comment.
With PYRE_ALWAYS_PORTAL defaulting on, this makes non-portal callees carry the Portal input shape and emit the frame-vable locals prologue. However the existing inline_call_* descent still goes through run_sub_jitcode_walk, which only seeds the explicit argument slots (jitcode_dispatch.rs:19009-19028) and never seeds the callee's portal frame/ec registers or activates the fresh-frame fold added for try_walker_inline_user_call. Any non-portal callee entered by an inline_call_* whose body starts with the new LOAD_FAST/STORE_FAST vable ops will therefore read an unseeded frame (OpRef::NONE) and abort with VableBoxNotSeeded, so enabling the default Portal shape needs the same frame/fold seeding on this path.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit c4ba1ac). 1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
…nert) Add the register-to-register fold that lets a branchless leaf callee carrying the portal frame-vable locals prologue stay strict-inlinable. Own-frame getarrayitem_vable_r / setarrayitem_vable_r resolve through a per-slot OpRef shadow instead of the metainterp vable path, emitting no GC op — the fresh_virtualizable case (is_virtualizable_getset returns False when 'fresh_virtualizable' in flags, jtransform.py:990-993; pycode.py:275 fresh_frame = jit.hint(frame, access_directly=True, fresh_virtualizable=True)). - FBW_CALLEE_LOCALS_OPREF + FBW_STRICT_FOLD_FRAME_REG thread-local stacks, pushed/popped by InlineRecursionGuard alongside FBW_CALLEE_LOCALS_CONCRETE. - Short-circuits in getarrayitem_vable_via_metainterp / setarrayitem_vable_via_metainterp: when the op reads/writes the active fold frame register, fold via the shadow. - callee_fast_path_inlinable gains a callee_frame_reg param and accepts own-frame vable ops via inline_resolvable_seeded_frame_op. - Strict path seeds each param into slot i and activates the fold before the sub-walk. Gated on PYRE_ALWAYS_PORTAL (fbw_always_portal_enabled); default OFF leaves callee_portal_frame_reg = u16::MAX so every new path is inert and byte- identical. dynasm/cranelift 159/159; inline_helper 985375007 @ 0.24s. Assisted-by: Claude
…tal flip (default ON)
Split the fused `is_portal` bool into its two independent RPython concepts:
- `is_true_portal` (portal_jd is not None): jit_merge_point marker +
jitdriver stamp + LOAD_GLOBAL/LOAD_CONST namespace const-fold split.
- `frame_inputs` / `frame_is_portal` (reds=['frame','ec'] + frame-vable
locals prologue): the INPUT SHAPE.
Under the always-portal flip (default ON, PYRE_ALWAYS_PORTAL=0 to roll back)
every drained per-code jitcode is Portal-shaped, so its whole body is walkable
as a portal and the trace.rs `!built_as_portal` decline narrows to shapeless
skeletons only. A branchless leaf's frame-vable prologue folds via the
fresh-frame register-to-register fold (fresh_virtualizable,
jtransform.py:990-993), so it stays strict-inlinable — no VableBoxNotSeeded
abort, no 22x inline_helper regression.
codewriter.rs:
- frame_inputs = Portal when is_true_portal || always_portal; frame_is_portal
= matches!(Portal) gates the ~20 frame-vable dual-write sites (getfield/
setfield/getarrayitem/setarrayitem_vable on frame_var). Gating on
matches!(Portal), not has_frame(), keeps a Frame callee (flag=0) prologue-
free and byte-identical to the fused bool.
- jit_merge_point marker + LOAD_GLOBAL namespace split stay is_true_portal.
- finalize_jitcode `built_as_portal: frame_is_portal` records the input shape.
jitcode_dispatch.rs:
- setfield_vable_via_metainterp scalar fold: a leaf's valuestackdepth /
last_instr setfield on its own fresh frame folds to a no-op (completes the
getarrayitem/setarrayitem fold from the prior commit).
- fbw_always_portal_enabled default ON.
trace.rs / pyjitcode.rs: decline message + built_as_portal doc updated to
"no portal input shape".
Witnesses: inline_helper 985375007 @ 0.19s (was TIMEOUT on the prior blocked
attempt); short_circuit_side_effects built_as_portal=false 3->0, stdout
byte-identical; dynasm + cranelift check.py 159/159 default-on and with
PYRE_ALWAYS_PORTAL=0 rollback.
Assisted-by: Claude
bh_setfield_gc_r routed through the descriptor-sized byte writer shared with the Int/Float field stores. On wasm32 the Ref FieldDescrInfo reports field_size 8 while a pointer is 4 bytes, so the store overwrote the adjacent pointer field of the resumed virtual PyFrame (its locals_cells_stack_w), zeroing the frame's locals array pointer; the callee then read locals through a null array. Write the Ref field with a pointer-width usize store, matching llmodel.py:723 bh_setfield_gc_r. Int and Float keep the descriptor-sized writer. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d10432b91
ℹ️ 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".
| // is Portal-shaped under the always-portal flip, so a later portal | ||
| // walk of any body is admitted (`run_perfn_walk` no longer declines | ||
| // on `!built_as_portal`). Only shapeless skeletons carry `false`. | ||
| built_as_portal: frame_is_portal, |
There was a problem hiding this comment.
When PYRE_ALWAYS_PORTAL is on, a function first compiled through the callee path reaches this metadata assignment with frame_is_portal=true but is_true_portal=false, so the earlier is_true_portal/portal_jd_index gates do not emit jit_merge_point or loop_header ops for backward jumps. CallControl::get_jitcode also does not rebuild an existing payload when that same code is later registered as a portal, so marking it walkable here lets run_perfn_walk drive a stale callee-shaped body as a root portal; a hot loop function that was first seen as a callee can then be walked without the merge-point machinery needed to close/compile the loop. Either rebuild/stamp a true portal body on late registration or keep these payloads declined.
Useful? React with 👍 / 👎.
1 participant
Splits the fused per-CodeObject
is_portalbool into its two independent concepts and flips to always-portal input shape (default ON,PYRE_ALWAYS_PORTAL=0to roll back).What it does
is_true_portal(portal_jd is not None): keeps thejit_merge_pointmarker + jitdriver stamp + LOAD_GLOBAL/LOAD_CONST namespace const-fold split.frame_inputs/frame_is_portal(reds=['frame','ec']+ frame-vable locals prologue): the INPUT SHAPE, nowPortalfor every drained jitcode under the flip.Every drained per-code jitcode becomes Portal-shaped, so its whole body is walkable as a portal and the
trace.rs!built_as_portaldecline narrows to shapeless skeletons only.The blocker it resolves
The prior always-portal attempt regressed
inline_helper22x: forcing the frame-vable prologue into leaf callees madecallee_fast_path_inlinablereject them as non-static vable → strict-leaf declines → main loop stops compiling.Fix = the fresh-frame register-to-register fold (
f202ec29e57): a branchless leaf's own-framegetarrayitem_vable_r/setarrayitem_vable_rresolve through a per-slotOpRefshadow (FBW_CALLEE_LOCALS_OPREF) instead of the metainterp vable path, emitting no GC op — the runtime realization of thefresh_virtualizabletranslate-time elision (is_virtualizable_getsetreturns False when'fresh_virtualizable' in flags). The1fbd3ecdf48commit adds the scalarsetfield_vableno-op fold that completes it.Commits
f202ec29e57— strict fresh-frame vable fold machinery (inert until the flip; byte-identical 159×2).1fbd3ecdf48— de-conflate frame-input shape; always-portal flip (default ON).Verification
inline_helper985375007@ 0.18s (was TIMEOUT on the prior blocked attempt).short_circuit_side_effectsbuilt_as_portal3→0, stdout byte-identical.check.py: dynasm 160/160, cranelift 160/160, wasm 159/160 (the 1 wasm failsynth/calls_closuresis pre-existing — byte-identical wrong output under bothPYRE_ALWAYS_PORTAL=1and=0).cargo test -p pyre-jit-trace -p pyre-jit: 0 failed.Composes with the orthodox strict-multiframe snapshot routing (#440/#453) that this branch is rebased onto.
🤖 Generated with Claude Code