Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 90 additions & 0 deletions TASK.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
# Fix the typed-array aliasing regression in PR #9360

## The bug

`main` is correct. `main` + PR #9360 turns a `Uint8Array` view over another
typed array's buffer into mostly zeros.

```ts
const words = new Uint32Array(2);
const bytes = new Uint8Array(words.buffer);
words[0] = 0x01020304;
words[1] = 0x05060708;
for (let i = 0; i < 8; i++) out.push(bytes[i]);
```

| | result |
|---|---|
| node | `4 3 2 1 8 7 6 5` |
| perry + #9360 | `4 0 0 0 0 0 0 0` |

It also fails the committed fixture
`test-files/test_gap_typedarray_buffer_aliasing_7219.ts`, which is #7219's own
regression test — that fixture passes on `main` and fails with this PR.

## What is already established — do NOT re-derive these

1. **Culprit is one commit.** `main` + `7de78d0576` ALONE reproduces it. The six
perf commits stacked on top are not implicated.
2. **The write is fine.** After `words[0] = 0x01020304`, reading `words[0]` back
gives `16909060` exactly. The u32 store landed correctly.
3. **The metadata is fine.** `bytes.byteLength == 4`, `bytes.length == 4`,
`words.buffer.byteLength == 4` — all match node.
4. **It is NOT a stride error.** Reading `base + i*4` would give `4 8 0 0 …` on
the two-word case (index 4 hitting `words[1]`'s low byte). Actual output is
`4 0 0 0 0 0 0 0`, so that hypothesis is disproved.
5. **The codegen lowering is excluded by symbol evidence**, not inference:
`nm` on the compiled fixture shows ZERO references to `js_u8_buffer_read_f64`.
`try_lower_u8_buffer_read` never fires here. (An earlier `PERRY_U8_INLINE_READ=0`
A/B "ruling it out" was vacuous for the same reason — the path was never taken,
so the switch had nothing to disable. Do not repeat that experiment.)

So: element 0 reads correctly and every other index reads 0, while length and
byteLength are right. Live surface is the runtime side of `7de78d0576`:
`crates/perry-runtime/src/typedarray/access.rs` (+71) and
`crates/perry-runtime/src/buffer/header.rs` (+67).

## Fixes already ATTEMPTED AND FAILED — do not repeat

- Evicting the stale `PERRY_U8_INLINE_CACHE` admission in `register_view_meta`.
- Guarding both registry-miss recovery arms with `view_meta_of(addr).is_none()`.
- Narrowing both recovery arms from `is_registered_buffer(addr)` to
`is_uint8array_buffer(addr)`.

None changed the output. Note the last two were never confirmed to be REACHED
for this receiver — if you use them as evidence, first prove the arm executes
(add a temporary eprintln or counter), or you will repeat a vacuous experiment.

## Build and test

```
cd /Users/amlug/projects/perry/cx-9360
export CARGO_TARGET_DIR=/Users/amlug/agent-targets/cx9360
export PERRY_RUNTIME_DIR=$CARGO_TARGET_DIR/release
cargo build --release -p perry -p perry-runtime-static -p perry-stdlib-static
$CARGO_TARGET_DIR/release/perry test-files/test_gap_typedarray_buffer_aliasing_7219.ts -o /tmp/cx9360_fix
diff <(node --experimental-strip-types test-files/test_gap_typedarray_buffer_aliasing_7219.ts) <(/tmp/cx9360_fix)
```

Node must be v26.5.1 (matches `.node-version`). A build is ~8-10 minutes; the
compile+run of one fixture is seconds, so iterate on the fixture, not the build.

## Definition of done

1. The fixture above is byte-identical to node.
2. `RUST_TEST_THREADS=1 cargo test --release -p perry-runtime` is green
(perry-runtime tests are NOT parallel-safe; the flag is required).
3. `./scripts/check_file_size.sh`, `python3 scripts/raw_handle_debt.py`,
`python3 scripts/gc_runtime_root_holders.py`, `python3 scripts/addr_class_inventory.py`
and `cargo fmt --all -- --check` all pass.
4. The fix keeps #9360's actual feature working — it exists to recover
`Uint8Array` elements on a kind-registry miss. Do not fix the aliasing bug by
deleting the feature; if the recovery must narrow, say precisely which
receivers it still serves.

## Rules

- Do not touch `test-parity/gap_snapshot.json` or any baseline/allowlist file to
make a gate pass. Fix the code.
- Do not raise a ratchet ceiling.
- Report what you changed and WHY, and state any claim you could not verify.
14 changes: 14 additions & 0 deletions changelog.d/9360-u8-byte-reductions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
**Typed-array hot loops now retain their native numeric reductions.**
Buffer-backed `Uint8Array` reads recover the correct element on typed-array
registry misses and use a guard-validated inline byte-load lane for
module-global and declared-parameter receivers. Construction-proven
module-global numeric views now feed the same Number-by-construction proof as
body-local views, removing the rooted dynamic-add diamond from byte sums.

Bounded byte reductions may carry per-instruction `reassoc` when their complete
integer magnitude proof stays within the exact f64 range, allowing LLVM to
split the serial accumulator without enabling unsound global fast-math.
Module-init accumulators proven never to hold pointers also shed redundant
shadow slots and back-edge GC polls. On the measured `bench_buffer_readwrite`
shape this takes Perry from 94 ms to 34 ms against Node's 81 ms, while
unbounded f64 reductions remain unchanged.
28 changes: 28 additions & 0 deletions crates/perry-codegen/src/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,34 @@ impl LlBlock {
r
}

/// `fadd reassoc` for ONE instruction, independent of the module's
/// `--fast-math` setting (#9363).
///
/// Only the caller's proof licenses this: the addends are byte reads
/// (magnitude <= 255, or the `undefined`-box NaN out of range) and the
/// enclosing loop's trip count is bounded, so every partial sum is either
/// an exactly-representable integer far below 2^53 — where f64 addition
/// is associative, so ANY grouping is bit-identical — or a NaN, which
/// propagates through every grouping alike. That is an exactness argument
/// about the value range, not a tolerance argument, which is why it does
/// not need `--fast-math` (whose global reassociation is NOT sound for
/// arbitrary f64 chains and is correctly off by default).
///
/// `contract` is deliberately NOT added: FMA fusion changes rounding of
/// multiply/add pairs, which this proof says nothing about.
pub fn fadd_reassoc(&mut self, a: &str, b: &str) -> String {
let r = self.reg();
self.push_inst(crate::inst::LlInst::Bin {
dst: r.clone(),
op: "fadd",
pre: "reassoc ",
ty: "double",
a: a.to_string(),
b: b.to_string(),
});
r
}

pub fn fsub(&mut self, a: &str, b: &str) -> String {
let r = self.reg();
self.push_inst(crate::inst::LlInst::Bin {
Expand Down
2 changes: 2 additions & 0 deletions crates/perry-codegen/src/codegen/closure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -861,6 +861,8 @@ pub(super) fn compile_closure(
classes,
&cross_module.compile_time_constants,
&cross_module.module_dispatch,
// #9363: a closure body reads the same module-scope views.
&cross_module.module_global_proven_types,
);
if !versioned_loop_callback {
if let Some(callback_shapes) = cross_module.array_callback_shapes.get(&func_id) {
Expand Down
58 changes: 56 additions & 2 deletions crates/perry-codegen/src/codegen/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,7 @@ pub(super) fn compile_module_entry(
main.mark_entry_init_boundary();
let flat_const_ids: std::collections::HashSet<u32> =
cross_module.flat_const_arrays.keys().copied().collect();
let (main_shadow_slot_map, main_shadow_slot_clears_after_stmt) =
let (mut main_shadow_slot_map, _) =
enable_module_init_shadow_frame(main, &hir.init, &flat_const_ids);

let main_boxed_vars = module_boxed_vars.clone();
Expand Down Expand Up @@ -749,7 +749,34 @@ pub(super) fn compile_module_entry(
classes,
&cross_module.compile_time_constants,
&cross_module.module_dispatch,
// #9363: module-scope views need their construction proofs here
// too — passing an empty map kept top-level accumulator loops on
// the rooted/guarded path while in-function ones were clean.
&cross_module.module_global_proven_types,
);
// #9363: the same redundant-shadow-slot pruning `codegen/function.rs`
// does, which module init never got. The shadow map is built above
// from the CONSERVATIVE pointer-typed-locals scan, before the fact
// graph exists; a local the whole-write proof later shows can only
// hold a Number keeps a root slot it can never need. That slot is not
// just wasted stores: `local_is_inert_primitive` refuses any local
// with one, so the accumulator of `sum += buf[i]` was never inert,
// `loop_may_allocate` stayed true, and the inner loop kept a
// per-iteration volatile GC poll that blocks vectorization. In a
// function body the same loop was already clean — this was the whole
// top-level/in-function asymmetry.
//
// `enable_post_init_shadow_frame` sized the frame from the unpruned
// map, so the retained slot indices stay valid with holes, exactly as
// in the function-body twin.
main_shadow_slot_map.retain(|id, _| {
!main_native_facts
.number_by_construction_locals()
.contains(id)
});
let main_shadow_slot_clears_after_stmt =
crate::collectors::collect_shadow_slot_clear_points(&hir.init, &main_shadow_slot_map);

// #7109: the program-entry body participates in canonical (i32/u32/Str)
// selection on exactly the per-value rules a function body uses. There
// is no structural context reason to deny — see
Expand Down Expand Up @@ -1452,7 +1479,7 @@ pub(super) fn compile_module_entry(
init_fn.mark_entry_init_boundary();
let flat_const_ids: std::collections::HashSet<u32> =
cross_module.flat_const_arrays.keys().copied().collect();
let (init_shadow_slot_map, init_shadow_slot_clears_after_stmt) =
let (mut init_shadow_slot_map, _) =
enable_module_init_shadow_frame(init_fn, &hir.init, &flat_const_ids);

let init_boxed_vars = module_boxed_vars.clone();
Expand Down Expand Up @@ -1480,7 +1507,34 @@ pub(super) fn compile_module_entry(
classes,
&cross_module.compile_time_constants,
&cross_module.module_dispatch,
// #9363: module-scope views need their construction proofs here
// too — passing an empty map kept top-level accumulator loops on
// the rooted/guarded path while in-function ones were clean.
&cross_module.module_global_proven_types,
);
// #9363: the same redundant-shadow-slot pruning `codegen/function.rs`
// does, which module init never got. The shadow map is built above
// from the CONSERVATIVE pointer-typed-locals scan, before the fact
// graph exists; a local the whole-write proof later shows can only
// hold a Number keeps a root slot it can never need. That slot is not
// just wasted stores: `local_is_inert_primitive` refuses any local
// with one, so the accumulator of `sum += buf[i]` was never inert,
// `loop_may_allocate` stayed true, and the inner loop kept a
// per-iteration volatile GC poll that blocks vectorization. In a
// function body the same loop was already clean — this was the whole
// top-level/in-function asymmetry.
//
// `enable_post_init_shadow_frame` sized the frame from the unpruned
// map, so the retained slot indices stay valid with holes, exactly as
// in the function-body twin.
init_shadow_slot_map.retain(|id, _| {
!init_native_facts
.number_by_construction_locals()
.contains(id)
});
let init_shadow_slot_clears_after_stmt =
crate::collectors::collect_shadow_slot_clear_points(&hir.init, &init_shadow_slot_map);

// #7109: the module-init body participates in canonical (i32/u32/Str)
// selection on exactly the per-value rules a function body uses. There
// is no structural context reason to deny — see
Expand Down
4 changes: 4 additions & 0 deletions crates/perry-codegen/src/codegen/method.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,8 @@ pub(super) fn compile_method(
classes,
&cross_module.compile_time_constants,
&cross_module.module_dispatch,
// #9363: a method body reads the same module-scope views.
&cross_module.module_global_proven_types,
);
let mut index_clone_integer_locals = native_facts.integer_locals().clone();
index_clone_integer_locals.extend(index_param_ids.iter().copied());
Expand Down Expand Up @@ -1565,6 +1567,8 @@ pub(super) fn compile_static_method(
classes,
&cross_module.compile_time_constants,
&cross_module.module_dispatch,
// #9363: a method body reads the same module-scope views.
&cross_module.module_global_proven_types,
);

// Representation-selection context gates (see codegen/function.rs).
Expand Down
24 changes: 23 additions & 1 deletion crates/perry-codegen/src/collectors/hir_facts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ pub(crate) struct RepresentationFacts {
/// — it never widens the parallel-shadow `needs_i32_slot` gate. See
/// `collectors/loop_bounded_i32.rs`.
pub loop_bounded_i32_locals: HashSet<u32>,
/// #9363: accumulators whose `acc = acc + <byte read>` chain provably
/// stays below 2^53, so the update's `fadd` may carry `reassoc` and the
/// reduction can be split into parallel partial sums. Same trip-count
/// proof as `loop_bounded_i32_locals`, weaker conclusion — it changes no
/// storage decision, only an FMF flag. See `collectors/loop_bounded_i32.rs`.
pub reassociable_f64_accumulators: HashSet<u32>,
/// Locals whose canonical-i32 promotion is PROVABLE but not PROFITABLE
/// (#7128): written after declaration, no i32-consuming read anywhere in
/// the body, and at least one double-consuming read inside a loop — so the
Expand Down Expand Up @@ -224,6 +230,10 @@ impl TypeFacts {
&self.representation.loop_bounded_i32_locals
}

pub(crate) fn reassociable_f64_accumulators(&self) -> &HashSet<u32> {
&self.representation.reassociable_f64_accumulators
}

pub(crate) fn unprofitable_canonical_i32_locals(&self) -> &HashSet<u32> {
&self.representation.unprofitable_canonical_i32_locals
}
Expand Down Expand Up @@ -541,6 +551,14 @@ pub(crate) fn collect_type_facts(
} else {
HashSet::new()
};
// #9363: the reassociation admission runs independently of the canonical
// i32 gate — it is not a storage decision, so `PERRY_CANONICAL_I32_LOCALS=0`
// must not silently disable it.
let reassociable_f64_accumulators =
super::loop_bounded_i32::collect_reassociable_f64_accumulators(
stmts,
compile_time_constants,
);
// #7123: this set now includes accumulators whose integer-ness and full
// range were proved together (for example `sum += i % 1000`). The older
// integer provenance collector deliberately does not accept bare `%`, so
Expand Down Expand Up @@ -716,6 +734,7 @@ pub(crate) fn collect_type_facts(
not_bigint_locals,
int_valued_ta_locals,
loop_bounded_i32_locals,
reassociable_f64_accumulators,
unprofitable_canonical_i32_locals,
number_by_construction_locals,
},
Expand Down Expand Up @@ -777,6 +796,7 @@ pub(crate) fn collect_native_region_fact_graph(
classes: &HashMap<String, &perry_hir::Class>,
compile_time_constants: &HashMap<u32, f64>,
module_dispatch: &super::ModuleDispatchFacts,
module_global_proven_types: &HashMap<u32, perry_hir::types::Type>,
) -> NativeRegionFactGraph {
collect_native_region_fact_graph_with_spec_params(
stmts,
Expand All @@ -794,7 +814,7 @@ pub(crate) fn collect_native_region_fact_graph(
&HashSet::new(),
&HashSet::new(),
&HashSet::new(),
&HashMap::new(),
module_global_proven_types,
)
}

Expand Down Expand Up @@ -2195,6 +2215,7 @@ mod tests {
&HashMap::new(),
&constants,
&crate::collectors::ModuleDispatchFacts::default(),
&HashMap::new(),
);

assert!(graph.known_noalias_buffer_locals().contains(&1));
Expand Down Expand Up @@ -2286,6 +2307,7 @@ mod tests {
&HashMap::new(),
&HashMap::new(),
&crate::collectors::ModuleDispatchFacts::default(),
&HashMap::new(),
);

assert!(graph.integer_locals().contains(&1));
Expand Down
Loading
Loading