majit, jit-trace: the Option sum shell, a neg_inner descent, and named-struct constructor lowering - #1518
Conversation
`derive_program_metadata` handed the non-overlapping `[tag@0 | payload@8]`
shell to `core::result::Result` only; `core::option::Option` took the
`enum_layout` arm and borrowed Rust's native layout. In the built image that
registered `Option<i64>.__discriminant` and `Option<i64>::Some.__pos_0` both
at offset 0 of an 8-byte parent, so the payload write clobbered the tag.
A descent into any body holding a `checked_neg` -- which
`emit_tagged_pair_aggregate` lowers to `__discriminant = ne(v, i64::MIN)`
plus `__pos_0 = neg(v)` -- then read `-v` as the tag and left the match on
its `otherwise` arm, surfacing as
`UnsupportedOpname { key: "unreachable/" }` and a generic trace abort.
`codewriter/assembler.rs` carries the blackhole half of the same shell and
gated it on the two `Result` variants by name; `Option::Some` joins them,
so both sides place the payload past the inherited tag word.
After the change the same two descrs read `__discriminant off=0` and
`Some.__pos_0 off=8` in a 16-byte parent.
Assisted-by: Claude
Splits `descroperation::neg` at its `__neg__` override probe, the way `invert` was split at its probe and bool slot, and adds `try_walker_orthodox_unary_negative` on the `invert` model: admit an exact builtin `int` or `long`, guard the class and the exact `w_class`, then sub-walk `neg_inner` instead of emitting the negation by hand. The probe is what a descent cannot enter -- its `needs_numeric_unaryop_dispatch` is `dont_look_inside` and is the second operation the body executes. `neg` has no bool slot, so the split leaves only the probe behind and `neg_inner` keeps the bool operand. Dispatched ahead of `unary_negative_int` rather than replacing it, so that fold's `consulted` count reads whether the descent took the site. On `unary_int_loop_carried` the descent is `consulted=2 fired=2` and the fold `consulted=0`; the emitted ops move from the fold's `IntSubOvf` + `GuardNoOverflow` to the body's own `IntNeg` + `IntNe` + branch, which is `checked_neg`'s decomposition. Trace length 63 -> 73 ops, wall clock unchanged at 0.11s over 20M iterations. `-0`, `+/-(1 << 62)` and the `INT_MIN` promotion agree with `PYRE_NO_JIT=1` and cpython 3.14. Requires the preceding Option shell commit: without it every attempt aborted a trace instead of firing. Assisted-by: Claude
`spec_folds!` now holds 75 rows and closes at `diag.rs:419`; `specialize.rs` is 17,062 lines. The `try_walker_specialize_*` definition count is unchanged at 69 -- `unary_negative_descent`'s function is a `try_walker_orthodox_*`, not a fold. Both files described every row as a hand-written specialization. Three are not: `subscr_tuple_descent`, `unary_invert_descent` and `unary_negative_descent` sub-walk `w_tuple_getitem`, `invert_inner` and `neg_inner`, and carry a row only so `PYRE_FBW_NO_SPECIALIZE` can suppress and A/B them the way it does a fold. Each figure re-derived with the command §3.8 publishes beside it: 75, 69, 484, 17,062. Assisted-by: Claude
…cted to The doc claimed the descended body's `checked_neg` covers the `INT_MIN` promotion without the fold's `guard_value` pin. A census over all 484 synthetic fixtures says otherwise: the descent fires on six fixtures, and `unary_negative_int` still fires on `unary_negative.py`'s `main_int_min`. `PYRE_FBW_DEBUG_ABORT=1` puts the decline at pc=18 with symbolic 8853347690726780139, which `symbolic_fnaddr_paths` resolves to `<synthetic-transparent-ctor pyre_object::pyobject::PyObject>` -- the promotion arm's `w_long_new_fresh_rbigint_handle`. It is an `OrthodoxSubWalkTraceUnsupported`, so the cut-back runs and the fold serves the operand. So the fold is not retired. The descent's coverage is every non-promoting exact `int` plus the exact `long`, which reached no fold before. Assisted-by: Claude
`front::mir` records on `CallTarget::SyntheticTransparentCtor` whether the aggregate constructor resolved to a `TypeDeclKind::Struct`, and `jtransform` rewrites such a constructor to `OpKind::New` when it takes no arguments and `bh_size_spec_from_callcontrol` answers with a spec that lists fields. A spec with no field rows is excluded. It is materialized by the bare `make_size_descr_with_type_and_vtable` mint rather than `simple_descr_group_from_bh_size`, so it never enters `gc_cache._cache_size`, `register_unresolved_struct_tids` does not walk it, and its allocation tid stays the structural hash truncated to u32 -- which the walker rejects as `UnregisteredNewGcType`. Measured on `bench/synth/unary_negative.py`: 5 such aborts without the field requirement, 0 with it. 306 transparent constructors are left unlowered, counted as the `synthetic-transparent-ctor` entries of the generation's `jit_metadata.json` `symbolic_fnaddr_paths`. The arm cleared 177 of them when that A/B was run (480 -> 303); those two numbers come from the tree before the rebase onto main and the comparison has not been re-run since. `PYRE_GC_TYPE_COUNT=1` reports 1046 frozen GC types, so `TypeRegistry::MAX_TYPES` goes 1024 -> 4096. Sixteen `SyntheticTransparentCtor` struct literals in tests now call `CallTarget::synthetic_transparent_ctor`, which is how the front end spells the same empty-`owner_path` shape. Assisted-by: Claude
Rebasing onto main moved every structural number in the section, so re-run the four recipes it quotes: 70 `try_walker_specialize_*` definitions (68 in `specialize.rs`), 9,992 body lines inside that file's 17,298, and 77 `SPEC_FOLD_ROWS` rows -- `spec_folds!` now closes at `diag.rs:421`, so the quoted `sed` range and the per-file `rg -c` example move with it. The corpus glob answers 487 non-recursively and 533 recursively, the same 46-file over-count as before. The group table's `n` column sums to 73 and is not re-derived here; say so next to it rather than apportioning the difference across the groups. Assisted-by: Claude
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
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: 42a87f4487
ℹ️ 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".
| // payload row, so it needs no reconstruction. | ||
| let result_variant = layout_owner.ends_with("result::Result::Ok") | ||
| || layout_owner.ends_with("result::Result::Err") | ||
| || layout_owner.ends_with("option::Option::Some") |
There was a problem hiding this comment.
When a non-core enum is declared under a path ending in option::Option::Some (for example, mycrate::option::Option::Some), this suffix check classifies it as the synthetic core Option shell even though front::mir only assigns that shell to core::option::Option. Since bh_size_spec_from_callcontrol is also used by field descriptor lookup, the user enum's payload is consequently described at offset 8 with an injected discriminant instead of using its registered native layout, so reads and writes can access the wrong bytes. Use the same fully anchored core-path predicate as option_variant_ctor_tag rather than an ends_with match.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 42a87f4). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
1 participant
Three slices on the descent campaign, rebased onto
main(2b9371ede41).1.
Optiongets the explicit sum shellResultalready hadderive_program_metadatagave the non-overlapping[tag@0 | payload@8]shellto
core::result::Resultonly. Read from the generation'sjit_metadata.json,every registered
core::option::Option<T>had__discriminantat offset 0and
T::Some.__pos_0at offset 0 in an 8-byte parent — no room for a secondfield, so the payload write clobbered the tag and the discriminant switch took
its
otherwisearm.Reach is any descent into a body holding an
emit_tagged_pair_aggregatecaller(
checked_neg,usize::try_from,i64::try_from(&RBigInt)). Both halves arefixed:
front::mir'sexplicit_sum_shelland the blackhole'sresult_variantpredicate in
assembler.rs.⛔ A correctness probe cannot see this. The descent aborts, the residual runs
the real interpreter body, and the answers stay right. It surfaces only as
UnsupportedOpname { key: "unreachable/" }— a genericABORT_BRIDGE— underPYRE_FBW_DEBUG_ABORT=1. Measured on theneg_innersplit below:consulted=10 fired=0with +10 aborts before, 0 after.2. Descend
neg_innerfor-xon an exact builtinintorlongnegsplits the wayinvertdid: its__neg__override probe isdont_look_insideand is the second operation the body executes, soneg_inneris that body past it. The emitted guards are what let the caller skip the probe —
boolcarries its own type, and anint/longsubclass keeps the builtinob_typebut retagsw_class, so both side-exit to the residual.Trace shape moves as intended: the hand fold's
IntSubOvfx12 +GuardNoOverflowx18 (63 ops) becomesIntNegx6 +IntNex6 + GuardTrue/False(73 ops) —
checked_neg's own decomposition.⛔
unary_negative_intis NOT retired. Censused over all 484 fixtures: thedescent fires on 6, the fold still fires on 1 (
unary_negative.py:main_int_min,the INT_MIN promotion). Wall clock is unchanged — 0.11s over 20M iterations in
both arms. The commit message and the fold's doc were corrected to say what was
measured rather than what was expected.
3. A niladic named-struct transparent constructor lowers to
Newfront::mirrecords onCallTarget::SyntheticTransparentCtorwhether theaggregate resolved to a
TypeDeclKind::Struct;jtransformrewrites it toOpKind::Newwhen it takes no arguments andbh_size_spec_from_callcontrolanswers with a spec that lists fields.
The field requirement is the whole correctness argument. A field-less spec is
materialized by the bare
make_size_descr_with_type_and_vtablemint instead ofsimple_descr_group_from_bh_size, so it never entersgc_cache._cache_size,register_unresolved_struct_tidsnever walks it, and its allocation tid staysthe structural hash truncated to u32 — which the walker rejects as
UnregisteredNewGcType, after the descent has already run. Registering itinstead is not the alternative: an empty field list carries no ref offsets, so
the shape a registration would publish says "no pointers" for a struct whose
size says otherwise. The payload-less sum variant already declines for this
reason.
Found by locating the reported
type_id 31113171as the unique truncation matchamong the image's 5639 descrs — index 70,
size=16 is_gc_managed=true all_fielddescrs=[]. Only 12 such descrs exist image-wide, andfrozenisunchanged with or without the requirement, which is the direct evidence that
field-less owners were never registered in the first place.
Measured on
bench/synth/unary_negative.py: 5UnregisteredNewGcTypeabortswithout the field requirement, 0 with it;
Traces aborted0 in both thedescent-on and descent-off arms.
TypeRegistry::MAX_TYPESgoes 1024 -> 4096 (PYRE_GC_TYPE_COUNT=1reports 1046frozen;
header::TYPE_ID_BITSis 32, so the constant is pre-allocated capacity,not an encoding limit).
⛔ This does not open the
negwall. The remaining decline resolves to<synthetic-transparent-ctor pyre_object::pyobject::PyObject>, andPyObjecthas zero registered field rows in the image — it is one of the 12 the gate
declines. Registering those fields is a separate slice.
Verification
pyre/check.py --backend dynasm: ALL PASSED 497/497, rc=0extra_tests/run.py --dynasm-only --gated-only(the set CI blocks on): 105/105extra_tests/run.py --dynasm-only(full): 353/358. The 5 are unrelated to thisbranch —
builtin_sliceandstdlib_warningsfail under CPython too,stdlib_sqlitehas no_sqlite3built,stdlib_syshits agetsizeofstub,stdlib_posixhits the known descriptorTypeError. Judged by cause, not byan A/B against a pre-change binary.
cargo test --release -p majit-translate --lib: 3293 passed, 0 failed. Thesehad not been compiling — 16
SyntheticTransparentCtorstruct literals in testspredated the new field; they now call
CallTarget::synthetic_transparent_ctor,which is how the front end spells the same empty-
owner_pathshape.cargo test --release -p majit-translate --test test_make_jitcodes_produces_graph_keyed_output: 2 passedcargo fmt --all -- --checkandscripts/check-majit-boundary.py: cleanNotes on the rebase
Nine of the sixteen replayed commits were dropped as already upstream (#1507's
squash merge). Three add/add conflicts all resolved to
main's side, which wasa strict superset in each case. One redundant commit was dropped outright:
mainhad independently landed a byte-identicalunop_wrapper_residual_path, and because the two insertions sat at differentpositions git merged both copies in with no conflict — a clean hunk, not a
marked one.
The rebase also silently invalidated every structural count in design.md §3.8;
those were re-derived with the recipes the section itself quotes (70 definitions
/ 68 in
specialize.rs, 9,992 body lines inside 17,298, 77 rows, corpus 487).The group table's
ncolumn still sums to 73 and is flagged as not re-derivedrather than having the difference apportioned across its groups.