From bae4cdc757b291ce54b46a6376c4ccc8f3165b68 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 23 Sep 2025 12:30:21 +0100 Subject: [PATCH 01/15] Comments --- compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs index 36023f37aeb..b661c03b439 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -17,7 +17,7 @@ //! ``` //! //! If the shift amount is not a constant, 2^N is computed via square&multiply, -//! using the bits decomposition of exponent. +//! using the bits decomposition of the exponent. //! //! Pseudo-code of the computation: //! @@ -33,7 +33,7 @@ //! //! ## Unsigned shift-left //! -//! Shifting an unsigned integer to the right by N is the same as multiplying by 2^N. +//! Shifting an unsigned integer to the left by N is the same as multiplying by 2^N. //! However, since that can overflow the target bit size, the operation is done using //! Field, then truncated to the target bit size. //! From 4c60d63c8eb1df84e11e9b3e1bbb604fb3dd8052 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 23 Sep 2025 12:30:50 +0100 Subject: [PATCH 02/15] Make get_value_max_num_bits recursive --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 0eb40c8762f..df5839ff632 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -533,7 +533,7 @@ impl DataFlowGraph { Value::Instruction { instruction, .. } => { let value_bit_size = self.type_of_value(value).bit_size(); if let Instruction::Cast(original_value, _) = self[instruction] { - let original_bit_size = self.type_of_value(original_value).bit_size(); + let original_bit_size = self.get_value_max_num_bits(original_value); // We might have cast e.g. `u1` to `u8` to be able to do arithmetic, // in which case we want to recover the original smaller bit size; // OTOH if we cast down, then we don't need the higher original size. From 4bb395015fa86cec3e88a7cced75e3aaf653a276 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 23 Sep 2025 15:37:19 +0100 Subject: [PATCH 03/15] Typos --- .../src/ssa/opt/remove_bit_shifts.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs index b661c03b439..64f512e0f4e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -137,8 +137,8 @@ struct Context<'m, 'dfg, 'mapping> { } impl Context<'_, '_, '_> { - /// Insert ssa instructions which computes lhs << rhs by doing lhs*2^rhs - /// and truncate the result to bit_size + /// Insert SSA instructions which computes lhs << rhs by doing lhs*2^rhs + /// and truncate the result to `bit_size`. fn insert_wrapping_shift_left(&mut self, lhs: ValueId, rhs: ValueId) -> ValueId { let typ = self.context.dfg.type_of_value(lhs).unwrap_numeric(); let max_lhs_bits = self.context.dfg.get_value_max_num_bits(lhs); @@ -178,7 +178,7 @@ impl Context<'_, '_, '_> { let result = self.insert_truncate(result, typ.bit_size(), max_bit); self.insert_cast(result, typ) } else { - // Otherwise, the result might not bit in a FieldElement. + // Otherwise, the result might not fit in a FieldElement. // For this, if we have to do `lhs << rhs` we can first shift by half of `rhs`, truncate, // then shift by `rhs - half_of_rhs` and truncate again. assert!(typ.bit_size() <= 128); @@ -188,12 +188,12 @@ impl Context<'_, '_, '_> { // rhs_divided_by_two = rhs / 2 let rhs_divided_by_two = self.insert_binary(rhs, BinaryOp::Div, two); - // rhs_remainder = rhs - rhs_remainder + // rhs_remainder = rhs - rhs_divided_by_two let rhs_remainder = self.insert_binary(rhs, BinaryOp::Sub { unchecked: true }, rhs_divided_by_two); // pow1 = 2^rhs_divided_by_two - // pow2 = r^rhs_remainder + // pow2 = 2^rhs_remainder let pow1 = self.two_pow(rhs_divided_by_two); let pow2 = self.two_pow(rhs_remainder); @@ -208,7 +208,7 @@ impl Context<'_, '_, '_> { } } - /// Insert ssa instructions which computes lhs >> rhs by doing lhs/2^rhs + /// Insert SSA instructions which computes lhs >> rhs by doing lhs/2^rhs /// For negative signed integers, we do the division on the 1-complement representation of lhs, /// before converting back the result to the 2-complement representation. fn insert_shift_right(&mut self, lhs: ValueId, rhs: ValueId) -> ValueId { @@ -257,6 +257,7 @@ impl Context<'_, '_, '_> { /// Computes 2^exponent via square&multiply, using the bits decomposition of exponent /// Pseudo-code of the computation: + /// ```text /// let mut r = 1; /// let exponent_bits = to_bits(exponent); /// for i in 1 .. bit_size + 1 { @@ -264,6 +265,7 @@ impl Context<'_, '_, '_> { /// let b = exponent_bits[bit_size - i]; /// r = if b { 2 * r_squared } else { r_squared }; /// } + /// ``` fn two_pow(&mut self, exponent: ValueId) -> ValueId { // Require that exponent < bit_size, ensuring that `pow` returns a value consistent with `lhs`'s type. let max_bit_size = self.context.dfg.type_of_value(exponent).bit_size(); From 1a9edf4633c57acaf018fe42dd564b6eb81155d4 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 23 Sep 2025 15:42:41 +0100 Subject: [PATCH 04/15] Remove leftover print --- compiler/noirc_evaluator/src/ssa/parser/tests.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/parser/tests.rs b/compiler/noirc_evaluator/src/ssa/parser/tests.rs index ca80a934dc9..6fb41e9e4dc 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/tests.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/tests.rs @@ -7,7 +7,6 @@ use crate::{ fn assert_ssa_roundtrip(src: &str) { let ssa = Ssa::from_str(src).unwrap(); - println!("offset: {}", ssa.main().dfg.brillig_arrays_offset); let ssa = ssa.print_without_locations().to_string(); let ssa = trim_leading_whitespace_from_lines(&ssa); let src = trim_leading_whitespace_from_lines(src); From c61f8e60855e4ea5bb067810fd0afb9c46c2c1e8 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 24 Sep 2025 09:57:00 +0100 Subject: [PATCH 05/15] Fix typo in example --- .../src/ssa/opt/remove_if_else.rs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index 1b6f7436e02..47612c8f06d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -47,7 +47,7 @@ //! y[0] = 2; //! } //! -//! assert(y[0] == 1); +//! assert(y[0] == 3); //! } //! ``` //! @@ -74,14 +74,14 @@ //! ```ssa //! v13 = cast v0 as u32 //! v14 = cast v6 as u32 -//! v15 = unchecked_mul v14, u32 2 +//! v15 = unchecked_mul v14, u32 2 //! v16 = unchecked_add v13, v15 //! v17 = array_get v5, index u32 1 -> u32 //! v18 = array_get v8, index u32 1 -> u32 //! v19 = cast v0 as u32 //! v20 = cast v6 as u32 //! v21 = unchecked_mul v19, v17 -//! v22 = unchecked_mul v20, v18 +//! v22 = unchecked_mul v20, v18 //! v23 = unchecked_add v21, v22 //! v24 = make_array [v16, v23] : [u32; 2] //! ``` @@ -521,18 +521,18 @@ mod tests { let src = " acir(inline) impure fn main f0 { b0(v0: u1, v1: Field, v2: Field): - v3 = make_array [] : [Field] - v4 = allocate -> &mut u32 - v5 = allocate -> &mut [Field] + v3 = make_array [] : [Field] + v4 = allocate -> &mut u32 + v5 = allocate -> &mut [Field] enable_side_effects v0 v6 = cast v0 as u32 v7, v8 = call slice_push_back(v6, v3, v2) -> (u32, [Field]) - v9 = not v0 - v10 = cast v0 as u32 - v12 = if v0 then v8 else (if v9) v3 - enable_side_effects u1 1 + v9 = not v0 + v10 = cast v0 as u32 + v12 = if v0 then v8 else (if v9) v3 + enable_side_effects u1 1 v15, v16 = call slice_push_back(v10, v12, v2) -> (u32, [Field]) - v17 = array_get v16, index u32 0 -> Field + v17 = array_get v16, index u32 0 -> Field constrain v17 == Field 1 return } From 28a3ae3c9097e2e7ef80074deae83c593fd2960a Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 24 Sep 2025 09:57:36 +0100 Subject: [PATCH 06/15] Pre-check already asserted that this is the only block --- compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index 47612c8f06d..c53cf8b3a38 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -159,9 +159,6 @@ impl Context { fn remove_if_else(&mut self, function: &mut Function) -> RtResult<()> { let block = function.entry_block(); - // Make sure this optimization runs when there's only one block - assert_eq!(function.dfg[block].successors().count(), 0); - function.simple_optimization_result(|context| { let instruction_id = context.instruction_id; let instruction = context.instruction(); From 83d4c74378ac16005d0a78810ec786799dbe2a56 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 24 Sep 2025 10:12:22 +0100 Subject: [PATCH 07/15] Assert that the then_value has the expected type in the pre-condition --- .../noirc_evaluator/src/ssa/opt/remove_if_else.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index c53cf8b3a38..1371f94031c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -170,10 +170,6 @@ impl Context { let then_value = *then_value; let else_value = *else_value; - let typ = context.dfg.type_of_value(then_value); - // Numeric values should have been handled during flattening - assert!(!matches!(typ, Type::Numeric(_))); - let call_stack = context.dfg.get_instruction_call_stack_id(instruction_id); let mut value_merger = ValueMerger::new(context.dfg, block, &mut self.slice_sizes, call_stack); @@ -185,7 +181,6 @@ impl Context { else_value, )?; - let _typ = context.dfg.type_of_value(value); let results = context.dfg.instruction_results(instruction_id); let result = results[0]; @@ -336,13 +331,18 @@ fn remove_if_else_pre_check(func: &Function) { let instruction_ids = func.dfg[block_id].instructions(); for instruction_id in instruction_ids { - if matches!(func.dfg[*instruction_id], Instruction::IfElse { .. }) { + if let Instruction::IfElse { then_value, .. } = &func.dfg[*instruction_id] { assert!( func.dfg.instruction_results(*instruction_id).iter().all(|value| { matches!(func.dfg.type_of_value(*value), Type::Array(_, _) | Type::Slice(_)) }), "IfElse instruction returns unexpected type" ); + let typ = func.dfg.type_of_value(*then_value); + assert!( + !matches!(typ, Type::Numeric(_)), + "Numeric values should have been handled during flattening" + ); } } } From 08a8644f1b00067941bb48dc036ecfed89558fc1 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 24 Sep 2025 10:33:10 +0100 Subject: [PATCH 08/15] Add docs to some DFG methods --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 13 ++++--------- compiler/noirc_evaluator/src/ssa/ir/types.rs | 19 ++++++++++++------- .../src/ssa/opt/remove_if_else.rs | 11 ++++++----- 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index df5839ff632..4ea42f1e48d 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -603,17 +603,12 @@ impl DataFlowGraph { } } - /// Returns the Value::Array associated with this ValueId if it refers to an array constant. + /// Returns the item values in with this ValueId if it refers to an array constant, along with the type of the array item. /// Otherwise, this returns None. pub(crate) fn get_array_constant(&self, value: ValueId) -> Option<(im::Vector, Type)> { - if let Some(instruction) = self.get_local_or_global_instruction(value) { - match instruction { - Instruction::MakeArray { elements, typ } => Some((elements.clone(), typ.clone())), - _ => None, - } - } else { - // Arrays are shared, so cloning them is cheap - None + match self.get_local_or_global_instruction(value)? { + Instruction::MakeArray { elements, typ } => Some((elements.clone(), typ.clone())), + _ => None, } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/types.rs b/compiler/noirc_evaluator/src/ssa/ir/types.rs index 0648f2b5f2f..868e91f2e29 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/types.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/types.rs @@ -215,6 +215,8 @@ impl Type { /// The size of a type is defined as representing how many Fields are needed /// to represent the type. This is 1 for every primitive type, and is the number of fields /// for any flattened tuple type. + /// + /// Panics if `self` is not a [`Type::Array`] or [`Type::Slice`]. pub(crate) fn element_size(&self) -> usize { match self { Type::Array(elements, _) | Type::Slice(elements) => elements.len(), @@ -222,6 +224,16 @@ impl Type { } } + /// Return the types of items in this array/slice. + /// + /// Panics if `self` is not a [`Type::Array`] or [`Type::Slice`]. + pub(crate) fn element_types(self) -> Arc> { + match self { + Type::Array(element_types, _) | Type::Slice(element_types) => element_types, + other => panic!("element_types: Expected array or slice, found {other}"), + } + } + pub(crate) fn contains_slice_element(&self) -> bool { match self { Type::Array(elements, _) => { @@ -269,13 +281,6 @@ impl Type { } } - pub(crate) fn element_types(self) -> Arc> { - match self { - Type::Array(element_types, _) | Type::Slice(element_types) => element_types, - other => panic!("element_types: Expected array or slice, found {other}"), - } - } - pub(crate) fn first(&self) -> Type { match self { Type::Numeric(_) | Type::Function => self.clone(), diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index 1371f94031c..1105c64409f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -225,24 +225,25 @@ impl Context { }) } - //Get the tracked size of array/slices, or retrieve (and track) it for arrays. + /// Get the tracked size of array/slices, or retrieve (and track) it for arrays. fn get_or_find_capacity(&mut self, dfg: &DataFlowGraph, value: ValueId) -> u32 { match self.slice_sizes.entry(value) { Entry::Occupied(entry) => return *entry.get(), Entry::Vacant(entry) => { + // Check if the item was made by a MakeArray instruction, which can create slices as well. if let Some((array, typ)) = dfg.get_array_constant(value) { let length = array.len() / typ.element_types().len(); return *entry.insert(length as u32); } - + // For arrays we know the size statically. if let Type::Array(_, length) = dfg.type_of_value(value) { return *entry.insert(length); } + // For non-constant slices we can't tell the size, which would mean we can't merge it. + let dbg_value = &dfg[value]; + unreachable!("ICE: No size for slice {value} = {dbg_value:?}") } } - - let dbg_value = &dfg[value]; - unreachable!("No size for slice {value} = {dbg_value:?}") } } From f07a8427ef3770dcca0c13883c90b9f6f5f786d4 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 24 Sep 2025 11:00:19 +0100 Subject: [PATCH 09/15] Change SetTo to also return old and new --- .../src/ssa/opt/remove_if_else.rs | 43 ++++++++++++------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index 1105c64409f..c2388d719b9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -195,8 +195,9 @@ impl Context { match slice_capacity_change(context.dfg, intrinsic, arguments, results) { SizeChange::None => (), - SizeChange::SetTo(value, new_capacity) => { - self.slice_sizes.insert(value, new_capacity); + SizeChange::SetTo { old, new } => { + let old_capacity = self.get_or_find_capacity(context.dfg, old); + self.slice_sizes.insert(new, old_capacity); } SizeChange::Inc { old, new } => { let old_capacity = self.get_or_find_capacity(context.dfg, old); @@ -228,7 +229,7 @@ impl Context { /// Get the tracked size of array/slices, or retrieve (and track) it for arrays. fn get_or_find_capacity(&mut self, dfg: &DataFlowGraph, value: ValueId) -> u32 { match self.slice_sizes.entry(value) { - Entry::Occupied(entry) => return *entry.get(), + Entry::Occupied(entry) => *entry.get(), Entry::Vacant(entry) => { // Check if the item was made by a MakeArray instruction, which can create slices as well. if let Some((array, typ)) = dfg.get_array_constant(value) { @@ -249,12 +250,21 @@ impl Context { enum SizeChange { None, - SetTo(ValueId, u32), - - // These two variants store the old and new slice ids - // not their lengths which should be old_len = new_len +/- 1 - Inc { old: ValueId, new: ValueId }, - Dec { old: ValueId, new: ValueId }, + /// Make the size of the new slice equal to the old array. + SetTo { + old: ValueId, + new: ValueId, + }, + /// Make the size of the new slice equal to old+1. + Inc { + old: ValueId, + new: ValueId, + }, + /// Make the size of the new slice equal to old-1. + Dec { + old: ValueId, + new: ValueId, + }, } /// Find the change to a slice's capacity an instruction would have @@ -276,6 +286,8 @@ fn slice_capacity_change( } Intrinsic::SlicePopBack | Intrinsic::SliceRemove => { + // Expecting: len, slice, value = ... + assert_eq!(results.len(), 3); let old = arguments[1]; let new = results[1]; assert!(matches!(dfg.type_of_value(old), Type::Slice(_))); @@ -284,6 +296,8 @@ fn slice_capacity_change( } Intrinsic::SlicePopFront => { + // Expecting: value, len, slice = ... + assert_eq!(results.len(), 3); let old = arguments[1]; let new = results[results.len() - 1]; assert!(matches!(dfg.type_of_value(old), Type::Slice(_))); @@ -294,12 +308,11 @@ fn slice_capacity_change( Intrinsic::AsSlice => { assert_eq!(arguments.len(), 1); assert_eq!(results.len(), 2); - let length = match dfg.type_of_value(arguments[0]) { - Type::Array(_, length) => length, - other => unreachable!("slice_capacity_change expected array, found {other:?}"), - }; - assert!(matches!(dfg.type_of_value(results[1]), Type::Slice(_))); - SizeChange::SetTo(results[1], length) + let old = arguments[0]; + let new = results[1]; + assert!(matches!(dfg.type_of_value(old), Type::Array(_, _))); + assert!(matches!(dfg.type_of_value(new), Type::Slice(_))); + SizeChange::SetTo { old, new } } // These cases don't affect slice capacities From 1530509daf6a902b354d4135c8bbdaa1043fd549 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 24 Sep 2025 11:05:14 +0100 Subject: [PATCH 10/15] ArraySet only has 1 result --- compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index c2388d719b9..c1e13ea34b4 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -215,7 +215,7 @@ impl Context { // Track slice sizes through array set instructions Instruction::ArraySet { array, .. } => { let results = context.dfg.instruction_results(instruction_id); - let result = if results.len() == 2 { results[1] } else { results[0] }; + let result = results[0]; let old_capacity = self.get_or_find_capacity(context.dfg, *array); self.slice_sizes.insert(result, old_capacity); From dc3f32c3635b01b5b6542870172a2eba173a0961 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 24 Sep 2025 11:21:37 +0100 Subject: [PATCH 11/15] ValueMerger doesn't mutate the slice sizes map --- .../src/ssa/opt/flatten_cfg/value_merger.rs | 17 +++++++++-------- .../src/ssa/opt/remove_if_else.rs | 2 +- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs index bd6890a0045..3089a847eca 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -16,9 +16,9 @@ pub(crate) struct ValueMerger<'a> { dfg: &'a mut DataFlowGraph, block: BasicBlockId, - // Maps SSA array values with a slice type to their size. - // This must be computed before merging values. - slice_sizes: &'a mut HashMap, + /// Maps SSA array values with a slice type to their size. + /// This must be computed before merging values. + slice_sizes: &'a HashMap, call_stack: CallStackId, } @@ -27,7 +27,7 @@ impl<'a> ValueMerger<'a> { pub(crate) fn new( dfg: &'a mut DataFlowGraph, block: BasicBlockId, - slice_sizes: &'a mut HashMap, + slice_sizes: &'a HashMap, call_stack: CallStackId, ) -> Self { ValueMerger { dfg, block, slice_sizes, call_stack } @@ -144,14 +144,15 @@ impl<'a> ValueMerger<'a> { let mut merged = im::Vector::new(); let (element_types, len) = match &typ { - Type::Array(elements, len) => (elements, *len), + Type::Array(elements, len) => (elements.as_slice(), *len), _ => panic!("Expected array type"), }; + let element_count = element_types.len() as u32; + for i in 0..len { for (element_index, element_type) in element_types.iter().enumerate() { - let index = - u128::from(i * element_types.len() as u32 + element_index as u32).into(); + let index = u128::from(i * element_count + element_index as u32).into(); let index = self.dfg.make_constant(index, NumericType::length_type()); let typevars = Some(vec![element_type.clone()]); @@ -192,7 +193,7 @@ impl<'a> ValueMerger<'a> { let mut merged = im::Vector::new(); let element_types = match &typ { - Type::Slice(elements) => elements, + Type::Slice(elements) => elements.as_slice(), _ => panic!("Expected slice type"), }; diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index c1e13ea34b4..7e9b5f2d93f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -172,7 +172,7 @@ impl Context { let call_stack = context.dfg.get_instruction_call_stack_id(instruction_id); let mut value_merger = - ValueMerger::new(context.dfg, block, &mut self.slice_sizes, call_stack); + ValueMerger::new(context.dfg, block, &self.slice_sizes, call_stack); let value = value_merger.merge_values( then_condition, From f91a234bad8d58479017613d0fe21bc92df49b3d Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 24 Sep 2025 11:44:56 +0100 Subject: [PATCH 12/15] Make sure the slices have a capacity before merging --- .../src/ssa/ir/dfg/simplify/call.rs | 2 +- .../src/ssa/opt/flatten_cfg/value_merger.rs | 15 +++---- .../src/ssa/opt/remove_if_else.rs | 40 +++++++++++++------ 3 files changed, 34 insertions(+), 23 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/call.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/call.rs index c0888484a21..f6f4b52fd8d 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/call.rs @@ -521,7 +521,7 @@ fn simplify_slice_push_back( slice_sizes.insert(set_last_slice_value, slice_size / element_size); slice_sizes.insert(new_slice, slice_size / element_size); - let mut value_merger = ValueMerger::new(dfg, block, &mut slice_sizes, call_stack); + let mut value_merger = ValueMerger::new(dfg, block, &slice_sizes, call_stack); let Ok(new_slice) = value_merger.merge_values( len_not_equals_capacity, diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs index 3089a847eca..15462c478be 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -132,7 +132,7 @@ impl<'a> ValueMerger<'a> { /// Given an if expression that returns an array: `if c { array1 } else { array2 }`, /// this function will recursively merge array1 and array2 into a single resulting array - /// by creating a new array containing the result of self.merge_values for each element. + /// by creating a new array containing the result of `self.merge_values` for each element. pub(crate) fn merge_array_values( &mut self, typ: Type, @@ -198,27 +198,22 @@ impl<'a> ValueMerger<'a> { }; let then_len = self.slice_sizes.get(&then_value_id).copied().unwrap_or_else(|| { - let (slice, typ) = self.dfg.get_array_constant(then_value_id).unwrap_or_else(|| { - panic!("ICE: Merging values during flattening encountered slice {then_value_id} without a preset size"); - }); - (slice.len() / typ.element_types().len()) as u32 + panic!("ICE: Merging values during flattening encountered slice {then_value_id} without a preset size"); }); let else_len = self.slice_sizes.get(&else_value_id).copied().unwrap_or_else(|| { - let (slice, typ) = self.dfg.get_array_constant(else_value_id).unwrap_or_else(|| { - panic!("ICE: Merging values during flattening encountered slice {else_value_id} without a preset size"); - }); - (slice.len() / typ.element_types().len()) as u32 + panic!("ICE: Merging values during flattening encountered slice {else_value_id} without a preset size"); }); let len = then_len.max(else_len); + let element_count = element_types.len() as u32; let flattened_then_length = then_len * element_types.len() as u32; let flattened_else_length = else_len * element_types.len() as u32; for i in 0..len { for (element_index, element_type) in element_types.iter().enumerate() { - let index_u32 = i * element_types.len() as u32 + element_index as u32; + let index_u32 = i * element_count + element_index as u32; let index_value = u128::from(index_u32).into(); let index = self.dfg.make_constant(index_value, NumericType::length_type()); diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index 7e9b5f2d93f..ef9abc50a69 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -170,6 +170,10 @@ impl Context { let then_value = *then_value; let else_value = *else_value; + // Register values for the merger to use. + self.ensure_capacity(context.dfg, then_value); + self.ensure_capacity(context.dfg, else_value); + let call_stack = context.dfg.get_instruction_call_stack_id(instruction_id); let mut value_merger = ValueMerger::new(context.dfg, block, &self.slice_sizes, call_stack); @@ -196,18 +200,15 @@ impl Context { match slice_capacity_change(context.dfg, intrinsic, arguments, results) { SizeChange::None => (), SizeChange::SetTo { old, new } => { - let old_capacity = self.get_or_find_capacity(context.dfg, old); - self.slice_sizes.insert(new, old_capacity); + self.set_capacity(context.dfg, old, new, |c| c); } SizeChange::Inc { old, new } => { - let old_capacity = self.get_or_find_capacity(context.dfg, old); - self.slice_sizes.insert(new, old_capacity + 1); + self.set_capacity(context.dfg, old, new, |c| c + 1); } SizeChange::Dec { old, new } => { - let old_capacity = self.get_or_find_capacity(context.dfg, old); // We use a saturating sub here as calling `pop_front` or `pop_back` on a zero-length slice // would otherwise underflow. - self.slice_sizes.insert(new, old_capacity.saturating_sub(1)); + self.set_capacity(context.dfg, old, new, |c| c.saturating_sub(1)); } } } @@ -216,9 +217,7 @@ impl Context { Instruction::ArraySet { array, .. } => { let results = context.dfg.instruction_results(instruction_id); let result = results[0]; - - let old_capacity = self.get_or_find_capacity(context.dfg, *array); - self.slice_sizes.insert(result, old_capacity); + self.set_capacity(context.dfg, *array, result, |c| c); } _ => (), } @@ -226,6 +225,24 @@ impl Context { }) } + /// Set the capacity of the new slice based on the capacity of the old array/slice. + fn set_capacity( + &mut self, + dfg: &DataFlowGraph, + old: ValueId, + new: ValueId, + f: impl Fn(u32) -> u32, + ) { + let capacity = self.get_or_find_capacity(dfg, old); + self.slice_sizes.insert(new, f(capacity)); + } + + /// Make sure the capacity is recorded. + fn ensure_capacity(&mut self, dfg: &DataFlowGraph, slice: ValueId) { + let capacity = self.get_or_find_capacity(dfg, slice); + self.slice_sizes.insert(slice, capacity); + } + /// Get the tracked size of array/slices, or retrieve (and track) it for arrays. fn get_or_find_capacity(&mut self, dfg: &DataFlowGraph, value: ValueId) -> u32 { match self.slice_sizes.entry(value) { @@ -286,7 +303,7 @@ fn slice_capacity_change( } Intrinsic::SlicePopBack | Intrinsic::SliceRemove => { - // Expecting: len, slice, value = ... + // Expecting: len, slice, ...value = ... assert_eq!(results.len(), 3); let old = arguments[1]; let new = results[1]; @@ -296,8 +313,7 @@ fn slice_capacity_change( } Intrinsic::SlicePopFront => { - // Expecting: value, len, slice = ... - assert_eq!(results.len(), 3); + // Expecting: ...value, len, slice = ... let old = arguments[1]; let new = results[results.len() - 1]; assert!(matches!(dfg.type_of_value(old), Type::Slice(_))); From ca15cbc96dd700e477bbb552307dd875aa375781 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 24 Sep 2025 11:49:03 +0100 Subject: [PATCH 13/15] Format code a bit --- .../src/ssa/opt/flatten_cfg/value_merger.rs | 39 ++++++++----------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs index 15462c478be..966f8e0819a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -208,8 +208,8 @@ impl<'a> ValueMerger<'a> { let len = then_len.max(else_len); let element_count = element_types.len() as u32; - let flattened_then_length = then_len * element_types.len() as u32; - let flattened_else_length = else_len * element_types.len() as u32; + let flat_then_length = then_len * element_types.len() as u32; + let flat_else_length = else_len * element_types.len() as u32; for i in 0..len { for (element_index, element_type) in element_types.iter().enumerate() { @@ -220,40 +220,35 @@ impl<'a> ValueMerger<'a> { let typevars = Some(vec![element_type.clone()]); let mut get_element = |array, typevars, len| { - if len <= index_u32 { - panic!("get_element invoked with an out of bounds index"); - } else { - let get = Instruction::ArrayGet { array, index }; - let results = self.dfg.insert_instruction_and_results( - get, - self.block, - typevars, - self.call_stack, - ); - results.first() - } + assert!(index_u32 < len, "get_element invoked with an out of bounds index"); + let get = Instruction::ArrayGet { array, index }; + let results = self.dfg.insert_instruction_and_results( + get, + self.block, + typevars, + self.call_stack, + ); + results.first() }; // If it's out of bounds for the "then" slice, a value in the "else" *must* exist. // We can use that value directly as accessing it is always checked against the actual // slice length. - if index_u32 >= flattened_then_length { - let else_element = get_element(else_value_id, typevars, flattened_else_length); + if index_u32 >= flat_then_length { + let else_element = get_element(else_value_id, typevars, flat_else_length); merged.push_back(else_element); continue; } // Same for if it's out of bounds for the "else" slice. - if index_u32 >= flattened_else_length { - let then_element = - get_element(then_value_id, typevars.clone(), flattened_then_length); + if index_u32 >= flat_else_length { + let then_element = get_element(then_value_id, typevars, flat_then_length); merged.push_back(then_element); continue; } - let then_element = - get_element(then_value_id, typevars.clone(), flattened_then_length); - let else_element = get_element(else_value_id, typevars, flattened_else_length); + let then_element = get_element(then_value_id, typevars.clone(), flat_then_length); + let else_element = get_element(else_value_id, typevars, flat_else_length); merged.push_back(self.merge_values( then_condition, From 85d08db0ac9ead5466d19004847f1f4f00fc5c03 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 24 Sep 2025 14:52:57 +0100 Subject: [PATCH 14/15] Add documentation about the two/one complement shifting --- .../src/ssa/opt/remove_bit_shifts.rs | 44 ++++++++++++++----- 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs index b1b602eaab4..8a6c7c44798 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -217,8 +217,28 @@ impl Context<'_, '_, '_> { } /// Insert SSA instructions which computes lhs >> rhs by doing lhs/2^rhs - /// For negative signed integers, we do the division on the 1-complement representation of lhs, - /// before converting back the result to the 2-complement representation. + /// + /// For negative signed integers, we do the shifting using a technique based on how dividing a + /// 2-complement value can be done by converting to the 1-complement representation of lhs, + /// shifting, then converting back the result to the 2-complement representation. + /// + /// To understand the algorithm, take a look at how division works on pages 7-8 of + /// + /// + /// Division for a negative number represented as a 2-complement is implemented by the following steps: + /// 1. Convert to 1-complement by subtracting 1 from the value + /// 2. Shift right by the number of bits corresponding to the divisor + /// 3. Convert back to 2-complement by adding 1 to the result + /// + /// That's division in terms of shifting; we need shifting in terms of division. The following steps show how: + /// * `DIV(a) = SHR(a-1)+1` + /// * `SHR(a-1) = DIV(a)-1` + /// * `SHR(a) = DIV(a+1)-1` + /// + /// Hence we handle negative values in shifting by: + /// 1. Adding 1 to the value + /// 2. Dividing by 2^rhs + /// 3. Subtracting 1 from the result fn insert_shift_right(&mut self, lhs: ValueId, rhs: ValueId) -> ValueId { let lhs_typ = self.context.dfg.type_of_value(lhs).unwrap_numeric(); @@ -234,24 +254,24 @@ impl Context<'_, '_, '_> { // Get the sign of the operand; positive signed operand will just do a division as well let zero = self.numeric_constant(FieldElement::zero(), NumericType::signed(bit_size)); + // The sign will be 0 for positive numbers and 1 for negatives, so it covers both cases. let lhs_sign = self.insert_binary(lhs, BinaryOp::Lt, zero); let lhs_sign_as_field = self.insert_cast(lhs_sign, NumericType::NativeField); let lhs_as_field = self.insert_cast(lhs, NumericType::NativeField); - // For negative numbers, convert to 1-complement using wrapping addition of a + 1 - // Unchecked add as these are fields + // For negative numbers, we prepare for the division using a wrapping addition of a + 1. Unchecked add as these are fields. let add = BinaryOp::Add { unchecked: true }; - let one_complement = self.insert_binary(lhs_sign_as_field, add, lhs_as_field); - let one_complement = self.insert_truncate(one_complement, bit_size, bit_size + 1); - let one_complement = - self.insert_cast(one_complement, NumericType::signed(bit_size)); - // Performs the division on the 1-complement (or the operand if positive) - let shifted_complement = self.insert_binary(one_complement, BinaryOp::Div, pow); - // Convert back to 2-complement representation if operand is negative + let div_complement = self.insert_binary(lhs_sign_as_field, add, lhs_as_field); + let div_complement = self.insert_truncate(div_complement, bit_size, bit_size + 1); + let div_complement = + self.insert_cast(div_complement, NumericType::signed(bit_size)); + // Performs the division on the adjusted complement (or the operand if positive) + let shifted_complement = self.insert_binary(div_complement, BinaryOp::Div, pow); + // For negative numbers, convert back to 2-complement by subtracting 1. let lhs_sign_as_int = self.insert_cast(lhs_sign, lhs_typ); // The requirements for this to underflow are all of these: // - lhs < 0 - // - ones_complement(lhs) / (2^rhs) == 0 + // - div_complement(lhs) / (2^rhs) == 0 // As the upper bit is set for the ones complement of negative numbers we'd need 2^rhs // to be larger than the lhs bitsize for this to overflow. let sub = BinaryOp::Sub { unchecked: true }; From f5f411a416d1b111696c5dd1e067e1e16914b31d Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 24 Sep 2025 19:05:21 +0100 Subject: [PATCH 15/15] Do not store the capacity of arrays --- .../src/ssa/opt/remove_if_else.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index 4f654b7175d..d61d81f9b54 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -249,14 +249,17 @@ impl Context { new: ValueId, f: impl Fn(u32) -> u32, ) { + // No need to store the capacity of arrays, only slices. + if !matches!(dfg.type_of_value(new), Type::Slice(_)) { + return; + } let capacity = self.get_or_find_capacity(dfg, old); self.slice_sizes.insert(new, f(capacity)); } - /// Make sure the capacity is recorded. + /// Make sure the slice capacity is recorded. fn ensure_capacity(&mut self, dfg: &DataFlowGraph, slice: ValueId) { - let capacity = self.get_or_find_capacity(dfg, slice); - self.slice_sizes.insert(slice, capacity); + self.set_capacity(dfg, slice, slice, |c| c); } /// Get the tracked size of array/slices, or retrieve (and track) it for arrays. @@ -264,15 +267,15 @@ impl Context { match self.slice_sizes.entry(value) { Entry::Occupied(entry) => *entry.get(), Entry::Vacant(entry) => { + // For arrays we know the size statically, and we don't need to store it. + if let Type::Array(_, length) = dfg.type_of_value(value) { + return length; + } // Check if the item was made by a MakeArray instruction, which can create slices as well. if let Some((array, typ)) = dfg.get_array_constant(value) { let length = array.len() / typ.element_types().len(); return *entry.insert(length as u32); } - // For arrays we know the size statically. - if let Type::Array(_, length) = dfg.type_of_value(value) { - return *entry.insert(length); - } // For non-constant slices we can't tell the size, which would mean we can't merge it. let dbg_value = &dfg[value]; unreachable!("ICE: No size for slice {value} = {dbg_value:?}")