codegen: admit if (...) c++ bodies in the packed-f64 range tier (#9275)

changelog.d/9288-range-tier-conditional-body.md

@@ -0,0 +1,51 @@
+**A conditional-count loop now gets a packed clone whatever its bound is spelled**
+(#9275). The packed-f64 range tier rejected any `if (...) c++` body, so
+`for (k = 1; k < N; k++) if (a[k] > a[k-1]) c++` was compiled two entirely
+different ways depending on whether `N` was written `4096` or `a.length`: 31 ms
+against 4 ms for identical work, which is the difference between losing to node
+by 3.1x and beating it by 2.5x.
+
+An `if` died in a different place in each of the tier's two walkers. The
+single-statement walker opens `let [Stmt::Expr(expr)] = body else { return
+false }`, so a `Stmt::If` fails the destructure before any arm runs; the dense
+walker has arms for `Let`, `LocalSet`, `Update` and a generic `Expr`, and no
+`Stmt::If`, so it reached the trailing `_ => return false`. The versioned tier
+meanwhile accepts the shape at `stmt_is_packed_f64_loop_safe`'s `Stmt::If` arm,
+which recurses condition and both branches — two tiers with different admission
+power, and which one claims the loop decided by how the bound was written. It is
+the mirror image of #9259, where the `arr.length` spelling was the slow one.
+
+The new arm goes in the dense walk and only there, because dense mode's own
+safety argument carries over unchanged: its loads have no side exits, so an
+iteration runs entirely in the fast copy or entirely in the slow one and a
+branch cannot leave a half-applied iteration behind. The classic mode cannot
+take this — it permits a hole-read side exit that re-executes the iteration,
+which is precisely why it insists on a single statement whose one side effect
+happens last. That reasoning now lives in the arm, so the classic walker does
+not get "fixed" the same way later.
+
+`written` and `accesses` are threaded into the branch walks rather than rebuilt
+per branch: a scalar assigned inside a branch still has to shadow-check against
+the tracked arrays, and the tail check runs once on the merged set. Reads from
+both branches are recorded, so the entry guard validates the union of the
+windows rather than the taken path's — conservative in the safe direction, since
+a window only reachable on the untaken branch can cost the clone and never
+correctness. `break` and `continue` inside a branch stay rejected, with a test
+pinning that such a loop still compiles and returns node's answer through the
+generic path.
+
+Measured on a 4096-element array, self-timed min of 5, every timing paired with
+a `packed_f64.*` block count from the emitted IR: `if (a[k] > 0) c++` under a
+literal bound goes 31 ms to 12 ms (0 packed blocks to 18), and the offset form
+`if (a[k] > a[k-1]) c++` goes 47 ms to 25 ms. The literal-bounded `c += a[k]`
+body and the `arr.length`-bounded conditional are unchanged controls at 8 ms and
+4 ms, which is what makes the zeros above attributable to the body shape rather
+than to the bound. Output is node-identical on every fixture.
+
+The range tier's clone is still around 3x the versioned tier's for the same
+body, so this narrows the spelling gap from 7.75x to 3x rather than closing it.
+A float accumulator whose RHS reads a tracked array (`c += a[k]`) also remains
+unadmitted by the dense walk — not because of the conditional, since a plain
+`c += a[k]; c += 1.0;` body is rejected identically, but because `c + a[k]` can
+lower to a dynamic add and the accumulator needs a numeric proof this walk does
+not have. Both are recorded rather than left to be rediscovered.

crates/perry-codegen/src/stmt/loops.rs

@@ -1634,8 +1634,36 @@ fn packed_f64_range_loop_dense_body_collect(
     bound_local: Option<u32>,
     accesses: &mut std::collections::BTreeMap<u32, PackedF64RangeArrayAccess>,
 ) -> bool {
-    use perry_hir::Expr;
     let mut written: std::collections::HashSet<u32> = std::collections::HashSet::new();
+    packed_f64_range_loop_dense_stmts_collect(
+        ctx,
+        body,
+        counter_id,
+        bound_local,
+        accesses,
+        &mut written,
+    )
+        // Written arrays are allowed (masked stores above); a scalar `let`/set
+        // shadowing a tracked array id still rejects.
+        && !accesses.is_empty()
+        && accesses.keys().all(|arr_id| !written.contains(arr_id))
+}
+
+/// The statement walk behind [`packed_f64_range_loop_dense_body_collect`],
+/// split out so a conditional's branches can recurse into it.
+///
+/// `written` is threaded rather than rebuilt per branch: a scalar assigned
+/// inside an `if` still shadows a tracked array id for the caller's
+/// disjointness check, and rebuilding it per branch would lose that.
+fn packed_f64_range_loop_dense_stmts_collect(
+    ctx: &FnCtx<'_>,
+    body: &[Stmt],
+    counter_id: u32,
+    bound_local: Option<u32>,
+    accesses: &mut std::collections::BTreeMap<u32, PackedF64RangeArrayAccess>,
+    written: &mut std::collections::HashSet<u32>,
+) -> bool {
+    use perry_hir::Expr;
     for stmt in body {
         match stmt {
             Stmt::Let {
@@ -1711,12 +1739,70 @@ fn packed_f64_range_loop_dense_body_collect(
                     return false;
                 }
             }
+            // #9275: a conditional whose branches are themselves admitted
+            // scalar statements. The versioned tier already accepts this shape
+            // (`expr_is_packed_f64_loop_safe` recurses through `Expr::Compare`,
+            // and integer `c++` accumulators admit independently), so
+            // `for (k = 1; k < N; k++) if (a[k] > a[k-1]) c++` got a packed
+            // clone when its bound was written `arr.length` and none when it
+            // was written as a literal — 4ms against 31ms for identical work.
+            //
+            // Dense mode is where this belongs, and its own safety argument
+            // carries over unchanged: the fast loop's loads have no side
+            // exits, so an iteration runs entirely in the fast copy or
+            // entirely in the slow one, and a branch cannot leave a
+            // half-applied iteration behind. The classic mode above cannot
+            // take this — it permits a hole-read side exit that re-executes
+            // the iteration, which is why it insists on a single statement
+            // whose one side effect happens last.
+            //
+            // Reads from BOTH branches are recorded, so the entry guard
+            // validates the union of the windows rather than the taken path's.
+            // That is the conservative direction: a window only reachable on
+            // the untaken branch can make the guard decline a loop it would
+            // have run correctly, costing the clone and never correctness.
+            Stmt::If {
+                condition,
+                then_branch,
+                else_branch,
+            } => {
+                if !masked_window_expression_is_non_collecting(ctx, condition)
+                    || !packed_f64_range_loop_pure_expr_collect(
+                        condition, counter_id, true, accesses,
+                    )
+                {
+                    return false;
+                }
+                if !packed_f64_range_loop_dense_stmts_collect(
+                    ctx,
+                    then_branch,
+                    counter_id,
+                    bound_local,
+                    accesses,
+                    written,
+                ) {
+                    return false;
+                }
+                if let Some(else_branch) = else_branch {
+                    if !packed_f64_range_loop_dense_stmts_collect(
+                        ctx,
+                        else_branch,
+                        counter_id,
+                        bound_local,
+                        accesses,
+                        written,
+                    ) {
+                        return false;
+                    }
+                }
+            }
+            // `break` / `continue` stay rejected here: dense mode's guarantee
+            // is that the whole iteration runs in one copy, and an early exit
+            // out of a branch is a shape this walk has not reasoned about.
             _ => return false,
         }
     }
-    // Written arrays are allowed (masked stores above); a scalar `let`/set
-    // shadowing a tracked array id still rejects.
-    !accesses.is_empty() && accesses.keys().all(|arr_id| !written.contains(arr_id))
+    true
 }
 
 /// Match-time twin of `masked_window::masked_store_rhs_is_genuine_f64`: at

crates/perry/tests/issue_9275_range_conditional_body.rs

@@ -0,0 +1,198 @@
+//! Regression coverage for #9275: the packed-f64 RANGE tier rejected any
+//! `if (...) counter++` body, so a conditional-count loop got a fast clone
+//! only when its bound was spelled `arr.length`.
+//!
+//! The versioned tier already accepts the shape — `expr_is_packed_f64_loop_safe`
+//! recurses through `Expr::Compare`, and integer `c++` accumulators admit
+//! independently — so the identical body was 4 ms with an `arr.length` bound
+//! and 31 ms with a literal one, which is the difference between beating node
+//! by 2.5x and losing to it by 3.1x.
+//!
+//! The fix adds a `Stmt::If` arm to the DENSE range walk, and dense is the
+//! mode where it belongs: its loads have no side exits, so an iteration runs
+//! entirely in the fast copy or entirely in the slow one and a branch cannot
+//! leave a half-applied iteration behind. The classic range mode cannot take
+//! this — it permits a hole-read side exit that re-executes the iteration,
+//! which is exactly why it insists on a single statement whose one side effect
+//! happens last.
+//!
+//! These tests pin the admission and the answers, not a timing.
+
+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(),
+    )
+}
+
+fn packed_blocks(stderr: &str) -> usize {
+    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")
+        .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);
+}
+
+fn source(body: &str) -> String {
+    format!(
+        r#"
+function run(a: number[]): number {{
+  let c = 0.0;
+  for (let r = 0; r < 20; r++) {{
+    for (let k = 1; k < 512; k++) {{
+      {body}
+    }}
+  }}
+  return c;
+}}
+const a: number[] = [];
+for (let i = 0; i < 512; i++) a.push((i * 37) % 100);
+console.log(run(a));
+"#
+    )
+}
+
+/// The literal-bounded accumulate body always got a clone. It is the positive
+/// control: it shares the bound with the conditional cases below, so a zero
+/// there is attributable to the body shape and not to the bound.
+#[test]
+fn the_literal_bound_accumulate_body_still_gets_a_clone() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    let (_, stderr) = compile(dir.path(), &source("c += a[k];"));
+    assert!(
+        packed_blocks(&stderr) > 0,
+        "positive control: a literal-bounded `c += a[k]` must keep its packed clone"
+    );
+}
+
+#[test]
+fn a_literal_bounded_conditional_count_gets_a_clone() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    let (_, stderr) = compile(dir.path(), &source("if (a[k] > 50.0) c++;"));
+    assert!(
+        packed_blocks(&stderr) > 0,
+        "#9275: the range tier rejected any `if (...) c++` body, so this loop got no \
+         packed clone at all while the identical body with an `arr.length` bound did \
+         — 31ms against 4ms"
+    );
+}
+
+/// The offset form too: the conditional is what was rejected, not the index.
+#[test]
+fn a_literal_bounded_conditional_over_an_offset_read_gets_a_clone() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    let (_, stderr) = compile(dir.path(), &source("if (a[k] > a[k - 1]) c++;"));
+    assert!(
+        packed_blocks(&stderr) > 0,
+        "#9275: offset form of the same shape"
+    );
+}
+
+/// An `if`/`else` where both branches write. The condition carries an offset
+/// read, so the entry guard validates the whole window the statement touches.
+#[test]
+fn both_branches_are_admitted_and_agree_with_the_generic_path() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    let src = source("if (a[k] > a[k - 1]) { c++; } else { c--; }");
+    let (bin, stderr) = compile(dir.path(), &src);
+    assert!(
+        packed_blocks(&stderr) > 0,
+        "if/else body should be admitted"
+    );
+    for moving_gc in [false, true] {
+        assert_stdout(&run(&bin, dir.path(), moving_gc), "2660\n", moving_gc);
+    }
+}
+
+/// The boundary of this change, pinned deliberately.
+///
+/// A float accumulator whose RHS reads a tracked array (`c += a[k]`) is NOT
+/// admitted by the dense walk — and was not before this change either, which
+/// is checkable without an `if` at all: a plain two-statement body
+/// `c += a[k]; c += 1.0;` is rejected by dense mode on `main` and still is.
+/// The reason is not the conditional: `c + a[k]` can lower to a dynamic add,
+/// which is a collecting call, so the accumulator needs a numeric proof this
+/// walk does not have. That proof lives in the accumulator-admission path and
+/// is being widened separately.
+///
+/// This test exists so the limitation is recorded as a known boundary rather
+/// than rediscovered as a bug, and so that whoever widens the accumulator
+/// proof sees a case that should start getting a clone. The answer must be
+/// correct either way, via the generic path.
+#[test]
+fn a_float_accumulator_reading_the_array_is_not_admitted_but_is_correct() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    let src = source("if (a[k] > 50.0) { c += a[k]; }");
+    let (bin, _) = compile(dir.path(), &src);
+    for moving_gc in [false, true] {
+        assert_stdout(&run(&bin, dir.path(), moving_gc), "375180\n", moving_gc);
+    }
+}
+
+/// A `break` inside the branch is deliberately NOT admitted: dense mode's
+/// guarantee is that a whole iteration runs in one copy, and an early exit out
+/// of a branch is a shape the walk has not reasoned about. It must still
+/// compile and produce the right answer via the generic path.
+#[test]
+fn a_break_inside_the_branch_stays_on_the_generic_path_and_is_correct() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    let src = source("if (a[k] > 98.0) { break; } c += 1.0;");
+    let (bin, _) = compile(dir.path(), &src);
+    for moving_gc in [false, true] {
+        assert_stdout(&run(&bin, dir.path(), moving_gc), "520\n", moving_gc);
+    }
+}