diff --git a/changelog.d/11856-regfix-region-truthiness.md b/changelog.d/11856-regfix-region-truthiness.md new file mode 100644 index 0000000000..8adb7771c4 --- /dev/null +++ b/changelog.d/11856-regfix-region-truthiness.md @@ -0,0 +1,5 @@ +A value only tested for truthiness (a conditional's test, a `!` operand) no +longer asks a loop region for a Number lane, directly or through an +accumulator's value flow. A string field tested that way failed the region's +value test on every iteration: #10495 `own3` 246 -> 228 instructions per +operation. diff --git a/crates/perry-codegen/src/collectors/ptr_shape_numeric.rs b/crates/perry-codegen/src/collectors/ptr_shape_numeric.rs index 63f30b0cb2..52e374a241 100644 --- a/crates/perry-codegen/src/collectors/ptr_shape_numeric.rs +++ b/crates/perry-codegen/src/collectors/ptr_shape_numeric.rs @@ -598,6 +598,22 @@ pub(crate) fn region_number_flow_reads( locals.push(*id); return; } + // A value only tested for truthiness does not flow into the + // result: a conditional's test (its arms are the values) and a + // `!` operand (the result is a Boolean). + Expr::Conditional { + then_expr, + else_expr, + .. + } => { + deps(then_expr, reads, locals); + deps(else_expr, reads, locals); + return; + } + Expr::Unary { + op: perry_hir::UnaryOp::Not, + .. + } => return, _ => {} } perry_hir::walker::walk_expr_children(e, &mut |child| deps(child, reads, locals)); diff --git a/crates/perry-codegen/src/expr/region_loop_tests.rs b/crates/perry-codegen/src/expr/region_loop_tests.rs index 05c2e9b22f..c50d1b0468 100644 --- a/crates/perry-codegen/src/expr/region_loop_tests.rs +++ b/crates/perry-codegen/src/expr/region_loop_tests.rs @@ -17,7 +17,7 @@ use super::class_field_barrier_tests::ir_opts; use crate::{compile_module, CompileOptions}; use perry_hir::types::Type; use perry_hir::{ - BinaryOp, CompareOp, Expr, Function, Module, ModuleInitKind, Param, Stmt, UpdateOp, + BinaryOp, CompareOp, Expr, Function, Module, ModuleInitKind, Param, Stmt, UnaryOp, UpdateOp, }; use std::collections::{HashMap, HashSet}; @@ -562,6 +562,72 @@ fn a_receiver_read_beneath_a_property_access_requests_no_number_lane() { assert!(!ir.contains("rloop.guard.value"), "no R, so no value test"); } +/// A value only tested for truthiness is not a Number operand: neither a +/// conditional's test (`h += o.x ? 1 : 0`; the arms are the values) nor a `!` +/// operand asks the region for R, directly or through the accumulator's +/// value flow. A string `x` would otherwise fail the value test on every +/// iteration and the loop would pay the guard for nothing. +#[test] +fn a_truthiness_test_requests_no_number_lane() { + let ternary = |test: Expr| Expr::Conditional { + condition: Box::new(test), + then_expr: Box::new(Expr::Integer(1)), + else_expr: Box::new(Expr::Integer(0)), + }; + for (name, test) in [ + ("region_loop_truthy_cond", get("x")), + ( + "region_loop_truthy_not", + Expr::Unary { + op: UnaryOp::Not, + operand: Box::new(get("x")), + }, + ), + ] { + let ir = loop_ir( + name, + vec![Stmt::Expr(Expr::LocalSet( + H, + Box::new(Expr::Binary { + op: BinaryOp::Add, + left: Box::new(Expr::LocalGet(H)), + right: Box::new(ternary(test)), + }), + ))], + ); + let masks = prime_rep_masks(&ir); + assert!( + masks.iter().all(|&m| m == 0), + "{name}: a truthiness test is not a Number operand: {masks:?}\n{ir}" + ); + assert!( + !ir.contains("rloop.guard.value"), + "{name}: no R, so no value test" + ); + } + // Control: the same read as an arm IS the value, and asks for R. + let ir = loop_ir( + "region_loop_truthy_arm", + vec![Stmt::Expr(Expr::LocalSet( + H, + Box::new(Expr::Binary { + op: BinaryOp::Add, + left: Box::new(Expr::LocalGet(H)), + right: Box::new(Expr::Conditional { + condition: Box::new(Expr::LocalGet(N)), + then_expr: Box::new(get("x")), + else_expr: Box::new(Expr::Integer(0)), + }), + }), + ))], + ); + let masks = prime_rep_masks(&ir); + assert!( + !masks.is_empty() && masks.iter().all(|&m| m == 1), + "an arm is a Number operand: {masks:?}\n{ir}" + ); +} + /// The F-local fixed point follows the fresh F64 read through a temporary and /// a loop-carried accumulator. Entry is strict; G and post-loop code retain /// the ordinary dynamic add. Removing the scoped materialization or the diff --git a/crates/perry-codegen/src/stmt/region_loop/plan.rs b/crates/perry-codegen/src/stmt/region_loop/plan.rs index 3e44b13787..f3198a19aa 100644 --- a/crates/perry-codegen/src/stmt/region_loop/plan.rs +++ b/crates/perry-codegen/src/stmt/region_loop/plan.rs @@ -95,7 +95,9 @@ pub(super) fn fact_tree_leaves<'e>( /// another property access is that access's RECEIVER (`o.a.length` reads /// `o.a` as an object), and a read beneath an element access or a call is /// that operation's key, receiver or argument: none of them is a Number -/// operand, so none may ask the region for a Number lane (R). +/// operand, so none may ask the region for a Number lane (R). Nor is a value +/// only tested for truthiness: a conditional's test (`o.a ? 1 : 0`; its arms +/// are the values) and a `!` operand. fn number_operand_reads(e: &Expr, out: &mut Vec<(usize, Recv, String)>) { match e { Expr::PropertyGet { @@ -109,6 +111,18 @@ fn number_operand_reads(e: &Expr, out: &mut Vec<(usize, Recv, String)>) { Expr::IndexGet { .. } | Expr::Call { .. } | Expr::CallSpread { .. } | Expr::New { .. } => { return } + Expr::Conditional { + then_expr, + else_expr, + .. + } => { + number_operand_reads(then_expr, out); + number_operand_reads(else_expr, out); + return; + } + Expr::Unary { + op: UnaryOp::Not, .. + } => return, _ => {} } perry_hir::walker::walk_expr_children(e, &mut |child| number_operand_reads(child, out));