From 3ff018f7d04ad8335401442fe5d042f17fa99e0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 31 Aug 2026 14:51:18 +0200 Subject: [PATCH 1/2] codegen: admit `if (...) c++` bodies in the packed-f64 range tier (#9275) The range tier rejected any conditional body, so a conditional-count loop got a packed clone only when its bound was spelled `arr.length`. Identical work, 4ms with an `arr.length` bound and 31ms with a literal one -- the difference between beating node by 2.5x and losing to it by 3.1x. Two walkers, and an `if` died in a different place in each. 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 hits 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. That asymmetry is the defect. The arm goes in the DENSE walk, and only there. 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 exactly why it insists on a single statement whose one side effect happens last. That reasoning is written into 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, a window only reachable on the untaken branch can cost the clone and never correctness. `break`/`continue` inside a branch stay rejected: dense's guarantee is that a whole iteration runs in one copy, and an early exit out of a branch is a shape this walk has not reasoned about. A test pins that it still compiles and returns node's answer through the generic path. Measured, self-timed min of 5, 4096-element array, every timing paired with a `packed_f64.*` block count from the emitted IR: bound body blocks before after node k < 4096 c += a[k] 12 -> 12 8ms 8ms 9ms k < 4096 if (a[k] > 0) c++ 0 -> 18 31ms 12ms 10ms k < 4096 if (a[k] > a[k-1]) c++ 0 -> 18 47ms 25ms 10ms k < a.len if (a[k] > 0) c++ 10 -> 10 4ms 4ms 9ms Node-identical output on every fixture. The two unchanged rows are controls: the first shares the literal bound and always had a clone, so the zeros above were attributable to the body shape; the last is the versioned tier, untouched. Known boundary, pinned by a test rather than left to be rediscovered: a float accumulator whose RHS reads a tracked array (`c += a[k]`) is still not admitted by the dense walk. That is not the conditional -- a plain `c += a[k]; c += 1.0;` two-statement body is rejected identically, with no `if` involved -- but the numeric proof the accumulator needs, which lives in the accumulator-admission path and is being widened separately. Claude-Session: https://claude.ai/code/session_01Pcq6j6y57TdKSR2Zx2D187 --- crates/perry-codegen/src/stmt/loops.rs | 94 ++++++++- .../issue_9275_range_conditional_body.rs | 198 ++++++++++++++++++ 2 files changed, 288 insertions(+), 4 deletions(-) create mode 100644 crates/perry/tests/issue_9275_range_conditional_body.rs diff --git a/crates/perry-codegen/src/stmt/loops.rs b/crates/perry-codegen/src/stmt/loops.rs index 0d3750ca52..fc271d3eb9 100644 --- a/crates/perry-codegen/src/stmt/loops.rs +++ b/crates/perry-codegen/src/stmt/loops.rs @@ -1634,8 +1634,36 @@ fn packed_f64_range_loop_dense_body_collect( bound_local: Option, accesses: &mut std::collections::BTreeMap, ) -> bool { - use perry_hir::Expr; let mut written: std::collections::HashSet = 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, + accesses: &mut std::collections::BTreeMap, + written: &mut std::collections::HashSet, +) -> 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 diff --git a/crates/perry/tests/issue_9275_range_conditional_body.rs b/crates/perry/tests/issue_9275_range_conditional_body.rs new file mode 100644 index 0000000000..c248a4107a --- /dev/null +++ b/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); + } +} From 4e34d94f75b0b70ea4f2d3e101af17310a4d1071 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 31 Aug 2026 15:12:31 +0200 Subject: [PATCH 2/2] changelog: fragment for #9288 Claude-Session: https://claude.ai/code/session_01Pcq6j6y57TdKSR2Zx2D187 --- .../9288-range-tier-conditional-body.md | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 changelog.d/9288-range-tier-conditional-body.md diff --git a/changelog.d/9288-range-tier-conditional-body.md b/changelog.d/9288-range-tier-conditional-body.md new file mode 100644 index 0000000000..ba6267264b --- /dev/null +++ b/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.