jit: FBW walker per-walk TLS → explicit WalkContext/session ownership#509
Conversation
Replace the FBW_INLINE_CALLEE_CONSTS thread-local Vec-stack with an inline_callee_consts: Option<InlineCalleeConsts> field on WalkContext. The stack was only ever read at its innermost entry (fbw_current_inline_callee_consts), and the innermost entry is exactly the current sub-walk's WalkContext, so nesting is now carried by the call stack itself. Some(consts) at the three sub-walk constructions that previously pushed the TLS guard (try_walker_inline_user_call, drive_bridge_carrier_subwalk, drive_outer_frame_continuation); None at every other construction site, preserving the empty-stack behavior. The TLS static, the reader helper, and InlineCalleeConstsGuard are deleted. Guard-less nested walks can no longer observe the caller's innermost consts through TLS. Verified: check.py 161/161 on dynasm, cranelift and wasm; cargo test -p pyre-jit-trace (dynasm, release) 265 passed. Assisted-by: Claude
Replace FULL_BODY_SNAPSHOT_SYM, INLINE_SUBWALK_CAPTURE_BOUNDARY and
FBW_CARRIER_RESUME (each a TLS Cell with a save/restore RAII guard) with
a single Copy struct field WalkContext::fbw_mode (FbwWalkMode
{ snapshot_sym, inline_subwalk, carrier_resume }). These are
propagate-down walk modes: nested sub-walk contexts inherit the parent's
value via the same explicit-inheritance idiom as is_authoritative_executor.
Construction values are faithful to the former TLS ambient state at each
site: dispatch_via_miframe sets snapshot_sym; the bridge-carrier and
outer-continuation sub-walks set all three; try_walker_inline_user_call
sets inline_subwalk and inherits the rest; run_sub_jitcode_walk inherits
everything; the list-append canonical-body sub-walk (which held the
boundary guard) sets inline_subwalk around its run_sub_jitcode_walk call
inside its existing save/restore block; every other site is default.
Helpers without a WalkContext parameter (fbw_foriter_body_pc_from_op_pc,
branch_arm_resume_ref_liveness) now take the mode explicitly.
FBW_ABORT_IN_SUBWALK stays: it is a post-unwind out-channel read by the
trace driver after the sub-walk context is gone.
Verified: check.py 161/161 on dynasm, cranelift and wasm;
cargo test -p pyre-jit-trace (dynasm, release) 265 passed.
Assisted-by: Claude
Introduce WalkSession { framestack: Vec<InlineFrame>, abort_in_subwalk },
owned by the walk driver and threaded through WalkContext::session as
&RefCell — MetaInterp.framestack parity (pyjitpl.py:2475/2487, depth
scan :1390). Each InlineFrame carries the inlined callee's w_code and
its optional paused-caller snapshot, merging the two former stacks
(pushed at the same three sub-walk sites) into one entry; the two RAII
guards collapse into one unwind-safe InlineFrameGuard.
Absorbed thread-locals: FBW_INLINE_CODE_STACK (recursion-depth scan,
.last() callee reads, len), FBW_INLINE_PARENT_FRAMES (parent-chain
clone and the n_parents == n_callees valve, now filter-count vs len
over the merged framestack — same values), and FBW_ABORT_IN_SUBWALK
(now a session field; trace.rs reads it from the session it owns).
Sessions are created in run_perfn_walk, drive_bridge_carrier_walk and
drive_bridge_framestack_walk, and passed down through
dispatch_perfn_frame / dispatch_via_miframe /
drive_bridge_carrier_subwalk / drive_outer_frame_continuation;
dispatch_via_miframe_at_opcode_entry takes the session as a parameter.
Semantic fix: BranchGuardUnrestorableKeptStackPermanent now stamps
abort_in_subwalk at its raise point. The trace.rs branch-flush gate
previously read a TLS value only ever written by the abort-permanent
marker handler, i.e. a stale leftover from an unrelated earlier abort
on the same thread; the gate is now deterministic and declines exactly
when the abort fired inside an inline sub-walk.
Verified: check.py 161/161 on dynasm, cranelift and wasm;
cargo test -p pyre-jit-trace (dynasm, release) 265 passed.
Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit a53c05c). 1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
…rds (flag-false inert) (#524) * jit: #22 Dir3 tagged-int trace-boundary machinery (gated CAN_BE_TAGGED, flag-false inert) Direction-3 machinery so a hot-loop-carried tagged small int enters the JIT in the flag-false heap representation, all gated on `tagged_int::CAN_BE_TAGGED` (default false) so the flag-false trace emission is byte-identical: - eval.rs `untag_tagged_frame_locals`: convert a frame's tagged-immediate local slots to heap `W_IntObject` in place before a compiled loop reads them; called from `execute_assembler` and `try_function_entry_jit`. - state.rs `PyreSym` record-time seed: convert tagged locals (locals region only, `item_idx < nlocals`) to heap boxes on the `snapshot_for_tracing` copy so the recorded body matches the runtime-converted frame. - state.rs `wrapint`: record the JIT-made int box's class (`INT_TYPE`) so a later unbox takes the `GetfieldGc` arm that folds through `SetfieldGc` at loop-close instead of the `CastPtrToInt` tag-arith arm. - trace_opcode.rs `box_int_payload`: always box via heap `wrapint`; drop the in-trace tag-arith emitter (`IntAddOvf`/`IntOr`/`CastIntToPtr`). - jitcode_dispatch.rs `box_int_concrete`, `walker_unbox_int_typed` two-arm split, `walker_inputarg_is_converted_local` (reads `ctx.fbw_mode.snapshot_sym` after the #509 FBW-TLS removal). - trace.rs bridge seed: when `bridge_registers_r` names a genuine operand at a reserved EC color (free-regalloc reused the EC color for a live operand at a PC with no live EC read), seed the real operand instead of stranding the stale `ConstPtr(ec)`; the frame color keeps its unconditional #124 skip. Fixes the fib_recursive flag-true SIGSEGV where `fib(n-1)+fib(n-2)` consumed the EC pointer as the Add's int lhs. - resoperation.rs `OpRef::is_input_arg` helper. check.py dynasm 162/162, cranelift 162/162 at flag-false. Assisted-by: Claude * jit(tagged-int): gated tag-immediate guards at 4 deref boundaries (#22) All four guards are gated on `pyre_object::tagged_int::CAN_BE_TAGGED` (currently false) and are dead code at the flag's default, so flag-false trace output and interpreter behavior are byte-identical. - majit-gc collector get_actual_typeid: return None for a tagged immediate before the offset-0 header read, mirroring can_move's guard (gc/base.py:380 is_valid_gc_object). - interpreter builtin_str: stringify a tagged int to its decimal value before the is_str/unwrap_cell/ob_type derefs, mirroring py_str_wtf8 and py_repr_obj. - jit-trace loop_inlines_abort_permanent_callee: skip a tagged immediate in the globals-dict scan and the cell-contents scan before the ob_type / is_cell derefs. - jit assembler untag_const_ref_value: re-realize a tagged immediate as a distinct heap W_IntObject (w_int_new_unique) before it enters the jitcode constants_r pool, at each ConstRef pool-bake site. The pool is GC-root-walked (walk_jitcode_constants_refs), so it must hold only real gcrefs; the bake is byte-identical to the flag-false w_int_new path. Assisted-by: Claude
1 participant
Completes the TLS→explicit-ownership cleanup line started in #500: after this, the FBW walker keeps no per-walk state in thread-locals. Three slices, one commit each.
1.
51dc4de7edb— inline-callee consts ontoWalkContextFBW_INLINE_CALLEE_CONSTS(innermost-only TLS Vec-stack) becomesWalkContext.inline_callee_consts: Option<InlineCalleeConsts>—Someat exactly the three inlined-callee sub-walk constructions,Noneelsewhere. Kept separate fromCalleeLocalsShadowbecausedrive_outer_frame_continuationruns withcallee_shadow: Nonebut active consts. Guard-less nested walks can no longer observe the caller's consts through TLS.2.
5f1a88e8a25— walk-mode flags ontoWalkContextFULL_BODY_SNAPSHOT_SYM,INLINE_SUBWALK_CAPTURE_BOUNDARYandFBW_CARRIER_RESUME(TLS Cells with save/restore RAII guards) become one Copy fieldWalkContext.fbw_mode: FbwWalkMode { snapshot_sym, inline_subwalk, carrier_resume }, propagated parent→child by explicit inheritance (theis_authoritative_executoridiom). Includes the fourth, undocumented guard site (list-append canonical-body sub-walk) and the three walk-time reads #471 added intry_walker_inline_user_call, migrated to the field model during rebase.3.
744445afa4b— explicit walk-session framestackWalkSession { framestack: Vec<InlineFrame { w_code, parent: Option<InlineParentFrame> }>, abort_in_subwalk }, owned by the walk drivers in trace.rs and threaded asWalkContext.session: &RefCell<WalkSession>—MetaInterp.framestackparity (pyjitpl.py:2475/2487; depth scan :1390). AbsorbsFBW_INLINE_CODE_STACK+FBW_INLINE_PARENT_FRAMES(same three push sites → one entry, one unwind-safe guard; then_parents == n_calleesvalve becomes filter-count vs len over the merged stack) andFBW_ABORT_IN_SUBWALK(session field read by the driver that owns the walk).Semantic fix included:
BranchGuardUnrestorableKeptStackPermanentnow stampsabort_in_subwalkat its raise point. The branch-flush gate inrun_perfn_walkpreviously read a TLS value only ever written by the abort-permanent marker handler — i.e. a stale leftover from an unrelated earlier abort on the same thread. The gate is now deterministic: it declines exactly when the abort fired inside an inline sub-walk (callee-coordinateabort_jit_pc).Verification
pyre/check.py161/161 on dynasm, cranelift and wasm;cargo test -p pyre-jit-trace(dynasm, release) 265 passed.🤖 Generated with Claude Code