perf(codegen): flush packed accumulators at the throw site, restoring the fast path (7.99 → 0.95 ns)

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 { .. }

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)]);

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