jit(aarch64): close the loop gap vs PyPy in the backend; fix NaN float compares on both dynasm arches - #859
Conversation
WalkthroughThe PR adds AArch64 condition-code-aware guard lowering and immediate arithmetic paths, updates cross-target comparison allocation and x86 floating comparison handling, and strengthens Pyre virtual-reference frame rooting, restoration, and forcing. It also changes residual barrier recording and clarifies representation documentation. ChangesBackend condition-code flow
Virtual-reference frame safety
Documentation clarifications
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 83e8db8f76
ℹ️ 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".
| // takes precedence for that case (heaptracker.py:66-style | ||
| // "typeptr" filter does NOT apply to w_class in pyre). | ||
| (*ptr).w_class = get_instantiate(&*(vtable as *const PyType)); | ||
| if vtable as u64 == majit_metainterp::virtualref::JIT_VIRTUAL_REF_VTABLE { |
There was a problem hiding this comment.
When a guard-resume materializes a virtual reference on wasm32, VRefSizeDescr::vtable() has already narrowed the 64-bit JIT_VIRTUAL_REF_VTABLE constant to usize; casting it back to u64 cannot recover the high bits, so this comparison is always false. The code consequently enters the ordinary-object branch and dereferences the truncated magic value as a PyType *, crashing instead of materializing the vref. Recognize the vref descriptor without passing its tag through usize, or make the runtime tag word-sized consistently.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 79cea09). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/6cc8dbdd12cdec5a5e16658bc9658c0c0feb995b/pyre-interpreter/src/executioncontext.rs#L999
The fresh code now calls force_frame even though the preceding comment explicitly says this only compensates for the missing VIRTUAL_REF producer. On every sys.settrace/setprofile call made while the JIT is active, this forces the virtualizable through a different hook instead of reproducing upstream's f_backref vref force/finish lifecycle; it also leaves this consumer coupled to the missing producer and risks double or incorrectly scoped forcing once that producer is added. Implement the producer/root protocol and retain the upstream frame-chain walk rather than this acknowledged workaround.
AGENTS.md reference: AGENTS.md:L205-L207
ℹ️ 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 inputargs: &'a [InputArg] = self.inputargs; | ||
| let ops: &'a [Op] = self.operations; | ||
| self.trace_start_offset = self.mc.offset().0; | ||
| self.long_guard_branch = ops.len().saturating_mul(MAX_BYTES_PER_OP) >= BCOND_FORWARD_RANGE; |
There was a problem hiding this comment.
On AArch64, a trace with fewer than 1024 operations always selects short b.cond branches, but operation count does not bound the distance to the recovery labels: every guard's later generate_quick_failure can emit one store sequence per const_stores entry, so guards with wide failure states can place an early stub beyond the 1 MiB range. The subsequent debug assertion assumes a fixed 64 bytes per stub, does not measure those sequences, and is absent in release builds; such a valid trace therefore reaches dynasm finalization with an impossible relocation, whereas the previous unconditional veneer compiled it. Choose the form from an actual conservative bound that includes recovery code, or preserve/patch the long form when the finalized displacement requires it.
AGENTS.md reference: AGENTS.md:L194-L196
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-backend-dynasm/src/aarch64/assembler.rs`:
- Around line 2237-2252: Increase the per-guard stub estimate used in the
debug_assert around pending_guard_tokens so it covers the full minimal recovery
stub, including pointer saves, stores, and the callee-save footer, rather than
the current 64-byte charge. Ensure traces near MAX_BYTES_PER_OP select
long_guard_branch before finalize when their body plus guard stubs could exceed
BCOND_FORWARD_RANGE; use emitted guard-token byte accounting if the fixed
estimate cannot safely cover optional exception or const-store paths.
In `@majit/majit-backend-dynasm/src/x86/assembler.rs`:
- Around line 3339-3342: Update the result-location handling around emit_setcc
so comparisons targeting the rbp condition-code sentinel are routed through
flush_cc instead of materializing directly into rbp. Preserve the existing
direct emit_setcc path only for ordinary register destinations, ensuring the
live JIT-frame pointer and interpreter semantics remain intact.
🪄 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 Plus
Run ID: 6e716ab9-aab8-4cf8-9e09-9e3b7a3664f5
📒 Files selected for processing (14)
majit/majit-backend-dynasm/src/aarch64/assembler.rsmajit/majit-backend-dynasm/src/aarch64/opassembler.rsmajit/majit-backend-dynasm/src/aarch64/regalloc.rsmajit/majit-backend-dynasm/src/regalloc.rsmajit/majit-backend-dynasm/src/x86/assembler.rsmajit/majit-translate/src/front/mir.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/executioncontext.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/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-object/src/unicodeobject.rs
CI triage —
|
| binary | panics |
|---|---|
this branch (6cc8dbdd12) |
8/8 |
| pre-change baseline (this branch's merge-base build) | 8/8 |
Identical, so the commits here neither introduce nor aggravate it. The nursery-size band is narrow — 32768 and 1048576 are both 0/8 — so a single small value is not enough to call it unreproducible.
Being worked separately on fix-dynasm-gc-nested-listcomp (#872); deliberately not touched here.
2. cranelift int_loop exec 0.73s > pypy 0.36s ratio 2.0x > gate 2x
This branch touches no cranelift file (git diff --name-only <merge-base>..HEAD has zero matches for cranelift), and check.py's own bench table names int_loop as one of the "known slow-runner flakes" left deliberately at low headroom.
It is also already red on main itself for the same job/bench/backend — run 30378431710 at b8a00beb9. It is x86_64-only: pyre's absolute time on that runner is unchanged across recent main runs (0.71 / 0.80 / 0.72 / 0.79 s, all of which passed); the ratio crossed because pypy got faster on the runner. Local arm64 measurement on this tree, interleaved against pypy over 5 rounds, is 0.33s vs 0.30s — under 1.2x raw and below 1.0x once startup is subtracted the way check.py does.
The underlying cause is cranelift x86_64 register pressure (preserve_frame_pointers + enable_pinned_reg cost two of sixteen GPRs), not this branch's IR.
Local verification of this tree
python3 pyre/check.py --backend dynasm,cranelift was re-run after every commit here: 331 passed on both backends each time, the only failure being synth/ast_compile_roundtrip, which check.py marks BASEFAIL and which fails identically on cranelift.
— commented by Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f9a3d44e2
ℹ️ 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".
| pub fn force_all_frames(&mut self, is_being_profiled: bool) { | ||
| let mut frame = self.gettopframe_nohidden(); | ||
| while !frame.is_null() { | ||
| force_frame(frame); |
There was a problem hiding this comment.
When settrace or setprofile walks active JIT frames, this call bypasses PyPy's virtual-reference protocol: upstream executioncontext.py:323 force_all_frames forces frames solely by reading the f_backref vrefs, while the new comment here explicitly acknowledges that the actual VIRTUAL_REF producer is still missing and installs this direct virtualizable force only as an interim substitute. Fresh evidence in this revision is that admission at lines 991-999; implement the missing producer and preserve the upstream walk rather than shipping the shortcut, especially because adding the producer later would leave two independently maintained forcing paths.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
`gettopframe_nohidden` / `getnextframe_nohidden` are force-free, so `force_all_frames` no longer marked any visited frame as forced. Upstream gets that force out of the walk itself: `f_backref` holds a `jit.virtual_ref`, so reading it is a `jit_force_virtual` and `virtualref.force_virtual` runs `ResumeGuardForcedDescr.force_now`. Pyre's walk calls `force_vref` at the same points, but nothing stores a `JitVirtualRef` in the chain yet, so it is the identity; this consumer now states the force directly. Records `index_storage` in the `W_UnicodeObject` layout comment. Assisted-by: Claude
…orcing `ResidualFrameChainGuard::enter` compared `ec.topframeref` against the inline callee frame with a raw `ptr::eq`. `topframeref` holds a `jit.virtual_ref`, so once the tracer stores a `JitVirtualRef` there the comparison misses even when the vref names that very frame, and the guard writes `frame.f_backref = vref(frame)` — a self-loop in the chain for the duration of the residual. Resolve the referent instead, without forcing. Forcing would be wrong rather than merely expensive: a live vref carries `TOKEN_TRACING_RESCALL` across a residual and `force_virtual` clears it, which is the marker `tracing_after_residual_call` reads as "the callee forced this vref", so a reader that forced would report its own read as a callee escape. `vref_referent` is the non-forcing read; `chain_next_frame` in the root walker already had this shape and now shares it. The re-entry case no longer returns early. `PUBLISHED_INLINE_FRAME` was written only on the path past that check, and its consumer `flush_active_frame_escape` is the sub-walk escape redirect, so an early return leaves it null for the whole sub-walk. The chain write is now conditional on `entered` and the publish is unconditional; `Drop` restores only what it wrote. Corrects four comments stating that `vrefs_before_residual_call` / `vrefs_after_residual_call` / `stop_tracking_virtualref` are unported. They are ported on `TraceCtx` and wired on the metainterp leg; the walker does not call them. The comments now also record that `TraceCtx` and `PyreSym` carry separate `virtualref_boxes`, and that only `TraceCtx`'s is read by the bracket and the guard snapshots. Assisted-by: Claude
…er header
Two sites parked a raw `ec.topframeref` across code that can collect and
restored it afterwards: `ResidualFrameChainGuard`, across the concrete residual,
and the multi-frame blackhole drive. Frames themselves never move — `FrameBox`
allocates old-gen — so the raw copy is sound while the slot only ever holds a
`PyFrame`. Once the tracer stores a `JitVirtualRef` there the displaced value
is a nursery object, and a collection inside the guarded region would leave the
restore writing back a pre-move pointer with no crash at the store. Both now
push it onto `majit_gc::shadow_stack` and read it back before popping, so the
collector forwards it in place — the same treatment `CurrentFrameGuard` already
gives this field.
`materialize_virtual_object` seeded every allocation as a `PyObject`, reading
`get_instantiate` off `descr.vtable()`. A `JitVirtualRef` is a `GcStruct` whose
`('super', rclass.OBJECT)` slot carries the type-id constant itself, so that arm
would dereference the `JIT_VIRTUAL_REF_VTABLE` magic as a type object on the
first guard exit carrying a vref. Write the typeptr directly for that vtable
and leave the field replay to fill `virtual_token` / `forced`.
Assisted-by: Claude
`history.py getkind` returns `"int"` for a `Ptr` whose `TO._gckind` is `raw`, and `FuncType._gckind` is `raw`, so `Ptr(FuncType)` maps to `int`. The comment stated the upstream kind was `r` and described the `Int` slot as a departure from it. Assisted-by: Claude
…testing the boolean The regalloc emit path for INT_LT/LE/GT/GE/EQ/NE, UINT_*, PTR_EQ/NE, INSTANCE_PTR_EQ/NE, INT_IS_TRUE and INT_IS_ZERO emitted `cmp` + `cset` and left `guard_success_cc` unset, so the following GUARD_TRUE / GUARD_FALSE ran `emit_test_loc` over the materialized boolean before branching. Record the comparison's condition code and result register; a guard whose operand is exactly that register now takes the recorded condition (inverted for GUARD_FALSE) and emits only the conditional branch. `assembler.py:1186-1198 _walk_operations` folds the same pair by passing `prevop` into `guard_operations[...]` -> `regalloc.py:780 guard_impl` -> `dispatch_comparison(prevop)`. The record is cleared by `regalloc_perform` and by a register move, so only an adjacent pair folds, matching upstream's `operations[i + 1]` window. nested_loop's steady loop drops two `tst` instructions per iteration (IntLt->GuardTrue and IntIsTrue->GuardFalse) and both branches now consume the `cmp` flags directly. Assisted-by: Claude
…ializing a boolean
`force_allocate_reg_or_cc` hands a comparison the frame register as its
result when the next op consumes the flags directly, and the emit side
recognises that sentinel, publishes `guard_success_cc` and emits nothing.
The path was `#[cfg(target_arch = "x86_64")]`-gated because aarch64 had no
`flush_cc`: `cset x29, cc` would have destroyed the frame pointer.
Add `flush_cc` to the aarch64 assembler and lift the gate. The comparison
arms (INT_LT/LE/GT/GE/EQ/NE, UINT_*, PTR_EQ/NE, INSTANCE_PTR_EQ/NE,
INT_IS_TRUE, INT_IS_ZERO) route through it, and the guard arms take the
`load_condition_into_cc` shape the x86 assembler already uses, so a
published condition is honoured instead of being overwritten by the
`emit_test_loc` fallback. `genop_discard_cond_call` consumes a published
condition too — `opassembler.py:864 _emit_op_cond_call` skips its CMP when
`arglocs[0] is None` for the same reason.
`consider_int_is_true_j2` now allocates its result through
`force_allocate_reg_or_cc`. `regalloc.py:469 prepare_comp_unary` allocates
no destination at all for the folded form and `opassembler.py:210
emit_comp_op_int_is_true` emits the `cmp` alone, returning the condition.
A guard emits its own flag-setting code, so `regalloc_perform_guard` clears
the adjacent-comparison record on the way out.
Steady-loop machine code, counted from `MAJIT_DUMP` output over equal
numbers of compiled traces:
nested_loop cset 160 -> 0 tst 240 -> 80
int_loop cset 4 -> 0 tst 6 -> 2
raise_catch cset 1207 -> 0 tst 2011 -> 804
fannkuch cset 157851 -> 14028 tst 183327 -> 39504
Each folded pair goes from `cmp; cset xN, lt; tst xN, xN; b.ne` to
`cmp; b.lt`.
Assisted-by: Claude
`float_opcode_to_cc` mapped FLOAT_GT to `hi` and FLOAT_GE to `hs`, the
`seta` / `setae` spelling that is correct after x86 `ucomisd` because that
sets CF on an unordered compare. `fcmp` instead sets NZCV = 0b0011, so C is
set and `hi` (C set, Z clear) and `hs` (C set) are both TRUE when either
operand is NaN. `opassembler.py:314-315` uses `gt` and `ge`, which test
N == V and are false in that state.
Compiled loop over `x = float('nan')`, 30000 iterations:
before nan > 1.0 -> True 28558x nan >= 1.0 -> True 28358x
after nan > 1.0 -> False nan >= 1.0 -> False
FLOAT_LT / FLOAT_LE / FLOAT_EQ / FLOAT_NE already used `lo` / `ls` / `eq` /
`ne`, which match `opassembler.py:310-313` and are false (true for `ne`) on
an unordered compare.
Assisted-by: Claude
The regalloc float-comparison arm emitted `ucomisd a, b` followed by a
`SETcc` taken straight from a condition table: FLOAT_LT -> `setb`,
FLOAT_LE -> `setbe`, FLOAT_EQ -> `sete`, FLOAT_NE -> `setne`. UCOMISD sets
ZF = PF = CF = 1 when either operand is NaN, so all four report the wrong
answer for an unordered compare.
`assembler.py:1322 _cmpop_float` avoids it two ways, both ported here:
FLOAT_LT / FLOAT_LE compare in the reverse order so they can use the
`A` / `AE` forms, which are already false on unordered; FLOAT_EQ / FLOAT_NE
have no such form and get `assembler.py:1314
_if_parity_clear_zero_and_carry`, which clears ZF and CF via `cmp rbp, 0`
when PF is set.
`float_opcode_to_cc` had no other caller and is removed.
Compiled loop over `x = float('nan')`, 20000 iterations, `(lt, le, gt, ge,
eq, ne)` against CPython:
nan vs 1.0 before (19599, 19599, 0, 0, 19599, 601)
after ( 0, 0, 0, 0, 0, 20000)
1.0 vs nan before (19999, 19999, 0, 0, 19999, 1)
after ( 0, 0, 0, 0, 0, 20000)
Verified on x86_64-apple-darwin under Rosetta over 15 operand pairs
covering signed zeros, infinities and NaN in either position; all now match
CPython. FLOAT_GT / FLOAT_GE were already correct here — they are the two
the aarch64 table got wrong.
Assisted-by: Claude
`assembler.py:1293 flush_cc` opens with `assert self.guard_success_cc == rx86.cond_none`, so the invariant covers every call. Both backends checked it only inside the frame-register sentinel branch, leaving the materialize path unchecked. A condition still pending on entry was published by an earlier op and never consumed, which would make the following guard branch on it instead of on its own operand. Verified not to fire: the whole of pyre/bench and pyre/bench/synth (337 files) run clean against a release build with `-C debug-assertions=yes`. Assisted-by: Claude
…reach Every guard emitted `b.<inv_cc> skip; b =>label; skip:` so that the branch to the failure stub carried the unconditional `b`'s 26-bit displacement. That was a blanket workaround for one case: a trace larger than `b.cond`'s +1MB forward reach, which cannot be reached by a single conditional branch at all (logo's 70000-op trace). Decide it per trace instead. The failure-recovery stubs are written directly after the body, so a guard reaches its stub whenever the trace's own code stays inside that window; bound it by the operation count against a 1KB-per-operation ceiling. Traces over the bound keep the long form. The estimate is checked: `_assemble` asserts the emitted body plus a stub allowance fits the reach whenever the short form was used, and the whole of pyre/bench and pyre/bench/synth (337 files) runs clean against a release build with `-C debug-assertions=yes`. nested_loop's steady loop, which carries four guards, goes from 24 to 20 instructions. Assisted-by: Claude
`opassembler.py:94 emit_comp_op_int_mul_ovf` checks the product for overflow with `CMP_rr_shifted(ip0, res, 63)` — `cmp Xn, Xm, asr #63`, which takes the shift as part of the compare. The emit here did the shift into a scratch register first (`asr x14, dst, 63; cmp x15, x14`). nested_loop's steady loop goes from 20 to 19 instructions. Assisted-by: Claude
…gister `emit_cmp_loc_loc` materialized every immediate operand into x17 and then compared register-to-register. `opassembler.py:129 emit_int_comp_op` takes `CMP_ri` when the right-hand side is an immediate, and `codebuilder.py:389 CMP_ri` encodes a 12-bit unsigned field; wider values still need the register. dynasm's `cmp Xn|SP, #uimm` form rejects a dynamic register operand — it cannot tell whether the register is SP — so the instruction is encoded directly, as SUBS with Rd = xzr, the same word `CMP_ri` writes. Checked against llvm-mc for the range endpoints. A list-indexing loop's steady body (bounds check, class guard, storage-kind check, second bounds check, indexed load, add-overflow) goes from 34 to 33 instructions; the folded compare is the storage-kind check `cmp x11, #1`. Assisted-by: Claude
The two sites that fold a base offset into an array index emitted `mov x16, index; add x16, x16, #ofs`. `opassembler.py:403` does it with a single `ADD_ri(ip0, index, ofs)`; both sites already gate on the same `check_imm_arg` range, so the move is pure overhead. Encoded directly for the same reason as the compare immediate: dynasm's `add Xd|SP, Xn|SP, #uimm` form rejects a dynamic register operand. Checked against llvm-mc at both range endpoints. A list-indexing loop's steady body goes from 33 to 32 instructions. Assisted-by: Claude
`consider_float_cmp` and `consider_float_cmp_j2` allocated the result with `force_allocate_reg`. `x86/regalloc.py:682 _consider_float_cmp` uses `force_allocate_reg_or_cc`, so a comparison whose only consumer is the next guard gets the frame-register sentinel instead of a general-purpose register. Both emit sides now end in `flush_cc` rather than a bare `cset` / `SETcc`, matching `opassembler.py:138 emit_comp_op_float_*` and `assembler.py:1345 genop_cmp_float`. x86's `flush_cc` no longer clears the destination first. `assembler.py:1300` needs its `MOV imm0` because `SET_ir` writes only the low byte; `emit_setcc` here ends in `movzx r32, r8`, which zeroes bits 8..63 on its own, so the MOV was dead. Dropping it takes every comparison that does materialize a boolean back to two instructions. Every `fcmp` in the nbody and spectral_norm traces is now followed directly by a conditional branch where it was followed by `cset` (nbody 47768 sites, spectral_norm 1040). The largest emitted loop goes from 809 to 753 instructions on nbody and from 54 to 52 on spectral_norm. Wall clock is unchanged: interleaved, order-alternated min-of-9 over nbody, spectral_norm, float_loop, int_loop and fannkuch moves every bench by at most 1.1%, which is inside this box's noise floor. Verified against CPython on arm64 and on x86_64 under Rosetta, including a new fixture whose float booleans are consumed as values rather than by an adjacent guard. Output is identical between the two architectures across all 336 bench + synth fixtures except nbody, whose cross-architecture float drift reproduces byte-for-byte on an x86_64 build of the parent commit. check.py --backend dynasm is 331 passed, and all 341 fixtures run clean against a build with -C debug-assertions=yes. Assisted-by: Claude
…end arm Remove the append_inplace_wb_covered optimization that dropped the recorded list_write_barrier(W_ListObject) on w_list_append's Object-strategy in-place arm in favour of the backend COND_CALL_GC_WB_ARRAY on the block's setarrayitem. A guard-failure bridge re-materializes the items block and appends into it without that array barrier firing, so an old->young element store never enters the remembered set; a later minor collection frees the still-referenced young element and the collector reads a freed header (validate_type_id / object_total_size panic; nested_list_comprehension_hot sized up, dynasm only). Removes FbwWalkMode::append_inplace_wb_covered_receiver (field, setter in the w_list_append fold sub-walk) and the w_list_append_stores_into_gc_block_in_place predicate. (cherry picked from commit b301a3c)
|
Rebased onto current Added: cherry-pick of
|
| fixture | site | before | after |
|---|---|---|---|
nested_list_comprehension_hot |
object_total_size |
5/5 abort | 0/56 |
comprehension_object_append_hot |
object_total_size |
5/5 abort | 0/56 |
const_arg_call_resume |
object_total_size |
5/5 abort | 0/56 |
exception_traceback_frame_lineno |
copy_nursery_object |
5/5 abort | 0/56 |
(after = 7 nursery sizes x 8 reps each). #872's commit clears both sites, including const_arg_call_resume and the copy_nursery_object fixture, which its title does not name — the two sites shared one producer, a missing remembered-set entry for an appended old->young element.
The census also confirms the config CI actually runs (PYRE_STDLIB + MAJIT_STRICT=1 + MAJIT_STATS=1, default nursery) is 0 panics / 337 fixtures on arm64 and on x86_64 under Rosetta, both before and after.
Verification
python3 pyre/check.py --backend dynasm,cranelift: 331 passed on both backends. The sole failure is synth/ast_compile_roundtrip, which check.py itself marks BASEFAIL and which fails identically on cranelift.
Not addressed here
synth/attr_instance_shadows_class is 148x slower than pypy at its shipped N=30000 (14490 ms vs 98 ms) and scales superlinearly — doubling N roughly triples pyre's time while pypy stays flat. It is not a panic and not JIT-related (PYRE_NO_JIT=1 is slower). Profile: mapdict_root_walker_area 31.1%, OldGen::contains 25.3%. walk_mapdict_roots_area clones the entire INSTANCE_DICT side table on every minor collection with a per-entry mutex re-lock, so minor GC costs O(live instances) rather than O(nursery); there is no remembered set for that table. That is mapdict + collector work, a different subsystem from this branch's dynasm backend changes, so it is left for a separate change.
— commented by Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79cea09137
ℹ️ 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 recorded = ctx | ||
| .trace_ctx | ||
| .record_op_with_descr(call_opcode, &allboxes, descr.clone()); |
There was a problem hiding this comment.
For Object-strategy in-place appends, this records list_write_barrier as an ordinary residual call in every steady-state iteration even though the backend already emits COND_CALL_GC_WB_ARRAY; the stated failure occurs specifically when a guard-failure bridge re-materializes the items block without emitting that array barrier. Keeping an extra owner-barrier call in all normal traces masks that bridge-generation defect and imposes a duplicate call on this hot path, so restore the upstream recording shape and add the missing write barrier to the bridge/rematerialization path instead.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-backend-dynasm/src/regalloc.rs`:
- Around line 4135-4138: Preserve unordered-NaN semantics when fusing
FloatGt/FloatGe comparisons into condition-code results: update the
force_allocate_reg_or_cc handling at regalloc.rs lines 4135-4138 to materialize
or branch with an unordered-aware condition instead of publishing raw CC_G/CC_GE
from AArch64 FCMP, and apply the same correction to the J2 path at lines
4156-4158.
In `@pyre/pyre-interpreter/src/jit_fnaddr.rs`:
- Around line 1482-1485: Clarify the comment describing the residual barrier and
enclosing W_ListObject: state that the list slot keeps the appended element
reachable, while the barrier records the old W_ListObject so minor GC traces the
old-to-young edge. Preserve the existing inline sub-walk behavior and
reachability context.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs`:
- Around line 490-496: After assigning frame.f_backref in the entered branch,
invoke the existing GC write-barrier mechanism for the old-generation frame and
the newly stored saved_topframeref before publishing or using the shadow-stack
root. Keep the update scoped to this f_backref publication so minor GC preserves
the residual frame chain.
🪄 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 Plus
Run ID: d2ef540b-eea2-4b81-b288-48bfbb4ba33c
📒 Files selected for processing (16)
majit/majit-backend-dynasm/src/aarch64/assembler.rsmajit/majit-backend-dynasm/src/aarch64/opassembler.rsmajit/majit-backend-dynasm/src/aarch64/regalloc.rsmajit/majit-backend-dynasm/src/regalloc.rsmajit/majit-backend-dynasm/src/x86/assembler.rsmajit/majit-translate/src/front/mir.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/executioncontext.rspyre/pyre-interpreter/src/jit_fnaddr.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/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-object/src/listobject.rspyre/pyre-object/src/unicodeobject.rs
💤 Files with no reviewable changes (1)
- pyre/pyre-object/src/listobject.rs
| // x86/regalloc.py:682 — a float comparison whose only consumer is the | ||
| // next guard leaves its answer in the flags, like the integer one. | ||
| let ops_ref: &[Op] = self.operations; | ||
| let result_loc = self.force_allocate_reg_or_cc(op.pos.get(), ops_ref, i); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve unordered-NaN semantics before fusing FloatGt/FloatGe.
AArch64 FCMP on NaN leaves N=0, Z=0, C=1, V=1; raw b.gt and b.ge both succeed because N == V. Publishing CC_G/CC_GE through the sentinel therefore makes a following guard incorrectly pass for NaN. Add unordered-aware flag handling, or exclude these operations from CC fusion until that handling exists.
majit/majit-backend-dynasm/src/regalloc.rs#L4135-L4138: retain a NaN-correct materialization path or use an unordered-aware fused branch.majit/majit-backend-dynasm/src/regalloc.rs#L4156-L4158: apply the same handling to the J2 path.
📍 Affects 1 file
majit/majit-backend-dynasm/src/regalloc.rs#L4135-L4138(this comment)majit/majit-backend-dynasm/src/regalloc.rs#L4156-L4158
🤖 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-backend-dynasm/src/regalloc.rs` around lines 4135 - 4138,
Preserve unordered-NaN semantics when fusing FloatGt/FloatGe comparisons into
condition-code results: update the force_allocate_reg_or_cc handling at
regalloc.rs lines 4135-4138 to materialize or branch with an unordered-aware
condition instead of publishing raw CC_G/CC_GE from AArch64 FCMP, and apply the
same correction to the J2 path at lines 4156-4158.
| // the inline sub-walk must decline. The residual barrier remembers the | ||
| // enclosing `W_ListObject`, whose trace reaches every item slot, and is | ||
| // the only thing keeping an appended `old -> young` element reachable | ||
| // across a minor collection. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the barrier’s role in reachability.
The write barrier does not itself keep the appended element reachable; the list slot does. Its role is to record the old W_ListObject so minor GC traces that old-to-young edge.
Proposed wording
- // the inline sub-walk must decline. The residual barrier remembers the
- // enclosing `W_ListObject`, whose trace reaches every item slot, and is
- // the only thing keeping an appended `old -> young` element reachable
- // across a minor collection.
+ // the inline sub-walk must decline. The residual barrier remembers the
+ // enclosing `W_ListObject`, whose trace reaches every item slot, so the
+ // collector scans an appended `old -> young` edge during minor collection.📝 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.
| // the inline sub-walk must decline. The residual barrier remembers the | |
| // enclosing `W_ListObject`, whose trace reaches every item slot, and is | |
| // the only thing keeping an appended `old -> young` element reachable | |
| // across a minor collection. | |
| // the inline sub-walk must decline. The residual barrier remembers the | |
| // enclosing `W_ListObject`, whose trace reaches every item slot, so the | |
| // collector scans an appended `old -> young` edge during minor collection. |
🤖 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-interpreter/src/jit_fnaddr.rs` around lines 1482 - 1485, Clarify
the comment describing the residual barrier and enclosing W_ListObject: state
that the list slot keeps the appended element reachable, while the barrier
records the old W_ListObject so minor GC traces the old-to-young edge. Preserve
the existing inline sub-walk behavior and reachability context.
| if entered { | ||
| unsafe { | ||
| (*frame).f_backref = saved_topframeref; | ||
| (*ec).topframeref = frame; | ||
| } | ||
| } | ||
| let saved_root = majit_gc::shadow_stack::push(majit_ir::GcRef(saved_topframeref as usize)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Add a write barrier after publishing f_backref.
frame is old-gen while saved_topframeref can be a nursery JitVirtualRef. Rooting saved_root forwards only the shadow-stack slot; without remembering frame.f_backref, a minor GC leaves that field stale while residual code can still walk the chain.
Proposed fix
if entered {
unsafe {
(*frame).f_backref = saved_topframeref;
+ if pyre_object::gc_hook::try_gc_owns_object(frame as *mut u8) {
+ pyre_object::gc_hook::try_gc_write_barrier(frame as *mut u8);
+ }
(*ec).topframeref = frame;
}
}📝 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.
| if entered { | |
| unsafe { | |
| (*frame).f_backref = saved_topframeref; | |
| (*ec).topframeref = frame; | |
| } | |
| } | |
| let saved_root = majit_gc::shadow_stack::push(majit_ir::GcRef(saved_topframeref as usize)); | |
| if entered { | |
| unsafe { | |
| (*frame).f_backref = saved_topframeref; | |
| if pyre_object::gc_hook::try_gc_owns_object(frame as *mut u8) { | |
| pyre_object::gc_hook::try_gc_write_barrier(frame as *mut u8); | |
| } | |
| (*ec).topframeref = frame; | |
| } | |
| } | |
| let saved_root = majit_gc::shadow_stack::push(majit_ir::GcRef(saved_topframeref as usize)); |
🤖 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/residual_call.rs` around lines 490 -
496, After assigning frame.f_backref in the entered branch, invoke the existing
GC write-barrier mechanism for the old-generation frame and the newly stored
saved_topframeref before publishing or using the shadow-stack root. Keep the
update scoped to this f_backref publication so minor GC preserves the residual
frame chain.
1 participant
Two independent lines of work sit on this branch: the aarch64 backend's steady-loop code quality (which is where the loop-bench gap against PyPy actually was), a float-comparison miscompile on both dynasm architectures, and the earlier #841 review follow-ups + vref-producer hardening.
1. The loop gap vs PyPy was the guard veneer
Wall-clock A/B on the dev box is not usable for this: running the same binary under two labels put
nested_loopat 2.11x and 1.96x — opposite sides of the 2x gate — and the absolute level drifts ~40% between runs minutes apart. So the work was driven by disassembling pyre's steady loop and PyPy's steady loop for the same program and diffing them, which is deterministic and load-immune.For
while j < 30000: s = s + i*j; j = j + 1: pyre 24 instructions, PyPy 20.Root cause.
emit_bcond_to_labelemitted every guard asso the branch to the failure stub got the unconditional
b's 26-bit range. That was a blanket workaround for a single case — a trace larger thanb.cond's +1MB forward reach (logo's 70000-op trace), which cannot use one conditional branch at all. PyPy emits a single patchedb.cond(it appears asbrk #0injit-backend-dumpbefore patching).Every guard therefore cost two instructions instead of one, plus an extra branch in the fetch stream — and traces are guard-dense, so it scaled with everything. It is now decided per trace in
_assemble(ops.len() * 1024 >= 1<<20keeps the long form). The estimate is not taken on faith:_assembleasserts the emitted body plus a stub allowance fits the reach whenever the short form was used, and all 337pyre/bench+pyre/bench/synthfixtures run clean against a release build with-C debug-assertions=yes.Three smaller parity gaps in the same sweep, each a real divergence from
opassembler.py:int_mul_ovfasr x14, dst, 63+cmp x15, x14CMP_rr_shifted(ip0, res, 63)=cmp Xn, Xm, asr #63cmpreg,regemit_int_comp_optakesCMP_rifor a 12-bit operandmov x16, index+add x16, x16, #ofsADD_ri(ip0, index, ofs)The last two are encoded as raw words, because dynasm's
cmp/add Xd|SP, Xn|SP, #uimmforms reject a dynamic register operand — it cannot prove the register isn't SP. Both encodings arecodebuilder.py's own (CMP_ri,ADD_ri) and were checked againstllvm-mcat the range endpoints.Also on this branch: the compare→guard condition-code fold, which is two commits. The first records an adjacent comparison's cc so the guard branches on the live NZCV instead of re-testing a materialized boolean. The second completes the frame-register sentinel that x86 already had —
force_allocate_reg_or_cchands a comparison the frame register when the next op consumes the flags, and the emit side publishesguard_success_ccand emits nothing. It was#[cfg(target_arch = "x86_64")]-gated precisely because aarch64 had noflush_cc(cset x29, ccwould destroy the frame pointer). aarch64 now has one, the gate is lifted,consider_int_is_true_j2routes through it (regalloc.py:469 prepare_comp_unaryallocates no destination for the folded form), andgenop_discard_cond_callconsumes a published condition the way_emit_op_cond_callskips its CMP whenarglocs[0] is None.Result
Steady-loop instruction counts, pyre vs PyPy — pyre is now shorter on five of six:
Wall clock, taken on a quiet box with an interleaved, order-alternated min-of-11 harness, and reproduced on a reversed-order repeat (the after-column landed within ±0.05x both times):
nbodyis float-heavy and flat within the noise floor; nothing here touches float codegen paths.lstat 32 vs 21 is the remaining per-loop gap — list indexing carries two bounds checks, a class guard and a storage-kind guard, plus three instructions to materialize a constant class-pointer address each iteration. Upstream also hasprepare_comp_op_float_*/emit_comp_op_float_*(float compare folded into the following guard), which pyre still lacks. Both are follow-ups, not in this PR.Where the residual gap now lives (measured after these commits)
Worth recording because it inverts the reading this branch started from. With the veneer gone, wall clock tracks instruction count again:
acc + q[i] + q[j])Per-instruction cost is 1.1–1.4x, not the ~2.3x measured before the veneer fix, so instruction count is the dominant term and "the gap is per-op cost, not IR" no longer holds. The 15 extra instructions in the list loops are exactly the per-iteration class guard (6), storage-kind guard (3),
length/itemsreload (2) and bounds checks (5) that PyPy hoists into its preamble and pyre re-executes; PyPy's steady body carries none of them.That hoisting is the short-preamble heap import, which is separately known to be dead in pyre and to miscompile bridges when revived — an optimizer epic, not something reachable from the backend. The aarch64 backend itself is now at parity for these loops: the constant-first
mov+cmpand the four-instruction scaled array access that remain are byte-for-byte what PyPy emits (prepare_int_cmponly testsarg1;load_supported_factors = (1,)forces the explicitint_lshift).Ratios below
~0.15sof PyPy time are startup noise and must not be quoted — the same list loop reads as 4x at the fixture's own N and 1.67x once scaled.2. Float comparisons miscompiled NaN on both dynasm architectures
Found while reviewing the above. The live float-comparison arm (
regalloc_perform) emitted a bareSETcc/csetfrom a condition table with no unordered handling. One x86 table had been copied to aarch64, so each architecture got wrong exactly the half the other handles correctly:fcmpsets NZCV=0b0011lolshi→ Truehs→ Trueucomisdsets ZF=PF=CF=1setb→ Truesetbe→ Truesete→ Truesetne→ FalsesetasetaeIn a compiled loop with
x = float('nan'), 30000 iterations: aarch64 evaluatednan > 1.0as True 28558 times; x86 evaluatednan < 1.0True 27358 times,nan == 1.0True 26958 times, andnan != 1.0True only 3242 times (the interpreter prefix before the trace compiled).gt/getest N == V and are false when unordered, which is whatopassembler.py:310-315uses.hi/hstest C, whichfcmpsets for NaN.assembler.py:1322 _cmpop_float— FLOAT_LT/LE compare in the reverse operand order so they can use theA/AEforms, and FLOAT_EQ/NE get_if_parity_clear_zero_and_carry(jnp skip; cmp rbp, 0; skip:).Both verified against CPython over 15 operand pairs covering ±0.0, ±inf and NaN in either position; x86 verified under Rosetta on
x86_64-apple-darwin.cranelift and wasm were never affected —
FloatCC::LessThanandf64.ltare IEEE-ordered by construction.Worth flagging for future backend audits: the dead legacy
genop_float_cmpin both files gets this right (it swaps for lt/le and usessete+setnp). It has no callers. A reading-only review that lands on the dead spelling will conclude the backend is fine.3. #841 review follow-ups and vref-producer hardening
Unchanged from the previous revision of this PR; restated in brief.
force_all_framesforced nothing. Flagged by both reviewers; the finding is real, the attribution was not. Upstream gets the force out of the walk —pyframe.pydeclaresf_backref = jit.vref_None, sogetnextframe_nohidden'sframe.f_backref()is ajit_force_virtual. Making pyre's walkers force-free was correct; what is missing is the vref producer, soforce_vrefis the identity. Until it exists, this consumer states the force directly. Still unobservable — a regression bench was built, did not discriminate (same count underPYRE_NO_JIT=1), and was deleted rather than committed;sys.settracestill fires zero events under pyre, before and after #791.The other seven #841 findings were refuted, each verified against current code and re-attacked by an independent skeptic:
_warningsSTATE_NSis rooted viawalk_process_import_roots;hash("")cannot disagree between entry points; the wasm nursery-hook disarm is reachable only from a#[test]; the_warningsstale-categorylocal is always aW_TypeObjectand heap classes allocate old-gen; specialised tuples cannot reach thewrappeditemspath; theFnDefconstant is a synthetic 0-argCallwhoseresult_tyis the return value; the string-hash memo belongs to the threading epic as one change.Three vref hardening changes, each independently correct and a no-op today, each a crash the moment a producer exists: the frame-chain re-entry test resolved through
vref_referentrather than by forcing (forcing would clearTOKEN_TRACING_RESCALLand make a reader report its own read as a callee escape); the displacedtopframerefrooted throughmajit_gc::shadow_stackat the two sites that parked it raw across collectable code; and the vref given its own materializer header, sincematerialize_virtual_objectwas dereferencing theJIT_VIRTUAL_REF_VTABLEmagic as a type object.The producer itself is deliberately not here —
alloc_virtual_refisBox::into_raw(host heap, not GC-registered), the normal non-escapingleaveleavesforced == NULLwhich is exactly whateval.rs'sexpectaborts on, and the walker never calls the vref bracket. Those are design changes that cannot be exercised without the producer and vice versa. There is also a fork to decide first:stop_tracking_virtualrefmust recordVIRTUAL_REF_FINISHbefore theCALL, but pyre's walker records the call op first and concrete-executes after, in all four dispatchers.Five comments corrected across
trace.rs,jitcode_dispatch/mod.rsandmir.rs, including one that framed an orthodoxValueType::Intstamp as a deviation and invited a future "fix" toRefthat would be the actual deviation.Verification
python3 pyre/check.py --backend dynasm,craneliftre-run after every commit in section 1 and 2: 331 passed on both backends each time. The single failure issynth/ast_compile_roundtrip, which check.py itself marksBASEFAILand which fails identically on cranelift — a backend none of these changes touch.cargo test -p majit-backend-dynasmand-p majit-metainterppass.pyre/bench+pyre/bench/synthfixtures swept against a release build with-C debug-assertions=yes: neither the guard-branch range assumption norflush_cc's precondition ever fires.CMP_ri/ADD_riwords checked againstllvm-mc --disassemble --triple=aarch64at both range endpoints.int_mul_ovfboundary values, and every loop bench.A note for whoever reads CI: a perf-gate failure on this suite is not by itself evidence of a regression. The same tree failed
dynasm nested_loop ratio 2.3x > gate 2xand then passed minutes later with no code change; the noise floor on a shared box exceeds the gate margin. Re-run before believing it.— commented by Claude
Summary by CodeRabbit