diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index a7a4e03af9..3feeaba58b 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1061,7 +1061,7 @@ impl Context { found: array_typ.to_string(), call_stack: self.acir_context.get_call_stack(), } - .into()) + .into()); } } // The final array should will the flattened index at each outer array index @@ -1654,8 +1654,18 @@ impl Context { let mut new_slice = Vector::new(); self.slice_intrinsic_input(&mut new_slice, slice)?; - // TODO(#2461): make sure that we have handled nested struct inputs - new_slice.insert(index, element); + + // We do not return an index out of bounds error directly here + // as the length of the slice is dynamic, and length of `new_slice` + // represents the capacity of the slice, not the actual length. + // + // Constraints should be generated during SSA gen to tell the user + // they are attempting to insert at too large of an index. + // This check prevents a panic inside of the im::Vector insert method. + if index <= new_slice.len() { + // TODO(#2461): make sure that we have handled nested struct inputs + new_slice.insert(index, element); + } Ok(vec![ AcirValue::Var(new_slice_length, AcirType::field()), @@ -1682,8 +1692,22 @@ impl Context { let mut new_slice = Vector::new(); self.slice_intrinsic_input(&mut new_slice, slice)?; - // TODO(#2461): make sure that we have handled nested struct inputs - let removed_elem = new_slice.remove(index); + + // We do not return an index out of bounds error directly here + // as the length of the slice is dynamic, and length of `new_slice` + // represents the capacity of the slice, not the actual length. + // + // Constraints should be generated during SSA gen to tell the user + // they are attempting to remove at too large of an index. + // This check prevents a panic inside of the im::Vector remove method. + let removed_elem = if index < new_slice.len() { + // TODO(#2461): make sure that we have handled nested struct inputs + new_slice.remove(index) + } else { + // This is a dummy value which should never be used if the appropriate + // slice access checks are generated before this slice remove call. + AcirValue::Var(slice_length, AcirType::field()) + }; Ok(vec![ AcirValue::Var(new_slice_length, AcirType::field()), diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 6ccab6638f..3aa95e9f6e 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -341,6 +341,13 @@ impl FunctionBuilder { pub(crate) fn import_intrinsic_id(&mut self, intrinsic: Intrinsic) -> ValueId { self.current_function.dfg.import_intrinsic(intrinsic) } + + pub(crate) fn get_intrinsic_from_value(&mut self, value: ValueId) -> Option { + match self.current_function.dfg[value] { + Value::Intrinsic(intrinsic) => Some(intrinsic), + _ => None, + } + } } impl std::ops::Index for FunctionBuilder { diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 52a39c9e40..26d6739902 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -4,13 +4,16 @@ use acvm::{acir::BlackBoxFunc, BlackBoxResolutionError, FieldElement}; use iter_extended::vecmap; use num_bigint::BigUint; -use crate::ssa::ir::{ - basic_block::BasicBlockId, - dfg::DataFlowGraph, - instruction::Intrinsic, - map::Id, - types::Type, - value::{Value, ValueId}, +use crate::ssa::{ + ir::{ + basic_block::BasicBlockId, + dfg::{CallStack, DataFlowGraph}, + instruction::Intrinsic, + map::Id, + types::Type, + value::{Value, ValueId}, + }, + opt::flatten_cfg::value_merger::ValueMerger, }; use super::{Binary, BinaryOp, Endian, Instruction, SimplifyResult}; @@ -92,14 +95,22 @@ pub(super) fn simplify_call( Intrinsic::SlicePushBack => { let slice = dfg.get_array_constant(arguments[1]); if let Some((mut slice, element_type)) = slice { - for elem in &arguments[2..] { - slice.push_back(*elem); + // TODO(#2752): We need to handle the element_type size to appropriately handle slices of complex types. + // This is reliant on dynamic indices of non-homogenous slices also being implemented. + if element_type.element_size() != 1 { + // Old code before implementing multiple slice mergers + for elem in &arguments[2..] { + slice.push_back(*elem); + } + + let new_slice_length = + update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + + let new_slice = dfg.make_array(slice, element_type); + return SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]); } - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); - - let new_slice = dfg.make_array(slice, element_type); - SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) + simplify_slice_push_back(slice, element_type, arguments, dfg, block) } else { SimplifyResult::None } @@ -121,25 +132,8 @@ pub(super) fn simplify_call( } Intrinsic::SlicePopBack => { let slice = dfg.get_array_constant(arguments[1]); - if let Some((mut slice, typ)) = slice { - let element_count = typ.element_size(); - let mut results = VecDeque::with_capacity(element_count + 1); - - // We must pop multiple elements in the case of a slice of tuples - for _ in 0..element_count { - let elem = slice - .pop_back() - .expect("There are no elements in this slice to be removed"); - results.push_front(elem); - } - - let new_slice = dfg.make_array(slice, typ); - results.push_front(new_slice); - - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); - - results.push_front(new_slice_length); - SimplifyResult::SimplifiedToMultiple(results.into()) + if let Some((_, typ)) = slice { + simplify_slice_pop_back(typ, arguments, dfg, block) } else { SimplifyResult::None } @@ -174,6 +168,14 @@ pub(super) fn simplify_call( let elements = &arguments[3..]; let mut index = index.to_u128() as usize * elements.len(); + // Do not simplify the index is greater than the slice capacity + // or else we will panic inside of the im::Vector insert method + // Constraints should be generated during SSA gen to tell the user + // they are attempting to insert at too large of an index + if index > slice.len() { + return SimplifyResult::None; + } + for elem in &arguments[3..] { slice.insert(index, *elem); index += 1; @@ -195,6 +197,14 @@ pub(super) fn simplify_call( let mut results = Vec::with_capacity(element_count + 1); let index = index.to_u128() as usize * element_count; + // Do not simplify if the index is not less than the slice capacity + // or else we will panic inside of the im::Vector remove method. + // Constraints should be generated during SSA gen to tell the user + // they are attempting to remove at too large of an index. + if index >= slice.len() { + return SimplifyResult::None; + } + for _ in 0..element_count { results.push(slice.remove(index)); } @@ -257,6 +267,103 @@ fn update_slice_length( dfg.insert_instruction_and_results(instruction, block, None, call_stack).first() } +fn simplify_slice_push_back( + mut slice: im::Vector, + element_type: Type, + arguments: &[ValueId], + dfg: &mut DataFlowGraph, + block: BasicBlockId, +) -> SimplifyResult { + // The capacity must be an integer so that we can compare it against the slice length which is represented as a field + let capacity = dfg.make_constant((slice.len() as u128).into(), Type::unsigned(64)); + let len_equals_capacity_instr = + Instruction::Binary(Binary { lhs: arguments[0], operator: BinaryOp::Eq, rhs: capacity }); + let call_stack = dfg.get_value_call_stack(arguments[0]); + let len_equals_capacity = dfg + .insert_instruction_and_results(len_equals_capacity_instr, block, None, call_stack.clone()) + .first(); + let len_not_equals_capacity_instr = Instruction::Not(len_equals_capacity); + let len_not_equals_capacity = dfg + .insert_instruction_and_results( + len_not_equals_capacity_instr, + block, + None, + call_stack.clone(), + ) + .first(); + + let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + + for elem in &arguments[2..] { + slice.push_back(*elem); + } + let new_slice = dfg.make_array(slice, element_type); + + let set_last_slice_value_instr = + Instruction::ArraySet { array: new_slice, index: arguments[0], value: arguments[2] }; + let set_last_slice_value = dfg + .insert_instruction_and_results(set_last_slice_value_instr, block, None, call_stack) + .first(); + + let mut value_merger = ValueMerger::new(dfg, block, None, None); + let new_slice = value_merger.merge_values( + len_not_equals_capacity, + len_equals_capacity, + set_last_slice_value, + new_slice, + ); + + SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) +} + +fn simplify_slice_pop_back( + element_type: Type, + arguments: &[ValueId], + dfg: &mut DataFlowGraph, + block: BasicBlockId, +) -> SimplifyResult { + let element_types = match element_type.clone() { + Type::Slice(element_types) | Type::Array(element_types, _) => element_types, + _ => { + unreachable!("ICE: Expected slice or array, but got {element_type}"); + } + }; + + let element_count = element_type.element_size(); + let mut results = VecDeque::with_capacity(element_count + 1); + + let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); + + let element_size = dfg.make_constant((element_count as u128).into(), Type::field()); + let flattened_len_instr = Instruction::binary(BinaryOp::Mul, arguments[0], element_size); + let mut flattened_len = dfg + .insert_instruction_and_results(flattened_len_instr, block, None, CallStack::new()) + .first(); + flattened_len = update_slice_length(flattened_len, dfg, BinaryOp::Sub, block); + + // We must pop multiple elements in the case of a slice of tuples + for _ in 0..element_count { + let get_last_elem_instr = + Instruction::ArrayGet { array: arguments[1], index: flattened_len }; + let get_last_elem = dfg + .insert_instruction_and_results( + get_last_elem_instr, + block, + Some(element_types.to_vec()), + CallStack::new(), + ) + .first(); + results.push_front(get_last_elem); + + flattened_len = update_slice_length(flattened_len, dfg, BinaryOp::Sub, block); + } + + results.push_front(arguments[1]); + + results.push_front(new_slice_length); + SimplifyResult::SimplifiedToMultiple(results.into()) +} + /// Try to simplify this black box call. If the call can be simplified to a known value, /// that value is returned. Otherwise [`SimplifyResult::None`] is returned. fn simplify_black_box_func( diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index a786f6c308..8d3fda3cfe 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -144,14 +144,17 @@ use crate::ssa::{ dfg::{CallStack, InsertInstructionResult}, function::Function, function_inserter::FunctionInserter, - instruction::{BinaryOp, Instruction, InstructionId, Intrinsic, TerminatorInstruction}, + instruction::{BinaryOp, Instruction, InstructionId, TerminatorInstruction}, types::Type, - value::{Value, ValueId}, + value::ValueId, }, ssa_gen::Ssa, }; mod branch_analysis; +pub(crate) mod value_merger; + +use value_merger::ValueMerger; impl Ssa { /// Flattens the control flow graph of main such that the function is left with a @@ -189,7 +192,7 @@ struct Context<'f> { /// are overwritten as new stores are found. This overwriting is the desired behavior, /// as we want the most update to date value to be stored at a given address as /// we walk through blocks to flatten. - outer_block_stores: HashMap, + outer_block_stores: HashMap, /// Stores all allocations local to the current branch. /// Since these branches are local to the current branch (ie. only defined within one branch of @@ -207,7 +210,7 @@ struct Context<'f> { conditions: Vec<(BasicBlockId, ValueId)>, } -struct Store { +pub(crate) struct Store { old_value: ValueId, new_value: ValueId, } @@ -270,10 +273,7 @@ impl<'f> Context<'f> { for instruction in instructions { let (instruction, _) = self.inserter.map_instruction(instruction); if let Instruction::Store { address, value } = instruction { - let load = Instruction::Load { address }; - let load_type = Some(vec![self.inserter.function.dfg.type_of_value(value)]); - let old_value = self.insert_instruction_with_typevars(load, load_type).first(); - self.outer_block_stores.insert(address, Store { old_value, new_value: value }); + self.outer_block_stores.insert(address, value); } } } @@ -400,270 +400,6 @@ impl<'f> Context<'f> { self.insert_instruction_with_typevars(enable_side_effects, None); } - /// Merge two values a and b from separate basic blocks to a single value. - /// If these two values are numeric, the result will be - /// `then_condition * then_value + else_condition * else_value`. - /// Otherwise, if the values being merged are arrays, a new array will be made - /// recursively from combining each element of both input arrays. - /// - /// It is currently an error to call this function on reference or function values - /// as it is less clear how to merge these. - fn merge_values( - &mut self, - then_condition: ValueId, - else_condition: ValueId, - then_value: ValueId, - else_value: ValueId, - ) -> ValueId { - match self.inserter.function.dfg.type_of_value(then_value) { - Type::Numeric(_) => { - self.merge_numeric_values(then_condition, else_condition, then_value, else_value) - } - typ @ Type::Array(_, _) => { - self.merge_array_values(typ, then_condition, else_condition, then_value, else_value) - } - typ @ Type::Slice(_) => { - self.merge_slice_values(typ, then_condition, else_condition, then_value, else_value) - } - Type::Reference => panic!("Cannot return references from an if expression"), - Type::Function => panic!("Cannot return functions from an if expression"), - } - } - - fn merge_slice_values( - &mut self, - typ: Type, - then_condition: ValueId, - else_condition: ValueId, - then_value_id: ValueId, - else_value_id: ValueId, - ) -> ValueId { - let mut merged = im::Vector::new(); - - let element_types = match &typ { - Type::Slice(elements) => elements, - _ => panic!("Expected slice type"), - }; - - let then_len = self.get_slice_length(then_value_id); - let else_len = self.get_slice_length(else_value_id); - - let len = then_len.max(else_len); - - for i in 0..len { - for (element_index, element_type) in element_types.iter().enumerate() { - let index_value = ((i * element_types.len() + element_index) as u128).into(); - let index = self.inserter.function.dfg.make_constant(index_value, Type::field()); - - let typevars = Some(vec![element_type.clone()]); - - let mut get_element = |array, typevars, len| { - // The smaller slice is filled with placeholder data. Codegen for slice accesses must - // include checks against the dynamic slice length so that this placeholder data is not incorrectly accessed. - if len <= index_value.to_u128() as usize { - self.make_slice_dummy_data(element_type) - } else { - let get = Instruction::ArrayGet { array, index }; - self.insert_instruction_with_typevars(get, typevars).first() - } - }; - - let then_element = get_element(then_value_id, typevars.clone(), then_len); - let else_element = get_element(else_value_id, typevars, else_len); - - merged.push_back(self.merge_values( - then_condition, - else_condition, - then_element, - else_element, - )); - } - } - - self.inserter.function.dfg.make_array(merged, typ) - } - - /// Construct a dummy value to be attached to the smaller of two slices being merged. - /// We need to make sure we follow the internal element type structure of the slice type - /// even for dummy data to ensure that we do not have errors later in the compiler, - /// such as with dynamic indexing of non-homogenous slices. - fn make_slice_dummy_data(&mut self, typ: &Type) -> ValueId { - match typ { - Type::Numeric(_) => { - let zero = FieldElement::zero(); - self.inserter.function.dfg.make_constant(zero, Type::field()) - } - Type::Array(element_types, len) => { - let mut array = im::Vector::new(); - for _ in 0..*len { - for typ in element_types.iter() { - array.push_back(self.make_slice_dummy_data(typ)); - } - } - self.inserter.function.dfg.make_array(array, typ.clone()) - } - Type::Slice(_) => { - unreachable!("ICE: Slices of slice is unsupported") - } - Type::Reference => { - unreachable!("ICE: Merging references is unsupported") - } - Type::Function => { - unreachable!("ICE: Merging functions is unsupported") - } - } - } - - fn get_slice_length(&mut self, value_id: ValueId) -> usize { - let value = &self.inserter.function.dfg[value_id]; - match value { - Value::Array { array, .. } => array.len(), - Value::Instruction { instruction: instruction_id, .. } => { - let instruction = &self.inserter.function.dfg[*instruction_id]; - match instruction { - Instruction::ArraySet { array, .. } => self.get_slice_length(*array), - Instruction::Load { address } => { - let context_store = if let Some(store) = self.store_values.get(address) { - store - } else { - self.outer_block_stores - .get(address) - .expect("ICE: load in merger should have store from outer block") - }; - - self.get_slice_length(context_store.new_value) - } - Instruction::Call { func, arguments } => { - let func = &self.inserter.function.dfg[*func]; - let slice_contents = arguments[1]; - match func { - Value::Intrinsic(intrinsic) => match intrinsic { - Intrinsic::SlicePushBack - | Intrinsic::SlicePushFront - | Intrinsic::SliceInsert => { - self.get_slice_length(slice_contents) + 1 - } - Intrinsic::SlicePopBack - | Intrinsic::SlicePopFront - | Intrinsic::SliceRemove => { - self.get_slice_length(slice_contents) - 1 - } - _ => { - unreachable!("ICE: Intrinsic not supported, got {intrinsic:?}") - } - }, - _ => unreachable!("ICE: Expected intrinsic value but got {func:?}"), - } - } - _ => unreachable!("ICE: Got unexpected instruction: {instruction:?}"), - } - } - _ => unreachable!("ICE: Got unexpected value when resolving slice length {value:?}"), - } - } - - /// 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. - fn merge_array_values( - &mut self, - typ: Type, - then_condition: ValueId, - else_condition: ValueId, - then_value: ValueId, - else_value: ValueId, - ) -> ValueId { - let mut merged = im::Vector::new(); - - let (element_types, len) = match &typ { - Type::Array(elements, len) => (elements, *len), - _ => panic!("Expected array type"), - }; - - for i in 0..len { - for (element_index, element_type) in element_types.iter().enumerate() { - let index: FieldElement = - ((i * element_types.len() + element_index) as u128).into(); - let index = self.inserter.function.dfg.make_constant(index, Type::field()); - - let typevars = Some(vec![element_type.clone()]); - - let mut get_element = |array, typevars| { - let get = Instruction::ArrayGet { array, index }; - self.insert_instruction_with_typevars(get, typevars).first() - }; - - let then_element = get_element(then_value, typevars.clone()); - let else_element = get_element(else_value, typevars); - - merged.push_back(self.merge_values( - then_condition, - else_condition, - then_element, - else_element, - )); - } - } - - self.inserter.function.dfg.make_array(merged, typ) - } - - /// Merge two numeric values a and b from separate basic blocks to a single value. This - /// function would return the result of `if c { a } else { b }` as `c*a + (!c)*b`. - fn merge_numeric_values( - &mut self, - then_condition: ValueId, - else_condition: ValueId, - then_value: ValueId, - else_value: ValueId, - ) -> ValueId { - let block = self.inserter.function.entry_block(); - let then_type = self.inserter.function.dfg.type_of_value(then_value); - let else_type = self.inserter.function.dfg.type_of_value(else_value); - assert_eq!( - then_type, else_type, - "Expected values merged to be of the same type but found {then_type} and {else_type}" - ); - - let then_call_stack = self.inserter.function.dfg.get_value_call_stack(then_value); - let else_call_stack = self.inserter.function.dfg.get_value_call_stack(else_value); - - let call_stack = if then_call_stack.is_empty() { - else_call_stack.clone() - } else { - then_call_stack.clone() - }; - - // We must cast the bool conditions to the actual numeric type used by each value. - let then_condition = - self.insert_instruction(Instruction::Cast(then_condition, then_type), then_call_stack); - let else_condition = - self.insert_instruction(Instruction::Cast(else_condition, else_type), else_call_stack); - - let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value); - let then_value = self - .inserter - .function - .dfg - .insert_instruction_and_results(mul, block, None, call_stack.clone()) - .first(); - - let mul = Instruction::binary(BinaryOp::Mul, else_condition, else_value); - let else_value = self - .inserter - .function - .dfg - .insert_instruction_and_results(mul, block, None, call_stack.clone()) - .first(); - - let add = Instruction::binary(BinaryOp::Add, then_value, else_value); - self.inserter - .function - .dfg - .insert_instruction_and_results(add, block, None, call_stack) - .first() - } - /// Inline one branch of a jmpif instruction. /// /// This will continue inlining recursively until the next end block is reached where each branch @@ -754,9 +490,22 @@ impl<'f> Context<'f> { (self.inserter.resolve(*then_arg), self.inserter.resolve(else_arg)) }); + let block = self.inserter.function.entry_block(); + let mut value_merger = ValueMerger::new( + &mut self.inserter.function.dfg, + block, + Some(&self.store_values), + Some(&self.outer_block_stores), + ); + // Cannot include this in the previous vecmap since it requires exclusive access to self let args = vecmap(args, |(then_arg, else_arg)| { - self.merge_values(then_branch.condition, else_branch.condition, then_arg, else_arg) + value_merger.merge_values( + then_branch.condition, + else_branch.condition, + then_arg, + else_arg, + ) }); self.merge_stores(then_branch, else_branch); @@ -789,14 +538,34 @@ impl<'f> Context<'f> { let then_condition = then_branch.condition; let else_condition = else_branch.condition; - for (address, (then_case, else_case, old_value)) in new_map { - let value = self.merge_values(then_condition, else_condition, then_case, else_case); + let block = self.inserter.function.entry_block(); + + let mut value_merger = ValueMerger::new( + &mut self.inserter.function.dfg, + block, + Some(&self.store_values), + Some(&self.outer_block_stores), + ); + + // Merging must occur in a separate loop as we cannot borrow `self` as mutable while `value_merger` does + let mut new_values = HashMap::default(); + for (address, (then_case, else_case, _)) in &new_map { + let value = + value_merger.merge_values(then_condition, else_condition, *then_case, *else_case); + new_values.insert(address, value); + } + + // Replace stores with new merged values + for (address, (_, _, old_value)) in &new_map { + let value = new_values[address]; + let address = *address; self.insert_instruction_with_typevars(Instruction::Store { address, value }, None); if let Some(store) = self.store_values.get_mut(&address) { store.new_value = value; } else { - self.store_values.insert(address, Store { old_value, new_value: value }); + self.store_values + .insert(address, Store { old_value: *old_value, new_value: value }); } } } 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 new file mode 100644 index 0000000000..3041dddd57 --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -0,0 +1,336 @@ +use acvm::FieldElement; +use fxhash::FxHashMap as HashMap; + +use crate::ssa::ir::{ + basic_block::BasicBlockId, + dfg::{CallStack, DataFlowGraph}, + instruction::{BinaryOp, Instruction, Intrinsic}, + types::Type, + value::{Value, ValueId}, +}; + +use crate::ssa::opt::flatten_cfg::Store; + +pub(crate) struct ValueMerger<'a> { + dfg: &'a mut DataFlowGraph, + block: BasicBlockId, + store_values: Option<&'a HashMap>, + outer_block_stores: Option<&'a HashMap>, + slice_sizes: HashMap, +} + +impl<'a> ValueMerger<'a> { + pub(crate) fn new( + dfg: &'a mut DataFlowGraph, + block: BasicBlockId, + store_values: Option<&'a HashMap>, + outer_block_stores: Option<&'a HashMap>, + ) -> Self { + ValueMerger { + dfg, + block, + store_values, + outer_block_stores, + slice_sizes: HashMap::default(), + } + } + + /// Merge two values a and b from separate basic blocks to a single value. + /// If these two values are numeric, the result will be + /// `then_condition * then_value + else_condition * else_value`. + /// Otherwise, if the values being merged are arrays, a new array will be made + /// recursively from combining each element of both input arrays. + /// + /// It is currently an error to call this function on reference or function values + /// as it is less clear how to merge these. + pub(crate) fn merge_values( + &mut self, + then_condition: ValueId, + else_condition: ValueId, + then_value: ValueId, + else_value: ValueId, + ) -> ValueId { + match self.dfg.type_of_value(then_value) { + Type::Numeric(_) => { + self.merge_numeric_values(then_condition, else_condition, then_value, else_value) + } + typ @ Type::Array(_, _) => { + self.merge_array_values(typ, then_condition, else_condition, then_value, else_value) + } + typ @ Type::Slice(_) => { + self.merge_slice_values(typ, then_condition, else_condition, then_value, else_value) + } + Type::Reference => panic!("Cannot return references from an if expression"), + Type::Function => panic!("Cannot return functions from an if expression"), + } + } + + /// Merge two numeric values a and b from separate basic blocks to a single value. This + /// function would return the result of `if c { a } else { b }` as `c*a + (!c)*b`. + pub(crate) fn merge_numeric_values( + &mut self, + then_condition: ValueId, + else_condition: ValueId, + then_value: ValueId, + else_value: ValueId, + ) -> ValueId { + let then_type = self.dfg.type_of_value(then_value); + let else_type = self.dfg.type_of_value(else_value); + assert_eq!( + then_type, else_type, + "Expected values merged to be of the same type but found {then_type} and {else_type}" + ); + + let then_call_stack = self.dfg.get_value_call_stack(then_value); + let else_call_stack = self.dfg.get_value_call_stack(else_value); + + let call_stack = if then_call_stack.is_empty() { else_call_stack } else { then_call_stack }; + + // We must cast the bool conditions to the actual numeric type used by each value. + let then_condition = self + .dfg + .insert_instruction_and_results( + Instruction::Cast(then_condition, then_type), + self.block, + None, + call_stack.clone(), + ) + .first(); + let else_condition = self + .dfg + .insert_instruction_and_results( + Instruction::Cast(else_condition, else_type), + self.block, + None, + call_stack.clone(), + ) + .first(); + + let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value); + let then_value = self + .dfg + .insert_instruction_and_results(mul, self.block, None, call_stack.clone()) + .first(); + + let mul = Instruction::binary(BinaryOp::Mul, else_condition, else_value); + let else_value = self + .dfg + .insert_instruction_and_results(mul, self.block, None, call_stack.clone()) + .first(); + + let add = Instruction::binary(BinaryOp::Add, then_value, else_value); + self.dfg.insert_instruction_and_results(add, self.block, None, call_stack).first() + } + + /// 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. + pub(crate) fn merge_array_values( + &mut self, + typ: Type, + then_condition: ValueId, + else_condition: ValueId, + then_value: ValueId, + else_value: ValueId, + ) -> ValueId { + let mut merged = im::Vector::new(); + + let (element_types, len) = match &typ { + Type::Array(elements, len) => (elements, *len), + _ => panic!("Expected array type"), + }; + + for i in 0..len { + for (element_index, element_type) in element_types.iter().enumerate() { + let index = ((i * element_types.len() + element_index) as u128).into(); + let index = self.dfg.make_constant(index, Type::field()); + + let typevars = Some(vec![element_type.clone()]); + + let mut get_element = |array, typevars| { + let get = Instruction::ArrayGet { array, index }; + self.dfg + .insert_instruction_and_results(get, self.block, typevars, CallStack::new()) + .first() + }; + + let then_element = get_element(then_value, typevars.clone()); + let else_element = get_element(else_value, typevars); + + merged.push_back(self.merge_values( + then_condition, + else_condition, + then_element, + else_element, + )); + } + } + + self.dfg.make_array(merged, typ) + } + + fn merge_slice_values( + &mut self, + typ: Type, + then_condition: ValueId, + else_condition: ValueId, + then_value_id: ValueId, + else_value_id: ValueId, + ) -> ValueId { + let mut merged = im::Vector::new(); + + let element_types = match &typ { + Type::Slice(elements) => elements, + _ => panic!("Expected slice type"), + }; + + let then_len = self.get_slice_length(then_value_id); + self.slice_sizes.insert(then_value_id, then_len); + + let else_len = self.get_slice_length(else_value_id); + self.slice_sizes.insert(else_value_id, else_len); + + let len = then_len.max(else_len); + + for i in 0..len { + for (element_index, element_type) in element_types.iter().enumerate() { + let index_value = ((i * element_types.len() + element_index) as u128).into(); + let index = self.dfg.make_constant(index_value, Type::field()); + + let typevars = Some(vec![element_type.clone()]); + + let mut get_element = |array, typevars, len| { + // The smaller slice is filled with placeholder data. Codegen for slice accesses must + // include checks against the dynamic slice length so that this placeholder data is not incorrectly accessed. + if len <= index_value.to_u128() as usize { + self.make_slice_dummy_data(element_type) + } else { + let get = Instruction::ArrayGet { array, index }; + self.dfg + .insert_instruction_and_results( + get, + self.block, + typevars, + CallStack::new(), + ) + .first() + } + }; + + let then_element = get_element(then_value_id, typevars.clone(), then_len); + let else_element = get_element(else_value_id, typevars, else_len); + + merged.push_back(self.merge_values( + then_condition, + else_condition, + then_element, + else_element, + )); + } + } + + self.dfg.make_array(merged, typ) + } + + fn get_slice_length(&mut self, value_id: ValueId) -> usize { + let value = &self.dfg[value_id]; + match value { + Value::Array { array, .. } => array.len(), + Value::Instruction { instruction: instruction_id, .. } => { + let instruction = &self.dfg[*instruction_id]; + match instruction { + Instruction::ArraySet { array, .. } => { + let array = *array; + let len = self.get_slice_length(array); + self.slice_sizes.insert(array, len); + len + } + Instruction::Load { address } => { + let outer_block_stores = self.outer_block_stores.expect("ICE: A map of previous stores is required in order to resolve a slice load"); + let store_values = self.store_values.expect("ICE: A map of previous stores is required in order to resolve a slice load"); + let store_value = outer_block_stores + .get(address) + .expect("ICE: load in merger should have store from outer block"); + + if let Some(len) = self.slice_sizes.get(store_value) { + return *len; + } + + let store_value = if let Some(store) = store_values.get(address) { + if let Some(len) = self.slice_sizes.get(&store.new_value) { + return *len; + } + + store.new_value + } else { + *store_value + }; + + self.get_slice_length(store_value) + } + Instruction::Call { func, arguments } => { + let slice_contents = arguments[1]; + let func = &self.dfg[*func]; + match func { + Value::Intrinsic(intrinsic) => match intrinsic { + Intrinsic::SlicePushBack + | Intrinsic::SlicePushFront + | Intrinsic::SliceInsert => { + // `get_slice_length` needs to be called here as it is borrows self as mutable + let initial_len = self.get_slice_length(slice_contents); + self.slice_sizes.insert(slice_contents, initial_len); + initial_len + 1 + } + Intrinsic::SlicePopBack + | Intrinsic::SlicePopFront + | Intrinsic::SliceRemove => { + // `get_slice_length` needs to be called here as it is borrows self as mutable + let initial_len = self.get_slice_length(slice_contents); + self.slice_sizes.insert(slice_contents, initial_len); + initial_len - 1 + } + _ => { + unreachable!("ICE: Intrinsic not supported, got {intrinsic:?}") + } + }, + _ => unreachable!("ICE: Expected intrinsic value but got {func:?}"), + } + } + _ => unreachable!("ICE: Got unexpected instruction: {instruction:?}"), + } + } + _ => unreachable!("ICE: Got unexpected value when resolving slice length {value:?}"), + } + } + + /// Construct a dummy value to be attached to the smaller of two slices being merged. + /// We need to make sure we follow the internal element type structure of the slice type + /// even for dummy data to ensure that we do not have errors later in the compiler, + /// such as with dynamic indexing of non-homogenous slices. + fn make_slice_dummy_data(&mut self, typ: &Type) -> ValueId { + match typ { + Type::Numeric(_) => { + let zero = FieldElement::zero(); + self.dfg.make_constant(zero, Type::field()) + } + Type::Array(element_types, len) => { + let mut array = im::Vector::new(); + for _ in 0..*len { + for typ in element_types.iter() { + array.push_back(self.make_slice_dummy_data(typ)); + } + } + self.dfg.make_array(array, typ.clone()) + } + Type::Slice(_) => { + unreachable!("ICE: Slices of slice is unsupported") + } + Type::Reference => { + unreachable!("ICE: Merging references is unsupported") + } + Type::Function => { + unreachable!("ICE: Merging functions is unsupported") + } + } + } +} diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 9b9350118a..4d003c0594 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -8,7 +8,7 @@ mod assert_constant; mod constant_folding; mod defunctionalize; mod die; -mod flatten_cfg; +pub(crate) mod flatten_cfg; mod inlining; mod mem2reg; mod simplify_cfg; diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 20250f5470..f3acf534d3 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -12,7 +12,7 @@ use noirc_frontend::{ BinaryOpKind, }; -use crate::ssa::ir::types::NumericType; +use crate::ssa::ir::{instruction::Intrinsic, types::NumericType}; use self::{ context::FunctionContext, @@ -346,7 +346,11 @@ impl<'a> FunctionContext<'a> { let is_offset_out_of_bounds = self.builder.insert_binary(index, BinaryOp::Lt, array_len); let true_const = self.builder.numeric_constant(true, Type::bool()); - self.builder.insert_constrain(is_offset_out_of_bounds, true_const, None); + self.builder.insert_constrain( + is_offset_out_of_bounds, + true_const, + Some("Index out of bounds".to_owned()), + ); } fn codegen_cast(&mut self, cast: &ast::Cast) -> Values { @@ -496,11 +500,41 @@ impl<'a> FunctionContext<'a> { .arguments .iter() .flat_map(|argument| self.codegen_expression(argument).into_value_list(self)) - .collect(); + .collect::>(); + + self.codegen_intrinsic_call_checks(function, &arguments, call.location); self.insert_call(function, arguments, &call.return_type, call.location) } + fn codegen_intrinsic_call_checks( + &mut self, + function: ValueId, + arguments: &[ValueId], + location: Location, + ) { + if let Some(intrinsic) = + self.builder.set_location(location).get_intrinsic_from_value(function) + { + match intrinsic { + Intrinsic::SliceInsert => { + let one = self.builder.numeric_constant(1u128, Type::field()); + // We add one here in the case of a slice insert as a slice insert at the length of the slice + // can be converted to a slice push back + let len_plus_one = self.builder.insert_binary(arguments[0], BinaryOp::Add, one); + + self.codegen_slice_access_check(arguments[2], Some(len_plus_one)); + } + Intrinsic::SliceRemove => { + self.codegen_slice_access_check(arguments[2], Some(arguments[0])); + } + _ => { + // Do nothing as the other intrinsics do not require checks + } + } + } + } + /// Generate SSA for the given variable. /// If the variable is immutable, no special handling is necessary and we can return the given /// ValueId directly. If it is mutable, we'll need to allocate space for the value and store diff --git a/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/Nargo.toml b/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/Nargo.toml new file mode 100644 index 0000000000..fc7eb87be4 --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "slice_insert_failure" +type = "bin" +authors = [""] +compiler_version = "0.12.0" + +[dependencies] \ No newline at end of file diff --git a/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/Prover.toml b/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/Prover.toml new file mode 100644 index 0000000000..f28f2f8cc4 --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/Prover.toml @@ -0,0 +1,2 @@ +x = "5" +y = "10" diff --git a/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/src/main.nr b/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/src/main.nr new file mode 100644 index 0000000000..dad80bff7a --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/src/main.nr @@ -0,0 +1,11 @@ +fn main(x : Field, y : pub Field) { + let mut slice = [0; 2]; + if x == y { + slice = slice.push_back(y); + slice = slice.push_back(x); + } else { + slice = slice.push_back(x); + } + + slice = slice.insert(10, 100); +} \ No newline at end of file diff --git a/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/Nargo.toml b/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/Nargo.toml new file mode 100644 index 0000000000..795a4d863c --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "slice_remove_failure" +type = "bin" +authors = [""] +compiler_version = "0.12.0" + +[dependencies] \ No newline at end of file diff --git a/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/Prover.toml b/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/Prover.toml new file mode 100644 index 0000000000..f28f2f8cc4 --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/Prover.toml @@ -0,0 +1,2 @@ +x = "5" +y = "10" diff --git a/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/src/main.nr b/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/src/main.nr new file mode 100644 index 0000000000..7c308488ae --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/src/main.nr @@ -0,0 +1,11 @@ +fn main(x : Field, y : pub Field) { + let mut slice = [0; 2]; + if x == y { + slice = slice.push_back(y); + slice = slice.push_back(x); + } else { + slice = slice.push_back(x); + } + + let (removed_slice, removed_elem) = slice.remove(10); +} \ No newline at end of file diff --git a/tooling/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr b/tooling/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr index de5b4caef2..724f3f1618 100644 --- a/tooling/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr +++ b/tooling/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr @@ -21,6 +21,8 @@ fn regression_dynamic_slice_index(x: Field, y: Field) { dynamic_slice_merge_if(slice, x); dynamic_slice_merge_else(slice, x); dynamic_slice_merge_two_ifs(slice, x); + dynamic_slice_merge_mutate_between_ifs(slice, x, y); + dynamic_slice_merge_push_then_pop(slice, x, y); } fn dynamic_slice_index_set_if(mut slice: [Field], x: Field, y: Field) { @@ -165,35 +167,6 @@ fn dynamic_slice_merge_else(mut slice: [Field], x: Field) { assert(slice[slice.len() - 1] == 20); } -fn dynamic_slice_merge_two_ifs(mut slice: [Field], x: Field) { - if x as u32 > 10 { - assert(slice[x] == 0); - slice[x] = 2; - } else { - assert(slice[x] == 4); - slice[x] = slice[x] - 2; - slice = slice.push_back(10); - } - - assert(slice.len() == 6); - assert(slice[slice.len() - 1] == 10); - - if x == 20 { - slice = slice.push_back(20); - } else { - slice = slice.push_back(15); - } - // TODO(#2599): Breaks if the push back happens without the else case - // slice = slice.push_back(15); - - assert(slice.len() == 7); - assert(slice[slice.len() - 1] == 15); - - slice = slice.push_back(20); - assert(slice.len() == 8); - assert(slice[slice.len() - 1] == 20); -} - fn dynamic_slice_index_set_nested_if_else_else(mut slice: [Field], x: Field, y: Field) { assert(slice[x] == 4); assert(slice[y] == 1); @@ -250,3 +223,93 @@ fn dynamic_slice_index_set_nested_if_else_if(mut slice: [Field], x: Field, y: Fi assert(slice[4] == 5); } +fn dynamic_slice_merge_two_ifs(mut slice: [Field], x: Field) { + if x as u32 > 10 { + assert(slice[x] == 0); + slice[x] = 2; + } else { + assert(slice[x] == 4); + slice[x] = slice[x] - 2; + slice = slice.push_back(10); + } + + assert(slice.len() == 6); + assert(slice[slice.len() - 1] == 10); + + if x == 20 { + slice = slice.push_back(20); + } + + slice = slice.push_back(15); + + assert(slice.len() == 7); + assert(slice[slice.len() - 1] == 15); + + slice = slice.push_back(20); + assert(slice.len() == 8); + assert(slice[slice.len() - 1] == 20); +} + + +fn dynamic_slice_merge_mutate_between_ifs(mut slice: [Field], x: Field, y: Field) { + if x != y { + slice[x] = 50; + slice = slice.push_back(y); + slice = slice.push_back(x); + } else { + slice[x] = slice[x] - 2; + slice = slice.push_back(x); + } + + slice = slice.push_back(30); + assert(slice.len() == 8); + + if x == 20 { + slice = slice.push_back(20); + } + + slice = slice.push_back(15); + + if x != 20 { + slice = slice.push_back(50); + } + + slice = slice.push_back(60); + assert(slice.len() == 11); + assert(slice[x] == 50); + assert(slice[slice.len() - 4] == 30); + assert(slice[slice.len() - 3] == 15); + assert(slice[slice.len() - 2] == 50); + assert(slice[slice.len() - 1] == 60); +} + +fn dynamic_slice_merge_push_then_pop(mut slice: [Field], x: Field, y: Field) { + if x != y { + slice[x] = 5; + slice = slice.push_back(y); + slice = slice.push_back(x); + assert(slice.len() == 7); + + let (popped_slice, elem) = slice.pop_back(); + assert(slice.len() == 7); + assert(elem == x); + slice = popped_slice; + } else { + slice = slice.push_back(x); + } + + slice = slice.push_back(30); + assert(slice.len() == 7); + + if x == 20 { + slice = slice.push_back(20); + } + + let (slice, elem) = slice.pop_back(); + assert(elem == 30); + + let (_, elem) = slice.pop_back(); + assert(elem == y); +} + + diff --git a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr index 1e5a7a605a..f9069a48e3 100644 --- a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr +++ b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr @@ -110,10 +110,24 @@ fn merge_slices_if(x: Field, y: Field) { assert(slice[5] == 30); let slice = merge_slices_mutate_between_ifs(x, y); - assert(slice.len() == 6); + assert(slice.len() == 8); assert(slice[3] == 5); assert(slice[4] == 30); assert(slice[5] == 15); + assert(slice[6] == 50); + assert(slice[7] == 60); + + merge_slices_push_then_pop(x, y); + + let slice = merge_slices_push_then_insert(x, y); + assert(slice.len() == 7); + assert(slice[1] == 50); + assert(slice[2] == 0); + assert(slice[5] == 30); + assert(slice[6] == 100); + + let slice = merge_slices_remove_between_ifs(x, y); + assert(slice.len() == 5); } fn merge_slices_else(x: Field) { @@ -178,19 +192,44 @@ fn merge_slices_mutate_two_ifs(x: Field, y: Field) -> [Field] { } else { slice = slice.push_back(x); } + if x == 20 { slice = slice.push_back(20); + } + + slice = slice.push_back(15); + slice = slice.push_back(30); + + slice +} + +fn merge_slices_mutate_between_ifs(x: Field, y: Field) -> [Field] { + let mut slice = [0; 2]; + if x != y { + slice = slice.push_back(y); + slice = slice.push_back(x); } else { - slice = slice.push_back(15); + slice = slice.push_back(x); } - // TODO(#2599): Breaks if the push back happens without the else case - // slice = slice.push_back(15); + slice = slice.push_back(30); + if x == 20 { + slice = slice.push_back(20); + } + + slice = slice.push_back(15); + + if x != 20 { + slice = slice.push_back(50); + } + + slice = slice.push_back(60); + slice } -fn merge_slices_mutate_between_ifs(x: Field, y: Field) -> [Field] { +fn merge_slices_push_then_pop(x: Field, y: Field) { let mut slice = [0; 2]; if x != y { slice = slice.push_back(y); @@ -203,11 +242,62 @@ fn merge_slices_mutate_between_ifs(x: Field, y: Field) -> [Field] { if x == 20 { slice = slice.push_back(20); + } + + let (slice, elem) = slice.pop_back(); + assert(slice.len() == 4); + assert(elem == 30); + + let (slice, elem) = slice.pop_back(); + assert(slice.len() == 3); + assert(elem == x); +} + +fn merge_slices_push_then_insert(x: Field, y: Field) -> [Field] { + let mut slice = [0; 2]; + if x != y { + slice = slice.push_back(y); + slice = slice.push_back(x); } else { + slice = slice.push_back(x); + } + + slice = slice.push_back(30); + + if x == 20 { + slice = slice.push_back(20); slice = slice.push_back(15); } - // TODO(#2599): Breaks if the push back happens without the else case - // slice = slice.push_back(15); + + slice = slice.insert(1, 50); + + // Test that we can use slice insert the same as slice push back + slice = slice.insert(6, 100); + + slice +} + +fn merge_slices_remove_between_ifs(x: Field, y: Field) -> [Field] { + let mut slice = [0; 2]; + if x != y { + slice = slice.push_back(y); + slice = slice.push_back(x); + } else { + slice = slice.push_back(x); + } + + let (mut slice, elem) = slice.remove(2); + assert(elem == y); + + if x == 20 { + slice = slice.push_back(20); + } + + slice = slice.push_back(15); + + if x != 20 { + slice = slice.push_back(50); + } slice }