jit: overflow-arm specialization, paused-caller resume PC fix, dynasm GuardOverflow CC, attr live markers - #750
Conversation
|
Warning Review limit reached
Next review available in: 38 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 Plus Run ID: 📒 Files selected for processing (17)
✨ 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: 1050de6934
ℹ️ 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".
| jc.index as u32, | ||
| fallthrough, | ||
| jc.payload.after_residual_marker_for_jitcode_pc(call_jit_pc), | ||
| inline_call_return_marker(&jc.payload, call_jit_pc), |
There was a problem hiding this comment.
When an inlined residual call is followed by a fallthrough opcode that emits its own jitcode, the immediate post-call live byte returned here is decodable but is not the pc-map marker for the fallthrough PC. The later depth_trivia_for_jitcode_pc / result_color_trivia_for_jitcode_pc lookups then resolve this marker by predecessor to the call opcode itself, so the paused caller snapshot can use the pre-call stack depth/result color instead of the call-result slot. This corrupts multi-frame resume for common shapes like a call followed by a real consumer/store; keep this immediate marker for the blackhole return PC but source depth/result color from the after-residual fallthrough tables keyed by call_jit_pc.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 2d92251). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
36f80cd to
6b792bc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b792bc42a
ℹ️ 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".
| unsafe { pyre_object::longobject::jit_bigint_fits_int(rhs_payload) }; | ||
| ctx.trace_ctx | ||
| .set_opref_concrete(count_fits, majit_ir::Value::Int(count_fits_concrete)); | ||
| walker_emit_guard_with_snapshot(ctx, op_pc, OpCode::GuardTrue, &[count_fits])?; |
There was a problem hiding this comment.
When recording W_LongObject(0) << huge_long_count, the interpreter path now returns a W_LongObject for the zero-base overflow case, so this specialization passes the is_long(boxed_result_obj) gate instead of declining. For that same recorded input jit_bigint_fits_int(rhs_payload) is 0, but this line still emits GuardTrue(count_fits), producing a trace whose new guard fails for the operands being recorded rather than matching the interpreter hot path; decline before emitting when the concrete shift count does not fit, or skip this guard for the zero-left-shift case.
AGENTS.md reference: AGENTS.md:L15-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb42f5e24a
ℹ️ 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 raw = walker_unbox_int(ctx, op_pc, it, int_type_addr)?; | ||
| for (&it, &(v, is_fits_long)) in items.iter().zip(vals.iter()) { | ||
| let raw = if is_fits_long { | ||
| walker_unbox_long(ctx, op_pc, it, long_type_addr)? |
There was a problem hiding this comment.
When this Integer-list branch is recorded with a fits-in-word W_LongObject, the emitted path only goes through walker_unbox_long, which guards ob_type == LONG_TYPE and _fits_int but not the w_class check that is_plain_int1 used to choose ListStrategy::Integer. If the trace is later reused with an app-level int subclass backed by W_LongObject, it passes these guards even though list_strategy_for would choose Object storage, so the list literal stores an unboxed integer and loses the subclass object; add the same exact-w_class guard used by the specialised tuple/list-append paths before unboxing.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
invalidate() stored only the root flag, so a bridge attached before the call kept a clear generation flag and its GUARD_NOT_INVALIDATED stayed live. model.py:145 invalidate_loop activates the guards in the loop and its attached bridges; only a bridge compiled after the invalidation starts valid. Extends the generation unit test with a pre-invalidation bridge flag. Reported by the codex parity review on PR #738. Assisted-by: Claude
try_walker_specialize_binary_op_int emitted the int fast path with an unconditional guard_no_overflow even when the concrete Add/Sub/Mul result had overflowed into a W_Long. A loop whose operation always overflows then failed that guard on every iteration, and each recovery bridge re-entered the same specialization and was stillborn at its own guard, minting a new bridge every trace_eagerness failures without bound (measured 199 bridges / 39901 guard failures at 80k iterations; int_mul_ovf_bignum_promote showed the same root as a 49-abort storm). pyjitpl.py:1881 handle_possible_overflow_error follows the concrete outcome instead of speculating. Route the concretely-overflowing case to the generic residual leg, next to the existing FloorDiv/Mod zero-divisor bails. The always-overflow loop then compiles with the bignum residual and no overflow guard (0 bridges / 2 guard failures, wall time at pypy parity), and a bridge traced from a rarely-overflowing guard carries the generic arm, ending the stillborn chain. synth: int_mul_ovf_bignum_promote aborts 49 -> 0; binary_int_overflow_local_resume guard failures 10211 -> 312. check.py --synthetic-only dynasm 280/280. Assisted-by: Claude
Guards emitted inside LoadAttr/StoreAttr walker specializations had no per-opcode resume marker; derive_pc_live_indices_from_sparse rounded their PC back to the preceding opcode's marker, whose Python coordinate and stack depth belong to another opcode. A kept-stack bridge from such a guard (mapdict promoted-map deopt after a mid-loop attribute type change) then hit the marker-entry depth-mismatch decline in run_perfn_walk, and fbw_decline permanently disabled bridges for the whole loop: guard_failures grew linearly with the iteration count and no bridge ever compiled. Extend the existing FOR_ITER-body per-PC leading-marker insertion to LoadAttr/StoreAttr PCs (rpython/jit/codewriter/flatten.py:258-260, 282-286 puts a -live- at each guard resume point). Other while-loop body PCs stay excluded as before. mapdict_unboxed_type_change_attr: guard_failures 25002 -> 203 (n-invariant), bridges_compiled 0 -> 1 per shape. Compile counts on nbody/spectral_norm/fannkuch/class_attrs_methods/nested_loop_correctness are unchanged. Assisted-by: Claude
Both dynasm assemblers emitted GuardOverflow through the same arm as GuardNoOverflow, so the guard passed exactly when overflow did NOT occur. Upstream aliases GUARD_NO_OVERFLOW to guard_true and GUARD_OVERFLOW to guard_false of the producer's success condition (aarch64/opassembler.py:547-551, x86/assembler.py:1873-1874). Invert the taken guard_success_cc for GuardOverflow before the common guard emitter derives its fail condition. Nothing emitted GuardOverflow on these backends before, so the wrong condition was unreachable. The cranelift and wasm backends already branch on the correct sense. Assisted-by: Claude
Guards emitted inside an inlined callee encoded the paused caller frame's resume PC as the semantic-fallthrough marker, past intervening live and virtualizable-sync instructions. Blackhole return setup walks backward from that coordinate to recover the call's destination register (get_list_of_active_boxes reads bytecode[pc - 1], pyjitpl.py), so the overshot coordinate made it read an unrelated operand byte: on inline_bignum_bridge_twoclamp the byte was zero, the callee result overwrote caller r0 (the module PyFrame), the real result register was never written, and the following STORE_NAME residual dereferenced the clobbered frame and crashed. Add inline_call_return_marker(), which decodes the call instruction and certifies its exact next_pc with can_decode_live_vars — the coordinate a paused MIFrame.pc keeps while its callee runs — and use it for top-level, nested, and recipe-reconstructed paused callers. Guard-own-frame coordinates keep the existing after-residual marker. Also lets the kept_stack_boxed_in_handler exception-handler bridge compile: guard_failures drop from 50068 (linear) to 402. Assisted-by: Claude
… the generic leg
A concretely overflowing Add/Sub/Mul in the int binop walker
specialization previously bailed to the generic BINARY_OP residual,
which records an opaque CallMayForceR: per iteration that costs the
force-token store, operand boxing, GuardNotForced/GuardNoException/
GuardClass, and full generic dispatch inside the callee. PyPy keeps
specializing on the overflow arm (pyjitpl.py:1881
handle_possible_overflow_error -> intobject.py:494 _make_ovf2long):
int_*_ovf + guard_overflow, then a raw-int elidable rbigint helper.
Port that arm: record IntAddOvf/IntSubOvf/IntMulOvf on the unboxed
operands, guard_overflow, call the new elidable
jit_bigint_{add,sub,mul}_int_int payload helpers (rbigint.py:717/788/
873) on the raw ints, guard the newlong demote attempt with
jit_bigint_fits_int + GuardFalse, and box the payload inline —
mirroring the existing W_LongObject binop specialization. Both
concrete-overflow deferrals (the original bail and the rebased-in
duplicate) are superseded and removed.
int_mul_ovf_bignum_promote: 3.9x/4.8x pypy -> 1.9x dynasm / 0.7x
cranelift; gate header tightened from 30 to 8.
Assisted-by: Claude
charge_oldgen_external ran the old-gen membership probe (arena scan + rawmalloc hash lookup) for every address, but nursery objects are the common case on this path and are never old-gen. Answer them with the O(1) nursery range check first. Assisted-by: Claude
…t bigint helpers
- jit_bigint_{add,sub,mul}_int_int compute the result directly in i128
(exact for any i64 pair) instead of going through general bigint ops.
- jit_bigint_{add,sub} try an i128 fast path when both operands and the
result fit two limbs, falling back to the general op otherwise.
- alloc_bigint_nursery_collecting skips the memory-pressure and old-gen
external charge crossings when the external payload is at most
SMALL_EXTERNAL_EXEMPT_BYTES (64B = 8 limbs); the end-of-major
recompute absorbs the drift for survivors.
int_mul_ovf_bignum_promote steady phase: 90 -> 59 ns/iter
(pypy-ratio 5.0x -> 3.5x). check.py green on dynasm/cranelift/wasm.
Assisted-by: Claude
Demote every fitting bigint result to W_IntObject only where PyPy's newlong / space.newint does, instead of on every long op. Interpreter (descroperation.rs): - long add/sub/mul/and/or/xor, unary neg/abs/invert/pos, pow, lshift, floordiv, long%long, rshift with a normal count, and the trivial-base pow and zero-base lshift short-circuits box as W_LongObject (newlong). - long % machine-int still demotes (space.newint), and rshift with a count that overflows a machine int still yields space.newint(-1)/(0). - long_mod splits on the RHS kind: an int RHS demotes, a long RHS keeps the W_LongObject. - divmod with a long receiver keeps both parts as W_LongObject (_divmod/_int_divmod both newlong), bypassing the remainder demote. - 3-arg pow keeps W_LongObject unless all three operands are machine ints, matching descr_pow's W_LongObject(result); box_bigint_result is replaced by the kind-gated pow_mod_result. - int-overflow promotion and int MIN//-1 / MIN%-1 / int<<int keep the fits-checked demote (newlong_from_rbigint). JIT (specialize.rs, trace_opcode.rs, longobject.rs): - The long-binop specializer emitted a GuardFalse(fits_int(result)) demote valve premised on "a record-time W_LongObject never fits i64". That premise no longer holds, so remove the valve for the arithmetic ops (a fitting result stays a W_LongObject in the trace) and replace it for the shift ops with GuardTrue(fits_int(count)) so a huge-count replay deopts to the generic leg. Drop the decline-on-int branch (the shift huge-count case is caught by the existing !is_long decline). - Delete the dead jit_bigint_result_box helper and its tests. - Exclude a fits-int W_LongObject value in the namespace int-cell store fold explicitly instead of relying on a heapcache index miss. The W_IntObject/W_LongObject distinction is invisible at the Python level (equal repr/str/hash/eq/is), so this is a representation change only; a 3387-line int/long op matrix matches pypy3 exactly and check.py is green on dynasm/cranelift/wasm. Assisted-by: Claude
The 3-arg pow exp==0 fast path reduced 1 by the truncating `%` (bigint_mod), so `pow(2, 0, -13)` returned 1 instead of -12. Use mod_floor to match Python's `1 % m`. Assisted-by: Claude
…append int folds is_plain_int1 (IntegerListStrategy.is_correct_type and makespecialisedtuple2) admits an exact W_IntObject or a fits-in-word W_LongObject, and plain_int_w unwraps either. The three walker-native int folds required an exact &INT_TYPE and declined a fits-long to the residual; now they accept it: - try_walker_specialize_newlist and try_walker_specialize_newtuple classify each element with is_plain_int1, read the payload via jit_w_long_toint for a long, and unbox per element with walker_unbox_long (guard_class LONG_TYPE + jit_w_long_fits_int GuardTrue + jit_w_long_toint) or walker_unbox_int. - orthodox_list_append_recognize / orthodox_list_append_commit admit a fits-long value and pin guard_class(value, LONG_TYPE) for it; the descended w_list_append body unboxes the long through its own is_plain_int1 / plain_int_w, and a long grown out of i64 range deopts via the _fits_int guard. walker_unbox_long is the walker-native analogue of trace_unbox_long_with_resume. Assisted-by: Claude
…frame The j2 planning path spilled every guard fail arg that is dead on the fast path after the guard to a frame slot before the guard, so the guard's faillocs were frame slots. On a tight loop this is a store per iteration (e.g. the pre-add accumulator that feeds a GuardNoOverflow). The failure path already saves all managed GPRs/FPRs to their frame save-slots before rebuilding the frame, and both the faillocs encoder (append_guard_token_with_faillocs) and decoder (rebuild_faillocs_from_descr) handle register positions, so a fail arg captured from a register is fully recoverable. locs_for_fail now captures such values from their registers; the register frees naturally after the guard via longevity. Removes compute_deopt_spill_points / DeoptSpillPoint / deopt_spill_args_by_index and spill_j2_deopt_args. check.py dynasm 300/300. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/2d92251ee90cf09053adc7aa12fc125b267815d3/pyre-interpreter/src/objspace/descroperation.rs#L528-L529
When the left operand is a W_LongObject and the right operand is a machine int, this new branch demotes % to W_IntObject, but the PyPy long path’s _int_mod returns newlong just like _mod does, and this commit’s new divmod special case already preserves that long remainder shape. For plain %, the demotion makes later internal type/strategy/JIT checks see INT_TYPE for a value that should remain on the long path, so mixed long % int can diverge from the strict RPython/PyPy structure this port relies on.
ℹ️ 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".
1 participant
Perf-defect stack on top of #738's base. Six commits:
6a065ec6419—JitCellToken::invalidatenow sets the attached bridge flags (review follow-up from jit: mint a fresh GUARD_NOT_INVALIDATED flag per compiled bridge #738).68c47f8eb84— int binop specialization bails when concrete operands overflow instead of recording a wrapping concrete (superseded by the arm below, kept as history).492966d0a53— codewriter plants leading-live-markers at LoadAttr/StoreAttr PCs. Previously the FOR_ITER-body marker gate skipped attr PCs, so resume-marker rounding hit the previous marker and forced a permanent FBW decline.mapdict_attr_loop: 25002 guard failures → 203, trace compiles.26df830cf81— dynasm (aarch64 + x86) emittedGuardOverflowthrough theGuardNoOverflowarm, i.e. with the success CC un-inverted. Latent until now (nothing emitted GuardOverflow on dynasm); upstream aliases GUARD_NO_OVERFLOW→guard_true / GUARD_OVERFLOW→guard_false. cranelift and wasm were already correct.03dd08f5696— paused inline-caller frames in resume snapshots encoded the semantic-fallthrough marker instead of the immediate post-call-live-marker.BlackholeInterpreter::call_result_reg()walksbytecode[pc-1]backward to find the call's result register, so the wrong marker made it read an unrelated operand byte and clobber the caller's r0 with the callee result → SIGSEGV oninline_bignum_bridge_twoclamp(new bench). Pre-existing defect, unlocked by the overflow arm compiling bridges into inlined frames. Bonus:kept_stack_boxed_in_handler50068 guard failures → 402 with the exc-handler inline bridge now compiling.1050de69347— the overflow arm itself:try_walker_specialize_binary_op_intnow recordsint_{add,sub,mul}_ovf+guard_overflow+ an elidablecall_r(jit_bigint_*_int_int)on raw ints with inline boxing (ovf2long parity: pyjitpl.pyhandle_possible_overflow_error, intobject.py_make_ovf2long, rbigint.py 717/788/873), replacing the genericCallMayForceRleg (force-token store + operand boxing + 3 guards per iteration).int_mul_ovf_bignum_promote: 3.9x/4.8x → 1.9x dynasm / 0.7x cranelift; header tightenedmax-pypy-ratio30 → 8.Verification
pyre/check.py: dynasm 297/297, cranelift 297/297; wasm 293/294.synth/exception_metadata_hot— a pre-existing base-side regression bisected to jit: eliminate the guard-fail resume-decode backxlat inverse (jitcode-blackhole Slice 3') #727 (controla1de59bpasses, first bad01e2a08; this branch's commits exonerated by stack-swap A/B). Not addressed here.🤖 Generated with Claude Code