diff --git a/crates/perry-codegen/src/stmt/loops.rs b/crates/perry-codegen/src/stmt/loops.rs index 4d20d9c912..1f0d7353ff 100644 --- a/crates/perry-codegen/src/stmt/loops.rs +++ b/crates/perry-codegen/src/stmt/loops.rs @@ -553,6 +553,10 @@ fn emit_range_loop_accumulator_admission( matched.counter_id, slow_pre_label, block_prefix, + // The range tier publishes `window_validated: true` and its loads are + // hole-checked, so an `a[i +/- c]` read is lowered inline and yields a + // Number. See `accumulator_rhs_is_numeric`. + true, ) } @@ -865,9 +869,10 @@ fn emit_packed_numeric_accumulator_admission( counter_id: u32, slow_pre_label: &str, block_prefix: &str, + offset_reads_inlined: bool, ) -> PackedAccumulatorScope { let accumulators = super::stable_packed_accumulator::collect_numeric_accumulators( - ctx, body, array_id, counter_id, + ctx, body, array_id, counter_id, offset_reads_inlined, ); // Integer (`c++`) accumulators admit independently of the float set — // a pure count loop has no float accumulator at all. @@ -1048,6 +1053,11 @@ fn lower_packed_f64_versioned_for( matched.counter_id, &slow_pre_label, loop_label, + // This tier publishes `window_validated: false`, so + // `packed_f64_loop_fact_for_index` declines a non-zero offset and the + // read takes the generic path — which can produce `undefined`. #9259 + // is the work that would make an offset read inline here. + false, ); acc_scope.hoist_receivers(ctx, &[matched.array_id]); ctx.packed_f64_loop_facts.push(PackedF64LoopFact { diff --git a/crates/perry-codegen/src/stmt/stable_packed_accumulator.rs b/crates/perry-codegen/src/stmt/stable_packed_accumulator.rs index 398de2b935..0bc7abdec6 100644 --- a/crates/perry-codegen/src/stmt/stable_packed_accumulator.rs +++ b/crates/perry-codegen/src/stmt/stable_packed_accumulator.rs @@ -38,29 +38,57 @@ fn accumulator_rhs_is_numeric( expr: &Expr, array_id: u32, counter_id: u32, + offset_reads_inlined: bool, candidates: &std::collections::BTreeSet, ) -> bool { match expr { Expr::Number(_) | Expr::Integer(_) => true, - Expr::IndexGet { object, index } => matches!( - (object.as_ref(), index.as_ref()), - (Expr::LocalGet(a), Expr::LocalGet(i)) if *a == array_id && *i == counter_id - ), + Expr::IndexGet { object, index } => { + let Expr::LocalGet(a) = object.as_ref() else { + return false; + }; + if *a != array_id { + return false; + } + match index.as_ref() { + Expr::LocalGet(i) => *i == counter_id, + // `a[counter +/- c]`. Admissible only when the caller's tier + // emits an INLINE packed load for an offset index, because + // that load is what makes the value a Number: the range tier + // publishes `window_validated`, so its guard proved the whole + // window, and its hole-tolerant loads side-exit before + // producing a value. A tier that falls back to a generic read + // for the offset can yield `undefined`, and admitting that as + // numeric would be a wrong answer rather than a missed + // optimisation — so the caller states which it is. + // + // Without this the read is not numeric, the accumulator never + // earns its number proof, and every `+` in the enclosing + // expression lowers to a tag-test diamond over + // `js_dynamic_string_or_number_add` — the same cost #9060 and + // #9091 removed for the bare-counter form. + _ if offset_reads_inlined => { + crate::expr::packed_f64_loop_index_parts(index) + .is_some_and(|(i, _)| i == counter_id) + } + _ => false, + } + } Expr::LocalGet(id) => { candidates.contains(id) || crate::type_analysis::is_numeric_expr(ctx, expr) } Expr::Binary { left, right, .. } => { - accumulator_rhs_is_numeric(ctx, left, array_id, counter_id, candidates) - && accumulator_rhs_is_numeric(ctx, right, array_id, counter_id, candidates) + accumulator_rhs_is_numeric(ctx, left, array_id, counter_id, offset_reads_inlined, candidates) + && accumulator_rhs_is_numeric(ctx, right, array_id, counter_id, offset_reads_inlined, candidates) } Expr::NumberCoerce(operand) => { - accumulator_rhs_is_numeric(ctx, operand, array_id, counter_id, candidates) + accumulator_rhs_is_numeric(ctx, operand, array_id, counter_id, offset_reads_inlined, candidates) } Expr::Unary { op, operand } => { matches!( op, perry_hir::UnaryOp::Neg | perry_hir::UnaryOp::Pos | perry_hir::UnaryOp::BitNot - ) && accumulator_rhs_is_numeric(ctx, operand, array_id, counter_id, candidates) + ) && accumulator_rhs_is_numeric(ctx, operand, array_id, counter_id, offset_reads_inlined, candidates) } Expr::MathAbs(v) | Expr::MathSqrt(v) @@ -70,15 +98,15 @@ fn accumulator_rhs_is_numeric( | Expr::MathTrunc(v) | Expr::MathSign(v) | Expr::MathFround(v) => { - accumulator_rhs_is_numeric(ctx, v, array_id, counter_id, candidates) + accumulator_rhs_is_numeric(ctx, v, array_id, counter_id, offset_reads_inlined, candidates) } Expr::MathImul(l, r) | Expr::MathPow(l, r) => { - accumulator_rhs_is_numeric(ctx, l, array_id, counter_id, candidates) - && accumulator_rhs_is_numeric(ctx, r, array_id, counter_id, candidates) + accumulator_rhs_is_numeric(ctx, l, array_id, counter_id, offset_reads_inlined, candidates) + && accumulator_rhs_is_numeric(ctx, r, array_id, counter_id, offset_reads_inlined, candidates) } Expr::MathMin(values) | Expr::MathMax(values) => values .iter() - .all(|v| accumulator_rhs_is_numeric(ctx, v, array_id, counter_id, candidates)), + .all(|v| accumulator_rhs_is_numeric(ctx, v, array_id, counter_id, offset_reads_inlined, candidates)), _ => false, } } @@ -228,6 +256,7 @@ pub(super) fn collect_numeric_accumulators( body: &[Stmt], array_id: u32, counter_id: u32, + offset_reads_inlined: bool, ) -> Vec { if !packed_loop_numeric_accumulators_enabled() { return Vec::new(); @@ -255,7 +284,14 @@ pub(super) fn collect_numeric_accumulators( .filter(|id| { !writes[id].iter().all(|write| match write { Some(rhs) => { - accumulator_rhs_is_numeric(ctx, rhs, array_id, counter_id, &candidates) + accumulator_rhs_is_numeric( + ctx, + rhs, + array_id, + counter_id, + offset_reads_inlined, + &candidates, + ) } // `Update` (++/--): ToNumeric(Number) ± 1 is a Number. None => true, diff --git a/crates/perry-codegen/src/stmt/stable_packed_loop.rs b/crates/perry-codegen/src/stmt/stable_packed_loop.rs index 87bb58a80e..61a80d4eca 100644 --- a/crates/perry-codegen/src/stmt/stable_packed_loop.rs +++ b/crates/perry-codegen/src/stmt/stable_packed_loop.rs @@ -1665,7 +1665,9 @@ pub(super) fn lower( // requires `numeric_elements` — without the element proof the accumulator // walk's `array[counter]` leaf has nothing to stand on. let numeric_accumulators = if candidate.numeric_elements { - collect_numeric_accumulators(ctx, body, candidate.array_id, candidate.counter_id) + // The stable-packed tier is left as it was; widening it needs its own + // proof that an offset read lowers inline here. + collect_numeric_accumulators(ctx, body, candidate.array_id, candidate.counter_id, false) } else { Vec::new() }; diff --git a/crates/perry/tests/packed_loop_offset_read_accumulator.rs b/crates/perry/tests/packed_loop_offset_read_accumulator.rs new file mode 100644 index 0000000000..013a783175 --- /dev/null +++ b/crates/perry/tests/packed_loop_offset_read_accumulator.rs @@ -0,0 +1,169 @@ +//! Offset array reads (`a[i ± c]`) earn the packed clone's numeric proof +//! (#9259 follow-up). +//! +//! `accumulator_rhs_is_numeric` required a bare `Expr::LocalGet` index, so +//! `a[k - 1]` was not numeric, the accumulator never earned its number proof, +//! and every `+` in the enclosing expression lowered to a tag-test diamond +//! over `js_dynamic_string_or_number_add`. That cost 41 ms against 8 ms for +//! the same loop without the offset — the shape #9060 and #9091 already fixed +//! for the bare-counter form. +//! +//! **What these tests guard is soundness, not speed.** The perf is a +//! benchmark's job; the risk this change introduces is admitting a read as +//! numeric when the tier does not actually lower it inline. The admission is +//! threaded per tier (`offset_reads_inlined`): the range tier publishes +//! `window_validated` and hole-checks its loads, so an offset read yields a +//! Number; the versioned and stable-packed tiers pass `false`, because their +//! offset reads take the generic path and can produce `undefined`. +//! +//! Every case below puts a value the fast path must NOT treat as a raw double +//! inside the window — an out-of-range index, a hole, a non-numeric element, +//! a string that must concatenate rather than add. If someone later flips the +//! flag on a tier that does not inline offset reads, these are what break. + +use std::path::PathBuf; +use std::process::{Command, Output}; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +fn compile_and_run(source: &str) -> String { + let dir = tempfile::tempdir().expect("tempdir"); + let entry = dir.path().join("main.ts"); + let output = dir.path().join("main_bin"); + std::fs::write(&entry, source).expect("write entry"); + + let compile = Command::new(perry_bin()) + .current_dir(dir.path()) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .env("PERRY_NO_CACHE", "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) + ); + + let run: Output = Command::new(&output) + .current_dir(dir.path()) + .output() + .expect("run compiled binary"); + assert!( + run.status.success(), + "compiled binary failed\nstatus: {:?}\nstdout:\n{}\nstderr:\n{}", + run.status, + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + String::from_utf8_lossy(&run.stdout).trim().to_owned() +} + +const PRELUDE: &str = r#" +const a: number[] = []; +for (let i = 0; i < 64; i++) a.push(i); +"#; + +#[test] +fn an_offset_read_sums_correctly() { + let out = compile_and_run(&format!( + r#"{PRELUDE} +function f(): number {{ + let s = 0; + for (let k = 1; k < 64; k++) s = s + a[k] + a[k - 1]; + return s; +}} +console.log(f()); +"# + )); + assert_eq!(out, "3969"); +} + +#[test] +fn a_negative_index_at_the_window_start_is_not_a_number() { + // k = 0 reads a[-1]. If the offset read were admitted as numeric on a tier + // that does not lower it inline, the generic read's `undefined` would be + // consumed as a raw double instead of poisoning the sum to NaN. + let out = compile_and_run(&format!( + r#"{PRELUDE} +function f(): string {{ + let s = 0; + for (let k = 0; k < 64; k++) s = s + a[k - 1]; + return "neg:" + s; +}} +console.log(f()); +"# + )); + assert_eq!(out, "neg:NaN"); +} + +#[test] +fn an_offset_running_past_the_end_is_not_a_number() { + let out = compile_and_run(&format!( + r#"{PRELUDE} +function f(): string {{ + let s = 0; + for (let k = 0; k < 64; k++) s = s + a[k + 8]; + return "past:" + s; +}} +console.log(f()); +"# + )); + assert_eq!(out, "past:NaN"); +} + +#[test] +fn a_hole_inside_the_offset_window_is_not_a_number() { + let out = compile_and_run( + r#" +function f(): string { + const h: number[] = [1,2,3,4,5,6,7,8]; + delete h[3]; + let s = 0; + for (let k = 1; k < 8; k++) s = s + h[k - 1]; + return "hole:" + s; +} +console.log(f()); +"#, + ); + assert_eq!(out, "hole:NaN"); +} + +#[test] +fn a_non_numeric_element_inside_the_window_still_coerces() { + let out = compile_and_run( + r#" +function f(): string { + const m: any[] = [1,2,"x",4,5,6,7,8]; + let s = 0; + for (let k = 1; k < 8; k++) s = s + m[k - 1]; + return "mixed:" + s; +} +console.log(f()); +"#, + ); + assert_eq!(out, "mixed:3x4567"); +} + +#[test] +fn a_leading_string_element_concatenates_rather_than_adds() { + // The sharpest of these: a wrongly-admitted numeric proof turns `+` into a + // native `fadd`, which cannot concatenate. Node produces a string here. + let out = compile_and_run( + r#" +function f(): string { + const m: any[] = ["a",1,2,3,4,5,6,7]; + let s: any = 0; + for (let k = 1; k < 8; k++) s = s + m[k - 1]; + return "cat:" + s; +} +console.log(f()); +"#, + ); + assert_eq!(out, "cat:0a123456"); +}