Skip to content

jit: #22 enable tagged small-int representation (flip CAN_BE_TAGGED=true) + pre-flip correctness fixes#548

Open
youknowone wants to merge 13 commits into
mainfrom
miframe
Open

jit: #22 enable tagged small-int representation (flip CAN_BE_TAGGED=true) + pre-flip correctness fixes#548
youknowone wants to merge 13 commits into
mainfrom
miframe

Conversation

@youknowone

Copy link
Copy Markdown
Owner

Summary

Enables the tagged small-int representation (#22/#39): a small int is stored as an immediate (value << 1) | 1 pointer instead of a heap W_IntObject, eliminating the w_int_new malloc (the #2 interpreter warmup cost). Direction-B: the JIT never carries a tagged int — tagged values convert to a virtualizable heap box at trace boundaries, so optimized trace output stays byte-identical to the flag-false representation.

The final commit flips the tagged_int::CAN_BE_TAGGED master switch. GcConfig.taggedpointers (via build_gc) and the jit_fnaddr baked constant both read this const, so the collector skips tagged immediates and the JIT sees the enabled value automatically.

Commits (12, on top of origin/main)

Flip:

  • object: enable tagged small-int representation (CAN_BE_TAGGED=true) — the flip + 3 test updates that assumed the untagged representation.

Flip prerequisites (tagged-safety, flag-false inert / byte-identical):

  • jit: #22 route 28 tagged-reachable type-name derefs through type_name_of — tag-safe type_name_of primitive; routes raw (*(*obj).ob_type).name derefs that a tagged immediate can reach.
  • interp: precheck tagged int in exception_descr_str_wtf8 — the WTF-8 str() path was missing the tagged precheck its py_str sibling has (str(ValueError(5)) segfaulted at flag-true).
  • Earlier Align exceptiontable #22 gating commits (bool __bool__, iter() not-iterable, not-callable/not-iterable names).

Pre-flip correctness (unconditional; these crash flag-false too):

  • int: guard int_mod against i64::MIN % -1 overflow
  • list: clamp backwards slice assignment to an empty insertion
  • builtins: stack-check isinstance/issubclass native recursion
  • object: raise TypeError from object.__new__/__init__ on bad args
  • interp: guard recursive container repr with stack_check — deep dict/list/tuple repr now raises RecursionError instead of overflowing the C stack (fixes test_repr_deep in test_dict/list_tests).

Verification

  • cargo test -p pyre-object (unfiltered): 211 pass
  • check.py --backend dynasm,cranelift: dynasm 181/181 + cranelift 181/181 ALL PASSED
  • int-module --full diff: no new WORSENED (test_dict 27f→26f resolved by the repr guard)
  • GC stress ≥30×: 30/30, no crash
  • perf A/B (interleaved, drift-canceling): 7/7 neutral, no regression

Notes

Rebased onto origin/main; one conflict in object_descr_new (vs #526 abstract-class reject + #527 no-args TypeError) resolved by blending — our split_builtin_kwargs/excess-args structure plus the abstract reject in PyPy descr__new__ order (after the excess-args gate, before allocate). A codex parity review found no regressions introduced by these 12 commits.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@youknowone, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 50 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2c6d1e0a-8d28-4176-9384-86df28266ce3

📥 Commits

Reviewing files that changed from the base of the PR and between 048978b and e404e08.

📒 Files selected for processing (20)
  • majit/majit-backend-dynasm/src/x86/assembler.rs
  • majit/majit-backend-wasm/src/codegen.rs
  • pyre/pyre-interpreter/src/argument.rs
  • pyre/pyre-interpreter/src/baseobjspace.rs
  • pyre/pyre-interpreter/src/builtins.rs
  • pyre/pyre-interpreter/src/call.rs
  • pyre/pyre-interpreter/src/display.rs
  • pyre/pyre-interpreter/src/jit_fnaddr.rs
  • pyre/pyre-interpreter/src/module/_codecs/mod.rs
  • pyre/pyre-interpreter/src/module/math/interp_math.rs
  • pyre/pyre-interpreter/src/objspace/descroperation.rs
  • pyre/pyre-interpreter/src/opcode_ops.rs
  • pyre/pyre-interpreter/src/runtime_ops.rs
  • pyre/pyre-interpreter/src/type_methods.rs
  • pyre/pyre-interpreter/src/typedef.rs
  • pyre/pyre-jit/src/jit/assembler.rs
  • pyre/pyre-object/src/intobject.rs
  • pyre/pyre-object/src/listobject.rs
  • pyre/pyre-object/src/pyobject.rs
  • pyre/pyre-object/src/tagged_int.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch miframe

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 7edf34f).

Files in the reviewed diff
pyre/pyre-interpreter/src/baseobjspace.rs
pyre/pyre-interpreter/src/builtins.rs
pyre/pyre-interpreter/src/call.rs
pyre/pyre-interpreter/src/display.rs
pyre/pyre-interpreter/src/jit_fnaddr.rs
pyre/pyre-interpreter/src/module/_codecs/mod.rs
pyre/pyre-interpreter/src/module/math/interp_math.rs
pyre/pyre-interpreter/src/objspace/descroperation.rs
pyre/pyre-interpreter/src/opcode_ops.rs
pyre/pyre-interpreter/src/runtime_ops.rs
pyre/pyre-interpreter/src/type_methods.rs
pyre/pyre-interpreter/src/typedef.rs
pyre/pyre-object/src/intobject.rs
pyre/pyre-object/src/listobject.rs
pyre/pyre-object/src/pyobject.rs
pyre/pyre-object/src/tagged_int.rs

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

  • pyre/pyre-interpreter/src/typedef.rs:9537 ↔ pypy/objspace/std/objectobject.py:115: our object_slot_inherited unwraps staticmethod on both operands; PyPy’s _same_static_method unwraps only w_x. Thus __init__ = staticmethod(object.__init__) is incorrectly treated as inherited by the new object.__new__ checks, changing the PyPy error path/message.

3. Pre-existing mismatches (already present before this patch)

None.

4. Structural adaptations

  • pyre/pyre-object/src/tagged_int.rs:33 ↔ rpython/config/translationoption.py:185: Rust uses one compile-time CAN_BE_TAGGED constant, while RPython selects taggedpointers through translation configuration. The enabled representation and (value << 1) | 1 encoding match rpython/rtyper/lltypesystem/rtagged.py:144.

  • pyre/pyre-object/src/listobject.rs:1371 ↔ pypy/objspace/std/listobject.py:1746: Rust passes normalized start/end; PyPy passes start/step/slicelength. end.max(start) is the necessary representation conversion for PyPy’s non-negative slicelength invariant.

  • pyre/pyre-interpreter/src/baseobjspace.rs:578 ↔ rpython/rlib/rstack.py:42: explicit Rust stack checks protect native recursive tuple/union traversal, which does not receive RPython translator-inserted frame checks.

  • pyre/pyre-interpreter/src/display.rs:417 ↔ rpython/rlib/rstack.py:42: explicit Rust stack checking similarly protects native recursive repr container formatting.

In the generic `__bool__` tail of `bool_descr_new`, the TypeError path built
the returned type's name via `(*(*result).ob_type).name`. When `__bool__`
returns a tagged immediate the ob_type deref reads non-pointer bits. Add a
`CAN_BE_TAGGED`-gated arm that names a tagged immediate `"int"` without the
deref, matching the guard in `builtin_str`. Dead code at flag-false.

Assisted-by: Claude
The two "'{}' object is not iterable" TypeError builders in `iter`
(the `__iter__ = None` arm and the terminal fall-through) named the
type via `(*(*obj).ob_type).name`, a raw ob_type deref. A tagged
immediate passes every tag-safe type probe and falls through to these,
faulting on the deref. Route both through a `CAN_BE_TAGGED`-gated
`not_iterable_type_name` that names a tagged immediate `"int"` without
the deref. Dead code at flag-false.

Assisted-by: Claude
…es through it

The "must be X, not <name>" / "not iterable/subscriptable/unpackable"
TypeError builders named the offending type via a raw
`(*(*obj).ob_type).name`. A tagged immediate passes the tag-safe type
probes that precede these builders, then reaches the raw deref and reads
its non-pointer bits. Add `pyre_object::type_name_of`, a gated chokepoint
that names a tagged immediate `"int"` and otherwise keeps the raw
`ob_type.name` (not `r#type`, which would return the w_class subclass
name). Route the reachable sites through it: builtin_str encoding/errors/
decoding, checkattrname, sequence unpack builders, and the not-subscriptable
builder. Dead code at flag-false.

Assisted-by: Claude
…type_name_of

The CALL not-callable builder (runtime_ops.rs:425) and the GET_ITER
ensure_range_iter not-iterable fall-through (:1259) named the type via a
raw `(*(*obj).ob_type).name`, which a tagged immediate reaches after the
tag-safe probes reject it. Route both through `type_name_of`, matching the
other type-name builders. Dead code at flag-false.

Assisted-by: Claude
`va % vb` panics on i64::MIN % -1 (quotient 2**63 unrepresentable in i64),
the same case int_add/sub/mul already guard with checked arithmetic. Replace
with checked_rem, falling back to a BigInt mod_floor (result 0) on overflow.

int_floordiv is unaffected: its checked_div early-return at :418 catches the
sole overflow operand pair before the `%` at :422 is reached.

Assisted-by: Claude
w_list_setslice computed each strategy arm's window as start.min(len) ..
end.min(len) without raising the upper bound to the lower one. A backwards
slice (start > stop) then panicked in the object arm's Vec::splice (start >
end range) and silently truncated the tail in the int/float arms (e - s
underflowed usize). Clamp end up to start at the top of the function so a
backwards slice is a pure insertion at start, matching list_ass_slice's
`if (ihigh < ilow) ihigh = ilow`.

