jit: #22 tagged-int Dir3 trace-boundary machinery + 4 gated deref guards (flag-false inert)#524
Conversation
…D, 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
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
WalkthroughTagged immediate integers are handled explicitly across object inspection, string conversion, tracing, frame initialization, compiled execution, and assembler constant handling. JIT paths now preserve consistent logical and heap-concrete integer representations while avoiding unsafe pointer dereferences and unnecessary tag guards. ChangesTagged integer handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Frame
participant CompiledEntry
participant JITTrace
participant Assembler
participant CompiledCode
Frame->>CompiledEntry: provide locals and constants
CompiledEntry->>Frame: replace tagged local integers with heap objects
CompiledEntry->>JITTrace: execute normalized frame
JITTrace->>Assembler: emit concrete integer and constant references
Assembler->>CompiledCode: run code with normalized representations
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)pyre/pyre-jit-trace/src/jitcode_dispatch.rsast-grep timed out on this file 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 440b08d). 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
|
![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/5584174af37448cd5755f8c50c819573e9f7aecb/pyre-interpreter/src/builtins.rs#L4802
When CAN_BE_TAGGED is enabled and callers supply encoding or errors, builtin_str takes the decoding branch before reaching this new guard; for a tagged-int obj, buffer_as_bytes_like(obj) falls through and the TypeError path reads (*obj).ob_type, dereferencing the odd immediate instead of reporting int found. str(1, "utf-8")/keyword variants can therefore crash under the tagged-int flag, so the tagged-int handling needs to run before the has_encoding || has_errors path or that path must synthesize the int type name safely.
https://github.com/youknowone/pyre/blob/5584174af37448cd5755f8c50c819573e9f7aecb/pyre-jit/src/jit/assembler.rs#L1937-L1939
With CAN_BE_TAGGED enabled, this creates a fresh heap W_IntObject every time the same tagged immediate passes through untag_const_ref_value, before add_const_r can deduplicate by raw pointer. If a jitcode bakes the same ConstRef(tag_int(n)) in multiple places, those constants become distinct heap addresses, so later ptr_eq/Python is can observe false even though the interpreter tagged representation is the same immediate value. Reuse a materialized box per tagged payload/jitcode before inserting into constants_r.
ℹ️ 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".
Codex parity-review disposition (team-lead)Ran the codex parity review against §1 Parity regressions (3) — all on the tagged-int machinery, all conditioned "when tagging enabled"; the committed slice ships flag-FALSE (dead code, byte-identical), so none affect the default.
Verified live on a flag-true build vs CPython 3.14, all matching: §2 Other mismatches (1) — codewriter always-Portal ( §3 Pre-existing (1) — single-frame inline collapse ( §4 Structural adaptations (4) — free-threading GC ( Net: no §1/§2 fix lands on the tagged-int files — 1 intentional design (reclassified), 2 refuted false-positives, 1 out-of-scope. The committed slice stands as-is (flag stays OFF). — commented by Claude |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-interpreter/src/builtins.rs (1)
4783-4807: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the tagged-int guard before the decode branch.
has_encoding || has_errorsstill reachesis_str(obj)andbuffer_as_bytes_like(obj)first, so a tagged immediate can be treated like a pointer before the new fast path runs. Put the tagged-int check right afterobjis resolved.🤖 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/builtins.rs` around lines 4783 - 4807, Move the tagged-integer guard using pyre_object::tagged_int::is_tagged_int immediately after obj is resolved, before the has_encoding || has_errors decode branch. Return the formatted untagged decimal string there, and remove the later duplicate guard while leaving normal decoding behavior unchanged.
🤖 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.
Outside diff comments:
In `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 4783-4807: Move the tagged-integer guard using
pyre_object::tagged_int::is_tagged_int immediately after obj is resolved, before
the has_encoding || has_errors decode branch. Return the formatted untagged
decimal string there, and remove the later duplicate guard while leaving normal
decoding behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c89fb80a-ea6a-4099-9224-a1f587111dbe
📒 Files selected for processing (9)
majit/majit-gc/src/collector.rsmajit/majit-ir/src/resoperation.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/assembler.rs
1 participant
Summary
Direction-3 tagged-int trace-boundary machinery plus tag-immediate guards at four object-deref boundaries. All changes are gated on
pyre_object::tagged_int::CAN_BE_TAGGED(defaultfalse) and are dead code at the flag's default, so flag-false trace emission and interpreter behavior are byte-identical to before.Groundwork toward enabling tagged small ints (
#22): representing smallintas an immediate(v<<1)|1value to eliminate int-boxing malloc. This branch closes the four flag-true SIGSEGV crashes that blocked the enablement flip. The flip itself (CAN_BE_TAGGED=true) is not in this branch.Commits
Dir3 tagged-int trace-boundary machinery — a hot-loop-carried tagged int enters the JIT in the flag-false heap representation (never carries a tagged pointer in a trace):
untag_tagged_frame_locals(eval.rs): convert a frame's tagged-immediate local slots to heapW_IntObjectbefore a compiled loop reads them.snapshot_for_tracingcopy so the recorded body matches the runtime-converted frame.wrapintclass-record,box_int_payloadheap-only,walker_unbox_int_typedtwo-arm split, bridge ec-color seed fix.Gated tag-immediate guards at 4 deref boundaries — each fixes one flag-true crash class:
get_actual_typeid: returnNonefor a tagged immediate before the header read (mirrorscan_move).builtin_str: stringify a tagged int to decimal before theob_typederefs (mirrorspy_str_wtf8/py_repr_obj).loop_inlines_abort_permanent_callee: skip a tagged immediate in the globals-dict and cell scans.untag_const_ref_value: re-realize a tagged immediate as a heapW_IntObject(w_int_new_unique) before it enters the GC-root-walked jitcodeconstants_rpool, at eachConstRefpool-bake site.Verification
check.py --backend dynasm,cranelift: cranelift 163/163, dynasm 162/163 — the one red is theraise_catchvs-pypy 1.5x timing gate, a boundary jitter flake (re-ran 3×: 1.5x/1.4x/1.0x), not a correctness regression. Slice is gated dead-code so the tree is byte-identical to base.CAN_BE_TAGGED=true, local only)check.py --backend dynasm,cranelift: 163/163 both backends. All four formerly-crashing tests (fannkuch, nested_for_varying_trip, polymorphic_slot_retype, str_fstring) pass; fannkuch still JITs (loops_compiled=5).Follow-ups (documented, for the eventual flip, not in this branch)
builtin_strtag-guard sits after theencoding/errorsblock that derefsob_type;str(tagged, encoding=)would still deref tagged at flip-true (no test reaches it). Move the guard earlier at flip time.Operand::ConstRef(tagged)at record time instead of a heap-box const — the assembler-boundary normalization covers it; the optimizer-side root is the single-root ideal for cleanup.🤖 Generated with Claude Code
Summary by CodeRabbit