diff --git a/changelog.d/7076-loop-purity-whitelist.md b/changelog.d/7076-loop-purity-whitelist.md new file mode 100644 index 0000000000..94354e2bcc --- /dev/null +++ b/changelog.d/7076-loop-purity-whitelist.md @@ -0,0 +1,36 @@ +### Fixed / Performance + +**Pure numeric loops no longer emit a GC back-edge poll they cannot need.** +`loop_may_allocate` (`crates/perry-codegen/src/loop_purity.rs`) decides, per loop +back edge, whether a `js_gc_loop_safepoint()` call has to be emitted to drain a +deferred minor collection. Its whitelist omitted relational comparisons, +arithmetic `Binary` and `Update`, so `for (let i = 0; i < n; i++) { sum = sum + 1; }` +failed the purity test on its *condition*, its *body* and its *update* — three +runtime calls per iteration in a loop that allocates nothing. + +Measured on a Raspberry Pi 5 (Cortex-A76, 2.400 GHz verified before and after via +`vcgencmd measure_clock arm`, idle), 12 interleaved reps under `perf stat`: +**12,604,634,901 → 252,268,002 instructions retired (49.97x, cv 0.00%/0.01%)**, +8.87x cycles, 9.93x wall. + +The widening reuses `expr_is_inert_primitive` (#6975) rather than growing a second +predicate answering the same question. Those operators run ToPrimitive / ToNumeric, +and a user-defined `valueOf` is arbitrary JS that allocates and collects — recursing +into the operands never sees that, since two plain `LocalGet`s recurse clean while +the *operator* calls into user code. They are alloc-free only when every operand is +a proven non-pointer primitive. `Add` additionally requires +`expr_is_known_non_pointer_shadow_value` on both operands, because it is the only +operator whose result can be a fresh heap value: a string *literal* is inert, and +`"a" + "b"` still allocates. + +`expr_is_inert_primitive` also gained one restriction, in the safe direction for +`expr_may_trigger_gc` as well: `local_is_inert_primitive` refuses module-level +globals. `local_types` and `shadow_slot_map` are computed per function from that +function's body alone, and a module global can be assigned an object by a +different function the scan never sees. + +Regression coverage is 13 unit tests in `loop_purity.rs` plus 7 IR tests in +`crates/perry-codegen/tests/loop_safepoint_purity.rs`, and was sabotage-verified +in both directions: breaking the implementation five ways turns exactly the +intended tests red, and the one guard no fixture can isolate is documented as +such in the test header rather than claimed as covered. diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index b1a11e9deb..0934f0aba6 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -157,42 +157,94 @@ pub(crate) fn expr_may_trigger_gc(ctx: &FnCtx<'_>, expr: &Expr) -> bool { /// A local carrying an object — or one with a reserved shadow slot, which /// means it is pointer-possible regardless of its refined type — is not inert, /// because `ToPrimitive` on it dispatches to whatever the object defines. -fn expr_is_inert_primitive(ctx: &FnCtx<'_>, expr: &Expr) -> bool { +/// +/// Also the whitelist behind the loop back-edge poll +/// (`crate::loop_purity::loop_may_allocate`): "can evaluating this run user +/// code or allocate?" is the same question there, so the answer comes from +/// here rather than from a second copy that drifts. +pub(crate) fn expr_is_inert_primitive(ctx: &FnCtx<'_>, expr: &Expr) -> bool { match expr { Expr::Undefined | Expr::Null | Expr::Bool(_) | Expr::Number(_) | Expr::Integer(_) => true, // A heap value, but ToPrimitive on a string is the identity: no user - // code, no allocation. (`+` is excluded below, since concatenation + // code, no allocation. (`+` is restricted below, since concatenation // does allocate.) Expr::String(_) => true, - Expr::LocalGet(id) => { - !ctx.shadow_slot_map.contains_key(id) - && matches!( - ctx.local_types.get(id), - Some( - HirType::Number - | HirType::Int32 - | HirType::Boolean - | HirType::Null - | HirType::Void - | HirType::Never - ) - ) - } + Expr::LocalGet(id) => local_is_inert_primitive(ctx, *id), + // `++` / `--` on an inert local runs ToNumeric over a value that is + // already a non-pointer primitive, then a numeric add and a store: no + // user code, no allocation. (`x++` on a BigInt DOES allocate a fresh + // BigInt — but `HirType::BigInt` is not in the inert set, and a + // BigInt-typed local is pointer-typed, so it also has a shadow slot.) + // + // [`expr_may_trigger_gc`] deliberately does not route `Update` here and + // keeps it on the conservative catch-all: #6951's question is about + // operand lists, where an embedded `Update` is vanishingly rare. The + // loop-poll caller is the one that needs it (`for (…; …; i++)`). + Expr::Update { id, .. } => local_is_inert_primitive(ctx, *id), Expr::Unary { operand, .. } => expr_is_inert_primitive(ctx, operand), Expr::Compare { left, right, .. } => { expr_is_inert_primitive(ctx, left) && expr_is_inert_primitive(ctx, right) } - // `+` allocates whenever it is a concatenation, so it is never inert - // even over two string literals. Expr::Binary { op, left, right } => { - !matches!(op, perry_hir::BinaryOp::Add) - && expr_is_inert_primitive(ctx, left) + expr_is_inert_primitive(ctx, left) && expr_is_inert_primitive(ctx, right) + // `+` is the one operator whose RESULT can be a fresh heap + // value: with a string operand it concatenates, and that + // allocates. Inert operands alone do not rule that out — + // `Expr::String` is inert — so `Add` additionally demands that + // neither operand can BE a heap reference, which is exactly + // `expr_is_known_non_pointer_shadow_value`. Two operands that + // provably hold no pointer cannot be strings, so the `+` is a + // numeric add and allocates nothing. + && (!matches!(op, perry_hir::BinaryOp::Add) + || (super::expr_is_known_non_pointer_shadow_value(ctx, left) + && super::expr_is_known_non_pointer_shadow_value(ctx, right))) } _ => false, } } +/// [`expr_is_inert_primitive`] for a bare local id — the shared half of its +/// `LocalGet` and `Update` arms. +/// +/// Three independent facts have to line up, and none alone is enough: +/// +/// * the refined type is a non-pointer primitive, so `ToPrimitive` on it is +/// the identity and dispatches to nothing; +/// * no shadow slot is reserved for the local — `collect_pointer_typed_locals`' +/// verdict that the local is not pointer-typed. A reserved slot means +/// pointer-possible regardless of what the refined type says; and +/// * the binding is not a module-level global. `local_types` and the +/// shadow-slot map are both computed per function, from that function's body +/// alone, so a module global that a *different* function assigns an object +/// to still looks like a number here. Those per-function facts are sound for +/// a genuine local and not for a global, so a global is never inert. +/// +/// What this does NOT defend against is a *lying annotation*: `let n: number` +/// that is handed an object anyway. Nothing here catches that — but nothing +/// else in the compiler does either, and it is not this predicate's assumption +/// to make good on. `collect_pointer_typed_locals` reserves root slots from the +/// same declared type, so such a local has no shadow slot and the precise scan +/// cannot see the object at all; the value is already unrooted long before any +/// coercion of it reaches a poll decision. Honesty of scalar annotations is a +/// standing invariant of the precise-root design, inherited here rather than +/// introduced. +pub(crate) fn local_is_inert_primitive(ctx: &FnCtx<'_>, id: u32) -> bool { + !ctx.shadow_slot_map.contains_key(&id) + && !ctx.module_globals.contains_key(&id) + && matches!( + ctx.local_types.get(&id), + Some( + HirType::Number + | HirType::Int32 + | HirType::Boolean + | HirType::Null + | HirType::Void + | HirType::Never + ) + ) +} + /// Does any expression after index `i` reach a collection point? /// /// This is the gate for protecting value `i`: a value that nothing allocating diff --git a/crates/perry-codegen/src/loop_purity.rs b/crates/perry-codegen/src/loop_purity.rs index 57faed9853..5fdc25a335 100644 --- a/crates/perry-codegen/src/loop_purity.rs +++ b/crates/perry-codegen/src/loop_purity.rs @@ -54,16 +54,31 @@ pub(crate) fn body_needs_asm_barrier(body: &[Stmt]) -> bool { /// True when the loop body may allocate (or otherwise call into the runtime and /// trip a GC), so a moving-GC back-edge poll (`js_gc_loop_safepoint`) must be -/// emitted to drain any deferred minor. Reuses the LLVM-purity whitelist: a body -/// that is fully LLVM-pure performs no call / allocation / heap mutation, so it -/// can never cross a nursery trigger and defer a collection — the poll would be -/// a guaranteed no-op that only defeats vectorization. Conservative in the SAFE -/// direction: anything not provably pure is treated as "may allocate" and gets -/// the poll. A spurious poll costs a little vectorization; a missing one only -/// delays a deferred minor to the next safepoint (bounded by the moving-GC hard -/// cap) — never a correctness or UAF hazard. -pub(crate) fn loop_may_allocate(body: &[Stmt], controls: &[&Expr]) -> bool { - !body.iter().all(stmt_alloc_free) || controls.iter().any(|expr| !expr_alloc_free(expr)) +/// emitted to drain any deferred minor. A loop that provably performs no call, +/// allocation or heap mutation can never cross a nursery trigger and defer a +/// collection — the poll would be a guaranteed no-op that only defeats +/// vectorization. +/// +/// **One-sided, and the direction matters.** `false` must mean "provably cannot +/// allocate": a loop that emits no poll never yields to the collector, so a +/// wrong `false` on a loop that *can* allocate leaves it spinning with a +/// deferred collection undrained. Anything not provably alloc-free stays `true` +/// and gets its poll; a spurious poll only costs some vectorization. +/// +/// `is_inert` answers "can evaluating this expression run user code or +/// allocate?" for the coercing operators. In production it is +/// [`crate::expr::temp_root::expr_is_inert_primitive`] — the predicate #6975 +/// introduced for the argument-rooting decision, because it answers exactly +/// this question and two copies of it would drift. It is injected rather than +/// called directly so this module stays free of `FnCtx` and both directions of +/// its answer stay unit-testable. +pub(crate) fn loop_may_allocate( + body: &[Stmt], + controls: &[&Expr], + is_inert: &dyn Fn(&Expr) -> bool, +) -> bool { + !body.iter().all(|s| stmt_alloc_free(s, is_inert)) + || controls.iter().any(|expr| !expr_alloc_free(expr, is_inert)) } /// Like `stmt_is_pure`, but the question is narrower — "can this allocate (or @@ -72,28 +87,37 @@ pub(crate) fn loop_may_allocate(body: &[Stmt], controls: &[&Expr]) -> bool { /// numeric updates never allocate, and typed-array element WRITES store into a /// fixed-size backing buffer that never grows. Generic `IndexSet` is NOT /// accepted: a plain JS-array index write can grow (reallocate) the backing -/// store. This lets a `for (…) acc += arr[i]` reduction stay poll-free (LLVM can -/// vectorize) while `keep.push({…})` (a Call) still gets its poll. -fn stmt_alloc_free(s: &Stmt) -> bool { +/// store, while `keep.push({…})` (a Call) still gets its poll. +/// +/// Note that a `for (…) acc += arr[i]` REDUCTION is not yet covered end to end: +/// the element read is alloc-free on its own, but the `+` that consumes it goes +/// through `is_inert`, which does not admit `BufferIndexGet` / `Uint8ArrayGet`. +/// Admitting them is a real follow-up — a typed-array element read is a number +/// by construction (#6996) — but it needs its own soundness argument for the +/// dynamic-key lowerings that fall through to property lookup, so it is not +/// bundled in here. +fn stmt_alloc_free(s: &Stmt, is_inert: &dyn Fn(&Expr) -> bool) -> bool { match s { - Stmt::Expr(e) => expr_alloc_free(e), - Stmt::Let { init, .. } => init.as_ref().is_none_or(expr_alloc_free), + Stmt::Expr(e) => expr_alloc_free(e, is_inert), + Stmt::Let { init, .. } => init.as_ref().is_none_or(|e| expr_alloc_free(e, is_inert)), Stmt::If { condition, then_branch, else_branch, } => { - expr_alloc_free(condition) - && then_branch.iter().all(stmt_alloc_free) + expr_alloc_free(condition, is_inert) + && then_branch.iter().all(|s| stmt_alloc_free(s, is_inert)) && else_branch .as_ref() - .is_none_or(|b| b.iter().all(stmt_alloc_free)) + .is_none_or(|b| b.iter().all(|s| stmt_alloc_free(s, is_inert))) } Stmt::While { condition, body } => { - expr_alloc_free(condition) && body.iter().all(stmt_alloc_free) + expr_alloc_free(condition, is_inert) + && body.iter().all(|s| stmt_alloc_free(s, is_inert)) } Stmt::DoWhile { body, condition } => { - expr_alloc_free(condition) && body.iter().all(stmt_alloc_free) + expr_alloc_free(condition, is_inert) + && body.iter().all(|s| stmt_alloc_free(s, is_inert)) } Stmt::For { init, @@ -101,18 +125,20 @@ fn stmt_alloc_free(s: &Stmt) -> bool { update, body, } => { - init.as_deref().is_none_or(stmt_alloc_free) - && condition.as_ref().is_none_or(expr_alloc_free) - && update.as_ref().is_none_or(expr_alloc_free) - && body.iter().all(stmt_alloc_free) + init.as_deref().is_none_or(|s| stmt_alloc_free(s, is_inert)) + && condition + .as_ref() + .is_none_or(|e| expr_alloc_free(e, is_inert)) + && update.as_ref().is_none_or(|e| expr_alloc_free(e, is_inert)) + && body.iter().all(|s| stmt_alloc_free(s, is_inert)) } - Stmt::Labeled { body, .. } => stmt_alloc_free(body), + Stmt::Labeled { body, .. } => stmt_alloc_free(body, is_inert), Stmt::Break | Stmt::Continue | Stmt::LabeledBreak(_) | Stmt::LabeledContinue(_) => true, _ => false, } } -fn expr_alloc_free(e: &Expr) -> bool { +fn expr_alloc_free(e: &Expr, is_inert: &dyn Fn(&Expr) -> bool) -> bool { match e { Expr::Undefined | Expr::Null @@ -130,8 +156,12 @@ fn expr_alloc_free(e: &Expr) -> bool { // Typed/buffer reads are fixed-layout numeric loads. Generic // IndexGet/IndexUpdate are deliberately excluded: proxies, accessors, // and coercion hooks can run user code and allocate. - Expr::BufferIndexGet { buffer, index } => expr_alloc_free(buffer) && expr_alloc_free(index), - Expr::Uint8ArrayGet { array, index } => expr_alloc_free(array) && expr_alloc_free(index), + Expr::BufferIndexGet { buffer, index } => { + expr_alloc_free(buffer, is_inert) && expr_alloc_free(index, is_inert) + } + Expr::Uint8ArrayGet { array, index } => { + expr_alloc_free(array, is_inert) && expr_alloc_free(index, is_inert) + } // Typed-array element WRITES store into a fixed-size backing buffer that // never grows/reallocates. (Generic `IndexSet` is deliberately absent — // a plain JS-array write can grow the array and allocate.) @@ -139,30 +169,64 @@ fn expr_alloc_free(e: &Expr) -> bool { buffer, index, value, - } => expr_alloc_free(buffer) && expr_alloc_free(index) && expr_alloc_free(value), + } => { + expr_alloc_free(buffer, is_inert) + && expr_alloc_free(index, is_inert) + && expr_alloc_free(value, is_inert) + } Expr::Uint8ArraySet { array, index, value, - } => expr_alloc_free(array) && expr_alloc_free(index) && expr_alloc_free(value), - Expr::LocalSet(_, val) => expr_alloc_free(val), + } => { + expr_alloc_free(array, is_inert) + && expr_alloc_free(index, is_inert) + && expr_alloc_free(value, is_inert) + } + Expr::LocalSet(_, val) => expr_alloc_free(val, is_inert), + // Strict `===` / `!==` never coerce, and `&&` / `||` / `??` only run + // ToBoolean, which on an object is a tag test — no user code on either. + // So these stay open to operands of ANY type, which is strictly more + // than `is_inert` would admit. Expr::Compare { op: CompareOp::Eq | CompareOp::Ne, left, right, } - | Expr::Logical { left, right, .. } => expr_alloc_free(left) && expr_alloc_free(right), + | Expr::Logical { left, right, .. } => { + expr_alloc_free(left, is_inert) && expr_alloc_free(right, is_inert) + } + // Same story: `!x` is ToBoolean, `typeof x` reads a tag, `void x` + // discards. None of them reach a user-defined conversion. Expr::Unary { op: UnaryOp::Not, operand, } | Expr::TypeOf(operand) - | Expr::Void(operand) => expr_alloc_free(operand), + | Expr::Void(operand) => expr_alloc_free(operand, is_inert), + // The COERCING operators: relational and loose comparisons, arithmetic + // and bitwise `Binary`, the remaining `Unary` forms (`-x`, `+x`, `~x`) + // and `x++` / `x--` all run ToPrimitive / ToNumeric on their operands, + // and a user-defined `valueOf` / `Symbol.toPrimitive` / `toString` is + // arbitrary JS: it allocates, and it collects. Recursing into the + // operands does NOT see that — `a < b` over two plain locals recurses + // clean while the comparison itself can call into user code, which is + // the hole #6975 closed one abstraction over. So these are alloc-free + // only when `is_inert` proves every operand is a non-pointer primitive + // that ToPrimitive cannot dispatch on. `+` additionally has to rule out + // concatenation, which `is_inert` does. + Expr::Compare { .. } | Expr::Binary { .. } | Expr::Unary { .. } | Expr::Update { .. } => { + is_inert(e) + } Expr::Conditional { condition, then_expr, else_expr, - } => expr_alloc_free(condition) && expr_alloc_free(then_expr) && expr_alloc_free(else_expr), + } => { + expr_alloc_free(condition, is_inert) + && expr_alloc_free(then_expr, is_inert) + && expr_alloc_free(else_expr, is_inert) + } _ => false, } } @@ -170,7 +234,64 @@ fn expr_alloc_free(e: &Expr) -> bool { #[cfg(test)] mod allocation_tests { use super::*; - use perry_hir::BinaryOp; + use perry_hir::{BinaryOp, LogicalOp, UpdateOp}; + + /// A local the real `expr_is_inert_primitive` would call inert: refined to + /// a non-pointer primitive, no shadow slot, not a module global. + const NUM: u32 = 1; + /// A second one, so the two-operand shapes can be built from proven locals. + const NUM2: u32 = 2; + /// A local it would REFUSE: `any`-typed / shadow-slotted / a module global — + /// i.e. one that can hold an object with a user-defined `valueOf`. + const OBJ: u32 = 9; + + /// Stand-in for `expr_is_inert_primitive` with `NUM`/`NUM2` proven and + /// `OBJ` not. Mirrors the real predicate's recursion so the shapes under + /// test exercise the same tree walk; the real predicate itself is covered + /// end to end by `tests/loop_safepoint_purity.rs`, which needs an `FnCtx`. + fn stub_inert(e: &Expr) -> bool { + match e { + Expr::Undefined | Expr::Null | Expr::Bool(_) | Expr::Number(_) | Expr::Integer(_) => { + true + } + Expr::LocalGet(id) | Expr::Update { id, .. } => *id == NUM || *id == NUM2, + Expr::Unary { operand, .. } => stub_inert(operand), + Expr::Compare { left, right, .. } | Expr::Binary { left, right, .. } => { + stub_inert(left) && stub_inert(right) + } + _ => false, + } + } + + /// The production wiring's shape: `is_inert` is consulted only for the + /// coercing operators. + fn may_allocate(body: &[Stmt], controls: &[&Expr]) -> bool { + loop_may_allocate(body, controls, &stub_inert) + } + + /// `is_inert` answering "nothing is ever inert" — the reading a stale or + /// broken predicate would produce. Every test that asserts a loop LOSES its + /// poll re-runs under this to prove the poll came from the operand proof + /// and not from the shape being unreachable. + fn may_allocate_nothing_inert(body: &[Stmt], controls: &[&Expr]) -> bool { + loop_may_allocate(body, controls, &|_| false) + } + + fn lt(left: Expr, right: Expr) -> Expr { + Expr::Compare { + op: CompareOp::Lt, + left: Box::new(left), + right: Box::new(right), + } + } + + fn add(left: Expr, right: Expr) -> Expr { + Expr::Binary { + op: BinaryOp::Add, + left: Box::new(left), + right: Box::new(right), + } + } #[test] fn generic_index_update_keeps_the_loop_safepoint() { @@ -180,7 +301,7 @@ mod allocation_tests { op: BinaryOp::Add, prefix: false, }; - assert!(loop_may_allocate(&[Stmt::Expr(update)], &[])); + assert!(may_allocate(&[Stmt::Expr(update)], &[])); } #[test] @@ -189,7 +310,184 @@ mod allocation_tests { object: Box::new(Expr::LocalGet(1)), index: Box::new(Expr::Integer(0)), }; - assert!(loop_may_allocate(&[], &[&condition])); + assert!(may_allocate(&[], &[&condition])); + } + + // ------------------------------------------------------- the widening --- + + /// `for (let i = 0; i < n; i++) { sum = sum + 1; }` — every one of the + /// three back-edge decisions (condition, body, update) must come back + /// "cannot allocate". + #[test] + fn proven_numeric_for_loop_drops_all_three_safepoints() { + let condition = lt(Expr::LocalGet(NUM), Expr::LocalGet(NUM2)); + let update = Expr::Update { + id: NUM, + op: UpdateOp::Increment, + prefix: false, + }; + let body = vec![Stmt::Expr(Expr::LocalSet( + NUM2, + Box::new(add(Expr::LocalGet(NUM2), Expr::Number(1.0))), + ))]; + + assert!(!may_allocate(&[], &[&condition]), "condition poll"); + assert!(!may_allocate(&body, &[]), "body poll"); + assert!(!may_allocate(&[], &[&update]), "update poll"); + + // Sabotage: with nothing proven inert, all three come back. This is + // what makes the assertions above load-bearing — the shapes are only + // poll-free because the operands were proven. + assert!(may_allocate_nothing_inert(&[], &[&condition])); + assert!(may_allocate_nothing_inert(&body, &[])); + assert!(may_allocate_nothing_inert(&[], &[&update])); + } + + /// The hazard #6975 named: `a < b` recurses to two clean `LocalGet`s while + /// the comparison ITSELF runs ToPrimitive. An operand that can carry a + /// user-defined `valueOf` must keep the poll. + #[test] + fn relational_condition_over_a_coercible_local_keeps_the_safepoint() { + let coercible = lt(Expr::LocalGet(NUM), Expr::LocalGet(OBJ)); + assert!(may_allocate(&[], &[&coercible])); + + // Both directions: swapping only the operand's provenance flips it. + let proven = lt(Expr::LocalGet(NUM), Expr::LocalGet(NUM2)); + assert!(!may_allocate(&[], &[&proven])); + } + + /// Same for `<=` / `>` / `>=` and for the LOOSE equalities, which coerce + /// where `===` does not. + #[test] + fn every_coercing_comparison_over_a_coercible_local_keeps_the_safepoint() { + for op in [ + CompareOp::Lt, + CompareOp::Le, + CompareOp::Gt, + CompareOp::Ge, + CompareOp::LooseEq, + CompareOp::LooseNe, + ] { + let e = Expr::Compare { + op, + left: Box::new(Expr::LocalGet(OBJ)), + right: Box::new(Expr::Integer(0)), + }; + assert!(may_allocate(&[], &[&e]), "{op:?} must keep its poll"); + } + } + + /// Arithmetic is not pure either: `sum + obj` runs `obj.valueOf()`. + #[test] + fn arithmetic_over_a_coercible_local_keeps_the_safepoint() { + let body = vec![Stmt::Expr(Expr::LocalSet( + NUM, + Box::new(add(Expr::LocalGet(NUM), Expr::LocalGet(OBJ))), + ))]; + assert!(may_allocate(&body, &[])); + } + + /// And `obj++` runs ToNumeric on the object. + #[test] + fn incrementing_a_coercible_local_keeps_the_safepoint() { + let update = Expr::Update { + id: OBJ, + op: UpdateOp::Increment, + prefix: false, + }; + assert!(may_allocate(&[], &[&update])); + } + + /// `-x` / `+x` / `~x` coerce; only `!x` (ToBoolean) does not. + #[test] + fn coercing_unary_over_a_coercible_local_keeps_the_safepoint() { + for op in [UnaryOp::Neg, UnaryOp::Pos, UnaryOp::BitNot] { + let e = Expr::Unary { + op, + operand: Box::new(Expr::LocalGet(OBJ)), + }; + assert!(may_allocate(&[], &[&e]), "{op:?} must keep its poll"); + } + } + + // ------------------------------------------- the pre-existing arms ------ + + /// `===` / `!==` never coerce, so they stay open to operands of any type. + /// Narrowing them to `is_inert` would be a silent pessimization, and this + /// pins that: it passes with NOTHING proven inert. + #[test] + fn strict_equality_stays_open_to_any_operand() { + let e = Expr::Compare { + op: CompareOp::Eq, + left: Box::new(Expr::LocalGet(OBJ)), + right: Box::new(Expr::Null), + }; + assert!(!may_allocate_nothing_inert(&[], &[&e])); + } + + /// Same for `!x`, `typeof x` and `x && y`: ToBoolean and the tag read never + /// reach a user-defined conversion. + #[test] + fn boolean_and_tag_operators_stay_open_to_any_operand() { + let not = Expr::Unary { + op: UnaryOp::Not, + operand: Box::new(Expr::LocalGet(OBJ)), + }; + let type_of = Expr::TypeOf(Box::new(Expr::LocalGet(OBJ))); + let and = Expr::Logical { + op: LogicalOp::And, + left: Box::new(Expr::LocalGet(OBJ)), + right: Box::new(Expr::LocalGet(OBJ)), + }; + assert!(!may_allocate_nothing_inert(&[], &[¬])); + assert!(!may_allocate_nothing_inert(&[], &[&type_of])); + assert!(!may_allocate_nothing_inert(&[], &[&and])); + } + + /// A call is the shape the poll exists for. Nothing about the widening may + /// let one through, however numeric its arguments look. + #[test] + fn a_call_in_the_body_always_keeps_the_safepoint() { + let body = vec![Stmt::Expr(Expr::Call { + callee: Box::new(Expr::FuncRef(0)), + args: vec![Expr::LocalGet(NUM)], + type_args: Vec::new(), + byte_offset: 0, + })]; + assert!(may_allocate(&body, &[])); + } + + /// A call nested behind an otherwise-inert operator, too: the recursion has + /// to reach it. `is_inert` refuses `Call` outright, so `i < f()` keeps its + /// poll. + #[test] + fn a_call_inside_a_comparison_keeps_the_safepoint() { + let e = lt( + Expr::LocalGet(NUM), + Expr::Call { + callee: Box::new(Expr::FuncRef(0)), + args: Vec::new(), + type_args: Vec::new(), + byte_offset: 0, + }, + ); + assert!(may_allocate(&[], &[&e])); + } + + /// A nested loop's own statements are walked: an allocating inner body + /// keeps the OUTER poll as well. + #[test] + fn an_allocating_inner_loop_keeps_the_outer_safepoint() { + let inner = Stmt::While { + condition: lt(Expr::LocalGet(NUM), Expr::LocalGet(NUM2)), + body: vec![Stmt::Expr(Expr::Call { + callee: Box::new(Expr::FuncRef(0)), + args: Vec::new(), + type_args: Vec::new(), + byte_offset: 0, + })], + }; + assert!(may_allocate(&[inner], &[])); } } diff --git a/crates/perry-codegen/src/stmt/loops.rs b/crates/perry-codegen/src/stmt/loops.rs index 1c308e4cf4..646918be01 100644 --- a/crates/perry-codegen/src/stmt/loops.rs +++ b/crates/perry-codegen/src/stmt/loops.rs @@ -5266,7 +5266,18 @@ pub(crate) fn emit_gc_loop_safepoint( // poll for pure (non-allocating) bodies so numeric/vectorizable loops stay // call-free (a poll defeats LLVM auto-vectorization — measured ~2x on a tight // scalar reduction). See `loop_may_allocate` for the safe-direction rationale. - if !crate::loop_purity::loop_may_allocate(body, controls) { + // + // The coercing operators (`i < n`, `sum + 1`, `i++`) are alloc-free only + // over operands `expr_is_inert_primitive` proves are non-pointer + // primitives — a user-defined `valueOf` is arbitrary JS. The borrow of + // `ctx` ends with the block so the poll emission below can take it + // mutably. + let needs_poll = { + let is_inert = + |e: &perry_hir::Expr| crate::expr::temp_root::expr_is_inert_primitive(ctx, e); + crate::loop_purity::loop_may_allocate(body, controls, &is_inert) + }; + if !needs_poll { return; } ctx.block().call_void("js_gc_loop_safepoint", &[]); diff --git a/crates/perry-codegen/tests/loop_safepoint_purity.rs b/crates/perry-codegen/tests/loop_safepoint_purity.rs new file mode 100644 index 0000000000..f312c95a6c --- /dev/null +++ b/crates/perry-codegen/tests/loop_safepoint_purity.rs @@ -0,0 +1,383 @@ +//! The loop back-edge GC poll (`js_gc_loop_safepoint`) and the purity proof +//! that removes it. +//! +//! `crate::loop_purity::loop_may_allocate` decides, per loop back edge, +//! whether a deferred minor collection could be waiting to be drained. Its +//! whitelist used to omit relational comparisons, arithmetic `Binary` and +//! `Update`, so `for (let i = 0; i < n; i++) { sum = sum + 1; }` failed the +//! test on its own condition, body AND update — three runtime calls per +//! iteration in a loop that allocates nothing. +//! +//! The widening reuses `expr_is_inert_primitive` (#6975): those operators run +//! ToPrimitive / ToNumeric, and a user-defined `valueOf` is arbitrary JS that +//! allocates. The unit tests in `loop_purity.rs` pin the walk with an injected +//! predicate; these compile real HIR so the REAL predicate — `local_types`, +//! `shadow_slot_map`, `module_globals` and all — is the thing under test. +//! +//! Every "no poll" assertion below is paired with a case that differs in one +//! operand and MUST keep its poll. That pairing is the point, and it was +//! verified by breaking the implementation five ways and checking that exactly +//! the intended tests went red: +//! +//! | sabotage | red | +//! |-----------------------------------------------------|---------------------------------------| +//! | `Add` loses its non-pointer operand condition | `concatenating_two_string_literals…` | +//! | `local_is_inert_primitive` drops the module-global guard | `a_module_global_accumulator…` | +//! | the injected predicate answers `true` for everything | all four operand-proof cases | +//! | the injected predicate answers `false` for everything (the pre-change behaviour) | `proven_numeric_counted_loop…` | +//! | `local_is_inert_primitive` drops the shadow-slot guard | *nothing* — see below | +//! +//! That last row is honest rather than reassuring. The shadow-slot half of +//! `local_is_inert_primitive` is defensive redundancy: `collect_pointer_typed_locals` +//! reserves slots from a local's *declared/inferred* type, so any local whose +//! refined type is already `Number`/`Int32`/… has no slot either way, and no +//! fixture separates the two halves. It is kept because it is #6975's own +//! formulation and costs a hash lookup. + +use perry_codegen::{compile_module, AppMetadata, CompileOptions}; +use perry_hir::types::Type; +use perry_hir::{BinaryOp, CompareOp, Expr, Module, ModuleInitKind, Stmt, UpdateOp}; + +fn entry_opts() -> CompileOptions { + CompileOptions { + target: None, + is_entry_module: true, + non_entry_module_prefixes: Vec::new(), + nextjs_path_init_modules: Vec::new(), + import_function_prefixes: std::collections::HashMap::new(), + import_function_ffi_aliases: std::collections::HashMap::new(), + import_function_origin_names: std::collections::HashMap::new(), + import_function_v8_specifiers: std::collections::HashMap::new(), + import_function_node_submodule: std::collections::HashMap::new(), + namespace_node_submodules: std::collections::HashMap::new(), + namespace_v8_specifiers: std::collections::HashMap::new(), + namespace_member_prefixes: std::collections::HashMap::new(), + namespace_member_origin_names: std::collections::HashMap::new(), + emit_ir_only: true, + verify_native_regions: false, + disable_buffer_fast_path: false, + namespace_imports: Vec::new(), + imported_classes: Vec::new(), + imported_enums: Vec::new(), + imported_async_funcs: std::collections::HashSet::new(), + type_aliases: std::collections::HashMap::new(), + imported_func_param_counts: std::collections::HashMap::new(), + imported_func_has_rest: std::collections::HashSet::new(), + imported_func_synthetic_arguments: std::collections::HashSet::new(), + imported_func_return_types: std::collections::HashMap::new(), + imported_vars: std::collections::HashSet::new(), + output_type: "executable".to_string(), + needs_stdlib: false, + needs_ui: false, + needs_geisterhand: false, + geisterhand_port: 7676, + enabled_features: Vec::new(), + native_module_init_names: Vec::new(), + js_module_specifiers: Vec::new(), + bundled_extensions: Vec::new(), + native_library_functions: Vec::new(), + i18n_table: None, + fast_math: false, + fp_contract_mode: perry_codegen::FpContractMode::Off, + app_metadata: AppMetadata::default(), + namespace_entries: Vec::new(), + dynamic_import_path_to_prefix: std::collections::HashMap::new(), + deferred_module_prefixes: std::collections::HashSet::new(), + module_init_deps: Vec::new(), + is_dynamic_import_target: false, + debug_locations: false, + module_source: None, + debug_source_line_offset: 0, + } +} +fn module_with_init(name: &str, init: Vec) -> Module { + Module { + name: name.to_string(), + imports: Vec::new(), + exports: Vec::new(), + classes: Vec::new(), + interfaces: Vec::new(), + type_aliases: Vec::new(), + enums: Vec::new(), + globals: Vec::new(), + functions: Vec::new(), + script_global_functions: Vec::new(), + references_global_this: false, + annexb_global_undefined_names: Vec::new(), + init, + exported_native_instances: Vec::new(), + exported_func_return_native_instances: Vec::new(), + exported_objects: Vec::new(), + exported_functions: Vec::new(), + widgets: Vec::new(), + uses_fetch: false, + uses_webassembly: false, + extern_funcs: Vec::new(), + init_was_unrolled: false, + has_top_level_await: false, + init_kind: ModuleInitKind::Eager, + async_step_closures: std::collections::HashSet::new(), + closure_display_names: std::collections::HashMap::new(), + class_display_names: std::collections::HashMap::new(), + closure_source_text: std::collections::HashMap::new(), + async_generator_funcs: std::collections::HashSet::new(), + gen_param_prologue_len: std::collections::HashMap::new(), + } +} + +fn ir_for(name: &str, init: Vec) -> String { + String::from_utf8(compile_module(&module_with_init(name, init), entry_opts()).unwrap()) + .expect("LLVM IR should be UTF-8") +} + +/// Same, but `exported` names are exported module variables — which is what +/// promotes a module-level `let` to a `@perry_global_*` slot. +fn ir_for_with_exported_vars(name: &str, init: Vec, exported: &[&str]) -> String { + let mut m = module_with_init(name, init); + m.exported_objects = exported.iter().map(|s| s.to_string()).collect(); + String::from_utf8(compile_module(&m, entry_opts()).unwrap()).expect("LLVM IR should be UTF-8") +} + +/// A CALL to the poll. The `declare void @js_gc_loop_safepoint()` line is +/// emitted unconditionally, so only a call site counts. +const POLL: &str = "call void @js_gc_loop_safepoint()"; + +const N: u32 = 1; +const SUM: u32 = 2; +const I: u32 = 3; + +fn let_stmt(id: u32, name: &str, ty: Type, init: Expr) -> Stmt { + Stmt::Let { + id, + name: name.to_string(), + ty, + mutable: true, + init: Some(init), + } +} + +/// `for (let i = 0; i < n; i++) { }`, with `n` and `sum` declared at +/// module scope from the caller's declarations. +/// +/// Note that the DECLARED type is not what decides inertness — the pointer +/// scan tracks the values a local actually receives, so `let n: any = 1000` +/// is still proven numeric, and correctly so. To make a local coercible it has +/// to actually be handed a heap value, which is what `coercible()` does. +fn counted_loop(n: Stmt, sum: Stmt, body: Vec) -> Vec { + let mut stmts = vec![n, sum]; + stmts.extend(counted_loop_only(body)); + stmts +} + +/// Just the `for` statement, for fixtures that declare `n` / `sum` themselves. +fn counted_loop_only(body: Vec) -> Vec { + vec![Stmt::For { + init: Some(Box::new(let_stmt(I, "i", Type::Number, Expr::Number(0.0)))), + condition: Some(Expr::Compare { + op: CompareOp::Lt, + left: Box::new(Expr::LocalGet(I)), + right: Box::new(Expr::LocalGet(N)), + }), + update: Some(Expr::Update { + id: I, + op: UpdateOp::Increment, + prefix: false, + }), + body, + }] +} + +/// `sum = sum + `. +/// A local that provably holds a heap value: `any`-typed and initialized with +/// an object. Whatever that object's `valueOf` does is arbitrary JS, so every +/// coercion of this local is a potential allocation — and the shadow-slot scan +/// reserves it a slot, which is the fact `expr_is_inert_primitive` reads. +fn coercible(id: u32, name: &str) -> Stmt { + let_stmt(id, name, Type::Any, Expr::Object(Vec::new())) +} + +/// A local proven to hold a number. +fn numeric(id: u32, name: &str, v: f64) -> Stmt { + let_stmt(id, name, Type::Number, Expr::Number(v)) +} + +fn accumulate(rhs: Expr) -> Vec { + vec![Stmt::Expr(Expr::LocalSet( + SUM, + Box::new(Expr::Binary { + op: BinaryOp::Add, + left: Box::new(Expr::LocalGet(SUM)), + right: Box::new(rhs), + }), + ))] +} + +// ------------------------------------------------------------ the win ------ + +/// The headline case. Condition, body and update are all proven-numeric, so +/// none of the three back edges may emit a poll. +#[test] +fn proven_numeric_counted_loop_emits_no_back_edge_poll() { + let ir = ir_for( + "loop_poll_numeric.ts", + counted_loop( + numeric(N, "n", 1000.0), + numeric(SUM, "sum", 0.0), + accumulate(Expr::Number(1.0)), + ), + ); + assert!( + !ir.contains(POLL), + "a loop that allocates nothing must emit no back-edge poll — \ + `i < n`, `sum + 1` and `i++` are all over proven-numeric locals:\n{ir}" + ); +} + +// ---------------------------------------------------- the safe direction --- + +/// Sabotage #1 — the loop BOUND becomes an object. Nothing else changes: same +/// condition node, same body, same update. `i < n` now runs ToPrimitive on a +/// value that can define `valueOf`, so the poll has to survive. +/// +/// This is the pairing that gives the test above its teeth: a predicate that +/// waved everything through would pass that one and fail this one. +#[test] +fn a_coercible_bound_keeps_the_back_edge_poll() { + let ir = ir_for( + "loop_poll_any_bound.ts", + counted_loop( + coercible(N, "n"), + numeric(SUM, "sum", 0.0), + accumulate(Expr::Number(1.0)), + ), + ); + assert!( + ir.contains(POLL), + "`i < n` over an object-valued bound can run a user `valueOf`, which \ + allocates — the poll must survive:\n{ir}" + ); +} + +/// Sabotage #2 — the ACCUMULATOR becomes an object, so `sum + 1` is the +/// coercing operator instead of the comparison. +#[test] +fn a_coercible_accumulator_keeps_the_back_edge_poll() { + let ir = ir_for( + "loop_poll_any_acc.ts", + counted_loop( + numeric(N, "n", 1000.0), + coercible(SUM, "sum"), + accumulate(Expr::Number(1.0)), + ), + ); + assert!( + ir.contains(POLL), + "`sum + 1` over an object-valued accumulator can run a user `valueOf`:\n{ir}" + ); +} + +/// Sabotage #3 — `+` over two string LITERALS. This is the case that isolates +/// the `Add` rule and nothing else: a string literal is inert (ToPrimitive on +/// it is the identity, no user code), so "both operands inert" is satisfied — +/// and the concatenation still allocates a fresh string every iteration. +/// +/// `Add` therefore carries an extra condition that neither operand may BE a +/// heap reference. Delete it — leave `Add` merely "inert operands" — and this +/// test goes red while every other test in the file stays green. +#[test] +fn concatenating_two_string_literals_keeps_the_back_edge_poll() { + let ir = ir_for( + "loop_poll_concat.ts", + counted_loop( + numeric(N, "n", 1000.0), + let_stmt(SUM, "sum", Type::String, Expr::String(String::new())), + vec![Stmt::Expr(Expr::LocalSet( + SUM, + Box::new(Expr::Binary { + op: BinaryOp::Add, + left: Box::new(Expr::String("a".to_string())), + right: Box::new(Expr::String("b".to_string())), + }), + ))], + ), + ); + assert!( + ir.contains(POLL), + "`\"a\" + \"b\"` allocates a fresh string every iteration, even though \ + both operands are inert:\n{ir}" + ); +} + +/// Sabotage #4 — an outright allocation in the body. The catch-all has always +/// covered this; it must keep covering it. +#[test] +fn an_object_literal_in_the_body_keeps_the_back_edge_poll() { + let ir = ir_for( + "loop_poll_object.ts", + counted_loop( + numeric(N, "n", 1000.0), + let_stmt(SUM, "sum", Type::Any, Expr::Undefined), + vec![Stmt::Expr(Expr::LocalSet( + SUM, + Box::new(Expr::Object(Vec::new())), + ))], + ), + ); + assert!( + ir.contains(POLL), + "an object literal per iteration is exactly what the poll exists \ + for:\n{ir}" + ); +} + +/// Sabotage #5 — a CALL in the body. A call can reach anything, including a +/// collection, so no amount of numeric proof around it may drop the poll. +#[test] +fn a_call_in_the_body_keeps_the_back_edge_poll() { + let ir = ir_for( + "loop_poll_call.ts", + counted_loop( + numeric(N, "n", 1000.0), + numeric(SUM, "sum", 0.0), + vec![Stmt::Expr(Expr::Call { + callee: Box::new(Expr::LocalGet(SUM)), + args: vec![Expr::Number(1.0)], + type_args: Vec::new(), + byte_offset: 0, + })], + ), + ); + assert!( + ir.contains(POLL), + "a call in the body can reach a collection point:\n{ir}" + ); +} + +/// Sabotage #6 — the accumulator is a module-level EXPORTED binding, so it +/// lives in a `@perry_global_*` slot rather than an alloca. +/// +/// Everything the inertness proof reads — the refined type, the shadow-slot +/// map — is computed from THIS function's body alone, and a module global can +/// be assigned an object by a different function or a different module that +/// this scan never sees. `local_is_inert_primitive` therefore refuses module +/// globals outright, and this pins that: the loop is textually identical to +/// the proven-numeric one, and must still keep its poll. +#[test] +fn a_module_global_accumulator_keeps_the_back_edge_poll() { + let init = counted_loop( + numeric(N, "n", 1000.0), + numeric(SUM, "sum", 0.0), + accumulate(Expr::Number(1.0)), + ); + let ir = ir_for_with_exported_vars("loop_poll_module_global.ts", init, &["sum"]); + assert!( + ir.contains("@perry_global_"), + "the fixture only means something if `sum` really became a module \ + global:\n{ir}" + ); + assert!( + ir.contains(POLL), + "a module global's type and pointer-ness are only known per function, \ + so it is never inert and the poll must survive:\n{ir}" + ); +}