Assisted-by: Claude
isinstance()/issubclass() recurse in native Rust over nested tuple and
PEP-604 union classinfo without pushing a Python frame, so the frame-level
stack_check() chokepoints never fire and a deeply nested classinfo blows the
C stack (segfault/timeout) instead of raising RecursionError. Add
stack_check() at each head after unwrap_cell.

Assisted-by: Claude
object.__new__ asserted on an empty arg list (SIGABRT when called with no
type) and silently returned a typeless instance for a non-type first
argument; object.__init__ was a no-op that swallowed excess arguments.
Port the descr__new__/descr__init__ excess-args decision from
objectobject.py: no arg -> "not enough arguments"; non-type -> "X is not a
type object (...)"; excess args -> "takes exactly one argument" or "takes
no arguments" depending on whether the class overrides __new__/__init__,
unwrapping the __new__ staticmethod for the identity compare. Type names in
the messages come from r#type()+w_type_get_name (the Python type), not the
raw ob_type layout name.

Assisted-by: Claude
A static sweep of all 67 live `(*(*X).ob_type).name` derefs found 28 on
error/fallback paths that an arbitrary user value — and so a tagged int at
CAN_BE_TAGGED=true — can reach, beyond the crash-observed set the earlier
batch closed. Route each through pyre_object::type_name_of, which folds to
the same raw deref at flag-false (byte-identical) and returns "int" for a
tagged immediate. The remaining 39 derefs are narrowed by an is_int
early-return, sit on the None arm of an r#type match, or are the
type_name_of chokepoint itself, and stay raw.

