diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 23a24c984f5..771a139a8c3 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -72,9 +72,23 @@ struct Context { /// which utilizes this internal memory for ACIR generation. internal_memory_blocks: HashMap, BlockId>, + /// Maps an internal memory block to its length + /// + /// This is necessary to keep track of an internal memory block's size. + /// We do not need a separate map to keep track of `memory_blocks` as + /// the length is set when we construct a `AcirValue::DynamicArray` and is tracked + /// as part of the `AcirValue` in the `ssa_values` map. + /// The length of an internal memory block is determined before an array operation + /// takes place thus we track it separate here in this map. + internal_mem_block_lengths: HashMap, + /// Number of the next BlockId, it is used to construct /// a new BlockId max_block_id: u32, + + /// Maps SSA array values to their slice size and any nested slices internal to the parent slice. + /// This enables us to maintain the slice structure of a slice when performing an array get. + slice_sizes: HashMap, Vec>, } #[derive(Clone)] @@ -169,7 +183,9 @@ impl Context { initialized_arrays: HashSet::new(), memory_blocks: HashMap::default(), internal_memory_blocks: HashMap::default(), + internal_mem_block_lengths: HashMap::default(), max_block_id: 0, + slice_sizes: HashMap::default(), } } @@ -685,6 +701,11 @@ impl Context { } } Type::Slice(_) => { + // TODO(#3188): Need to be able to handle constant index for slices to seriously reduce + // constraint sizes of nested slices + // This can only be done if we accurately flatten nested slices as other we will reach + // index out of bounds errors. + // Do nothing we only want dynamic checks for slices } _ => unreachable!("ICE: expected array or slice type"), @@ -725,8 +746,24 @@ impl Context { let mut dummy_predicate_index = predicate_index; // We must setup the dummy value to match the type of the value we wish to store - let dummy = - self.array_get_value(&store_type, block_id, &mut dummy_predicate_index)?; + let slice_sizes = if store_type.contains_slice_element() { + self.compute_slice_sizes(store, None, dfg); + self.slice_sizes.get(&store).cloned().ok_or_else(|| { + InternalError::UnExpected { + expected: "Store value should have slice sizes computed".to_owned(), + found: "Missing key in slice sizes map".to_owned(), + call_stack: self.acir_context.get_call_stack(), + } + })? + } else { + vec![] + }; + let dummy = self.array_get_value( + &store_type, + block_id, + &mut dummy_predicate_index, + &slice_sizes, + )?; Some(self.convert_array_set_store_value(&store_value, &dummy)?) } @@ -822,11 +859,31 @@ impl Context { mut var_index: AcirVar, dfg: &DataFlowGraph, ) -> Result { - let (_, _, block_id) = self.check_array_is_initialized(array, dfg)?; + let (array_id, _, block_id) = self.check_array_is_initialized(array, dfg)?; let results = dfg.instruction_results(instruction); let res_typ = dfg.type_of_value(results[0]); - let value = self.array_get_value(&res_typ, block_id, &mut var_index)?; + + let value = if !res_typ.contains_slice_element() { + self.array_get_value(&res_typ, block_id, &mut var_index, &[])? + } else { + let slice_sizes = self + .slice_sizes + .get(&array_id) + .expect("ICE: Array with slices should have associated slice sizes"); + + // The first max size is going to be the length of the parent slice + // As we are fetching from the parent slice we just want its internal + // slize sizes. + let slice_sizes = slice_sizes[1..].to_vec(); + + let value = self.array_get_value(&res_typ, block_id, &mut var_index, &slice_sizes)?; + + // Insert the resulting slice sizes + self.slice_sizes.insert(results[0], slice_sizes); + + value + }; self.define_result(dfg, instruction, value.clone()); @@ -838,6 +895,7 @@ impl Context { ssa_type: &Type, block_id: BlockId, var_index: &mut AcirVar, + slice_sizes: &[usize], ) -> Result { let one = self.acir_context.add_constant(FieldElement::one()); match ssa_type.clone() { @@ -855,20 +913,31 @@ impl Context { let mut values = Vector::new(); for _ in 0..len { for typ in element_types.as_ref() { - values.push_back(self.array_get_value(typ, block_id, var_index)?); + values.push_back(self.array_get_value( + typ, + block_id, + var_index, + slice_sizes, + )?); } } Ok(AcirValue::Array(values)) } - Type::Slice(_) => { - // TODO(#2752): need SSA values here to fetch the len like we do for a Type::Array - // Update this to enable fetching slices from nested arrays - Err(InternalError::UnExpected { - expected: "array".to_owned(), - found: ssa_type.to_string(), - call_stack: self.acir_context.get_call_stack(), + Type::Slice(element_types) => { + // It is not enough to execute this loop and simply pass the size from the parent definition. + // We need the internal sizes of each type in case of a nested slice. + let mut values = Vector::new(); + + let (current_size, new_sizes) = + slice_sizes.split_first().expect("should be able to split"); + + for _ in 0..*current_size { + for typ in element_types.as_ref() { + values + .push_back(self.array_get_value(typ, block_id, var_index, new_sizes)?); + } } - .into()) + Ok(AcirValue::Array(values)) } _ => unreachable!("ICE - expected an array or slice"), } @@ -907,13 +976,10 @@ impl Context { // However, this size is simply the capacity of a slice. The capacity is dependent upon the witness // and may contain data for which we want to restrict access. The true slice length is tracked in a // a separate SSA value and restrictions on slice indices should be generated elsewhere in the SSA. - let array_len = match &array_typ { - Type::Array(_, _) => { - // Flatten the array length to handle arrays of complex types - array_typ.flattened_size() - } - Type::Slice(_) => self.flattened_slice_size(array_id, dfg), - _ => unreachable!("ICE - expected an array"), + let array_len = if !array_typ.contains_slice_element() { + array_typ.flattened_size() + } else { + self.flattened_slice_size(array_id, dfg) }; // Since array_set creates a new array, we create a new block ID for this @@ -935,11 +1001,28 @@ impl Context { self.array_set_value(store_value, result_block_id, &mut var_index)?; - let arr_element_type_sizes = self.internal_block_id(&array_id); + // Set new resulting array to have the same slice sizes as the instruction input + if let Type::Slice(element_types) = &array_typ { + let has_internal_slices = + element_types.as_ref().iter().any(|typ| typ.contains_slice_element()); + if has_internal_slices { + let slice_sizes = self + .slice_sizes + .get(&array_id) + .expect( + "ICE: Expected array with internal slices to have associated slice sizes", + ) + .clone(); + let results = dfg.instruction_results(instruction); + self.slice_sizes.insert(results[0], slice_sizes); + } + } + + let element_type_sizes = self.init_element_type_sizes_array(&array_typ, array_id, dfg)?; let result_value = AcirValue::DynamicArray(AcirDynamicArray { block_id: result_block_id, len: array_len, - element_type_sizes: arr_element_type_sizes, + element_type_sizes, }); self.define_result(dfg, instruction, result_value); Ok(()) @@ -998,7 +1081,7 @@ impl Context { match value { Value::Array { .. } | Value::Instruction { .. } => { let value = self.convert_value(array_id, dfg); - let len = if matches!(array_typ, Type::Array(_, _)) { + let len = if !array_typ.contains_slice_element() { array_typ.flattened_size() } else { self.flattened_slice_size(array_id, dfg) @@ -1026,6 +1109,8 @@ impl Context { ) -> Result { let element_type_sizes = self.internal_block_id(&array_id); // Check whether an internal type sizes array has already been initialized + // Need to look into how to optimize for slices as this could lead to different element type sizes + // for different slices that do not have consistent sizes if self.initialized_arrays.contains(&element_type_sizes) { return Ok(element_type_sizes); } @@ -1033,21 +1118,18 @@ impl Context { let mut flat_elem_type_sizes = Vec::new(); flat_elem_type_sizes.push(0); match array_typ { - Type::Array(element_types, _) => { - for (i, typ) in element_types.as_ref().iter().enumerate() { - flat_elem_type_sizes.push(typ.flattened_size() + flat_elem_type_sizes[i]); - } - } - Type::Slice(element_types) => { + Type::Array(_, _) | Type::Slice(_) => { match &dfg[array_id] { Value::Array { array, .. } => { - for i in 0..element_types.len() { + self.compute_slice_sizes(array_id, None, dfg); + + for (i, value) in array.iter().enumerate() { flat_elem_type_sizes.push( - self.flattened_slice_size(array[i], dfg) + flat_elem_type_sizes[i], + self.flattened_slice_size(*value, dfg) + flat_elem_type_sizes[i], ); } } - Value::Instruction { .. } => { + Value::Instruction { .. } | Value::Param { .. } => { // An instruction representing the slice means it has been processed previously during ACIR gen. // Use the previously defined result of an array operation to fetch the internal type information. let array_acir_value = self.convert_value(array_id, dfg); @@ -1057,11 +1139,19 @@ impl Context { .. }) => { if self.initialized_arrays.contains(&inner_elem_type_sizes) { + let type_sizes_array_len = self.internal_mem_block_lengths.get(&inner_elem_type_sizes).copied().ok_or_else(|| + InternalError::General { + message: format!("Array {array_id}'s inner element type sizes array does not have a tracked length"), + call_stack: self.acir_context.get_call_stack(), + } + )?; self.copy_dynamic_array( inner_elem_type_sizes, element_type_sizes, - element_types.len() + 1, + type_sizes_array_len, )?; + self.internal_mem_block_lengths + .insert(element_type_sizes, type_sizes_array_len); return Ok(element_type_sizes); } else { return Err(InternalError::General { @@ -1072,10 +1162,9 @@ impl Context { } } AcirValue::Array(values) => { - for i in 0..element_types.len() { + for (i, value) in values.iter().enumerate() { flat_elem_type_sizes.push( - Self::flattened_value_size(&values[i]) - + flat_elem_type_sizes[i], + Self::flattened_value_size(value) + flat_elem_type_sizes[i], ); } } @@ -1109,20 +1198,59 @@ impl Context { .into()); } } + // The final array should will the flattened index at each outer array index let init_values = vecmap(flat_elem_type_sizes, |type_size| { let var = self.acir_context.add_constant(FieldElement::from(type_size as u128)); AcirValue::Var(var, AcirType::field()) }); + let element_type_sizes_len = init_values.len(); self.initialize_array( element_type_sizes, - init_values.len(), + element_type_sizes_len, Some(AcirValue::Array(init_values.into())), )?; + self.internal_mem_block_lengths.insert(element_type_sizes, element_type_sizes_len); + Ok(element_type_sizes) } + fn compute_slice_sizes( + &mut self, + current_array_id: ValueId, + parent_array: Option, + dfg: &DataFlowGraph, + ) { + let (array, typ) = match &dfg[current_array_id] { + Value::Array { array, typ } => (array, typ.clone()), + _ => return, + }; + + if !matches!(typ, Type::Slice(_)) { + return; + } + + let element_size = typ.element_size(); + let true_len = array.len() / element_size; + if let Some(parent_array) = parent_array { + let sizes_list = + self.slice_sizes.get_mut(&parent_array).expect("ICE: expected size list"); + sizes_list.push(true_len); + for value in array { + self.compute_slice_sizes(*value, Some(parent_array), dfg); + } + } else { + // This means the current_array_id is the parent array + // The slice sizes should follow the parent array's type structure + // thus we start our sizes list with the parent array size. + self.slice_sizes.insert(current_array_id, vec![true_len]); + for value in array { + self.compute_slice_sizes(*value, Some(current_array_id), dfg); + } + } + } + fn copy_dynamic_array( &mut self, source: BlockId, @@ -1148,29 +1276,12 @@ impl Context { ) -> Result { let element_type_sizes = self.init_element_type_sizes_array(array_typ, array_id, dfg)?; - let element_size = array_typ.element_size(); - - let element_size_var = - self.acir_context.add_constant(FieldElement::from(element_size as u128)); - let outer_offset = self.acir_context.div_var( - var_index, - element_size_var, - AcirType::unsigned(32), - self.current_side_effects_enabled_var, - )?; - let inner_offset_index = self.acir_context.modulo_var( - var_index, - element_size_var, - 32, - self.current_side_effects_enabled_var, - )?; - let inner_offset = - self.acir_context.read_from_memory(element_type_sizes, &inner_offset_index)?; - + let predicate_index = + self.acir_context.mul_var(var_index, self.current_side_effects_enabled_var)?; let flat_element_size_var = - self.acir_context.read_from_memory(element_type_sizes, &element_size_var)?; - let var_index = self.acir_context.mul_var(outer_offset, flat_element_size_var)?; - self.acir_context.add_var(var_index, inner_offset) + self.acir_context.read_from_memory(element_type_sizes, &predicate_index)?; + + Ok(flat_element_size_var) } fn flattened_slice_size(&mut self, array_id: ValueId, dfg: &DataFlowGraph) -> usize { @@ -1190,6 +1301,10 @@ impl Context { let array_acir_value = self.convert_value(array_id, dfg); size += Self::flattened_value_size(&array_acir_value); } + Value::Param { .. } => { + let array_acir_value = self.convert_value(array_id, dfg); + size += Self::flattened_value_size(&array_acir_value); + } _ => { unreachable!("ICE: Unexpected SSA value when computing the slice size"); } diff --git a/compiler/noirc_evaluator/src/ssa/ir/types.rs b/compiler/noirc_evaluator/src/ssa/ir/types.rs index e69e936372d..7fe0713e748 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/types.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/types.rs @@ -79,6 +79,18 @@ impl Type { } } + pub(crate) fn contains_slice_element(&self) -> bool { + match self { + Type::Array(elements, _) => { + elements.iter().any(|element| element.contains_slice_element()) + } + Type::Slice(_) => true, + Type::Numeric(_) => false, + Type::Reference => false, + Type::Function => false, + } + } + /// Returns the flattened size of a Type pub(crate) fn flattened_size(&self) -> usize { let mut size = 0; 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 3041dddd573..32979f78632 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 @@ -194,7 +194,8 @@ impl<'a> ValueMerger<'a> { 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_usize = i * element_types.len() + element_index; + let index_value = (index_usize as u128).into(); let index = self.dfg.make_constant(index_value, Type::field()); let typevars = Some(vec![element_type.clone()]); @@ -202,7 +203,7 @@ impl<'a> ValueMerger<'a> { 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 { + if len <= index_usize { self.make_slice_dummy_data(element_type) } else { let get = Instruction::ArrayGet { array, index }; @@ -239,6 +240,11 @@ impl<'a> ValueMerger<'a> { Value::Instruction { instruction: instruction_id, .. } => { let instruction = &self.dfg[*instruction_id]; match instruction { + // TODO(#3188): A slice can be the result of an ArrayGet when it is the + // fetched from a slice of slices or as a struct field. + // However, we need to incorporate nested slice support in flattening + // in order for this to be valid + // Instruction::ArrayGet { array, .. } => {} Instruction::ArraySet { array, .. } => { let array = *array; let len = self.get_slice_length(array); @@ -323,7 +329,9 @@ impl<'a> ValueMerger<'a> { self.dfg.make_array(array, typ.clone()) } Type::Slice(_) => { - unreachable!("ICE: Slices of slice is unsupported") + // TODO(#3188): Need to update flattening to use true user facing length of slices + // to accurately construct dummy data + unreachable!("ICE: Cannot return a slice of slices from an if expression") } Type::Reference => { unreachable!("ICE: Merging references is unsupported") diff --git a/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/src/main.nr b/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/src/main.nr index cd10b5e7347..0be7d688608 100644 --- a/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/src/main.nr +++ b/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/src/main.nr @@ -30,7 +30,7 @@ fn main(mut x : [Foo; 4], y : pub Field) { } else { x[y].a = 100; } - assert(x[y].a == 50); + assert(x[3].a == 50); if y == 2 { x[y - 1].b = [50, 51, 52]; diff --git a/tooling/nargo_cli/tests/execution_success/regression_mem_op_predicate/src/main.nr b/tooling/nargo_cli/tests/execution_success/regression_mem_op_predicate/src/main.nr index 2592d02ec2e..aa71a5f6cf0 100644 --- a/tooling/nargo_cli/tests/execution_success/regression_mem_op_predicate/src/main.nr +++ b/tooling/nargo_cli/tests/execution_success/regression_mem_op_predicate/src/main.nr @@ -1,8 +1,8 @@ fn main(mut x: [u32; 5], idx: Field) { // We should not hit out of bounds here as we have a predicate // that should not be hit - if idx as u32 < 3 { + if idx as u32 < 3 { x[idx] = 10; - } - assert(x[4] == 111); + } + assert(x[4] == 111); } diff --git a/tooling/nargo_cli/tests/execution_success/slice_struct_field/Nargo.toml b/tooling/nargo_cli/tests/execution_success/slice_struct_field/Nargo.toml new file mode 100644 index 00000000000..9530ebf9271 --- /dev/null +++ b/tooling/nargo_cli/tests/execution_success/slice_struct_field/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "slice_struct_field" +type = "bin" +authors = [""] + +[dependencies] diff --git a/tooling/nargo_cli/tests/execution_success/slice_struct_field/Prover.toml b/tooling/nargo_cli/tests/execution_success/slice_struct_field/Prover.toml new file mode 100644 index 00000000000..7127baac5bf --- /dev/null +++ b/tooling/nargo_cli/tests/execution_success/slice_struct_field/Prover.toml @@ -0,0 +1 @@ +y = "3" diff --git a/tooling/nargo_cli/tests/execution_success/slice_struct_field/src/main.nr b/tooling/nargo_cli/tests/execution_success/slice_struct_field/src/main.nr new file mode 100644 index 00000000000..481e9c60a05 --- /dev/null +++ b/tooling/nargo_cli/tests/execution_success/slice_struct_field/src/main.nr @@ -0,0 +1,125 @@ +struct FooParent { + parent_arr: [Field; 3], + foos: [Foo], +} + +struct Bar { + inner: [Field; 3], +} + +struct Foo { + a: Field, + b: [Field], + bar: Bar, +} + +fn main(y : pub Field) { + let mut b_one = [2, 3, 20]; + b_one = b_one.push_back(20); + // let b_one = Vec::from_slice([2, 3, 20]); + let foo_one = Foo { a: 1, b: b_one, bar: Bar { inner: [100, 101, 102] } }; + let mut b_two = [5, 6, 21]; + b_two = b_two.push_back(21); + // let b_two = Vec::from_slice([5, 6, 21]); + let foo_two = Foo { a: 4, b: b_two, bar: Bar { inner: [103, 104, 105] } }; + let foo_three = Foo { a: 7, b: [8, 9, 22], bar: Bar { inner: [106, 107, 108] } }; + let foo_four = Foo { a: 10, b: [11, 12, 23], bar: Bar { inner: [109, 110, 111] } }; + + let mut x = [foo_one, foo_two]; + x = x.push_back(foo_three); + x = x.push_back(foo_four); + + assert(x[y - 3].a == 1); + let struct_slice = x[y - 3].b; + for i in 0..4 { + assert(struct_slice[i] == b_one[i]); + } + + assert(x[y - 2].a == 4); + let struct_slice = x[y - 2].b; + for i in 0..4 { + assert(struct_slice[i] == b_two[i]); + } + + assert(x[y - 1].a == 7); + let struct_slice = x[y - 1].b; + assert(struct_slice[0] == 8); + assert(struct_slice[1] == 9); + assert(struct_slice[2] == 22); + + assert(x[y].a == 10); + let struct_slice = x[y].b; + assert(struct_slice[0] == 11); + assert(struct_slice[1] == 12); + assert(struct_slice[2] == 23); + assert(x[y].bar.inner == [109, 110, 111]); + + // TODO: Enable merging nested slices + // if y != 2 { + // x[y].a = 50; + // } else { + // x[y].a = 100; + // } + // assert(x[3].a == 50); + + // if y == 2 { + // x[y - 1].b = [50, 51, 52]; + // } else { + // x[y - 1].b = [100, 101, 102]; + // } + // assert(x[2].b[0] == 100); + // assert(x[2].b[1] == 101); + // assert(x[2].b[2] == 102); + + assert(x[y - 3].bar.inner == [100, 101, 102]); + assert(x[y - 2].bar.inner == [103, 104, 105]); + assert(x[y - 1].bar.inner == [106, 107, 108]); + assert(x[y].bar.inner == [109, 110, 111]); + + let q = x.push_back(foo_four); + let foo_parent_one = FooParent { parent_arr: [0, 1, 2], foos: x }; + let foo_parent_two = FooParent { parent_arr: [3, 4, 5], foos: q }; + let mut foo_parents = [foo_parent_one]; + foo_parents = foo_parents.push_back(foo_parent_two); + + // TODO: Merging nested slices is broken + // if y == 2 { + // foo_parents[y - 2].foos[y - 1].b[y - 1] = 5000; + // } else { + // foo_parents[y - 2].foos[y - 1].b[y - 1] = 1000; + // } + assert(foo_parents[y - 2].foos[y - 1].a == 7); + foo_parents[y - 2].foos[y - 1].a = 50; + assert(foo_parents[y - 2].foos[y - 1].a == 50); + + assert(foo_parents[y - 2].foos[y - 2].b[y - 1] == 21); + foo_parents[y - 2].foos[y - 2].b[y - 1] = 5000; + assert(foo_parents[y - 2].foos[y - 2].b[y - 1] == 5000); + + let b_array = foo_parents[y - 2].foos[y - 3].b; + assert(b_array[0] == 2); + assert(b_array[1] == 3); + assert(b_array[2] == 20); + assert(b_array[3] == 20); + + let b_array = foo_parents[y - 2].foos[y - 2].b; + assert(b_array[0] == 5); + assert(b_array[1] == 6); + assert(b_array[2] == 5000); + assert(b_array[3] == 21); + + let b_array = foo_parents[y - 2].foos[y - 1].b; + assert(b_array[0] == 8); + assert(b_array[1] == 9); + assert(b_array[2] == 22); + + // Fixed by #3410 + // let b_array = foo_parents[y - 2].foos[y].b; + // assert(b_array[0] == 11); + // assert(b_array[1] == 12); + // assert(b_array[2] == 23); + + // assert(foo_parents[y - 2].foos[y - 3].bar.inner == [100, 101, 102]); + // assert(foo_parents[y - 2].foos[y - 2].bar.inner == [103, 104, 105]); + // assert(foo_parents[y - 2].foos[y - 1].bar.inner == [106, 107, 108]); +} \ No newline at end of file