majit dynasm: partition the parallel-move keys, fault outside the operand contract, and fix the field-width witness - #1256
Conversation
…nd contract
`loc_as_key` returned a register's bare number and a frame slot's byte
offset. A frame offset of 0x1000 is an ordinary slot for a large trace and
collided with a general register's key; the two locations then shared one
`pending_dests` entry and the move for whichever lost was never emitted.
Registers now key positive with a per-file bias and stack slots key `!offset`,
which is negative, so the two spaces cannot meet and `remap_frame_layout_mixed`
no longer conflates a general register with an XMM one.
`remap_frame_layout_mixed` looked for a wide source's second word at
`key + WORD`. Stack keys run backwards against offsets, so that read the slot
on the other side; `stack_key_next_word` subtracts.
The three `RegallocMoves` implementations answered a location outside the
contract with `_ => {}`. A dropped `regalloc_mov` leaves a destination that
`pending_dests` counted holding a stale value, and a dropped `regalloc_pop`
leaves the machine stack pointer shifted. All six arms now panic, and
`loc_as_key` panics on `Loc::Addr` rather than minting a key no emitter can
retire.
Assisted-by: Claude
The witness was `fn(&Struct) -> T` applied to `|s| s.field`, which returns the field by value and so moves out of a shared borrow. That compiles only for a `Copy` field: an eight-byte field that is not `Copy` failed expansion with a borrow error naming generated code, for a struct the lowering had no complaint about. The closure now yields `&T` and the width is read off the reference. The doc header said the undeclared default is the machine word; it is `i64`, which differs from `usize` on wasm32. Assisted-by: Claude
…field The control read the `size` descr through `if let Some(..)` and `size` was named in no declaration, so its assertion never ran. The fixture's `residual_writes` now names `size`, and the test requires the slot instead of skipping when it is absent. `generation` is an eight-byte field that is not `Copy`, declared in none of the field maps, so it takes the undeclared-scalar default and the macro emits its width witness over it. A witness written by value stops compiling on it. Assisted-by: Claude
`jump.py:7` asserts the destination keys are unique; the port inserted them and let a repeat displace the entry silently. A repeated destination shares one `srccount` entry while `pending_dests` counts both, so the second one is never retired: every key reaches -1, the scheduling loop stops making progress, and the cycle-breaking arm finds no key at or above zero to park, leaving the call spinning. `insert` returns the displaced value, so the check reuses the store already made. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 28 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: 📒 Files selected for processing (6)
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 371e660). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 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: 255002c56d
ℹ️ 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".
| // without one the control skips: it asserted nothing, which is the | ||
| // failure mode it exists to rule out for the pointer field. | ||
| sel.data => [jit_scramble_pointer_field], | ||
| sel.size => [jit_scramble_pointer_field], |
There was a problem hiding this comment.
usize field before registering it
When this test target is compiled for supported wasm32, adding sel.size makes field_scalar_tokens emit the undeclared-field witness, which requires the field to be eight bytes, while PointerFieldStack::size: usize is four bytes there. The test therefore fails during macro expansion before exercising the wasm backend; add int_fields = { PointerFieldStack::size => usize } so the descriptor and witness use the field's actual target-dependent width.
Useful? React with 👍 / 👎.
`regloc.py:82 RawEbpLoc.is_stack` returns True and `FrameLoc` inherits it, so both spellings of a frame-pointer location are stack locations. `Loc::is_stack` named only `Frame`, which made a `Loc::Ebp` operand look like a register to the parallel move: it skipped the scratch register and handed the backend a memory-to-memory move no machine encodes. The three `RegallocMoves` methods then had no arm for it either, so it reached the wildcard. `ebp_loc_pat!` matches both variants and binds the `RawEbpLoc` they share, keeping one arm per machine instruction rather than one per spelling. The frame-to-frame identity arm now compares offsets instead of stack positions, which is what upstream's `_getregkey()` equality compares. `Loc::Addr` still faults: `AddressLoc` overrides neither `_getregkey` nor, for its `'a'`/`'m'` codes, the `value` the inherited one reads (`regloc.py:207-250`), so a parallel move handed one raises upstream as well. Assisted-by: Claude
Naming `size` in the write set sends it through the undeclared-scalar default, whose witness requires eight bytes. `usize` is four on a 32-bit target, so the fixture stopped compiling under wasm32 during macro expansion. `int_fields` now declares it, and the descr and witness use the field's actual width. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 371e660274
ℹ️ 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".
| debug_assert!( | ||
| offset >= 0, |
There was a problem hiding this comment.
When a parallel move contains a Loc::Ebp with a negative displacement, this check disappears in release builds even though RawEbpLoc::new accepts every i32 and the newly documented operand contract includes bare EBP locations. For example, offset -4097 maps through !offset to 4096, exactly the key assigned to GPR 0, so two destinations trigger the duplicate-key panic and source/destination dependencies can alias. Preserve the upstream backend-specific key shapes (x86 keys raw EBP displacements while constraining FrameLoc; AArch64 keys stack positions) or use a tagged key rather than relying on this debug-only nonnegativity assumption.
AGENTS.md reference: AGENTS.md:L288-L290
Useful? React with 👍 / 👎.
1 participant
Follow-up to #1241, addressing its review round.
CodeRabbit findings
loc_as_keycollided registers with frame slots. The function biased register keys by0x1000/0x2000and keyed a frame slot on its raw byte offset. An offset of0x1000is an ordinary slot for a large trace, so the two spaces met: both locations landed on onepending_destsentry and the move for whichever lost was never emitted. Registers now key positive with the per-file bias and stack slots key!offset, which is negative. Upstream argues the two cannot meet because offsets start above the register file (regloc.py:117-120) and asserts it rather than trusting it; that argument does not survive a per-class bias.remap_frame_layout_mixedread a wide source's second word atkey + WORD. Stack keys run backwards against offsets, so that read the slot on the other side.stack_key_next_wordsubtracts._ => {}arms across both backends silently dropped moves. A droppedregalloc_movleaves a destination thatpending_destscounted holding a stale value; a droppedregalloc_popleaves the machine stack pointer shifted. All six panic now, the trait documents its operand contract, andloc_as_keypanics onLoc::Addrrather than minting a key no emitter can retire.Copyfields. It wasfn(&Struct) -> Tapplied to|s| s.field, which returns the field by value and so moves out of a shared borrow. An eight-byte field that is notCopyfailed expansion with a borrow error naming generated code. The closure now yields&T. The doc header also called the undeclared default the machine word; it isi64, which differs fromusizeon wasm32.sizedescr throughif let Some(..)andsizewas named in no declaration, so its assertion executed zero times — the exact failure mode it exists to rule out for the pointer field. The fixture now namessizeinresidual_writesand the test requires the slot.A
generation: Generationmember (#[repr(transparent)] struct Generation(i64), deliberately notCopy) was added to that fixture. It is declared in no field map, so it takes the undeclared-scalar default and the macro emits its width witness over it: a witness written by value stops compiling on it.Codex parity review §3
jump.py:7asserts the destination keys are unique; the port inserted them and let a repeat displace the entry silently. A repeated destination shares onesrccountentry whilepending_destscounts both, so the second is never retired — every key reaches -1, the scheduling loop stops making progress, and the cycle-breaking arm finds no key at or above zero to park, leaving the call spinning.insertreturns the displaced value, so the check reuses the store already made. This also means the assert cannot fire on existing traffic: a duplicate would already hang.Verification
cargo test -p majit-metainterp --features dynasm,cargo test -p majit-backend-dynasm --features dynasm: 0 failedcargo check --workspace --all-targets,cargo fmt --all --check: cleanpython3 ./pyre/check.py --backend dynasm: ALL PASSED 436/436jit_interpconsumer: output byte-identical (996310 bytes, md57fcdbfff0af449c4283c008e3ca317ce), JIT and interpreter agree— opened by Claude