gh#346: ll_alloc_and_set list-allocation family + full newlist_clear compound jitcode opcode - #1065
Merged
Conversation
…to rbuilder Add `build_ll_arrayclear_helper_graph` and `build_ll_arrayfill_helper_graph`, each a free `fn(name, item_lltype) -> Result<PyGraph, TyperError>` over a bare `Ptr(GcArray(ITEM))` operand. ll_arrayclear: three-block element loop storing the null/zero element into each slot (`getarraysize` -> `int_lt` -> `setarrayitem` + `int_add`); the `ZERO` element is `None` for a `Ptr` item, `float(0.0)` for a float item, else `Int(0)`. No write barrier, matching the NULL-store rule. The `must_split_gc_address_space` `else` fast path (`itemoffsetof` / `cast_ptr_to_adr` / `sizeof` / `raw_memclear`) is not emitted, mirroring `build_ll_arraycopy_helper_graph` which lowers the sibling `rgc.ll_arraycopy` to the same bare element loop and defers its `raw_memcopy` address machinery. ll_arrayfill: emits the `we_are_translated()` translated arm (as `build_ll_mallocstr_helper_graph` resolves the same predicate) — one `gc_writebarrier(p)` before the loop, then `bare_setarrayitem(p, i, item)` per slot. Add two graph-shape unit tests asserting block count, op names/order, the loop structure, and the barrier placement for each helper. Assisted-by: Claude
Add `OpKind::NewArrayClear { length, item_ty, array_type_id }` to the rich
codewriter model and a `"newlist_clear"` arm in `_handle_list_call` that emits
it, mirroring `do_fixed_newlist_clear` (jtransform.py:1858-1863). The
`base.starts_with("newlist")` dispatch guard in `handle_builtin_call` already
routes the oopspec; the arm reads the `count` operand as the array length and
builds a `Ref`-item arraydescr.
Encode the new op in the assembler as `new_array_clear/id>r` (length reg +
arraydescr + ref result), reusing `arraydescrof(item_ty, array_type_id,
len_offset=Some(0))` — the same descr shape ArrayRead/ArrayWrite mint. Extend
the exhaustive OpKind matches: remap/clone, operand-var walkers (inline.rs,
result_exc.rs, jtransform remap_op), purity (not pure — a fresh allocation),
raise class (MemoryErrorOnly), ll_result_type (Ref), and the
flowspace-adapter post-rtyper diagnostic tables.
The oopspec-attach mechanism is the existing `CallControl::mark_oopspec(path,
"newlist_clear(count)")`, which writes `FuncEffects.oopspec` — the exact field
`get_oopspec` reads at `handle_builtin_call`. Minted rtyper flowspace graphs
never cross into `CallControl`, so a `LowLevelFunction.oopspec` field would not
reach the reader; `mark_oopspec` is the field the codewriter dispatch keys on.
Add two tests: a direct `_handle_list_call` lowering test and an end-to-end
rendezvous test that marks the oopspec on a throwaway call target and asserts
the transform emits `new_array_clear`.
Assisted-by: Claude
Replace the deferred `rtype_alloc_and_set` / alloc-family stubs in rlist.rs with the real helper-graph implementation (rlist.py:346-537). `rtype_alloc_and_set` dispatches on the concrete `hop.r_result` repr (ListRepr -> resized, FixedSizeListRepr -> fixed) and forwards to a repr-generic body that mints an `ll_alloc_and_set` / `ll_fixed_alloc_and_set` helper and gendirectcalls it with `(count, item)`; the `LIST` is folded into the helper identity. `build_alloc_and_set_family` mints the six sub-helpers in dependency order (newlist, rgc.ll_arrayclear, rgc.ll_arrayfill, setitem_fast, clear, nonnull, jit, nojit), converts each to a callee const via `functionptr_const`, and builds the dispatch graph — the rtype_builder_new orchestration shape. The six build_ll_alloc_and_set* graphs: - dispatch: `int_force_ge_zero(count)` then a two-arm `we_are_jitted` SpecTag branch to the jit / nojit sub-helpers. - jit: `_ll_zero_or_null(item)` branch to clear / nonnull. - nojit: `ll_newlist(count)`, zero/null branch; the zero arm emits `ll_arrayclear(items)` unless the element is a Ptr (already zeroed by ll_newlist), the nonzero arm emits `ll_arrayfill(items, item)`. - clear (`_ll_alloc_and_clear`): `ll_newlist(count)` -> `ll_arrayclear(items)`; `malloc_zero_filled` collapses the early return away. - nonnull: `ll_newlist(count)` then a runtime `while i < count: setitem_fast(l, i, item)` loop. `_ll_zero_or_null` is realized inline per element type: Char/UniChar cast to int + int_is_true, integer int_is_true, Float float_is_true, Ptr ptr_nonzero, each negated by bool_not. Items are read per layout (resized: getfield "items"; fixed: the receiver IS the array ptr). lib.rs attaches the `newlist_clear(count)` oopspec under the single-segment path `["_ll_alloc_and_clear"]` (the minted graph's name), so a JIT-path call lowers to `new_array_clear` instead of residualizing. Assisted-by: Claude
Add the "alloc_and_set" arm to translate_operation beside "newlist", routing to rlist::rtype_alloc_and_set (rtyper.py:534-535). Add a translate_operation dispatch test asserting the arm emits one ll_alloc_and_set direct_call and mints the dispatch helper graph. Assisted-by: Claude
Model the resizable-list compound allocation opimpl_newlist_clear
(pyjitpl.py:792-798) records: new(struct) + setfield(length) +
new_array_clear(items) + setfield(items). The variant carries the same
{length, item_ty, array_type_id} shape as NewArrayClear; the owner struct
"list" and its length/items fields are implicit.
Wire the variant into every exhaustive OpKind match: inline (remap,
variable refs, is_pure_op=false), result_exc operand vars, jtransform
remap, call op_can_raise (MemoryErrorOnly), legacy_annotator result type
(Ref), flowspace_adapter variant name + diag, assembler variant name +
opname ("newlist_clear"). The dedicated newlist_clear/idddd>r emit arm is
not added here; the variant falls through encode_op's generic path.
Assisted-by: Claude
…oopspecs alloc_and_set_sub_names named the clear sub-helper _ll_alloc_and_clear for both the Fixed and Resized layouts, so the (name, args, ret) helper cache and the mark_oopspec CallPath collided — the resized registration served or overwrote the fixed one. Rename the Fixed layout's clear helper to _ll_fixed_alloc_and_clear (matching its _ll_fixed_* siblings); Resized keeps _ll_alloc_and_clear. Register the newlist_clear(count) oopspec on both leaf paths so each minted clear helper lowers to new_array_clear instead of residualizing. Assisted-by: Claude
…mit arm encode_op's descriptor-less `other =>` default builds the key newlist_clear/i>r and get_opnum silently mints an unregistered dynamic opcode for it (setdefault parity) — no runtime handler is wired to that byte, so a NewListClear reaching the assembler before its dedicated newlist_clear/idddd>r emit arm exists would miscompile silently instead of failing. Add an explicit NewListClear arm to encode_op that panics, so the "emit arm before producer" ordering is enforced by the code. Correct the opname-map comment to cover both descriptor-bearing clear ops, and fix the round-trip test fixture's array_type_id to a plausible items array identity rather than the enclosing struct name. Assisted-by: Claude
…ld registry
The rtyper synthesises GcStruct("list", ("length", Signed), ("items",
Ptr(GcArray(ITEM)))) (translator/rtyper/rlist.rs:1112-1124). Because it is
not a Rust source type Charon extracts, it never appears in
program.struct_fields, so fielddescrof/compute_struct_size return None/0 for
owner "list" and an OpKind::New keyed on it trips the
bh_size_spec_from_callcontrol().unwrap_or_else(panic) at
codewriter/assembler.rs:1595.
Inject the {length, items} shape into program.struct_fields before the
set_struct_fields snapshot and the struct_layouts loop. Offsets accumulate
by field order via get_type_flag: i64 -> (Signed, 8), a leading-& items
spelling -> (Pointer, Ref, 8), giving length@0, items@8, struct size 16.
Assisted-by: Claude
get_type_flag classifies any leading-& spelling as (Pointer, Ref, word) before parsing the element, so &[i64] and &() are identical for the layout. Use &() so the inert element type does not read as committing the resized list to i64 elements — the real element type lives on the op's item_ty (the new_array_clear / newlist_clear arraydescr), never in this header layout, which is element-uniform. Assisted-by: Claude
Replace the fail-loud placeholder in encode_op with the real emit: length register, then four descriptors in the handler's read order — structdescr (the "list" header Size descr, vtable 0), lengthdescr and itemsdescr (Field descrs for "length"@0 / "items"@8 on owner "list"), arraydescr (from the op item_ty / array_type_id) — then the ref result. The key is newlist_clear/idddd>r, already registered (insns.rs) and wired to handler_newlist_clear (blackhole.rs), so get_opnum resolves the reserved byte instead of minting a dynamic one. The length/items fielddescrs are interned via the free fielddescrof helper, mirroring the FieldWrite arm; they resolve their offsets from the "list" layout registered in the field registry. Assisted-by: Claude
…ment type
The newlist_clear codewriter arm emitted a bare new_array_clear with a
hardcoded Ref(None) element for every result. For a resized list result
(Ptr(GcStruct("list", {length, items}))) that allocated an items array with
no struct header — later length/items reads hit absent offsets — and it
GC-traced non-pointer element slots as pointers.
Fork on op.result.concretetype().TO, mirroring _handle_list_call's
resizable = isinstance(LIST, GcStruct) split (jtransform.py:1762-1785): a
"list" struct pointee lowers to the compound NewListClear
(do_resizable_newlist_clear), an array pointee to NewArrayClear
(do_fixed_newlist_clear). Both recover item_ty from the element lltype via
getkind, never hardcoded. A pointee that is neither (a generic OBJECTPTR
from an untyped result) keeps the prior bare NewArrayClear, pending the
typed-result test rewrite.
Assisted-by: Claude
…ar while tracing The trace dispatcher panicked on BC_NEWLIST_CLEAR (only BC_NEW / BC_NEW_WITH_VTABLE had an alloc arm). Add the arm on the BC_NEW template, mirroring opimpl_newlist_clear (pyjitpl.py:792-798): one opcode both records four resops (New, SetfieldGc(length), NewArrayClear, SetfieldGc(items)) so the optimizer can virtualize the list header and its items block, and executes the two live allocations plus the two field stores so later steps of the same trace read live memory. The struct header and items block are allocated no-collect on the same discipline BC_NEW follows: a headerless descr goes to the no-collect nursery, a typed descr to non-moving old-gen, so the unrooted struct pointer held in the register bank across the items allocation cannot be stranded. The items block is zero-filled (the CLEAR) with its length word at len_offset, and the ref store into the header fires a write barrier. frame.rs read_newlist_clear decodes the idddd>r operand septuple (1B length reg + 4×2B descr + 1B dest) beside read_new / read_setfield_gc. jitcode/assembler.rs JitCodeBuilder::newlist_clear emits that byte layout for the dispatch test, which asserts the four-op sequence, the sbox→both setfields and abox→items dataflow, the resolved field offsets, and a live non-null header ref. Assisted-by: Claude
…c matchers to NewArrayClear The test that stamped a generic GcRef result asserted item_ty == Ref(None) under the name ..._lowers_to_new_array_clear, framing that as the fixed-list lowering. After the layout fork, a generic OBJECTPTR result is neither the "list" header struct nor an items array, so it takes the Fallback arm, not the fixed-array arm. Rename it ..._untyped_result_falls_back_to_ref_array and document that the input is unreachable in a real translation (the rtyper always stamps the list type) and that the recovered-shape arms are covered by the resized and fixed fork tests. The three oopspec-dispatch tests matched NewArrayClear | NewListClear permissively. Their GcRef results all take the Fallback arm, which only ever emits NewArrayClear, so drop the dead NewListClear alternative and assert NewArrayClear specifically. Assisted-by: Claude
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
✨ 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: 13d533c2fe
ℹ️ 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".
Comment on lines
+6616
to
+6619
| NewlistClearShape::Resized { | ||
| item_ty: element_value_type(&a.OF), | ||
| array_type_id: None, | ||
| } |
There was a problem hiding this comment.
When this handles a real rtyped newlist_clear for a resized list, the items-array lltype is discovered but its descriptor identity is dropped by returning array_type_id: None. The assembler’s no-identity path mints a BhDescr::Array with type_id/gc_type_id == 0; then BC_NEWLIST_CLEAR falls back to alloc_zeroed for the live items block instead of a GC-typed allocation. For a resizable list whose items are refs, the GC-managed list header’s items field can therefore point at memory with no GC header, so the next collection or element store can lose/corrupt references. Please carry/reconstruct the ARRAY identity/gc tid from the recovered lltype instead of discarding it.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.
Inline comments:
In `@majit/majit-metainterp/src/pyjitpl/dispatch.rs`:
- Around line 10717-10728: Extend the CLEAR allocation test beyond recorded ops
by validating the live memory referenced by sbox_val. Read the list header at
length_offset and items_offset and assert both stores contain the expected
length and items pointer, then inspect the allocated items block and assert it
is zero-filled. Preserve the existing struct-size and non-null-pointer
assertions.
In `@majit/majit-metainterp/src/pyjitpl/frame.rs`:
- Around line 343-350: Update the doc comment for read_newlist_clear to describe
the decoded operands as a six-tuple rather than a septuple, keeping the listed
names and canonical layout unchanged.
In `@majit/majit-translate/src/model.rs`:
- Around line 669-691: Update the NewArrayClear documentation to remove the
claim that item_ty is always Ref. Explain that newlist_clear_shape can recover
Int or Float for non-pointer arrays, while Ref applies to GC-pointer arrays, and
ensure the documented emitted result/key behavior remains accurate for all
supported item types.
In `@majit/majit-translate/src/translator/rtyper/lltypesystem/rbuilder.rs`:
- Around line 6878-6914: Extend the array-clear test coverage around
build_ll_arrayclear_helper_graph by verifying the ZERO operand of the
setarrayitem operation for both pointer and float element types. Assert that
pointer elements use a null ConstValue with the matching pointer type, while
Float elements use ConstValue::float(0.0) with LowLevelType::Float; retain the
existing Signed/default-arm coverage.
In `@majit/majit-translate/src/translator/rtyper/rlist.rs`:
- Around line 9318-9333: Eliminate duplicated clear-helper name literals by
defining a shared exported source of truth in rlist.rs, such as
CLEAR_HELPER_NAMES, and use it in alloc_and_set_sub_names for both ListLayout
variants. Update the lib.rs mark_oopspec registration loop to iterate over the
same constant, preserving the existing names and ensuring producer and consumer
registrations cannot drift.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 930a807a-c6b1-4c94-bcbe-0f835a541af9
📒 Files selected for processing (15)
majit/majit-metainterp/src/jitcode/assembler.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-metainterp/src/pyjitpl/frame.rsmajit/majit-translate/src/codewriter/assembler.rsmajit/majit-translate/src/codewriter/call.rsmajit/majit-translate/src/codewriter/jtransform.rsmajit/majit-translate/src/front/result_exc.rsmajit/majit-translate/src/inline.rsmajit/majit-translate/src/lib.rsmajit/majit-translate/src/model.rsmajit/majit-translate/src/translator/rtyper/flowspace_adapter.rsmajit/majit-translate/src/translator/rtyper/legacy_annotator.rsmajit/majit-translate/src/translator/rtyper/lltypesystem/rbuilder.rsmajit/majit-translate/src/translator/rtyper/rlist.rsmajit/majit-translate/src/translator/rtyper/rtyper.rs
Comment on lines
+10717
to
+10728
| // The New carries a SizeDescr sized like the list header. | ||
| let struct_size = recorder.ops()[0] | ||
| .getdescr() | ||
| .and_then(|d| d.as_size_descr().map(|s| s.size())); | ||
| assert_eq!(struct_size, Some(16)); | ||
| // The sbox result is bound to a live (non-null) list-header pointer. | ||
| let sbox_val = recorder.ops()[0].get_value(); | ||
| assert!( | ||
| matches!(sbox_val, Some(Value::Ref(r)) if r.as_usize() != 0), | ||
| "list header pointer must be a non-null ref, got {sbox_val:?}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the concrete side of the allocation, not only the recorded ops.
The arm has a dual discipline: it records four resops and it performs the live stores. The test covers only the recording half. It checks that the header pointer is non-null, but it does not check any written memory.
Three behaviors stay untested:
- The length store at
length_offset(arm Line 7883). - The items-pointer store at
items_offset(arm Line 7924). - The zero-fill of the items block, which is the
CLEARthe opcode is named for.
A regression that dropped any of the three still passes this test. The header pointer is already available from get_value(), so reading the two offsets back is cheap.
💚 Proposed concrete-side assertions
// The sbox result is bound to a live (non-null) list-header pointer.
let sbox_val = recorder.ops()[0].get_value();
+ let header = match sbox_val {
+ Some(Value::Ref(r)) if r.as_usize() != 0 => r.as_usize(),
+ other => panic!("list header pointer must be a non-null ref, got {other:?}"),
+ };
+ // The live stores this arm performs alongside the recording: the
+ // length word at offset 0 and the items pointer at offset 8.
+ let (stored_len, items_ptr) = unsafe {
+ (
+ *((header as *const u8).add(0) as *const i64),
+ *((header as *const u8).add(8) as *const i64),
+ )
+ };
+ assert_eq!(stored_len, 3, "length field must hold the list length");
+ assert_ne!(items_ptr, 0, "items field must hold the items-block pointer");
+ // The CLEAR half: every element of the items block reads back zero.
+ let items_off = recorder.ops()[2]
+ .getdescr()
+ .and_then(|d| d.as_array_descr().map(|a| a.basesize()))
+ .expect("NewArrayClear must carry an array descr");
+ for i in 0..3usize {
+ let elem = unsafe {
+ *((items_ptr as usize as *const u8).add(items_off + i * 8) as *const i64)
+ };
+ assert_eq!(elem, 0, "items[{i}] must be cleared");
+ }
- assert!(
- matches!(sbox_val, Some(Value::Ref(r)) if r.as_usize() != 0),
- "list header pointer must be a non-null ref, got {sbox_val:?}"
- );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // The New carries a SizeDescr sized like the list header. | |
| let struct_size = recorder.ops()[0] | |
| .getdescr() | |
| .and_then(|d| d.as_size_descr().map(|s| s.size())); | |
| assert_eq!(struct_size, Some(16)); | |
| // The sbox result is bound to a live (non-null) list-header pointer. | |
| let sbox_val = recorder.ops()[0].get_value(); | |
| assert!( | |
| matches!(sbox_val, Some(Value::Ref(r)) if r.as_usize() != 0), | |
| "list header pointer must be a non-null ref, got {sbox_val:?}" | |
| ); | |
| } | |
| // The New carries a SizeDescr sized like the list header. | |
| let struct_size = recorder.ops()[0] | |
| .getdescr() | |
| .and_then(|d| d.as_size_descr().map(|s| s.size())); | |
| assert_eq!(struct_size, Some(16)); | |
| // The sbox result is bound to a live (non-null) list-header pointer. | |
| let sbox_val = recorder.ops()[0].get_value(); | |
| let header = match sbox_val { | |
| Some(Value::Ref(r)) if r.as_usize() != 0 => r.as_usize(), | |
| other => panic!("list header pointer must be a non-null ref, got {other:?}"), | |
| }; | |
| // The live stores this arm performs alongside the recording: the | |
| // length word at offset 0 and the items pointer at offset 8. | |
| let (stored_len, items_ptr) = unsafe { | |
| ( | |
| *((header as *const u8).add(0) as *const i64), | |
| *((header as *const u8).add(8) as *const i64), | |
| ) | |
| }; | |
| assert_eq!(stored_len, 3, "length field must hold the list length"); | |
| assert_ne!(items_ptr, 0, "items field must hold the items-block pointer"); | |
| // The CLEAR half: every element of the items block reads back zero. | |
| let items_off = recorder.ops()[2] | |
| .getdescr() | |
| .and_then(|d| d.as_array_descr().map(|a| a.basesize())) | |
| .expect("NewArrayClear must carry an array descr"); | |
| for i in 0..3usize { | |
| let elem = unsafe { | |
| *((items_ptr as usize as *const u8).add(items_off + i * 8) as *const i64) | |
| }; | |
| assert_eq!(elem, 0, "items[{i}] must be cleared"); | |
| } |
🤖 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 `@majit/majit-metainterp/src/pyjitpl/dispatch.rs` around lines 10717 - 10728,
Extend the CLEAR allocation test beyond recorded ops by validating the live
memory referenced by sbox_val. Read the list header at length_offset and
items_offset and assert both stores contain the expected length and items
pointer, then inspect the allocated items block and assert it is zero-filled.
Preserve the existing struct-size and non-null-pointer assertions.
Comment on lines
+343
to
+350
| /// Decode a `newlist_clear/idddd>r` operand septuple, returning | ||
| /// `(length_reg, struct_descr_idx, length_descr_idx, items_descr_idx, | ||
| /// array_descr_idx, dest)`. Canonical layout: 1B length_reg + 2B | ||
| /// structdescr_idx + 2B lengthdescr_idx + 2B itemsdescr_idx + 2B | ||
| /// arraydescr_idx + 1B dest_reg — the exact read order | ||
| /// `handler_newlist_clear` (`blackhole.py:1173-1180`) and the | ||
| /// codewriter emit (`assembler.rs` NewListClear arm) agree on. | ||
| pub fn read_newlist_clear(&mut self) -> (usize, usize, usize, usize, usize, usize) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the operand count in the doc comment.
The doc says "operand septuple", but the function returns six values and the prose lists six names. The operand count is the decode contract for this instruction. Keep the count exact so a later change does not size the tuple from the wrong number.
📝 Proposed doc fix
- /// Decode a `newlist_clear/idddd>r` operand septuple, returning
+ /// Decode a `newlist_clear/idddd>r` operand sextuple, returning
/// `(length_reg, struct_descr_idx, length_descr_idx, items_descr_idx,
/// array_descr_idx, dest)`. Canonical layout: 1B length_reg + 2B📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Decode a `newlist_clear/idddd>r` operand septuple, returning | |
| /// `(length_reg, struct_descr_idx, length_descr_idx, items_descr_idx, | |
| /// array_descr_idx, dest)`. Canonical layout: 1B length_reg + 2B | |
| /// structdescr_idx + 2B lengthdescr_idx + 2B itemsdescr_idx + 2B | |
| /// arraydescr_idx + 1B dest_reg — the exact read order | |
| /// `handler_newlist_clear` (`blackhole.py:1173-1180`) and the | |
| /// codewriter emit (`assembler.rs` NewListClear arm) agree on. | |
| pub fn read_newlist_clear(&mut self) -> (usize, usize, usize, usize, usize, usize) { | |
| /// Decode a `newlist_clear/idddd>r` operand sextuple, returning | |
| /// `(length_reg, struct_descr_idx, length_descr_idx, items_descr_idx, | |
| /// array_descr_idx, dest)`. Canonical layout: 1B length_reg + 2B | |
| /// structdescr_idx + 2B lengthdescr_idx + 2B itemsdescr_idx + 2B | |
| /// arraydescr_idx + 1B dest_reg — the exact read order | |
| /// `handler_newlist_clear` (`blackhole.py:1173-1180`) and the | |
| /// codewriter emit (`assembler.rs` NewListClear arm) agree on. | |
| pub fn read_newlist_clear(&mut self) -> (usize, usize, usize, usize, usize, usize) { |
🤖 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 `@majit/majit-metainterp/src/pyjitpl/frame.rs` around lines 343 - 350, Update
the doc comment for read_newlist_clear to describe the decoded operands as a
six-tuple rather than a septuple, keeping the listed names and canonical layout
unchanged.
Comment on lines
+669
to
+691
| /// RPython `new_array_clear(v_length, arraydescr)` — the cleared | ||
| /// fixed-size array allocation `do_fixed_newlist_clear` emits | ||
| /// (`jtransform.py:1858-1863`, `corresponds to rtyper.rlist. | ||
| /// ll_alloc_and_clear: needs to clear the items`). Also the | ||
| /// allocation half `do_fixed_newlist` picks (`jtransform.py:1851-1855`) | ||
| /// when `ARRAY.OF` is a GC `Ptr` or `Struct` — the exact case that | ||
| /// distinguishes `new_array_clear` from the uninitialised `new_array`. | ||
| /// | ||
| /// `length` is the item count (`_get_initial_newlist_length`, | ||
| /// `jtransform.py:1835-1842`). `item_ty` / `array_type_id` resolve the | ||
| /// `arraydescr` at assembly exactly like `ArrayRead`/`ArrayWrite`; a | ||
| /// cleared list holds GC pointers (`Ptr(GcArray(OBJECTPTR))`), so | ||
| /// `item_ty` is `Ref`. The allocator zero-fills every slot | ||
| /// (`bhimpl_new_array_clear`, `blackhole.py:1311-1313`), so the result | ||
| /// register is always a fresh `Ref` ('r') and the emitted key is | ||
| /// `new_array_clear/id>r`. | ||
| NewArrayClear { | ||
| length: crate::flowspace::model::Variable, | ||
| item_ty: ValueType, | ||
| /// ARRAY identity for `cpu.arraydescrof(ARRAY)`, same role as | ||
| /// `ArrayRead::array_type_id`. | ||
| array_type_id: Option<String>, | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Update the item_ty doc for NewArrayClear; it no longer always resolves to Ref.
The doc says a cleared array's item_ty is always Ref because "a cleared list holds GC pointers." This PR's own newlist_clear_shape (in jtransform.rs) now recovers Int/Float element types for the Fixed shape of NewArrayClear from a non-pointer array's LowLevelType, confirmed by handle_list_call_newlist_clear_fixed_recovers_int_item_ty. The blanket claim is stale for that new producer. Clarify the doc so a future assembler implementer does not assume item_ty is always Ref for this opcode.
📝 Proposed doc fix
- /// `arraydescr` at assembly exactly like `ArrayRead`/`ArrayWrite`; a
- /// cleared list holds GC pointers (`Ptr(GcArray(OBJECTPTR))`), so
- /// `item_ty` is `Ref`. The allocator zero-fills every slot
+ /// `arraydescr` at assembly exactly like `ArrayRead`/`ArrayWrite`. A
+ /// cleared array of GC pointers (`Ptr(GcArray(OBJECTPTR))`) has
+ /// `item_ty` `Ref`; other producers (e.g. the `newlist_clear` oopspec
+ /// dispatch in `_handle_list_call`) populate `item_ty` with the
+ /// array's actual recovered element type (`Int`, `Float`, or `Ref`).
+ /// The allocator zero-fills every slot📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// RPython `new_array_clear(v_length, arraydescr)` — the cleared | |
| /// fixed-size array allocation `do_fixed_newlist_clear` emits | |
| /// (`jtransform.py:1858-1863`, `corresponds to rtyper.rlist. | |
| /// ll_alloc_and_clear: needs to clear the items`). Also the | |
| /// allocation half `do_fixed_newlist` picks (`jtransform.py:1851-1855`) | |
| /// when `ARRAY.OF` is a GC `Ptr` or `Struct` — the exact case that | |
| /// distinguishes `new_array_clear` from the uninitialised `new_array`. | |
| /// | |
| /// `length` is the item count (`_get_initial_newlist_length`, | |
| /// `jtransform.py:1835-1842`). `item_ty` / `array_type_id` resolve the | |
| /// `arraydescr` at assembly exactly like `ArrayRead`/`ArrayWrite`; a | |
| /// cleared list holds GC pointers (`Ptr(GcArray(OBJECTPTR))`), so | |
| /// `item_ty` is `Ref`. The allocator zero-fills every slot | |
| /// (`bhimpl_new_array_clear`, `blackhole.py:1311-1313`), so the result | |
| /// register is always a fresh `Ref` ('r') and the emitted key is | |
| /// `new_array_clear/id>r`. | |
| NewArrayClear { | |
| length: crate::flowspace::model::Variable, | |
| item_ty: ValueType, | |
| /// ARRAY identity for `cpu.arraydescrof(ARRAY)`, same role as | |
| /// `ArrayRead::array_type_id`. | |
| array_type_id: Option<String>, | |
| }, | |
| /// RPython `new_array_clear(v_length, arraydescr)` — the cleared | |
| /// fixed-size array allocation `do_fixed_newlist_clear` emits | |
| /// (`jtransform.py:1858-1863`, `corresponds to rtyper.rlist. | |
| /// ll_alloc_and_clear: needs to clear the items`). Also the | |
| /// allocation half `do_fixed_newlist` picks (`jtransform.py:1851-1855`) | |
| /// when `ARRAY.OF` is a GC `Ptr` or `Struct` — the exact case that | |
| /// distinguishes `new_array_clear` from the uninitialised `new_array`. | |
| /// | |
| /// `length` is the item count (`_get_initial_newlist_length`, | |
| /// `jtransform.py:1835-1842`). `item_ty` / `array_type_id` resolve the | |
| /// `arraydescr` at assembly exactly like `ArrayRead`/`ArrayWrite`. A | |
| /// cleared array of GC pointers (`Ptr(GcArray(OBJECTPTR))`) has | |
| /// `item_ty` `Ref`; other producers (e.g. the `newlist_clear` oopspec | |
| /// dispatch in `_handle_list_call`) populate `item_ty` with the | |
| /// array's actual recovered element type (`Int`, `Float`, or `Ref`). | |
| /// The allocator zero-fills every slot | |
| /// (`bhimpl_new_array_clear`, `blackhole.py:1311-1313`), so the result | |
| /// register is always a fresh `Ref` ('r') and the emitted key is | |
| /// `new_array_clear/id>r`. | |
| NewArrayClear { | |
| length: crate::flowspace::model::Variable, | |
| item_ty: ValueType, | |
| /// ARRAY identity for `cpu.arraydescrof(ARRAY)`, same role as | |
| /// `ArrayRead::array_type_id`. | |
| array_type_id: Option<String>, | |
| }, |
🤖 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 `@majit/majit-translate/src/model.rs` around lines 669 - 691, Update the
NewArrayClear documentation to remove the claim that item_ty is always Ref.
Explain that newlist_clear_shape can recover Int or Float for non-pointer
arrays, while Ref applies to GC-pointer arrays, and ensure the documented
emitted result/key behavior remains accurate for all supported item types.
Comment on lines
+6878
to
+6914
| #[test] | ||
| fn build_ll_arrayclear_zeroes_each_slot_in_a_bare_element_loop() { | ||
| use super::Hlvalue; | ||
| let helper = super::build_ll_arrayclear_helper_graph("ll_arrayclear", LowLevelType::Signed) | ||
| .expect("build_ll_arrayclear_helper_graph"); | ||
| assert_eq!(helper.func.name, "ll_arrayclear"); | ||
| let inner = helper.graph.borrow(); | ||
|
|
||
| // start: length = len(p); i = 0 (a single unconditional exit). | ||
| let startblock = inner.startblock.borrow(); | ||
| let start_ops: Vec<&str> = startblock | ||
| .operations | ||
| .iter() | ||
| .map(|o| o.opname.as_str()) | ||
| .collect(); | ||
| assert_eq!(start_ops, vec!["getarraysize"]); | ||
| assert_eq!(startblock.inputargs.len(), 1); // p | ||
| assert_eq!(startblock.exits.len(), 1); | ||
| drop(startblock); | ||
|
|
||
| let (count, all_ops) = walk_ops(&inner.startblock); | ||
| // start, cond, body, returnblock. | ||
| assert_eq!(count, 4); | ||
| let n = |name: &str| all_ops.iter().filter(|o| o.as_str() == name).count(); | ||
| assert_eq!(n("getarraysize"), 1); | ||
| assert_eq!(n("int_lt"), 1); // loop header | ||
| assert_eq!(n("setarrayitem"), 1); // p[i] = ZERO | ||
| assert_eq!(n("int_add"), 1); // i += 1 | ||
| assert_eq!(n("gc_writebarrier"), 0); // NULL store needs no barrier | ||
| assert_eq!(n("raw_memclear"), 0); // fast path deferred | ||
|
|
||
| // Void return. | ||
| let Hlvalue::Variable(ret) = &inner.returnblock.borrow().inputargs[0] else { | ||
| panic!("returnblock inputarg must be a Variable"); | ||
| }; | ||
| assert_eq!(ret.concretetype.borrow().clone(), Some(LowLevelType::Void)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover the non-default zero_value arms.
build_ll_arrayclear_helper_graph selects the ZERO element constant from three arms: a null for a Ptr item, 0.0 for the float family, and Int(0) otherwise. This test exercises only LowLevelType::Signed, which is the default arm.
The Ptr arm is the one that matters most. It stores a null of the item type into a GC-visible slot, and a wrong constant type there surfaces late. Extend the test to assert the stored constant for a Ptr element and a Float element.
💚 Proposed added coverage
#[test]
fn build_ll_arrayclear_zero_constant_matches_the_element_type() {
use super::Hlvalue;
let probe = |item: LowLevelType| -> (super::ConstValue, Option<LowLevelType>) {
let helper = super::build_ll_arrayclear_helper_graph("ll_arrayclear", item)
.expect("build_ll_arrayclear_helper_graph");
let inner = helper.graph.borrow();
let (_count, _ops) = walk_ops(&inner.startblock);
// The body block holds the single `setarrayitem`.
let mut found = None;
let mut seen = std::collections::HashSet::new();
let mut stack = vec![inner.startblock.clone()];
while let Some(b) = stack.pop() {
if !seen.insert(std::rc::Rc::as_ptr(&b) as usize) {
continue;
}
let bb = b.borrow();
for op in &bb.operations {
if op.opname == "setarrayitem" {
let Hlvalue::Constant(c) = &op.args[2] else {
panic!("ZERO operand must be a Constant");
};
found = Some((c.value.clone(), c.concretetype.clone()));
}
}
for link in &bb.exits {
if let Some(t) = link.borrow().target.clone() {
stack.push(t);
}
}
}
found.expect("body must store a ZERO element")
};
let ptr_item = super::array_ptr_lltype(&LowLevelType::Signed);
let (value, ty) = probe(ptr_item.clone());
assert_eq!(value, super::ConstValue::None, "Ptr item clears to null");
assert_eq!(ty, Some(ptr_item));
let (value, ty) = probe(LowLevelType::Float);
assert_eq!(value, super::ConstValue::float(0.0));
assert_eq!(ty, Some(LowLevelType::Float));
}🤖 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 `@majit/majit-translate/src/translator/rtyper/lltypesystem/rbuilder.rs` around
lines 6878 - 6914, Extend the array-clear test coverage around
build_ll_arrayclear_helper_graph by verifying the ZERO operand of the
setarrayitem operation for both pointer and float element types. Assert that
pointer elements use a null ConstValue with the matching pointer type, while
Float elements use ConstValue::float(0.0) with LowLevelType::Float; retain the
existing Signed/default-arm coverage.
Comment on lines
+9318
to
+9333
| /// The two layouts must name their clear sub-helper DISTINCTLY, otherwise | ||
| /// the `(name, args, ret)` helper cache and the `mark_oopspec` CallPath | ||
| /// registration collide: a resized `newlist_clear` registration would | ||
| /// overwrite (or serve) the fixed one. Fixed prefixes `_ll_fixed_*`, | ||
| /// resized stays unprefixed. | ||
| #[test] | ||
| fn alloc_and_set_sub_names_clear_helpers_are_per_layout_distinct() { | ||
| let (_, fixed_clear, _, _, _) = alloc_and_set_sub_names(ListLayout::Fixed); | ||
| let (_, resized_clear, _, _, _) = alloc_and_set_sub_names(ListLayout::Resized); | ||
| assert_ne!( | ||
| fixed_clear, resized_clear, | ||
| "fixed and resized clear helpers must be distinct names" | ||
| ); | ||
| assert_eq!(fixed_clear, "_ll_fixed_alloc_and_clear"); | ||
| assert_eq!(resized_clear, "_ll_alloc_and_clear"); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Pin the clear-helper names against the oopspec registration.
alloc_and_set_sub_names_clear_helpers_are_per_layout_distinct asserts the two spellings on the producer side only. The consumer side is lib.rs Lines 1848-1853, which calls mark_oopspec for the literal strings "_ll_alloc_and_clear" and "_ll_fixed_alloc_and_clear". Nothing ties the two lists together.
If either side is renamed, the newlist_clear(count) oopspec stops matching, the clear helper residualizes instead of lowering to new_array_clear, and no test fails. That is the defect class this PR fixes.
Add an assertion that walks the same source of truth both sites use. One option: export the clear name from alloc_and_set_sub_names and have the lib.rs loop iterate over it instead of repeating the literals, which removes the drift entirely rather than testing for it.
♻️ Proposed change to remove the duplicated literals
In rlist.rs, expose the per-layout clear name:
/// The `newlist_clear(count)` oopspec is keyed on these graph names by
/// `lib.rs`; exporting them keeps the registration and the mint in sync.
pub(crate) const CLEAR_HELPER_NAMES: [&str; 2] =
["_ll_alloc_and_clear", "_ll_fixed_alloc_and_clear"];Then drive both alloc_and_set_sub_names and the lib.rs loop from that constant:
- for clear_name in ["_ll_alloc_and_clear", "_ll_fixed_alloc_and_clear"] {
+ for clear_name in
+ crate::translator::rtyper::rlist::CLEAR_HELPER_NAMES
+ {
call_control.mark_oopspec(
parse::CallPath::from_segments([clear_name]),
"newlist_clear(count)".to_string(),
);
}🤖 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 `@majit/majit-translate/src/translator/rtyper/rlist.rs` around lines 9318 -
9333, Eliminate duplicated clear-helper name literals by defining a shared
exported source of truth in rlist.rs, such as CLEAR_HELPER_NAMES, and use it in
alloc_and_set_sub_names for both ListLayout variants. Update the lib.rs
mark_oopspec registration loop to iterate over the same constant, preserving the
existing names and ensuring producer and consumer registrations cannot drift.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 13d533c). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
youknowone
added a commit
that referenced
this pull request
Aug 7, 2026
The findings were measured on `pc-map` on 2026-07-05. Re-measured on `ec-wiring` at base 58fcd37, thirteen of the fifteen issues the document tracks are closed and the priority order has inverted. F1: gh#366/367/368/369 closed; `metadata.pc_map` and `resume_jitcode_pc_for` have zero hits and `resume::SnapshotFrame.pc` is the JitCode byte offset. The surviving `pc_map` matches are the compile-time exit-recovery `Vec<usize>` in jit/codewriter.rs and jit/flatten.rs, a different thing. Residue recorded: recorder.rs's SnapshotFrame doc still describes the deleted translation, py_pc is stored rather than derived, and build_state_field_snapshot stamps the JitCode offset into py_pc (unproven, needs a repro). F2: verified done — `is_full_body_walk`, `PYRE_FULL_BODY_WALK` and `OpcodeHandler for MIFrame` have zero hits each. F3: regressed to 15 registrations against MAX_EXTRA_ROOT_WALKERS = 16; the 16th caller hits `panic!("capacity exceeded")` at startup. F4: gh#346 and gh#373 closed, coverage still landing (#1065); abort_permanent unchanged in scale, but the exit criterion is the census, not a match count. F5: gate-triage.md now exists but the population grew from 119 matches to 245 distinct PYRE_* identifiers. Sequencing amended to WS3 > WS2 > WS1-residue > WS4. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 7, 2026
The findings were measured on `pc-map` on 2026-07-05. Re-measured on `ec-wiring` at base 58fcd37, thirteen of the fifteen issues the document tracks are closed and the priority order has inverted. F1: gh#366/367/368/369 closed; `metadata.pc_map` and `resume_jitcode_pc_for` have zero hits and `resume::SnapshotFrame.pc` is the JitCode byte offset. The surviving `pc_map` matches are the compile-time exit-recovery `Vec<usize>` in jit/codewriter.rs and jit/flatten.rs, a different thing. Residue recorded: recorder.rs's SnapshotFrame doc still describes the deleted translation, py_pc is stored rather than derived, and build_state_field_snapshot stamps the JitCode offset into py_pc (unproven, needs a repro). F2: verified done — `is_full_body_walk`, `PYRE_FULL_BODY_WALK` and `OpcodeHandler for MIFrame` have zero hits each. F3: regressed to 15 registrations against MAX_EXTRA_ROOT_WALKERS = 16; the 16th caller hits `panic!("capacity exceeded")` at startup. F4: gh#346 and gh#373 closed, coverage still landing (#1065); abort_permanent unchanged in scale, but the exit criterion is the census, not a match count. F5: gate-triage.md now exists but the population grew from 119 matches to 245 distinct PYRE_* identifiers. Sequencing amended to WS3 > WS2 > WS1-residue > WS4. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 7, 2026
… on a non-measurement (#1095) * jit: stamp the qmut abort's own subwalk coordinate, and re-seed a live-NULL operand slot A walk that executed residual side effects and then fails to commit its end state falls back to the legacy replay from the traced region's entry, which runs those residuals a second time. Two shapes reached that fallback; both show up under `PYRE_FBW_CENSUS=1` as `committed=false effects>0`. `WalkSession::abort_in_subwalk` is sticky — `claim_abort_coordinate` only ever sets it — so an inline sub-walk abort the walk recovered from left it true for every later abort in the same trace attempt, and `flush_qmut_abort_state`'s gate then read a root-frame abort as a callee coordinate. The `ForceQuasiImmutable` raise in `dispatch_residual_call_iRd_kind` now stamps it from `fbw_mode.inline_subwalk` at the raise point, as the two kept-stack branch-guard raises already do. `reseed_vstack_from_shadow` rejected a NULL const-ptr shadow slot outright, because a NULL there can also mean a slot the portal never wrote. It now accepts one carrying the `virtualizable_live_null_slots` marker, which records that the last executed store into that slot wrote a NULL. PUSH_NULL's `self_or_null` sentinel is such a slot and stays live across the whole callable/args/kwargs build ahead of a CALL; the reorder region re-seeds the mirror in the middle of that build, and the rejected slot made `capture_vstack_mirror_image` refuse the image, leaving an escape inside the call with no blackhole resume. `capture_vstack_mirror_image`'s decline line gains the Python pc and the mirror boxes. The LoadName cell-fold gate comment is rewritten to the measured state: with the gate lifted the `bench/synth` corpus is output-correct, and what fails is `exception_reraise_tb_depth_jitstress` at 13.0x against its 4x pypy gate plus four benches' jit-stats. Measured with the gate lifted, in-place arms: `iter57/real_exception` 100003 -> 100000, `exception_reentry_guard_finally_residual` `leaked 4 reentry 2` -> `leaked 0 reentry 0`. Assisted-by: Claude * jit: record why reseed_vstack_from_callee_shadow keeps its NULL const-ptr rejection The callee-shadow reseed is the structural twin of `reseed_vstack_from_shadow` and rejects a NULL const-ptr the same way, but its source is a sparse `HashMap`, where a present key is already the write-witness the dense virtualizable array needed a per-slot side table to supply. So the clause discards a proven write whose value happens to be PUSH_NULL's `self_or_null`. Measured before writing this: dropping the clause leaves `check.py --backend dynasm` at 386/386 with no jit-stats movement and no baseline change, so the corpus does not distinguish the two behaviours. Behaviour unchanged; the comment records the asymmetry and the measurement. Assisted-by: Claude * rework.md: refresh the audit against the current tree The findings were measured on `pc-map` on 2026-07-05. Re-measured on `ec-wiring` at base 58fcd37, thirteen of the fifteen issues the document tracks are closed and the priority order has inverted. F1: gh#366/367/368/369 closed; `metadata.pc_map` and `resume_jitcode_pc_for` have zero hits and `resume::SnapshotFrame.pc` is the JitCode byte offset. The surviving `pc_map` matches are the compile-time exit-recovery `Vec<usize>` in jit/codewriter.rs and jit/flatten.rs, a different thing. Residue recorded: recorder.rs's SnapshotFrame doc still describes the deleted translation, py_pc is stored rather than derived, and build_state_field_snapshot stamps the JitCode offset into py_pc (unproven, needs a repro). F2: verified done — `is_full_body_walk`, `PYRE_FULL_BODY_WALK` and `OpcodeHandler for MIFrame` have zero hits each. F3: regressed to 15 registrations against MAX_EXTRA_ROOT_WALKERS = 16; the 16th caller hits `panic!("capacity exceeded")` at startup. F4: gh#346 and gh#373 closed, coverage still landing (#1065); abort_permanent unchanged in scale, but the exit criterion is the census, not a match count. F5: gate-triage.md now exists but the population grew from 119 matches to 245 distinct PYRE_* identifiers. Sequencing amended to WS3 > WS2 > WS1-residue > WS4. Assisted-by: Claude * rework.md: correct the F5 gate count to a reproducible measurement The refresh recorded 245 distinct `PYRE_*` identifiers against the audit's original 119. That figure does not reproduce: tracked `*.rs` holds 131 distinct identifiers, all tracked files 174, and 548 raw matches. The quantity comparable to the original "distinct `PYRE_*` env gates" is the set of names actually read from the environment, which is 126. The command is now stated in the document so the number can be re-derived, along with the three other counts it is easy to confuse it with. Assisted-by: Claude * check.py: do not fail a ratio gate whose baseline is clamped to the floor `_exec_time` clamps a startup-subtracted time to `EXEC_TIME_FLOOR_S` so ratios cannot divide by ~0. When the pypy baseline lands there, the ratio is `pyre_exec / EXEC_TIME_FLOOR_S` and the ceiling it is compared against is an absolute wall-clock budget of `ceiling * EXEC_TIME_FLOOR_S` seconds, fitted on whichever host wrote the header. The comparison table already marks those ratios `~` and prints "ratio is not a measurement"; the gate failed the run on them anyway. `failed_bound` now returns None whenever the baseline is clamped, instead of requiring the backend to be at the floor as well. Only the ceiling changes behaviour: the floor arms at `exec_baseline >= FLOOR_GATE_MIN_BASELINE_S`, which a clamped baseline is always under. The gate can therefore only pass more than before, never fail more. The `[... clamped to floor; ratio not a measurement]` suffix in `_gate_fail_detail` is unreachable once a clamped baseline returns no bound, and is removed; the `~` legend states the consequence instead. Three consecutive `main` runs failed this way on three different fixtures across two runners: global_cell_shortpreamble_hot 24.1x > 19x and class_reassign_hot 49.2x > 47x on ubuntu-24.04, reentrant_key_eq_mutation 10.3x > 5x on macos-latest (runs 31079972573, 31080288895). Discriminator, cranelift, `class_reassign_hot` with its ceiling temporarily set to 1: the previous check.py reports SLOWER "exec 0.13s > pypy 0.01s ratio 27.0x > gate 1x [pypy exec clamped to floor; ratio not a measurement]", this one reports PASS. With the same ceiling of 1 on seqiter_tuple_error_parity, whose pypy exec is a measurement, this check.py still reports SLOWER at 18.3x — the ceiling is untouched wherever the baseline is real. The three fixtures above pass with their own ceilings restored. Assisted-by: Claude * posix: correct which stat rejection precedes the platform's dir_fd check `stat_entry` consults `HAVE_FSTATAT` while unwrapping `dir_fd`, above the descriptor branch, so the descriptor+`dir_fd` conflict is unreachable where `fstatat` does not exist. The comment claimed both fd-conflict rejections come first. #1081 corrected the same claim in `extra_tests/parity_tests/os_stat_file_descriptor.py` and cites `_DirFD_Unavailable` (`interp_posix.py:285-292`) for it; this is the statement of it that sits next to the code. Assisted-by: Claude * bench: re-record the wasm jit-stats for exception_reused_object_tb_not_doubled `fbw_blackhole_adopted_single_frame` reads 3 where the baseline had no entry for it. `loops_compiled=4` and `bridges_compiled=3` are unchanged, so the trace shape is the same and what moved is that the walk now adopts the blackhole resume image instead of falling back to the replay from the traced region's entry. Attributed by measuring both arms with the same command, `check.py --backend wasm --synthetic-only --synthetic-pattern exception_reused_object_tb_not_doubled`: with `ff503b5d746` reverse-applied in place the bench reports ALL PASSED against the existing baseline, and with it restored it reports the 0 -> 3 change. The control arm took 2m32s against the treatment arm's 4s, which is the wasm module being relinked rather than reused. The counter arrived with #1064 and this bench's baselines were last recorded at `da5e6fb38c7` (#1059), so absence from the baseline did not by itself say which of the two it was. No CI job runs `--backend wasm`, so the wasm baselines are not gated there either. The other four keys the re-record adds -- fbw_blackhole_adopted_multi_frame, fbw_store_journal_rollback_failed, field_pos_attached_misplaced, field_pos_spec_misplaced -- are counters that did not exist at #1059 and are pinned at 0 here for the first time. The dynasm and cranelift baselines are not re-recorded: both backends still report ALL PASSED for this bench. Assisted-by: Claude * bench: restore bridges_compiled and guard_failures on three synth baselines `9d2fff92649` (#1063) re-recorded 993 jit-stats baselines. All but four gained only the two new `field_pos_*_misplaced=0` keys; three changed a value: binary_int_overflow_local_resume bridges 5 -> 6 guards 647 -> 686 exc_bridge_entry_guard_not_removed bridges 4 -> 5 guards 809 -> 1009 list_append_write_barrier_gc bridges 5 -> 6 guards 1345 -> 1562 Five runs report the pre-#1063 values and none reports the recorded ones: dynasm, cranelift and wasm here, and `main`'s own CI on ubuntu-24.04 and macos-latest at 9d2fff9 -- run 31139317566, jobs 92747505633 and 92748753166, on a tree carrying no commit from this branch. The three benches fail identically on all three backends in each of them. Only those two keys are restored; #1063's two added keys stay. The fourth bench it revalued, getattribute_override_no_bind, is left as recorded: it passes here and in that CI run, so its new values do reproduce. Assisted-by: Claude
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ports the resizable-list allocation path end to end: the rtyper
ll_alloc_and_sethelper-graph family (T1–T4) and the fullnewlist_clearcompound jitcode opcode (N1–N7), replacing the earlier codewriter arm that miscompiled a resized-list result.13 commits, one coherent epic. No backend (dynasm/cranelift/wasm) resop changes —
newlist_clearis a jitcode opcode that both the tracer and the blackhole decompose into already-wired primitives (New+SetfieldGc+NewArrayClear+SetfieldGc).The two defects this closes
newlist_clearcodewriter arm hardcodeditem_ty: Ref(None)and discardedop.result.concretetype. For a resized list result (Ptr(GcStruct("list", {length, items}))) it emitted a barenew_array_clear— an items array with no struct header, so downstream.length/.itemsreads hit absent offsets, and non-pointer element slots were GC-traced as pointers. N5 forks on the result list layout (newlist_clear_shape, mirroring upstream'sresizable = isinstance(LIST, GcStruct)): a"list"struct pointee lowers toNewListClear, an array pointee toNewArrayClear, both recoveringitem_tyfrom the element lltype viamodel::getkind— never hardcoded._ll_alloc_and_clear, so the resizedmark_oopspecregistration overwrote the fixed one. N6 renames the fixed layout's helper_ll_fixed_alloc_and_clearand registers thenewlist_clear(count)oopspec on both paths.What each commit does
Family (T1–T4)
b5637937418— portrgc.ll_arrayclear/ll_arrayfillhelper-graph builders intorbuilder.rs(three-block element loops overPtr(GcArray(ITEM)); arrayfill emits onegc_writebarrierbefore the loop).f59d96d3302— lower thenewlist_clearoopspec tonew_array_clearin the codewriter; addOpKind::NewArrayClearand wire it through the exhaustive matches.c36c2ef470f— implement thell_alloc_and_setfamily inrlist.rs:rtype_alloc_and_setdispatches on the result repr and gendirectcalls a minted helper;build_alloc_and_set_familymints six sub-helpers with per-element-type zero/null realization.b7ad03253e3— dispatch thealloc_and_setoperation tortype_alloc_and_setin the rtyper.Opcode (N1–N7)
50a9249b150(N2) — add compoundOpKind::NewListClear { length, item_ty, array_type_id }besideNewArrayClear; wire every exhaustive OpKind match.238e5958f6b(N6) — per-layout clear-helper name + both oopspecs (C2 fix).d10c07d8220— fail-loud placeholder arm in the assembler (enforces emit-before-producer; replaced by N3 below).e931aaeb7ab+9bea0bccd04(N1) — register the synthetic resizable-list"list"GcStruct in the field registry (length@0,items@8, size 16); items field spelled as a bare pointer.481459860db(N3) — emitnewlist_clear/idddd>r: length int register + four interned descrs in the handler read order (Size structdescr,length/itemsField descrs, items array descr) + ref result.839857f4a28(N5) — fork on the result list layout, recovering the element type (C1 fix).d10f3ac0c0b(N4) — decomposeBC_NEWLIST_CLEARin the tracer intoNew + 2×SetfieldGc + NewArrayClear, live-allocating the header (no-collect nursery/old-gen) and cleared items block with a write barrier; adds theframe.rsdecoder and the emit test builder.13d533c2fe8(N7) — relabel the untyped-result test (..._untyped_result_falls_back_to_ref_array) and pin the three oopspec-dispatch matchers toNewArrayClear.GC safety (N4)
The struct header is allocated no-collect on the
BC_NEWdiscipline (headerless → no-collect nursery, typed → old-gen), then the items block viaalloc_oldgen_typed— the non-collecting, non-moving mark-sweep path — so the unrooted struct pointer held in the register bank across the items allocation cannot move or be freed. The hand-inlined items allocation is byte-identical todynasm_alloc_oldgen_varsize_typed_and_set_len(runner.rs). The record order and dataflow (sboxfeeds both setfields as arg0,aboxfeeds the items setfield as arg1) matchopimpl_newlist_clear.Scope note
Census yield is ~0 by construction — no current producer emits
alloc_and_set→_ll_alloc_and_clearreaching a resized list into a lifting graph. The value is the C1 correctness fix plus structural parity withopimpl_newlist_clear/bhimpl_newlist_clear/do_resizable_newlist_clear. This is correct-when-reached infrastructure.Known backlog (pre-existing, not introduced, likely unreachable): the
array_type_id: Nonepath collapses sub-word element widths throughgetkind, so a hypothetical resized list of sub-word elements would get an 8-byte-stride items descr. Corpus resizable lists are word-element (int/float/ref).Verification
cargo test --all --no-default-features --features dynasm— green (0 failures,default_bh_builder_unwired_set_matches_task_85_snapshot+production_bh_builder_covers_every_build_emitted_opnameboth ok).majit-translatelib 3118 passed,majit-metainterplib 1443 passed.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests