majit-translate, majit, jit: a payload enum's variant fields seated after the inherited discriminant, PCSEQ at every frame depth, and the portal's can_enter_jit tallies - #1667
Conversation
`PyFrame::close_loop` filled the field with `vec![]`. In a jitcode that `Vec::new` is a residual call whose path the translator has no binding for, so a walk that reaches a back edge through `close_loop` stops there; the field is written at five sites and read at none. Assisted-by: Claude
…ited discriminant
A payload enum is modelled as RPython's sum-type subclass layout: the base
carries `__discriminant`, each variant subclass carries its own fields and
inherits the base's byte 0. When `layout_for_target` returns `None` the
variant's field offsets fall back to default packing from byte 0, so the first
payload field is placed on the discriminant's byte.
`pyre_interpreter::pyopcode::StepResult` has no recorded layout. Its
`CloseLoop` variant was `{jump_args@0, loop_header_pc@8}` and `Return` was
`{__pos_0@0}`, so `close_loop`'s jitcode wrote discriminant 2 to byte 0 and
overwrote it with `jump_args` on the next op; `execute_jump_backward` read the
null back as tag 0 and reported `Continue`.
The explicit sum shell that seats the tag at 0 and the payload from 8 was
reached by an allowlist of two type names, `core::result::Result` and
`core::option::Option`. Compute the condition those two share instead: apply
the shell when the base registered a `__discriminant` row the variant inherits
and the host layout would seat a payload field inside the tag's bytes, taking
an absent layout as overlapping since default packing starts at byte 0. The
guard reuses `!fieldless && !tag_recorded_but_unspellable`, the same test the
base row's own registration uses, so an `I128`/`U128` tag — which registers no
base row — keeps byte 0 for its payload.
`int_type_byte_width` maps the tag spelling Charon records beside the offset to
a byte count; an unrecorded width answers a machine word.
Assisted-by: Claude
…he jitcode `pcseq_branch` returned early unless `frames.len() == 1`, so only the root jitcode's own switches and branches printed and the whole descent was silent. A portal walk reporting `Continue` on a back edge then gave no reading between the portal's arm and the opcode handler, while a static decode of that chain read correct at every hop. Drop the depth gate and add `d=<depth>` to each line. The line that named the divergence — `execute_jump_backward`'s own `switch` on the `StepResult` discriminant — sits three frames down. Assisted-by: Claude
…nter export Every portal instrument is a `std::env::var` read — `PYRE_PORTAL_METATRACE`, `PYRE_PORTAL_METATRACE_ENTRY`, `_SKIP`, `MAJIT_PCSEQ` — and the wasm guest has no environment, so none of them can fire there. Add `PORTAL_DIAG`, two counters read through one `pyre_jit::eval::portal_diag(slot)` by both consumers: the native `[jit-stats] portal_diag` line from `maybe_print_jit_stats`, and the `pyre_jit_portal_diag` / `pyre_jit_portal_diag_len` exports the wasm runner prints as the same line. Exported rather than imported, as `pyre_jit_bridge_diag` already is, because an import shifts the JIT's function-index space. Both bumps sit inside `can_enter_jit`'s body, which `jtransform` rewrites into the `loop_header` operation and so never traces. `pyre-jit` is one of the four LLBC crates, so a counter in `eval_loop_jit`'s `CloseLoop` arm is lowered into the portal jitcode, and `AtomicU64::fetch_add` is outside the LLBC set: it becomes a symbolic residual between the arm and its `loop_header`, which no `pyre-jit` path can bind. `portal_activation_bracketed` is reached from `funccall_valuestack` with no `dont_look_inside` or may-force boundary, so it carries no counter either. The runner reports a slot the guest exports but its own legend does not name as `slot<N>`, rather than dropping it as the `bridge_diag` mirror would. `can_enter_jit` and `can_enter_jit_taken` are absent from `JITSTATS_SNAPSHOT_FIELDS`, so no recorded baseline carries them. Assisted-by: Claude
…q_branch's root-frame claim
`pcseq_branch`'s doc still described the depth gate the previous commit
removed ("Print one control-flow edge of the root jitcode frame", "Silent for
an inlined callee").
`get_jitcode_calldescr` takes the address of `bh_portal_runner(&[i64], &[i64],
&[i64]) -> i64` and pairs it with a `"r"` -> `'r'` descr, and its comment reads
as if that pair were how pyre calls a Python function. It is not dispatched:
`bhimpl_recursive_call_*` reaches the portal through `get_portal_runner`
(`bh_portal_runner_c`, `"iirrr"`), and pyre's `inline_call` handlers read their
callee from a build-time descr pool whose `fnaddr` comes from
`JitCodeBuilder::set_native_entry`, which a runtime CodeObject jitcode is never
in. Say that, and say which of the two functions the descr describes.
`executor.rs`'s Float arm named `bh_portal_runner(all_i, all_r, all_f)` as what
`portal_runner_adr` holds; `call_jit.rs:2819` sets it to `bh_portal_runner_c`.
The i64-return contract the comment exists to pin holds for both.
Assisted-by: Claude
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 6 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (12)
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
https://github.com/youknowone/pyre/blob/b552ab5a9acd37f36d885148365d88d5c0d432d4/pyre-interpreter/src/pyopcode.rs#L137
For the ordinary PyFrame::close_loop path, this changes the interpreter's exact empty argument list into None solely because the translator lacks a binding for Vec::new. That accommodates a generation defect by changing interpreter structure rather than making the generated JIT preserve the source; restore the Vec<V> payload and teach the translator to lower or bind the empty-vector construction.
AGENTS.md reference: AGENTS.md:L29-L32
ℹ️ 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 explicit_sum_shell = name == "core::result::Result" | ||
| || name == "core::option::Option" | ||
| || (base_has_discriminant && payload_seats_on_tag); |
There was a problem hiding this comment.
When this new arm selects a non-Result/Option enum such as StepResult, the front end assigns its tag to offset 0 and payloads from offset 8, but is_explicit_shell_variant_owner still recognizes only Result::{Ok, Err} and Option::Some. Consequently, bh_size_spec_from_callcontrol omits the inherited __discriminant from the variant's all_fielddescrs; when such a value is virtualized and later forced or resumed, the tag is not restored and the following match can again observe Continue. Carry the explicit-shell designation into the codewriter instead of leaving its nominal allowlist unchanged.
AGENTS.md reference: AGENTS.md:L29-L32
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit b552ab5). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
Merging this PR will not alter performance
Comparing 
Footnotes
|
1 participant
The portal metatrace walk stopped reporting
Continueon a back edge. Thecause was not in the portal: a payload enum's variant fields were packed from
byte 0, on top of the discriminant the variant inherits from its base.
The defect
majit-translatemodels a Rust payload enum the wayrclass.py:499-518does —a base carrying only
__discriminant, and one subclass per variant carryingthat variant's fields and inheriting the base's byte 0. Seating the payload
after the tag was gated on a two-name allowlist (
Result,Option). Everyother payload enum fell through to default packing, which puts field
iati*8— field 0 on the tag's byte.StepResult, what every opcode handler returns, is one of them. Measured descrsbefore the fix:
StepResult.__discriminant__discriminant@0StepResult::CloseLoopjump_args@0,loop_header_pc@8StepResult::Return__pos_0@0So
close_loopwrote tag2to byte 0 and the next op overwrote it withjump_args(aNone, i.e. null). The reader saw tag0—Continue. Backendindependent; it is in the jitcode, not in any assembler.
The fix
Replace the allowlist with the question it was standing in for: does the base
register a
__discriminantrow that this variant inherits, and does anyvariant's field land inside the tag's byte range?
tag_recorded_but_unspellableregisters no base row at all, so a payload at 0there aliases nothing — that is the
base_has_discriminantterm. An unrecordedtag width reads as a machine word, and an enum for which Charon recorded no
layout answers "yes, seats on the tag" outright: that is precisely the case
that hid this bug, because the variant offsets then come from default packing.
An earlier attempt spelled the overlap test as
enum_layout.is_some_and(...), which did nothing —StepResulthas no Charonlayout, so the guarded case and the broken case were disjoint.
After
MAJIT_PCSEQon the portal walk, with the same script:and the run answers correctly instead of aborting.
pyre/check.py --backend dynasm: ALL PASSED 536/536. The predicate movesevery payload enum in the LLBC set that Charon recorded no layout for, not just
StepResult, so that is the number that matters here.The instrument that found it, and one for wasm
pcseq_branchreturned early unlessframes.len() == 1, so only the rootjitcode's branches printed. The line that settled this —
execute_jump_backward'sown
switchon theStepResultdiscriminant — is three frames down. It nowprints at every depth with
d=<depth>and the owning jitcode's name.Every other portal instrument is a
std::env::varread (PYRE_PORTAL_METATRACE,..._ENTRY,..._SKIP,MAJIT_PCSEQ), and the wasm guest has noenvironment, so none of them can fire there.
PORTAL_DIAGis two countersread through one
pyre_jit::eval::portal_diag(slot)by both consumers: thenative
[jit-stats] portal_diagline, and thepyre_jit_portal_diag/pyre_jit_portal_diag_lenexports the wasm runner prints as the same line.Exported rather than imported, as
pyre_jit_bridge_diagalready is — an importshifts the JIT's function-index space.
Both bumps sit inside
can_enter_jit's body, whichjtransformrewrites intothe
loop_headeroperation and so never traces. That placement is the wholedesign:
pyre-jitis one of the four LLBC crates, so a counter ineval_loop_jit'sCloseLooparm gets lowered into the portal jitcode, andAtomicU64::fetch_addis outside the LLBC set — it would become a symbolicresidual sitting between the arm and its
loop_header, which nopyre-jitpath can bind. The instrument would have blocked the thing it was measuring.
The runner reports a slot the guest exports but its own legend does not name as
slot<N>, rather than dropping it the way thebridge_diagpositional mirrorwould. Neither counter is in
JITSTATS_SNAPSHOT_FIELDS, so no recordedbaseline carries them.
The reading it buys
First portal measurement ever taken from inside the wasm guest,
hot.py(a 200k-iteration
while), against the native dynasm binary on the samescript:
can_enter_jitcan_enter_jit_takenloops_compiledguard_failuresBoth compile the one loop and take it twice — once on the initial trace, once
after the guard failure. The counts differ in the untaken polls, i.e. the
interpreted warm-up before the loop goes hot, which is a threshold-accounting
difference and not a portal one.
back_edge_pollsreads 0 in both runs whilecan_enter_jitreads four figures, which is worth knowing before anyone usesback_edge_pollsas the portal-entry count.The runner also prints, unprompted, the reason this line had to exist at all:
Also here
Three comments that named the wrong thing.
pcseq_branch's doc still describedthe depth gate this branch removes.
get_jitcode_calldescrreads as if the(fnaddr, calldescr)it stamps on every CodeObject jitcode were how pyre callsa Python function — it is never dispatched (
bhimpl_recursive_call_*goesthrough
get_portal_runner; pyre'sinline_callhandlers read a build-timedescr pool a runtime jitcode is never in), and the descr beside it describes
bh_portal_runner_c, not the slice-takingbh_portal_runnerwhose address ittakes.
executor.rs's Float arm named the same wrong function asportal_runner_adr's value.