Two loop-perf folds, and the wasm value-numbering defect the second one exposed - #1061
Conversation
|
Warning Review limit reached
Next review available in: 19 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (69)
WalkthroughThis PR updates JIT correctness paths for wasm local sizing, inline default-argument binding, list reference stores, and specialized tuple GC type registration. It adds parity tests for inline defaults and object-list append folding, and refreshes affected synthetic benchmark ChangesJIT correctness and validation
Estimated code review effort: 4 (Complex) | ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 898d431b76
ℹ️ 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".
| for a in op.getarglist().iter() { | ||
| widen(a.to_opref(), &mut max_var); | ||
| } |
There was a problem hiding this comment.
Preserve float types for operand-only locals
When the folded value this loop now admits is a Float that survives only as an operand/constant-pool seed, num_vars is widened for its raw id but collect_value_types still marks only input args and op results as F64, leaving this new local as I64. A later Float* op resolves the same OpRef via emit_resolve_f64 and emits local.get of an i64 local where wasm expects f64, so folded-float short-preamble traces are rejected/invalid instead of compiling correctly; the operand census needs to propagate the value type (or decline) along with increasing num_vars.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit a39b76c). 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
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dc774a838
ℹ️ 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".
| for a in op.getarglist().iter() { | ||
| widen(a.to_opref(), &mut max_var); | ||
| } |
There was a problem hiding this comment.
Include operand-only refs in wasm ref tracking
When this loop now admits non-constant operand ids that have no producer, only num_vars is widened. The Ref census used by RefHomes, write_barrier_base, and LABEL captures still marks only input args/op results, so a pool-seeded Ref used as a store value or live across a collecting call is treated as an i64 scalar: wasm can omit the write barrier/root slot and the GC may miss a young object or fail to forward it. The same operand walk needs to propagate Ref-ness, or decline, rather than only increasing max_var.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
… entries `Cls_ii` / `Cls_ff` / `Cls_oo` each carry their own `ob_type` and their typeids already sit in `SUBCLASS_RANGE_HIERARCHY`, but no `register_vtable_for_type` bound those PyTypes to those typeids. `subclass_range` therefore answered "unknown" for a specialised tuple, so `protect_speculative_field` rejected any pure field fold on a constant one and the optimizer raised `InvalidLoop` for the whole trace instead of declining the fold. Register the three vtables and add the matching entries to the alias census the registration is asserted against. Assisted-by: Claude
…ised defaults tuple or a keyword hole `w_tuple_new` routes every arity-2 tuple through `makespecialisedtuple2`, so a callee with exactly two defaulted parameters carries a `W_SpecialisedTupleObject_*` as `__defaults__`. `positional_defaults_for_inline` accepted the array-backed `wrappeditems` layout only, so such a call stayed a residual. Read `Cls_ii` through its unboxed `value0` / `value1` plus `wrapint` and `Cls_oo` through its object slots; `Cls_ff` has no walker float-field read to pair with `wrapfloat` and stays residual. `fbw_reorder_call_kw_args` separately required every parameter to be filled from a passed argument, so a keyword call that left one to its default stayed residual too. It now leaves a hole, and the default filling parameter `p` is picked as `defs_w[p - (co_argcount - len(defs_w))]` instead of taking the tail of `defs_w`, so a hole anywhere in the parameter list binds. `synth/calls_closures` runs at median 0.551 of its previous wall clock over 9 pairwise rounds, against a 0.96-1.05 self-A/B band; its `default_keyword_args` section goes 0.163s -> 0.018s of execution-only time, and measured on its own that loop runs at median 0.282. Assisted-by: Claude
… object-strategy appends fold `prepare_list_ref_store` wraps `list_write_barrier` in a `push_roots` bracket so the stored value survives the safepoint inside the barrier's ownership query. The bracket's zero-arg root-stack resolve has no registered fnaddr, so when the orthodox `w_list_append` descent reached it the call carried a `symbolic_fnaddr_for_path` hash and `try_execute_residual_call_via_executor` declined the whole sub-walk. Every Object-strategy append therefore fell back to the generic residual — in both the `lst.append(x)` method form and the LIST_APPEND comprehension form — while the Integer arm, which has no bracket, folded. Mark `prepare_list_ref_store` `dont_look_inside` and register it, and extend `is_list_write_barrier` to match the wrapper so the residual keeps its exemption from the FBW body-effect accounting. The Object arm now records `guard_value(strategy)` + `getfield(items)` + `arraylen_gc` + `setarrayitem_gc` + `setfield(length)` with the barrier as its one residual, in place of a `Method` allocation plus `CallMayForce`. `synth/list_ops` `list_obj_append_pop` exec 0.179s -> 0.112s (19.9x -> 11.1x pypy); the merged file's wall clock A/B median is 0.823 over 9 pairs against a 0.998 self band. The new capacity and strategy guards are visible in jit-stats: 16 patterns re-recorded for `bridges_compiled` / `guard_failures`, and `loops_compiled` is unchanged everywhere. Most deltas match on dynasm and cranelift; `pickle_terminal_raise_resume` does not — cranelift 664 -> 668 against dynasm 450 -> 482 — and its pre-fold values already differed between the two backends. New fixture `parity_tests/list_object_append_fold.py`. Assisted-by: Claude
…ld's residual call the uniform word ABI Two defects kept the object-strategy `list.append` fold from being correct on the wasm backend. `collect_guards_and_vars` raised `max_var` for input args, op results and `LABEL` args only, never for an ordinary op argument. A value the constants pool alone binds — no producing op, the case `unbound_pool_const_seeds` exists to seed in the prologue — therefore did not raise it. `next_value_pos` is that same count, and `majit_gc::rewrite::remove_ref_constants` numbers the `LoadFromGcTable` results it inserts from there upward, so the load took the pool-bound value's id. The two then shared one local, and because the value now had a producing op `unbound_pool_const_seeds` emitted no prologue store — its own `raw >= num_vars` guard had been skipping the value as well. The read at the earlier op preceded the store. Concretely a `NewArrayClear` length read the zero wasm initializes a local to, so the allocated `ItemsBlock` had capacity 0 while the trace set `length = 1` and stored `items[0]`. The next interpreted `append` saw `length != capacity`, skipped the grow and wrote past the block; the next allocation's type-id header then overwrote that word. `sre_wasm_min` answered 29818 for 30000 and `exception_traceback_lineno_chain` mismatched. A residual call target must carry the uniform word ABI: the backend lowers an `Int`/`Ref`-result residual to a `call_indirect` whose static type comes from the descr alone, and a raw `(*mut PyObject, *mut PyObject) -> *mut PyObject` is `(i32, i32) -> i32` on wasm32, which traps `indirect call type mismatch`. Spell `prepare_list_ref_store`'s signature with `*mut PyObject` so `emit_helper_call_target_fn` emits the `extern "C" fn(i64, i64) -> i64` trampoline, and register that trampoline instead of the raw fn. The registered paths are unchanged, so `is_list_write_barrier` and the path-keyed build-to-runtime re-pairing are unaffected. 14 wasm jit-stats patterns re-recorded — the append fold's deltas, a strict subset of the 16 the native backends record (`exception_catching_frame_tb_node` and `exception_traceback_lineno_chain` move on the natives only). check.py: dynasm 382/382, cranelift 382/382, wasm 378/378. Assisted-by: Claude
`W_DictObject.dstrategy` held a `&'static dyn DictStrategy`. Upstream's `dstrategy` (`dictmultiobject.py:325`) is a single instance pointer, and `W_ModuleDictObject.mstrategy` beside it already is one (`*mut ModuleDictStrategy`); the regular dict was the outlier. The deviation costs two things. A Rust `&dyn` carries the vtable in the *reference*, so the dict has no field holding the strategy's identity for a `guard_class` to read — which is how RPython pins the receiver of the virtual `get_strategy().getitem(..)` call it inlines, and therefore the shape any traced dict lookup needs. And the singletons are unit structs, so the reference's data word is the address of a zero-sized static, which Rust does not guarantee to differ between distinct strategies. The trait's own doc records the consequence — `strategy_kind()` exists "because pointer comparison on the `&'static dyn DictStrategy` slot is unreliable for ZST strategies" — and `W_ModuleDictObject::set_strategy` was performing exactly that unreliable comparison, casting both sides to `*const ()` to test for `OBJECT_DICT_STRATEGY`. Introduce `DictStrategyRef`, a `#[repr(C)]` holder for the trait object, and one `static` per singleton. `dstrategy` becomes `&'static DictStrategyRef`: one word, and distinct per strategy because the holders are not zero-sized. `DictStrategyRef` derefs to `dyn DictStrategy`, so every dispatch through the slot is unchanged. `set_strategy` and the two allocator entry points take the holder; `get_strategy` still hands out the trait object. The module dict's identity test becomes a `ptr::eq` on holders. `DictStrategy` cannot carry a `Sync` bound — `ModuleDictStrategy`'s `GlobalCache` holds `*mut PyObject` — so the holder asserts it instead; a module dict never uses one. cargo test --all --no-default-features --features dynasm: 7445 passed, 0 failed, 101 targets. Assisted-by: Claude
… unicode-strategy dict folds `OS_DICT_LOOKUP` and its optimizer arm `_optimize_call_dict_lookup` (`optimizeopt/heap.rs`) were ported but had no producer anywhere in `pyre/`: no recorder emitted a call carrying `OopSpecIndex::DictLookup`, so the arm was dead code and every `d[k]` residualized through the generic subscript path with `RandomEffects`. Add the recorder arm. `try_walker_specialize_subscr` now takes a dict leg when the receiver is a canonical `W_DictObject` on `StrategyKind::Unicode`, the key is an exact canonical `str`, and the concrete probe hits. It emits exact class/`w_class` guards on both operands, a `GuardValue` on the one-word `dstrategy` slot pinning `UNICODE_DICT_STRATEGY_REF`, an elidable `ll_strhash` call for the key digest, the `dict.lookup` call itself, `IntGe`/`GuardTrue` on the returned index, and a `GuardNonnull`-protected value read. The lookup call is `EF_CANNOT_RAISE`. What makes that honest is the strategy guard: `UnicodeDictStrategy::setitem` hands the dict to `OBJECT_DICT_STRATEGY_REF` the moment a non-exact-`str` key is stored, so while the guard holds every stored key is an exact `str` and the comparisons are WTF-8 byte equality. `w_dict_unicode_lookup_index` enforces rather than assumes this: it runs the probe inside the callback-free bracket, and a key pair the builtin ladder cannot decide breaks the probe and reports a miss, which side-exits to the generic residual. `extraeffect < 6` is what the recorded op diff shows: `ForceToken`, both vable/`py_pc` `SetfieldGc` stamps, `GuardNotForced` and `GuardNoException` are all gone from the compiled loop. Supporting pieces: * `W_DICT_DESCR_GROUP` gains `dstrategy_word` and `dstorage_lookup_ns`, appended after `keys_version` so the existing census index 0 is unmoved. Neither key is spelled as its Rust field name: `get_field_descr` caches on `(struct_key, field_name)` and the LLBC analyzer resolves field accesses by name, so a census key that collides with a real field would hand the analyzer whichever type won the LazyLock race. * `dict_lookup_entries_array_descr`, the second `extradescrs` member the optimizer arm requires. * `jit_dict_value_at`, the index-to-value read. * Both `pyre-object` helpers take `*mut PyObject` rather than `PyObjectRef`: `dont_look_inside` decides syntactically whether to emit the `__majit_call_target_*` word-ABI trampoline, and an alias path is not a raw-pointer token. `jit_fnaddr` registers the trampolines, not the raw functions, so the baked funcbox address matches the registered one on wasm. * `w_dict_unicode_lookup_index` holds the dict's own lock over the table read, like every other reader of `dstorage`. Measured, median of pairwise ratios over 6 alternating rounds: `same_key` 0.673, `two_keys` 0.697, `fresh_key` 0.916 (its cost is the str-slice allocation the fold does not touch). jit-stats for the folded loop: `loops_compiled=1 bridges_compiled=0 loops_aborted=0 guard_failures=1`. `_optimize_call_dict_lookup` does not remove the loop body's lookup: the peeled preamble and the body are optimized separately, so `cached_dict_reads` does not cross the label. Upstream behaves the same way. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4dc416084
ℹ️ 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".
| walker_emit_guard_with_snapshot(ctx, op_pc, OpCode::GuardTrue, &[nonneg])?; | ||
|
|
||
| let value = ctx.trace_ctx.call_ref_typed_with_effect( | ||
| crate::helpers::jit_dict_value_at as *const (), |
There was a problem hiding this comment.
Keep dict lookup and value read atomic
In a free-threaded run, another thread can mutate this exact-str dict after the emitted dict.lookup call has returned an entry index but before this separate jit_dict_value_at call reads that index. Both helpers acquire and drop the dict operation guard independently, so the saved index is no longer tied to the requested key and can read a different entry after an insert/delete or strategy switch; the folded d[key] path needs to return/read the value under the same dict operation boundary, or otherwise revalidate the key/version before using the index.
Useful? React with 👍 / 👎.
… the strategy id from the holder The `dict.lookup` fold emits the lookup and the value read as two residuals, and neither holds the dict's lock across the gap. `jit_dict_value_at` bounds-checked the index and nothing else, so a concurrent `del` between the two calls — which compacts the table, `IndexMap::shift_remove` closing the hole — left the index in range and naming a *different* live key. The answer was a wrong value, not a crash. `ll_dict_getitem_with_hash` has no such window: upstream runs the lookup and `d.entries[i].value` as one translated sequence. Read the value through `w_dict_unicode_value_at_checked`, which under one lock re-checks that the strategy still shares the `IndexMap<ObjectKey, _>` storage shape and that the entry at the index still holds the key the lookup was given — hash first, then the byte comparison under the callback-free bracket. Anything else answers null, and the caller's existing `GuardNonnull` side-exits to the generic residual. The re-validation also makes the optimizer's own CSE sound rather than merely bounded: `_optimize_call_dict_lookup` may delete a repeat of the same `(dict, key)` probe, so the index arriving at the value read can have been produced by an earlier iteration. `w_dict_strategy_id` was still deriving its stamp from the data half of `dstrategy.imp`. Every strategy singleton is a unit struct, so that word is the address of a zero-sized static and Rust does not guarantee two of them differ — two strategies could stamp the same id and a transition between them would be invisible to the iterators comparing it. Take the holder's own address: `DictStrategyRef` is not zero-sized and there is one per singleton. The unit test covers the case the bounds check cannot see — three keys, look up the second, delete the first, and read at the stale index, which is still in range and still holds a live entry. Removing the key re-validation fails it. Assisted-by: Claude
Rebasing onto origin/main conflicted on 44 `.jitstats` files, every one the same shape: the base had begun recording `fbw_rolled_back_with_effects` while this branch had moved `guard_failures`. The conflicts were resolved key-wise — the union, taking this branch's value where both sides named the same counter — but those values were measured on the *pre-rebase* base, so they were provisional. These are the measurements on the rebased tree. Every value here was confirmed against CI, which ran the same commit against the blended baselines and reported its own observations: * the 7 dynasm and 8 cranelift moves match the macos-latest job exactly (3610, 606, 817, 607, 1670, 1202, and 463/656 for the two backends); * the 10 wasm moves match the ubuntu-24.04 job exactly, including every `bridges_compiled` — that is the only CI job that runs the wasm backend. `pickle_ctor_args.cranelift` is the one value CI does not corroborate, because the ratio gate returns before `_apply_snapshot_gate` and that bench fails the ratio gate on CI — on origin/main too, at 44.8x and 62.3x against a 36x ceiling, so its counters are never reached there. 436 -> 201 is what reproduces here across two independently built binaries. No badness counter moved anywhere in this set: `loops_aborted`, `internal_compile_panics`, the three `descr_set_*` and `fbw_rolled_back_with_effects` are unchanged. With these recorded, `check.py --backend dynasm` and `--backend cranelift` are 386/386 locally. Assisted-by: Claude
…_hot_callee_tb_node_once The fixture arrived with dynasm and cranelift baselines and no wasm one, so the wasm gate reported "no committed jit-stats baseline" rather than comparing anything. That is a failure by construction — check.py treats an absent baseline as a way for the gate to be silently disarmed, not as a pass — and origin/main is red on it at the same sha for the same reason. Unlike the rest of this branch's baselines, this value is not corroborated by CI: CI only reports that the file is missing, and its log does not print the raw counters. What supports it is that every other wasm baseline recorded on this host matched the ubuntu-24.04 job's observation exactly, all ten of them including each `bridges_compiled`. Kept as its own commit: it closes a gap that predates this branch, so it can be taken or dropped independently of the fold work. Assisted-by: Claude
…y flag is the same
The doc claimed an `IndexMap` "keeps no free-slot bookkeeping, so every flag
answers the same index". That reads as a general `ll_dict_lookup` port and it
is not one. On a miss `FLAG_STORE` answers the free slot the caller is to write
into and `FLAG_DELETE` walks to the tombstone — both read exactly the free-slot
bookkeeping that is absent here — while this helper answers -1 for every miss.
A store or delete lookup would be wrong, not conservative.
Nothing emits either: the recorder only ever passes 0, and the optimizer
requires `arg(4)` to be a constant in {0,1}, which is why the parameter exists
at all. Name it and pin it with a debug assertion, so a future producer that
passes 1 stops here instead of silently reading -1 as "absent".
`debug_assert` is compiled out of the release profile, which does not enable
debug assertions, so the shipped code is unchanged.
Assisted-by: Claude
15 synthetic fixtures move on all three backends: the arity-2 BUILD_TUPLE virtualization composes with the walker setfield_gc store and the FOR_ITER RETURN_VALUE admission from #1068 and the loop-perf folds from #1061, so the sre, exception-traceback and comprehension traces take fewer side exits — `nested_list_comprehension_hot` drops from 6 bridges / 1202 guard failures to 2 / 401, `sre_wasm_min` from 8 / 1849 to 5 / 1161. The 30 macro baselines only gain `field_pos_attached_misplaced` and `field_pos_spec_misplaced` at 0, the counters #1053 added to the binary without recording them here. Assisted-by: Claude
… baseline `bench: re-record five jit-stats baselines` wrote guard_failures=1838 against the pre-rebase base. #1061 had moved the same counter to 2037, and the rebase kept the branch side. Local wasm reads 2037, and main's ubuntu job passes this bench against its committed 2037, so the branch value was the stale one. Assisted-by: Claude
15 synthetic fixtures move on all three backends: the arity-2 BUILD_TUPLE virtualization composes with the walker setfield_gc store and the FOR_ITER RETURN_VALUE admission from #1068 and the loop-perf folds from #1061, so the sre, exception-traceback and comprehension traces take fewer side exits — `nested_list_comprehension_hot` drops from 6 bridges / 1202 guard failures to 2 / 401, `sre_wasm_min` from 8 / 1849 to 5 / 1161. The 30 macro baselines only gain `field_pos_attached_misplaced` and `field_pos_spec_misplaced` at 0, the counters #1053 added to the binary without recording them here. Assisted-by: Claude
15 synthetic fixtures move on all three backends: the arity-2 BUILD_TUPLE virtualization composes with the walker setfield_gc store and the FOR_ITER RETURN_VALUE admission from #1068 and the loop-perf folds from #1061, so the sre, exception-traceback and comprehension traces take fewer side exits — `nested_list_comprehension_hot` drops from 6 bridges / 1202 guard failures to 2 / 401, `sre_wasm_min` from 8 / 1849 to 5 / 1161. The 30 macro baselines only gain `field_pos_attached_misplaced` and `field_pos_spec_misplaced` at 0, the counters #1053 added to the binary without recording them here. Assisted-by: Claude
… baseline `bench: re-record five jit-stats baselines` wrote guard_failures=1838 against the pre-rebase base. #1061 had moved the same counter to 2037, and the rebase kept the branch side. Local wasm reads 2037, and main's ubuntu job passes this bench against its committed 2037, so the branch value was the stale one. Assisted-by: Claude
15 synthetic fixtures move on all three backends: the arity-2 BUILD_TUPLE virtualization composes with the walker setfield_gc store and the FOR_ITER RETURN_VALUE admission from #1068 and the loop-perf folds from #1061, so the sre, exception-traceback and comprehension traces take fewer side exits — `nested_list_comprehension_hot` drops from 6 bridges / 1202 guard failures to 2 / 401, `sre_wasm_min` from 8 / 1849 to 5 / 1161. The 30 macro baselines only gain `field_pos_attached_misplaced` and `field_pos_spec_misplaced` at 0, the counters #1053 added to the binary without recording them here. Assisted-by: Claude
… baseline `bench: re-record five jit-stats baselines` wrote guard_failures=1838 against the pre-rebase base. #1061 had moved the same counter to 2037, and the rebase kept the branch side. Local wasm reads 2037, and main's ubuntu job passes this bench against its committed 2037, so the branch value was the stale one. Assisted-by: Claude
15 synthetic fixtures move on all three backends: the arity-2 BUILD_TUPLE virtualization composes with the walker setfield_gc store and the FOR_ITER RETURN_VALUE admission from #1068 and the loop-perf folds from #1061, so the sre, exception-traceback and comprehension traces take fewer side exits — `nested_list_comprehension_hot` drops from 6 bridges / 1202 guard failures to 2 / 401, `sre_wasm_min` from 8 / 1849 to 5 / 1161. The 30 macro baselines only gain `field_pos_attached_misplaced` and `field_pos_spec_misplaced` at 0, the counters #1053 added to the binary without recording them here. Assisted-by: Claude
15 synthetic fixtures move on all three backends: the arity-2 BUILD_TUPLE virtualization composes with the walker setfield_gc store and the FOR_ITER RETURN_VALUE admission from #1068 and the loop-perf folds from #1061, so the sre, exception-traceback and comprehension traces take fewer side exits — `nested_list_comprehension_hot` drops from 6 bridges / 1202 guard failures to 2 / 401, `sre_wasm_min` from 8 / 1849 to 5 / 1161. The 30 macro baselines only gain `field_pos_attached_misplaced` and `field_pos_spec_misplaced` at 0, the counters #1053 added to the binary without recording them here. Assisted-by: Claude
15 synthetic fixtures move on all three backends: the arity-2 BUILD_TUPLE virtualization composes with the walker setfield_gc store and the FOR_ITER RETURN_VALUE admission from #1068 and the loop-perf folds from #1061, so the sre, exception-traceback and comprehension traces take fewer side exits — `nested_list_comprehension_hot` drops from 6 bridges / 1202 guard failures to 2 / 401, `sre_wasm_min` from 8 / 1849 to 5 / 1161. The 30 macro baselines only gain `field_pos_attached_misplaced` and `field_pos_spec_misplaced` at 0, the counters #1053 added to the binary without recording them here. Assisted-by: Claude
15 synthetic fixtures move on all three backends: the arity-2 BUILD_TUPLE virtualization composes with the walker setfield_gc store and the FOR_ITER RETURN_VALUE admission from #1068 and the loop-perf folds from #1061, so the sre, exception-traceback and comprehension traces take fewer side exits — `nested_list_comprehension_hot` drops from 6 bridges / 1202 guard failures to 2 / 401, `sre_wasm_min` from 8 / 1849 to 5 / 1161. The 30 macro baselines only gain `field_pos_attached_misplaced` and `field_pos_spec_misplaced` at 0, the counters #1053 added to the binary without recording them here. Assisted-by: Claude
15 synthetic fixtures move on all three backends: the arity-2 BUILD_TUPLE virtualization composes with the walker setfield_gc store and the FOR_ITER RETURN_VALUE admission from #1068 and the loop-perf folds from #1061, so the sre, exception-traceback and comprehension traces take fewer side exits — `nested_list_comprehension_hot` drops from 6 bridges / 1202 guard failures to 2 / 401, `sre_wasm_min` from 8 / 1849 to 5 / 1161. The 30 macro baselines only gain `field_pos_attached_misplaced` and `field_pos_spec_misplaced` at 0, the counters #1053 added to the binary without recording them here. Assisted-by: Claude
…x argument-handling sites (#1063) * builtins: layout-checked object.__new__, buffer request kinds, and six argument-handling sites `object.__new__` now runs `check_user_subclass`, and `type` gets its own Layout typedef (`TYPE_TYPE`) instead of sharing `object`'s — that identity is what the check reads, so `object.__new__(int)` and `object.__new__(<metaclass>)` are refused. Buffer requests are split by kind: `bytes()` / `bytearray()` read their source with `BUF_FULL_RO` (a strided memoryview is copied out, not refused), while bytes-method operands — `replace`, `strip`, `join`, `translate`, the fill char — require the C-contiguity `BUF_SIMPLE` carries. `bytes.startswith` / `endswith` convert the operand before the `start > len(value)` early-out, so an empty window no longer hides a prefix of the wrong type. A supplied `None` is a value, not an omitted argument, for `bytes.center` / `ljust` / `rjust`'s fill char, `bytes.decode`'s encoding and errors, `bytearray.pop`'s index, and `memoryview.cast`'s shape. `builtin_str` spells the utf-8 default out where it previously passed `None` through. Surplus positional arguments are rejected by `str.replace`, `bytes.center` / `ljust` / `rjust`, `bytearray.remove` and `memoryview.cast`. An unset `__slots__` read reports `%T` — the bare type name — through `raiseattrerror`, matching the same miss taken through the descriptor's `__get__`. `type(1, (), {})` names argument 1 instead of reporting an arity error, and argument 1's message says `string` like its two siblings. `SyntaxError.__str__` splits its filename with `ntpath` rules on windows. Assisted-by: Claude * bytearray: compare against any BUF_SIMPLE exporter, not only bytes-like `cmp_guard_bytearray` admitted only bytes and bytearray, so `bytearray(b'ab') == array.array('B', [97, 98])` answered `False` and `bytearray(b'ab') < memoryview(b'b')` raised. `descr_eq` / `descr_ne` / `_comparison_helper` (bytearrayobject.py) hand a non-bytes-like operand to `space.acquire_py_buffer(w_other, space.BUF_SIMPLE)` and turn only the TypeError that raises into `NotImplemented`; a released view's ValueError and a strided view's BufferError propagate. The six dunders are now built from `bytearray_compare`, which keeps the by-layout `compare_slot` for the bytes-like arms and for any receiver the slot was not meant for, and reads the receiver's data after the acquisition since a `__buffer__` slot is app-level code. `bytes` keeps the narrow guard: its comparisons never acquire a buffer, which is what makes `b'ab' == array.array('B', [97, 98])` `False`. `ordering_satisfies` replaces the two spellings of the `_memcmp`-result mapping in `descroperation`. `pad_fillchar`'s doc records why `str.ljust` / `rjust` keep refusing a buffer fill char: `convert_arg_to_w_unicode` decodes one, but CPython refuses it for all three methods and pyre follows CPython there. Assisted-by: Claude * builtins: metaclass one-argument guard, declaring-type arity names, and nine argument-rejection sites `type_descr_new` accepted `Metaclass(x)` as the one-argument `type(x)` form and returned `x`'s type; `descr__new__` (typeobject.py:901-908) takes that path only when the metatype is `type` itself. Restore the guard and report the count through the two wordings upstream splits it into. `type.mro` carried no declared arity, so `int.mro(1)` computed the MRO and dropped the surplus argument. A builtin's arity and keyword errors named the receiver's own class. An instance receiver now names the class that declares the method (`MyList([1]).append()` reports `list.append()`); a type receiver keeps naming itself, which is what a builtin bound to a class reports. Argument rejections reworded at nine sites: - `issubclass()` arg 2 names the accepted kinds - `raise X from Y` distinguishes its cause check from `e.__cause__ = x` - `UnicodeTranslateError` / `UnicodeDecodeError` / `UnicodeEncodeError` `__init__` report the count they received - `BaseExceptionGroup.__new__` names itself - `int.to_bytes` / `int.from_bytes` name the total parameter count once the call passes every parameter, and the positional limit otherwise - `bytes.decode` renders a `None` argument as `None` - `str.center` / `ljust` / `rjust` name the type of a non-str fill char rather than reporting its length - `float.fromhex` rejects a non-str operand Assisted-by: Claude * typedef: restore the fully-qualified name in the empty-slot AttributeError `descr_member_get`'s miss reported `getfulltypename` before d8fc362 narrowed it to the bare `%T` name; `test.test_descr.test_slots` pins the `module.__qualname__` form and the cpython_tests runner drives that module through its dotted-identity driver, so the narrowing turned the gate red. The unit test's name and expectation go back with it. `test_bad_new` regains the `@support.impl_detail(cpython=False)` marker the 3.14.6 stdlib import replaced with CPython's `@unittest.expectedFailure`: the layout check added in fdcce06 makes the test pass here, and an unexpected success fails the module. Assisted-by: Claude * jit: virtualize BUILD_TUPLE at arity 2 and seed a `*args` callee's vararg local `try_walker_specialize_newtuple_object` no longer declines arity 2. The canonical array-backed `W_TupleObject` is the shape `subscr_tuple`, `builtin_len`, `get_iter` and the array-backed arm of `unpack` already read, whereas a `makespecialisedtuple2` pair has an UNPACK fold and nothing else, so every other read of one forced it out of virtual state. The `spec_ii` arm stays as the fallback for a pair whose backing-array length never reached the heap-cache as a constant. Measured over an empty loop: `(i, i + 1)[1]` 258.6ns -> 0.1ns, `f()[1]` for a pair-returning `f` 1247.7ns -> 34.4ns, `d[(a, b)]` 1207.6ns -> 313.1ns. `try_walker_inline_resolved_user_call` accepts a `*args` callee and writes `newtuple(starargs_w)` into `scope_w[co_argcount]` (`argument.py:222-234 _match_signature`) instead of leaving the call residual. `**kwargs` and keyword-only callees stay residual, as does a zero-surplus call (the empty tuple is a singleton) and a bound method whose callee has no positional parameter to hold the receiver — its `callee_args[0]` is still the placeholder the resolved half replaces with `GetfieldGcR(Method.w_self)`. 300k calls: `f(*args)` 0.480s -> 0.000s, `c.m(*args)` 0.514s -> 0.000s, `f(a=i)` into `**kw` 0.254s -> 0.142s. jit-stats: the trace-built pair and a runtime-built specialised one meeting at one code location costs a side exit, so three fixtures gain a bridge (`binary_int_overflow_local_resume`, `exc_bridge_entry_guard_not_removed`, `list_append_write_barrier_gc`); `getattribute_override_no_bind` compiles one loop instead of two now that its `*args` callee inlines, and `pickle_ctor_args` sheds half its cranelift guard failures. The wasm baseline missing for `exception_escape_hot_callee_tb_node_once` is recorded. Assisted-by: Claude * bench: re-record the jitstats baselines the rebase left disagreeing 15 synthetic fixtures move on all three backends: the arity-2 BUILD_TUPLE virtualization composes with the walker setfield_gc store and the FOR_ITER RETURN_VALUE admission from #1068 and the loop-perf folds from #1061, so the sre, exception-traceback and comprehension traces take fewer side exits — `nested_list_comprehension_hot` drops from 6 bridges / 1202 guard failures to 2 / 401, `sre_wasm_min` from 8 / 1849 to 5 / 1161. The 30 macro baselines only gain `field_pos_attached_misplaced` and `field_pos_spec_misplaced` at 0, the counters #1053 added to the binary without recording them here. Assisted-by: Claude * bench: mark the thirteen fixtures cpython cannot usefully run Each carries `# pyre-check: skip-cpython` followed by the measured cpython and pyre times, the way the directive requires. The directive itself is on the base; this only names the fixtures that claim it. Assisted-by: Claude * parity: cover the arity-2 specialised tuple consumers `specialised_pair_consumers.py` reads the `_ii` / `_ff` / `_oo` pair layouts through `len()`, subscription and unpacking, at a constant index, at an alternating index and off a nested pair, with accumulators that do not cancel a swapped or mis-represented slot. The specialisation folds themselves are already on the base. Assisted-by: Claude * objspace: compare same-class specialised tuple pairs on their raw slots `compare_slot`'s tuple arm walked both operands with `w_tuple_getitem`, which for a `W_SpecialisedTupleObject_ii` / `_ff` builds a fresh box per element because the payload is an inline machine word. `specialised_tuple_same_class_eq` reproduces `specialisedtupleobject.py:113-127 descr_eq`: when both operands are the same specialised class the value slots compare raw, with the float arm falling back to the bit pattern so the same NaN in both slots stays equal (`float2longlong` upstream) while `+0.0` / `-0.0` are caught by the value compare. `_oo` slots still go through `eq_w`. Eq/Ne only — ordering keeps the generic walk, as upstream does. A mixed pair (one specialised, one array-backed) falls through to the existing element walk. Measured `(1, 2) == (1, 2)` on two loop-invariant pairs: 252.9ns -> 151.1ns. The remainder is not the boxing: `_ff` barely moves and an arity-3 array-backed comparison is 29ns, so ~120ns of arity-2 comparison is upstream of this arm. Assisted-by: Claude * bench: re-record the synthetic jitstats baselines from a fresh LLBC The baselines committed in e3d151e were recorded against a stale `build/llbc`: a `pyre-jit-trace` / `pyre-interpreter` edit invalidates the extraction fingerprint, and the JIT reads the function bodies it inlines out of those artefacts, so trace shape — not just field offsets — depends on them. The recorded counters therefore did not reproduce on CI, which extracts its own. `pyre/check.py (ubuntu-24.04)` failed with 49 jit-stats regressions across 19 benches on all three backends with identical numbers. Re-extracted `pyre-object pyre-interpreter pyre-jit`, rebuilt dynasm, cranelift and wasm with no `LLBC STALE` warning, and re-recorded. A local run now reproduces the CI numbers exactly, e.g. `nested_list_comprehension_hot` bridges 2 -> 6 and guard_failures 401 -> 1202. 84 counter values change across 21 benches (51 guard_failures, 33 bridges_compiled). 19 are the arity-2 tuple fold's mixed-representation side exits, which the ca7351f message under-reported for the same stale-artefact reason. Two are improvements from the specialised-pair subscript fold: `divmod_long_int_pair` guard_failures 9 -> 7 (its pair result now folds) and `exception_oserror_fields` 202 -> 201. The remaining 2218 added lines are `field_pos_attached_misplaced` / `field_pos_spec_misplaced`, counters #1053 added to the binary without recording them. check.py --synthetic-only: dynasm 371/371, cranelift 371/371, wasm 370/370. Assisted-by: Claude * builtins: check the metatype is a type before naming it in type.__new__ `type_descr_new` reached `new_arity_message` with an unvalidated first argument, and that read it through the `W_TypeObject` layout: `type.__new__(42, 1)` segfaulted and `type.__new__('s', 1)` reported `s.__new__() takes exactly 3 arguments (1 given)`, naming the str's own bytes. `descr__new__` (typeobject.py:886-911) decides the arity first and then runs `_precheck_for_new` (typeobject.py:1001-1003), so the one-name form now refuses a non-type with `X is not a type object (%T)` and the no-name form names it through the `%N` operand spelling — `W_Root.getname` (baseobjspace.py:90-94), which answers `?` when `__name__` is absent. `type.__new__(42)` answered `<class 'int'>` and now raises. Also folds the two `pos.len() == 1` arms, which had become the same branch, and saturates the reported argument count in the three unicode error initialisers; those are installed as `wrapper_descriptor`s that reject a zero-argument call before the body runs, so the subtraction was not reachable. Assisted-by: Claude * baseobjspace: hand the buffer request kind's flags to a `__buffer__` exporter `buffer_bytes` passed a literal `0` to `w_memoryview_new_with_flags` on every path, so a Python `__buffer__` saw `PyBUF_SIMPLE` even when the caller was `full_ro_buffer_bytes`, whose request is `BUF_FULL_RO`. An exporter that branches on the request observed the wrong one: `bytes(x)` on a `__buffer__` that requires `PyBUF_FORMAT` raised `BufferError` where cpython returns the bytes. `require_contiguous: bool` becomes a `BufferRequest` naming the two requests, and both the contiguity rule and the exporter flags are derived from it. `BUF_FULL_RO` moves next to it from `interp_buffer`, which already spelled the same constant. Assisted-by: Claude * jit: decline the object-slot arm of the specialised-pair subscript fold `try_walker_specialize_subscr_specialised_pair` reaches `W_SpecialisedTupleObject_oo.value0` / `value1` through `walker_emit_specialised_pair_item`, which reads them with a `getfield_gc_r`. That read is wrong code on this path. `test.test_datetime` holds `self.lt = (array('q', ut), array('q', ut))` and reads `self.lt[dt.fold]`; with the fold in place the next call in that frame comes out one positional argument short, so `bisect.bisect_right(lt, timestamp)` raises `TypeError: bisect_right() missing 1 required positional argument: 'x'` and the module goes `PASS -> FAIL` on the CPython gate. Measured on the full module, 550 tests: `PYRE_NO_JIT=1` passes while the JIT fails one. Declining only the `Object` kind passes. `MAJIT_NO_BRIDGE=1` still fails, so the exit is the main trace's and not a compiled bridge; executing the residual for the object arm and recording its concrete result, dropping the `replace_box`, and emitting the index guard through `walker_emit_guard_with_snapshot` each leave it failing. What makes the object-slot read itself wrong is not yet known. The decline sits in the subscript entry point rather than in `walker_emit_specialised_pair_item`, because UNPACK reaches the same slots through that helper with no index operand and is sound. The `ii` and `ff` arms share the class guard and the pinned index and keep their fold — over an empty loop, `II[0]` 0.1ns and `II[i & 1]` 0.7ns against 169.3ns and 175.5ns with the whole fold declined. `len()` on a pair is untouched. `OO[0]` returns to the residual at 193.1ns from 35.9ns. Assisted-by: Claude * bench: restore the sre_wasm_min1 jit-stats baselines to what this tree measures The rebase carried this branch's earlier recording through without raising a conflict: bridges_compiled=4 and guard_failures=803. All three backends read 3 and 603 against the rebased tree, which is what main records. Assisted-by: Claude * bench: re-record getattribute_override_no_bind's wasm jit-stats baseline The rebase resolved this file to main's side, which reads loops_compiled=2 and guard_failures=2. The tree measures 1 and 1 on wasm, matching the dynasm and cranelift baselines for the same fixture. The re-record also picks up the five counters added to the snapshot field set. Assisted-by: Claude * jit: seed the vararg tuple into the inlined callee's concrete frame The symbolic frame is built from `param_boxes`, which spans `seeded_locals` and so carries the packed `*args` tuple; the concrete frame beside it was built from the first `nparams` entries only. That frame is published on the interpreter frame chain for the whole sub-walk, so a residual running inside an admitted `*args` callee read the vararg name as unbound: def g(a, *args): return 'args' in sys._getframe().f_locals called in a hot `while` loop answered False on 5 of 200000 iterations, where pypy answers True on all of them. `_match_signature` writes the vararg tuple into `scope_w` like any other local (argument.py:222-234). `callee_arg_concretes` already holds the tuple at index `nparams` and is declined unless its length is `seeded_locals`, so both bounds stay in range. Assisted-by: Claude * type_methods: word the fill-character refusal per padding method `center` converts with `space.utf8_w` and `ljust`/`rjust` with `convert_arg_to_w_unicode` (unicodeobject.py:1101, 175-184), and the two refuse in different words. Both arms carried one shared string that matched neither: "ab".center(6, 1) pypy: expected str, got int object "ab".ljust(6, b"x") pypy: Can't convert 'bytes' object to str implicitly pyre, both: The fill character must be a unicode character, not X `arg_type_name` renders the same names `%T` does for all eight types checked. `decode_object`, which turns a buffer operand into a fill char for `ljust`/`rjust`, is still not imported; the doc comment now states that as the remaining difference instead of as the reason for a shared message. Assisted-by: Claude * builtins: prebuild the default encoding `str` hands to bytes.decode `builtin_str` wrapped a fresh "utf-8" for every `str(b, errors=...)` call that omits the encoding. `w_str_new` is immortal, so each one stays allocated for the life of the process. `warn::PrebuiltText` is the existing cell for this shape; `bytes_method_decode` only reads the encoding through `str_utf8_w`. Assisted-by: Claude * type_methods: report a non-bytes fill operand the way decode_object does `convert_arg_to_w_unicode` declines only `bytes` itself; every other non-str operand reaches `decode_object`, which reports a failed conversion as "decoding to str: %S" over the buffer error (unicodeobject.py:175-184, 1727-1739). The `ljust`/`rjust` arm now says that, with `None` rendered unquoted where a type name is quoted: "ab".ljust(6, 1) decoding to str: a bytes-like object is required, not 'int' "ab".ljust(6, None) decoding to str: a bytes-like object is required, not None "ab".ljust(6, b"x") Can't convert 'bytes' object to str implicitly All eight cases checked now print what pypy prints, byte for byte. Assisted-by: Claude * type: precheck the metatype on the four-argument type.__new__ path `type_descr_new` finds `(name, bases, dict)` by scanning for a str, so a four-position call whose name is not a str falls past the scan. That branch took `pos[0]` as the metatype only when it already was a type and otherwise left it null, which sent `type.__new__(42, 1, (), {})` on to report argument 1. `descr__new__` runs `_precheck_for_new` once the count is settled and before `_check_new_args` (typeobject.py:899), so the branch calls `precheck_for_new` first: type.__new__(42, 1, (), {}) before TypeError: type() argument 1 must be string, not int after TypeError: X is not a type object (int) The five-argument `super()` shape and every call whose name is a str are taken by the scan above and do not reach this branch. Assisted-by: Claude * jit: decline the arity-2 BUILD_TUPLE virtualization `try_walker_specialize_newtuple_object` emitted a canonical `W_TupleObject` virtual at every arity, including 2. At that arity the interpreter calls `makespecialisedtuple2` (specialisedtupleobject.py:169-179) instead, so the virtual is the one shape the runtime never builds: `Cls_ii` / `Cls_ff` / `Cls_oo` hold `value0` / `value1` inline and carry no `wrappeditems` block. The trace is self-consistent on its own, but a side exit puts a real pair in front of a consumer the trace chose for the canonical layout, and `try_walker_specialize_subscr_specialised_pair` reads a field that is not there. A pair built inside the loop and subscripted at an alternating non-negative index reaches it: t = (i, BIG) item = t[i & 1] which segfaults, or returns whatever the stale pointer lands on — one run answered `TypeError: unsupported operand type(s) for &: 'type' and 'int'`. `extra_tests/parity_tests/subscr_specialised_pair_shapes.py` fails both ways. Building the pair outside the loop, or indexing it only at a negative index, does not reach it. Arity 2 now falls to `try_walker_specialize_newtuple`, which builds the specialised shape the runtime builds. Arity 1 and 3 up are unchanged. Assisted-by: Claude
Four commits: two loop-perf folds, the vtable registration one of them needs,
and the wasm-backend defect the second fold exposed.
defs_winline — exactly two defaults never inlinedA call that filled a parameter from
defs_wwas residual when the callee hadexactly two defaults.
w_tuple_newroutes every arity-2 tuple throughmakespecialisedtuple2, so such a__defaults__is aW_SpecialisedTupleObject_ii/ff/oo, never the array-backedW_TupleObject,and
positional_defaults_for_inlinetestedptr::eq(ob_type, &TUPLE_TYPE).One and three defaults are plain tuples, hence a non-monotone table:
Widening the type test alone aborts the trace with
InvalidLoop: protect_speculative_field: descr vtable has no range— typeids10/11/12 were in
SUBCLASS_RANGE_HIERARCHYbut noregister_vtable_for_typebound the three
SPECIALISED_TUPLE_*_TYPEstatics to them. Hence the firstcommit.
fbw_reorder_call_kw_argsseparately requirednargs + receiver == nparams, so a keyword call leaving a middle parameter toits default never reached the defaults code at all; it now emits
OpRef::NONEholes and indexes
defs_w[p - (co_argcount - len(defs_w))].synth/calls_closureswall clock A/B median 0.551;default_keyword_argsexec 0.163s -> 0.018s.Object-strategy
list.appendnever foldedprepare_list_ref_storebracketslist_write_barrierinpush_roots(), whosezero-arg root-stack resolve has no registered fnaddr. The orthodox
w_list_appenddescent therefore met asymbolic_fnaddr_for_pathhash anddeclined the whole sub-walk, so every Object-strategy append fell back to
the generic residual — both
lst.append(x)and the LIST_APPEND comprehensionform — while the Integer arm, which has no bracket, folded.
synth/list_opslist_obj_append_popexec 0.179s -> 0.112s, level withits int twin instead of 1.8x behind it.
The wasm defect that fold exposed
collect_guards_and_varsraisedmax_varfor input args, op results andLABELargs only — never for an ordinary op argument. A value the constantspool alone binds did not raise it, and since
next_value_posis that count,remove_ref_constantsnumbered an insertedLoadFromGcTablestraight onto thepool-bound value's id. They shared one local and the read preceded the store.
A
NewArrayClearlength then read the zero wasm initializes a local to, so theItemsBlockhad capacity 0 while the trace setlength = 1and storeditems[0]; the next interpretedappendsawlength != capacity, skipped thegrow and wrote past the block.
Separately,
prepare_list_ref_storehad to carry the uniform word ABI — thebackend derives a residual
call_indirect's static type from the descr alone,and a raw
(*mut PyObject, *mut PyObject) -> *mut PyObjectis(i32, i32) -> i32on wasm32.Gates
check.py dynasm 382/382, cranelift 382/382, wasm 378/378 on this base.
cargo test --all --no-default-features --features dynasm7440/0 over 101targets; the cranelift subset 3030/0;
majit-backend-wasm --test codegen_test -- --ignored3/0; parity 179/180 (type_members_python314fails on thecpython arm, pre-existing). The cargo and parity numbers were taken one base
earlier and are re-running now.
16 native jit-stats patterns move (
bridges_compiled/guard_failures; thefold's new capacity and strategy guards), and 14 on wasm — a strict subset.
loops_compiledis unchanged everywhere. Deltas are not uniform across thetwo native backends:
pickle_terminal_raise_resumemoves cranelift 664 -> 668against dynasm 450 -> 482, and its pre-fold values already differed.
— authored by Claude
Summary by CodeRabbit
Bug Fixes
Tests