codegen: keep the packed clone when a length-bounded body reads a[k ± c] (#9259)

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.

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<String> {
                 // 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,
                             ));
                         }
                     }

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))
 }

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(

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);
+    }
+}