diff --git a/changelog.d/9274-length-bound-offset-reads.md b/changelog.d/9274-length-bound-offset-reads.md new file mode 100644 index 0000000000..31c3008350 --- /dev/null +++ b/changelog.d/9274-length-bound-offset-reads.md @@ -0,0 +1,44 @@ +**An `arr.length`-bounded packed-f64 loop no longer loses its fast clone when the +body reads `a[k ± c]`** (#9259). It did not lose it partially — it lost it +entirely, taking the plain `a[k]` in the same loop with it: 70 ms → 36 ms on a +4096-element accumulate loop, and 60 ms → 19 ms on a comparison loop, against +node's 13 ms and 16 ms. + +The cause was a cascade rather than a missed element load. Three separate +predicates encode "an index this loop's guard covers" as a bare +`Expr::LocalGet(counter_id)`, so `a[k - 1]` — an `Expr::Binary` — matched none of +them. The matcher's body walker declined, the offset read fell back to a helper +call, and the clone's own call-free scan then discarded the whole clone. + +Two matchers each covered half the shape and neither covered the combination. +`lower_packed_f64_versioned_for` understands the `i < arr.length` bound but +publishes `window_validated: false`; `lower_packed_f64_range_versioned_for` +validates the offset window but accepts only a literal or loop-invariant bound, +and per its own call-site comment runs only after the first declined. Since +`arr.length` is the idiomatic spelling, the natural form was the slow one. + +The fix admits a constant offset and pays the same inline `icmp ult idx, len` a +foreign counter already pays, taking the fact's existing side exit when it fails +— a compare and a never-taken branch, not a call, so the clone stays call-free. +The machinery existed already; what was missing was letting an offset index reach +it. Matcher and lowering now share one index parser, deliberately: a matcher that +admits what the lowering declines is not a missed optimisation, it is the same +9× regression arriving by another route. + +Soundness rests on the guard being stronger than its flag name suggests. The +versioned guard ends in `js_array_is_numeric_f64_layout`, a whole-array property +that answers 0 for a holes-flagged array, so a passing guard means every +in-bounds slot is raw f64; `window_validated: false` is a statement about bounds, +not holes, and bounds are exactly what the inline check re-establishes. The +compare is unsigned, so a negative index (`a[k-1]` at `k == 0`) exceeds any +length and side-exits. Reads only — a store side exit re-executes the iteration, +harmless for a read and double-applying for a store. + +Because the parser is shared, this also admits an offset on a foreign counter +(`a[j - 1]` for an enclosing loop's `j`), which is wider than the headline shape +and deliberate: the bounds check makes both cases identical. + +`s += a[k] + a[k-1]` still pays a dynamic add — `accumulator_rhs_is_numeric` and +`has_numeric_index_fact` carry the same bare-`LocalGet` assumption — which is why +the comparison loop gains 3.2× and the accumulate loop 1.94×. That is being +addressed separately. diff --git a/crates/perry-codegen/src/expr/index_get.rs b/crates/perry-codegen/src/expr/index_get.rs index be470399c7..7cd60fe0b6 100644 --- a/crates/perry-codegen/src/expr/index_get.rs +++ b/crates/perry-codegen/src/expr/index_get.rs @@ -43,7 +43,7 @@ use super::{ mod foreign_counter; mod guarded_array; pub(crate) use foreign_counter::packed_f64_loop_index_parts; -use foreign_counter::{foreign_packed_loop_read, packed_f64_loop_fact_for_index}; +use foreign_counter::{foreign_packed_loop_read, packed_f64_loop_offset_read}; mod inline_dyn_typed_array; use guarded_array::{ @@ -222,7 +222,11 @@ fn numeric_index_has_loop_array_index_proof(ctx: &FnCtx<'_>, object: &Expr, inde if !ctx.i32_counter_slots.contains_key(&idx_id) { return false; } - if packed_f64_loop_fact_for_index(ctx, *arr_id, index).is_some() { + // #9259: an offset index is still an integer array index even when the + // loop bound does not prove it in range, so it must not fall through to + // the runtime-key helper -- that call is what the clone's call-free scan + // rejects. In-range-ness is settled by the inline bounds check instead. + if packed_f64_loop_offset_read(ctx, *arr_id, index).is_some() { return true; } offset == 0 @@ -749,14 +753,19 @@ pub(crate) fn lower_numeric_index_get_for_number_context( } if let Expr::LocalGet(arr_id) = object.as_ref() { - if let Some((fact, idx_id, offset)) = - packed_f64_loop_fact_for_index(ctx, *arr_id, index.as_ref()) + if let Some((fact, idx_id, offset, needs_bounds_check)) = + packed_f64_loop_offset_read(ctx, *arr_id, index.as_ref()) { if let Some(i32_slot) = ctx.i32_counter_slots.get(&idx_id).cloned() { let arr_box = lower_expr(ctx, object)?; let idx_i32 = load_packed_loop_index_i32(ctx, &i32_slot, offset); return Ok(Some(lower_packed_f64_loop_index_get( - ctx, *arr_id, &arr_box, &idx_i32, &fact, false, + ctx, + *arr_id, + &arr_box, + &idx_i32, + &fact, + needs_bounds_check, ))); } } @@ -1631,14 +1640,19 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // The loop already proved `i < arr.length` and the // body provably can't change `arr.length`. if let Expr::LocalGet(arr_id) = object.as_ref() { - if let Some((fact, idx_id, offset)) = - packed_f64_loop_fact_for_index(ctx, *arr_id, index.as_ref()) + if let Some((fact, idx_id, offset, needs_bounds_check)) = + packed_f64_loop_offset_read(ctx, *arr_id, index.as_ref()) { if let Some(i32_slot) = ctx.i32_counter_slots.get(&idx_id).cloned() { let arr_box = lower_expr(ctx, object)?; let idx_i32 = load_packed_loop_index_i32(ctx, &i32_slot, offset); return Ok(lower_packed_f64_loop_index_get( - ctx, *arr_id, &arr_box, &idx_i32, &fact, false, + ctx, + *arr_id, + &arr_box, + &idx_i32, + &fact, + needs_bounds_check, )); } } diff --git a/crates/perry-codegen/src/expr/index_get/foreign_counter.rs b/crates/perry-codegen/src/expr/index_get/foreign_counter.rs index 84a6a46937..61a6b7b324 100644 --- a/crates/perry-codegen/src/expr/index_get/foreign_counter.rs +++ b/crates/perry-codegen/src/expr/index_get/foreign_counter.rs @@ -69,19 +69,31 @@ pub(crate) fn packed_f64_loop_index_parts(index: &Expr) -> Option<(u32, i32)> { } } -/// Look up a packed-f64 loop fact for `(arr, index-expr)`. Zero-offset -/// indices match any fact; non-zero offsets only match hole-tolerant facts -/// (established by the range guard, which validated the whole offset window — -/// the length-bound guard of the classic matcher only proves `i` itself). -pub(crate) fn packed_f64_loop_fact_for_index( +/// Look up a packed-f64 loop fact for `(arr, index-expr)`, reporting whether +/// a non-zero offset needs an inline bounds check rather than declining it. +/// +/// #9259: declining here was not merely losing one element load. The offset +/// read fell back to a helper CALL, and the versioned matcher's post-hoc +/// `fast_clone_not_call_free` scan then discarded the ENTIRE clone — so a +/// loop like `s += a[k] + a[k-1]` lost the fast path for `a[k]` too, 9x. The +/// bounds check restores it: `lower_packed_f64_loop_index_get` tests the index +/// against the live length and takes the fact's side exit, which is a compare +/// and a never-taken branch, not a call. That is the same treatment a foreign +/// counter already gets, and for the same reason — an index the loop bound +/// does not cover needs a run-time test, not a refusal. +/// +/// The comparison is UNSIGNED, so a negative index (`a[k-1]` at `k == 0`) +/// exceeds any length and takes the side exit. Reads only: a store side exit +/// has replay semantics this does not reason about. +pub(crate) fn packed_f64_loop_offset_read( ctx: &FnCtx<'_>, arr_id: u32, index: &Expr, -) -> Option<(PackedF64LoopFact, u32, i32)> { +) -> Option<(PackedF64LoopFact, u32, i32, bool)> { let (idx_id, offset) = packed_f64_loop_index_parts(index)?; let fact = packed_f64_loop_fact(ctx, arr_id, idx_id)?; - if offset != 0 && !fact.allow_holes && !fact.window_validated { - return None; - } - Some((fact, idx_id, offset)) + // A window-validated or hole-tolerant fact already covers the offset; only + // the length-bound guard of the classic matcher leaves it unproven. + let needs_bounds_check = offset != 0 && !fact.allow_holes && !fact.window_validated; + Some((fact, idx_id, offset, needs_bounds_check)) } diff --git a/crates/perry-codegen/src/stmt/loops.rs b/crates/perry-codegen/src/stmt/loops.rs index 4d20d9c912..ab1a0decaf 100644 --- a/crates/perry-codegen/src/stmt/loops.rs +++ b/crates/perry-codegen/src/stmt/loops.rs @@ -5516,18 +5516,36 @@ fn is_packed_f64_loop_foreign_read_index( if is_packed_f64_loop_index(object, index, arr_id, counter_id) { return true; } - let (perry_hir::Expr::LocalGet(object_id), perry_hir::Expr::LocalGet(index_id)) = - (object, index) - else { + let perry_hir::Expr::LocalGet(object_id) = object else { return false; }; - *object_id == arr_id - && *index_id != counter_id - && *index_id != arr_id - && ctx.integer_locals.contains(index_id) - && ctx.i32_counter_slots.contains_key(index_id) - && !ctx.boxed_vars.contains(index_id) - && !ctx.closure_captures.contains_key(index_id) + if *object_id != arr_id { + return false; + } + // #9259: parse `j` and `j ± c` with the SAME parser the read lowering uses + // (`packed_f64_loop_offset_read`). If the two disagree, the matcher admits + // a shape the lowering declines, that read emits a helper call, and the + // clone's call-free scan then discards the whole clone — which is exactly + // the 9x this issue is about, arriving by a different route. + let Some((index_id, offset)) = crate::expr::packed_f64_loop_index_parts(index) else { + return false; + }; + // The lowering loads the index from this slot; without it there is no i32 + // to bounds-check. + if index_id == arr_id || !ctx.i32_counter_slots.contains_key(&index_id) { + return false; + } + if index_id == counter_id { + // The loop's OWN counter at a constant offset (`a[k - 1]`, the EMA + // shape). The bound proves `k` in range, not `k ± c`, so this takes + // the identical treatment a foreign counter gets below: one inline + // `icmp ult` against the live length and the fact's existing side + // exit. Offset 0 was already accepted above. + return offset != 0; + } + ctx.integer_locals.contains(&index_id) + && !ctx.boxed_vars.contains(&index_id) + && !ctx.closure_captures.contains_key(&index_id) } fn is_packed_f64_loop_index( diff --git a/crates/perry/tests/issue_9259_length_bound_offset_reads.rs b/crates/perry/tests/issue_9259_length_bound_offset_reads.rs new file mode 100644 index 0000000000..4802e55aff --- /dev/null +++ b/crates/perry/tests/issue_9259_length_bound_offset_reads.rs @@ -0,0 +1,191 @@ +//! Regression coverage for #9259: an `arr.length`-bounded packed-f64 loop kept +//! its fast clone when the body read `a[k]`, but lost it **entirely** — not +//! partially — as soon as the body also read `a[k ± c]`. +//! +//! The failure was a cascade, not a missed element load. Three separate +//! predicates encode "an index this loop's guard covers" as a bare +//! `Expr::LocalGet(counter_id)`, so `a[k - 1]` (an `Expr::Binary`) matched +//! none of them. The matcher's body walker therefore declined +//! (`read_body_is_safe == false`, reported as `clone_not_call_free`), the read +//! fell back to a helper CALL, and the clone's call-free scan then discarded +//! the whole clone — taking the fast path for the plain `a[k]` with it. The +//! measured cost was 8 ms -> 72 ms on a 4096-element loop, flipping the shape +//! from beating node to 5.5x behind it. +//! +//! The fix admits a constant offset on the loop's own counter and pays the +//! same inline `icmp ult idx, len` a foreign counter already pays, taking the +//! fact's existing side exit when it fails. Reads only: a store side exit +//! re-executes the iteration, which is harmless for a read and would +//! double-apply a store. +//! +//! What these tests pin is the *admission*, not a timing: that the clone is +//! emitted at all for the offset body, and that the results still match the +//! generic path under a moving collector. + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +fn compile(dir: &Path, source: &str) -> (PathBuf, String) { + let entry = dir.join("main.ts"); + let output = dir.join("main_bin"); + std::fs::write(&entry, source).expect("write entry"); + + let compile = Command::new(perry_bin()) + .current_dir(dir) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .env("PERRY_NO_CACHE", "1") + .env("PERRY_LLVM_KEEP_IR", "1") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + ( + output, + String::from_utf8_lossy(&compile.stderr).into_owned(), + ) +} + +/// The emitted IR, located the way `PERRY_LLVM_KEEP_IR` reports it. +fn kept_ir(stderr: &str) -> String { + let path = stderr + .lines() + .find_map(|line| line.split("kept LLVM IR: ").nth(1)) + .map(str::trim) + .map(PathBuf::from) + .unwrap_or_else(|| panic!("PERRY_LLVM_KEEP_IR did not report an IR path\n{stderr}")); + std::fs::read_to_string(path).expect("read kept LLVM IR") +} + +fn packed_blocks(ir: &str) -> usize { + ir.lines() + .filter(|line| line.starts_with("packed_f64") && line.trim_end().ends_with(':')) + .count() +} + +fn run(bin: &Path, dir: &Path, moving_gc: bool) -> Output { + let mut command = Command::new(bin); + command.current_dir(dir); + if moving_gc { + command + .env("PERRY_GC_FORCE_EVACUATE", "1") + .env("PERRY_GC_VERIFY_EVACUATION", "1"); + } + command.output().expect("run compiled binary") +} + +fn assert_stdout(output: &Output, expected: &str, moving_gc: bool) { + assert!( + output.status.success(), + "binary failed with moving_gc={moving_gc}\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!(String::from_utf8_lossy(&output.stdout), expected); +} + +/// `a[k]` alone under an `arr.length` bound — the shape that always worked. +/// Present as the positive control: without it, a regression that stopped +/// admitting *every* packed loop would leave the offset test below passing +/// vacuously in the other direction. +const PLAIN: &str = r#" +function run(a: number[]): number { + let c = 0; + for (let r = 0; r < 20; r++) { + for (let k = 1; k < a.length; k++) { + if (a[k] > 0.0) c++; + } + } + return c; +} +const a: number[] = []; +for (let i = 0; i < 512; i++) a.push((i * 37) % 1000); +console.log(run(a)); +"#; + +/// The #9259 shape: same bound, same array, one constant-offset read added. +const OFFSET: &str = r#" +function run(a: number[]): number { + let c = 0; + for (let r = 0; r < 20; r++) { + for (let k = 1; k < a.length; k++) { + if (a[k] > a[k - 1]) c++; + } + } + return c; +} +const a: number[] = []; +for (let i = 0; i < 512; i++) a.push((i * 37) % 1000); +console.log(run(a)); +"#; + +#[test] +fn length_bounded_loop_keeps_its_clone_when_the_body_reads_an_offset() { + let dir = tempfile::tempdir().expect("tempdir"); + let (_, plain_stderr) = compile(dir.path(), PLAIN); + let plain = packed_blocks(&kept_ir(&plain_stderr)); + assert!( + plain > 0, + "positive control: the plain `a[k]` body must still get a packed clone, \ + otherwise the offset assertion below proves nothing" + ); + + let dir2 = tempfile::tempdir().expect("tempdir"); + let (_, offset_stderr) = compile(dir2.path(), OFFSET); + let offset = packed_blocks(&kept_ir(&offset_stderr)); + assert!( + offset > 0, + "#9259: adding `a[k - 1]` to an `arr.length`-bounded body discarded the \ + ENTIRE packed clone (the offset read fell back to a helper call, and \ + the call-free scan then rejected the clone), so the plain `a[k]` in \ + the same loop lost its fast path too — 8ms -> 72ms" + ); +} + +/// The offset read is bounds-checked against the live length and side-exits +/// rather than reading out of bounds, so the answer must match the generic +/// path — including when the collector is relocating underneath it. +#[test] +fn offset_reads_agree_with_the_generic_path_under_a_moving_collector() { + let dir = tempfile::tempdir().expect("tempdir"); + let (bin, _) = compile(dir.path(), OFFSET); + for moving_gc in [false, true] { + assert_stdout(&run(&bin, dir.path(), moving_gc), "9860\n", moving_gc); + } +} + +/// `k - 1` is negative on the first iteration when the loop starts at 0. The +/// inline check is an UNSIGNED compare, so the negative index exceeds any +/// length and takes the side exit into the generic clone, which returns +/// `undefined` for the missing element exactly as the slow path does. +#[test] +fn a_negative_offset_index_side_exits_instead_of_reading_out_of_bounds() { + let source = r#" +function run(a: number[]): number { + let seen = 0; + for (let k = 0; k < a.length; k++) { + const prev = a[k - 1]; + if (prev === undefined) seen++; + } + return seen; +} +const a: number[] = []; +for (let i = 0; i < 64; i++) a.push(i * 1.5); +console.log(run(a)); +"#; + let dir = tempfile::tempdir().expect("tempdir"); + let (bin, _) = compile(dir.path(), source); + for moving_gc in [false, true] { + assert_stdout(&run(&bin, dir.path(), moving_gc), "1\n", moving_gc); + } +}