From 70be66e57e14bb7f366177c4bf406db45e31f223 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 31 Aug 2026 00:51:46 +0200 Subject: [PATCH] perf(codegen): flush packed accumulators at the throw site, restoring the fast path (7.99 -> 0.95 ns) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parent commit took `throw` back out of the packed-f64 versioned loop because #9185 admitted it without emitting the loop-carried writeback on the unwind edge, which was a silent wrong answer. This puts the fast path back by fixing the actual defect. `PackedAccumulatorScope::finish` covers the fall-through exit and the side-exit trampolines cover a mid-iteration deopt, but both are blocks the clone BRANCHES to, which is how `break` and `continue` reach them. An unwind reaches neither: under invoke-EH `js_throw` leaves for the landing pad from inside the call itself, so a `catch` sees whatever is in the real slot at that moment. `flush_packed_accumulator_locals` emits the stores at the throw site, between lowering the operand (which may read or write an accumulator through the redirect) and the throw. It covers both promoted representations. A `+=` accumulator lives in a DOUBLE alloca; a `c++` counter lives in a separate i32 slot and converts back with `sitofp`. A flush walking only the float table would leave `c` stale while `s` looked right, so the added test asserts both. Unlike `finish` this does not unregister the redirects — lowering continues inside the clone afterwards. Iteration is sorted because the side tables are hash-keyed and IR order must not depend on hash order; __text is byte-identical across repeated builds of the same source. The admission now also checks the thrown operand the way `Stmt::Return` checks its value. #9185 admitted any throw and leaned on `stmt_array_length_effect` to reject the constructing ones, which states the requirement in a weaker place. Measured on the quiet host (Mac mini, load 1.5), 5 reps x 3 runs, stable to 0.01 ns: with_throw 7.99 -> 0.95 ns/op (node 1.10) 8.4x, now 1.16x FASTER than node with_break 0.95 -> 0.95 (node 1.11) no_throw 0.95 -> 0.95 (node 1.10) The tests were verified to guard the flush rather than merely pass beside it: with the flush call disabled, both regression tests fail (`s` reads 0 instead of 780, `c` reads 0 instead of 40) and pass again once restored. Size: __text +192 B (+0.002%). Fixes #9210. Refs #9151, #9185. --- crates/perry-codegen/src/stmt/loops.rs | 74 ++++++++++++++++++- crates/perry-codegen/src/stmt/mod.rs | 7 ++ .../tests/packed_loop_abrupt_statements.rs | 46 +++++++++++- 3 files changed, 120 insertions(+), 7 deletions(-) diff --git a/crates/perry-codegen/src/stmt/loops.rs b/crates/perry-codegen/src/stmt/loops.rs index 84b8aa3a6a..8a57441397 100644 --- a/crates/perry-codegen/src/stmt/loops.rs +++ b/crates/perry-codegen/src/stmt/loops.rs @@ -796,6 +796,68 @@ impl PackedAccumulatorScope { } } +/// Write the fast clone's promoted accumulators back to their real slots +/// WITHOUT ending the redirect scope. +/// +/// `PackedAccumulatorScope::finish` covers the fall-through exit and the +/// side-exit trampolines cover a mid-iteration deopt, but both are blocks the +/// clone BRANCHES to, and `break`/`continue` reach them the same way. An +/// unwind edge reaches neither: under invoke-EH `js_throw` leaves for the +/// landing pad from inside the call itself, so a `catch` observes whatever is +/// in the real slot at that moment. Emitting the stores at the throw site is +/// the only point that works, which is what #9185 was missing. +/// +/// Unlike `finish` this does NOT unregister the redirects: lowering continues +/// inside the clone afterwards, and later statements must keep reading the +/// promoted values. +pub(crate) fn flush_packed_accumulator_locals(ctx: &mut FnCtx<'_>) { + if ctx.numeric_accumulator_f64_slots.is_empty() + && ctx.deferred_integer_update_accumulators.is_empty() + { + return; + } + if ctx.block().is_terminated() { + return; + } + + // The side tables are hash-keyed, so collect and sort before emitting — + // IR order must not depend on hash iteration order. + let mut unboxed: Vec<(u32, String, String)> = ctx + .numeric_accumulator_f64_slots + .iter() + .filter_map(|(id, alloca)| { + ctx.locals + .get(id) + .map(|real_slot| (*id, alloca.clone(), real_slot.clone())) + }) + .collect(); + unboxed.sort_by_key(|(id, _, _)| *id); + for (_, alloca, real_slot) in &unboxed { + // Same argument as `finish`: a genuine double's bits are its nanbox, + // numbers carry no heap edge so no barrier, and leaving the shadow + // state conservative only ever costs an extra root scan. + let value = ctx.block().load(DOUBLE, alloca); + ctx.block().store(DOUBLE, &value, real_slot); + } + + let mut deferred: Vec<(u32, String, String)> = ctx + .deferred_integer_update_accumulators + .iter() + .filter_map(|id| { + let i32_slot = ctx.i32_counter_slots.get(id)?; + let dbl_slot = ctx.locals.get(id)?; + Some((*id, i32_slot.clone(), dbl_slot.clone())) + }) + .collect(); + deferred.sort_by_key(|(id, _, _)| *id); + for (_, i32_slot, dbl_slot) in &deferred { + let blk = ctx.block(); + let value = blk.load(I32, i32_slot); + let as_double = blk.sitofp(I32, &value, DOUBLE); + blk.store(DOUBLE, &as_double, dbl_slot); + } +} + fn emit_packed_numeric_accumulator_admission( ctx: &mut FnCtx<'_>, body: &[Stmt], @@ -5080,9 +5142,15 @@ fn stmt_is_packed_f64_loop_safe( // closure-captured accumulator was also correct, being boxed rather // than register-promoted, which is what kept the bug this narrow. // - // Re-admitting this needs the writeback emitted at the throw site, not - // just the admission; see #9210. - Stmt::Throw(_) => false, + // Admitted again now that `flush_packed_accumulator_locals` emits the + // writeback AT the throw site (#9210). The operand is checked the same + // way `Stmt::Return` checks its value; #9185 admitted any throw at all + // and leaned on `stmt_array_length_effect` to reject the constructing + // ones, which is a weaker guarantee than stating the requirement here. + Stmt::Throw(value) => { + packed_loop_abrupt_enabled() + && expr_is_packed_f64_loop_safe(ctx, value, arr_id, counter_id) + } Stmt::LabeledBreak(_) | Stmt::LabeledContinue(_) | Stmt::While { .. } diff --git a/crates/perry-codegen/src/stmt/mod.rs b/crates/perry-codegen/src/stmt/mod.rs index 33dd3590e9..6550f09bab 100644 --- a/crates/perry-codegen/src/stmt/mod.rs +++ b/crates/perry-codegen/src/stmt/mod.rs @@ -581,6 +581,13 @@ pub(crate) fn lower_stmt(ctx: &mut FnCtx<'_>, stmt: &Stmt) -> Result<()> { // of producing a rejected promise the caller can `.catch()`. Stmt::Throw(expr) => { let val = lower_expr(ctx, expr)?; + // Inside a packed fast clone the loop-carried accumulators live in + // allocas, and an unwind edge does not pass through the exit block + // that writes them back. Flush AFTER lowering the operand (which + // may itself read or write an accumulator through the redirect) and + // BEFORE the throw, since invoke-EH creates the unwind edge inside + // the call. A no-op outside a clone: the side tables are empty. + crate::stmt::loops::flush_packed_accumulator_locals(ctx); if ctx.is_async_fn && ctx.try_depth == 0 { let blk = ctx.block(); let handle = blk.call(crate::types::I64, "js_promise_rejected", &[(DOUBLE, &val)]); diff --git a/crates/perry/tests/packed_loop_abrupt_statements.rs b/crates/perry/tests/packed_loop_abrupt_statements.rs index 5d8c3d84f6..160084905e 100644 --- a/crates/perry/tests/packed_loop_abrupt_statements.rs +++ b/crates/perry/tests/packed_loop_abrupt_statements.rs @@ -196,13 +196,15 @@ fn throw_inside_the_loop_is_still_correct() { assert_eq!(out, "hit15 none2016"); } -/// #9185 admitted `throw` to the packed fast path; this is the case that -/// showed it was a silent wrong answer. +/// #9185 admitted `throw` to the packed fast path and this is the case that +/// showed it was a silent wrong answer; #9210 fixed it properly, so the loops +/// below are on the fast path again AND correct. /// /// `break` and `continue` leave the clone through normal CFG edges, which /// flush the loop-carried locals back to their frame slots. An unwind edge -/// does not, so the `catch` below read `s` from a slot the loop never -/// updated and got its pre-loop `0` instead of `780`. +/// reaches no such block, so the `catch` below read `s` from a slot the loop +/// never updated and got its pre-loop `0` instead of `780`. The fix emits the +/// writeback at the throw site; this test is that writeback's only guard. /// /// Every assertion here reads a loop-carried local AFTER the abrupt exit, /// which is precisely what #9185's own tests did not do. The `break` and @@ -256,3 +258,39 @@ fn a_loop_carried_local_survives_a_taken_throw() { "break 780 | continue 1024 | throwBefore 780 | throwAfter 820 | closure 780" ); } + +/// Both accumulator representations must survive the throw, not just the +/// float one. +/// +/// A `+=` accumulator lives in a `DOUBLE` alloca, but a `c++` counter is +/// promoted to a separate i32 slot and only converted back with `sitofp`. +/// Those are two distinct writeback paths in the clone, and the flush at the +/// throw site has to cover both — a fix that walked only the float table +/// would leave `c` reading `0` here while `s` looked correct. +#[test] +fn both_accumulator_kinds_survive_a_taken_throw() { + let out = compile_and_run(&format!( + "{PRELUDE} + const PRE = new Error(\"boom\"); + function bothAccumulators(): string {{ + let c = 0; let s = 0; + try {{ + for (let i = 0; i < arr.length; i++) {{ + if (arr[i] === 40) throw PRE; + c++; s += arr[i]; + }} + }} catch (e) {{ return \"c=\" + c + \" s=\" + s; }} + return \"none c=\" + c + \" s=\" + s; + }} + function counterOnly(): string {{ + let c = 0; + try {{ + for (let i = 0; i < arr.length; i++) {{ if (arr[i] === 17) throw PRE; c++; }} + }} catch (e) {{ return \"c=\" + c; }} + return \"none c=\" + c; + }} + console.log(bothAccumulators() + \" | \" + counterOnly()); + " + )); + assert_eq!(out, "c=40 s=780 | c=17"); +}