Sites: baseobjspace setitem_slot/set___class__/delattr; typedef
set.__init__/dict.__repr__/MappingProxyType/__bases__ setter/member-descr
get+set+delete/int.to_bytes/bytes.replace/removeprefix/removesuffix/fromhex/
int.from_bytes/bytes.decode enc+errors/descr_get_dict; builtins float();
opcode_ops list_extend/dict_update/dict_merge; math try_get_double; _codecs
encode+decode; call not-callable; type_methods str.encode.

Assisted-by: Claude
py_repr/dict_repr recurse into each element in native Rust with no Python
frame push and only ReprGuard (pointer-cycle detection); nothing bounded
the depth, so a deeply nested structure overflows the C stack. Add
stack_check() at the py_repr recursion hub so deep dict/list/tuple repr
raises RecursionError, matching test_repr_deep in test_dict/list_tests.
Mirrors the insert_stack_check pass on the recursive isinstance/issubclass
path.

Assisted-by: Claude
str() of a single-arg exception routes through this WTF-8 path; a
tagged-int arg was read as a pointer via unwrap_cell / ob_type
(str(ValueError(5)) segfaulted). Precheck the tagged immediate before the
deref and return None to fall back to py_str, which formats the value.
py_str already has this precheck; the WTF-8 sibling was missing it.

Assisted-by: Claude
Flip the tagged_int::CAN_BE_TAGGED master switch. A small int is now
stored as an immediate (value << 1) | 1 pointer instead of a heap
W_IntObject, so w_int_new avoids the malloc for taggable values. The
GcConfig.taggedpointers wire (build_gc) and the jit_fnaddr baked constant
both read this const, so the collector skips tagged immediates and the JIT
sees the enabled value automatically.

Update the three tests that assume the untagged representation:
- tagged_int enabled_after_flip asserts the switch is on
- intobject identity tests gate on CAN_BE_TAGGED && fits_tagged, expecting
  value-identical immediates for taggable ints

Gate battery: cargo test -p pyre-object 211 pass; check.py dynasm 180/180
+ cranelift 180/180; int-module --full diff no new WORSENED; GC stress
30/30; perf A/B interleaved 7/7 neutral, no regression.

Assisted-by: Claude
…asm/x86/unit-test axes

The local flip gate only ran check.py --backend dynasm,cranelift on aarch64,
missing the wasm backend, cargo unit tests, and x86_64. Five regressions:

- pyre-object/tagged_int.rs: fits_tagged/tag_int/untag_int computed in fixed
  i64 while PyObjectRef is pointer-width, truncating on wasm32. Recompute in
  isize/usize (byte-identical on 64-bit, 31-bit signed payload on wasm32).
- pyre-interpreter argument.rs/builtins.rs/typedef.rs: three local type_name_of
  helpers raw-deref ob_type in the None arm; a tagged immediate reaches it when
  the type registry is absent (unit-test harness). Precheck is_tagged_int.
- majit-backend-dynasm x86 CallMallocNurseryVarsizeFrame: spilled its result
  through a RAX detour, but the inline-bump allocator's clobber set is ECX/EDX
  only, so a live value regalloc bound to RAX was destroyed. Spill directly
  from the result register to the regalloc slot.
- majit-backend-wasm CastPtrToInt: on wasm32 a Ref widens to i64 by zero-extend,
  so a tagged negative small int (v<<1)|1 loses its high half; the arithmetic
  IntRshift untag then reads wrong. Sign-extend the low 32 bits (i32_wrap +
  i64_extend_i32_s); a no-op for real heap pointers and 64-bit operands.
- pyre-jit assembler.rs assemble_raise_accepts_const_ref test: odd ConstRef(7)
  sentinel now reads as a tagged immediate post-flip; use an even sentinel.

Assisted-by: Claude
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant