From c2f2f6261d72168668b0b007ed2b5729da272d96 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 14 Sep 2023 18:30:10 +0000 Subject: [PATCH 01/14] enabled dynamic indices on nested arrays --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 37 +++++++------------ .../src/ssa/opt/flatten_cfg.rs | 1 + .../dyn_index_fail_nested_array/Nargo.toml | 7 ++++ .../dyn_index_fail_nested_array/Prover.toml | 13 +++++++ .../dyn_index_fail_nested_array/src/main.nr | 8 ++++ .../nested_array_dynamic/Nargo.toml | 7 ++++ .../nested_array_dynamic/Prover.toml | 13 +++++++ .../nested_array_dynamic/src/main.nr | 22 +++++++++++ 8 files changed, 84 insertions(+), 24 deletions(-) create mode 100644 tooling/nargo_cli/tests/compile_failure/dyn_index_fail_nested_array/Nargo.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/dyn_index_fail_nested_array/Prover.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/dyn_index_fail_nested_array/src/main.nr create mode 100644 tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Nargo.toml create mode 100644 tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml create mode 100644 tooling/nargo_cli/tests/execution_success/nested_array_dynamic/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index a2943596a53..39d56706990 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -687,27 +687,7 @@ impl Context { } let read = self.acir_context.read_from_memory(block_id, &var_index)?; - let typ = match dfg.type_of_value(array) { - Type::Array(typ, _) => { - if typ.len() != 1 { - // TODO(#2461) - unimplemented!( - "Non-const array indices is not implemented for non-homogenous array" - ); - } - typ[0].clone() - } - Type::Slice(typ) => { - if typ.len() != 1 { - // TODO(#2461) - unimplemented!( - "Non-const array indices is not implemented for non-homogenous array" - ); - } - typ[0].clone() - } - _ => unreachable!("ICE - expected an array"), - }; + let typ = dfg.type_of_value(array); let typ = AcirType::from(typ); self.define_result(dfg, instruction, AcirValue::Var(read, typ)); Ok(read) @@ -744,16 +724,25 @@ impl Context { // Every array has a length in its type, so we fetch that from // the SSA IR. + // + // A slice's size must be fetched from the SSA value that represents the slice. + // 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 len = match dfg.type_of_value(array) { - Type::Array(_, len) => len, - Type::Slice(_) => { + Type::Array(typ, len) => { + // Flatten the array length to handle arrays of complex types + len * typ.len() + } + Type::Slice(typ) => { + // Fetch the true length of the slice from the array_set instruction let length = length .expect("ICE: array set on slice must have a length associated with the call"); let length_acir_var = self.convert_value(length, dfg).into_var()?; let len = self.acir_context.var_to_expression(length_acir_var)?.to_const(); let len = len .expect("ICE: slice length should be fully tracked and constant by ACIR gen"); - len.to_u128() as usize + len.to_u128() as usize * typ.len() } _ => unreachable!("ICE - expected an array"), }; diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index d57c2cc7933..9f93e982658 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -554,6 +554,7 @@ impl<'f> Context<'f> { 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(); + dbg!(index); let index = self.inserter.function.dfg.make_constant(index, Type::field()); let typevars = Some(vec![element_type.clone()]); diff --git a/tooling/nargo_cli/tests/compile_failure/dyn_index_fail_nested_array/Nargo.toml b/tooling/nargo_cli/tests/compile_failure/dyn_index_fail_nested_array/Nargo.toml new file mode 100644 index 00000000000..52a547e8d7b --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/dyn_index_fail_nested_array/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "dyn_index_fail_nested_array" +type = "bin" +authors = [""] +compiler_version = "0.11.1" + +[dependencies] \ No newline at end of file diff --git a/tooling/nargo_cli/tests/compile_failure/dyn_index_fail_nested_array/Prover.toml b/tooling/nargo_cli/tests/compile_failure/dyn_index_fail_nested_array/Prover.toml new file mode 100644 index 00000000000..00ffa6e4620 --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/dyn_index_fail_nested_array/Prover.toml @@ -0,0 +1,13 @@ +y = "2" + +[[x]] +a = "1" +b = "2" + +[[x]] +a = "3" +b = "4" + +[[x]] +a = "5" +b = "6" diff --git a/tooling/nargo_cli/tests/compile_failure/dyn_index_fail_nested_array/src/main.nr b/tooling/nargo_cli/tests/compile_failure/dyn_index_fail_nested_array/src/main.nr new file mode 100644 index 00000000000..e26625457d9 --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/dyn_index_fail_nested_array/src/main.nr @@ -0,0 +1,8 @@ +struct Foo { + a: Field, + b: Field, +} + +fn main(mut x : [Foo; 3], y : pub Field) { + assert(x[y + 2].a == 5); +} \ No newline at end of file diff --git a/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Nargo.toml b/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Nargo.toml new file mode 100644 index 00000000000..5be06d0f8af --- /dev/null +++ b/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "nested_array_dynamic" +type = "bin" +authors = [""] +compiler_version = "0.11.1" + +[dependencies] \ No newline at end of file diff --git a/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml b/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml new file mode 100644 index 00000000000..00ffa6e4620 --- /dev/null +++ b/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml @@ -0,0 +1,13 @@ +y = "2" + +[[x]] +a = "1" +b = "2" + +[[x]] +a = "3" +b = "4" + +[[x]] +a = "5" +b = "6" 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 new file mode 100644 index 00000000000..bda11468b84 --- /dev/null +++ b/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/src/main.nr @@ -0,0 +1,22 @@ + +struct Foo { + a: Field, + b: Field, +} + +fn main(mut x : [Foo; 3], y : pub Field) { + assert(x[y - 2].a == 1); + assert(x[y - 2].b == 2); + assert(x[y - 1].a == 3); + assert(x[y - 1].b == 4); + assert(x[y].a == 5); + assert(x[y].b == 6); + + if y != 2 { + x[y].a = 50; + } else { + x[y].a = 100; + } + + dep::std::println(x); +} From 45b37873e474006be4e573366c09de835f9832c6 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 14 Sep 2023 18:35:13 +0000 Subject: [PATCH 02/14] remove debug --- compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 9f93e982658..d57c2cc7933 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -554,7 +554,6 @@ impl<'f> Context<'f> { 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(); - dbg!(index); let index = self.inserter.function.dfg.make_constant(index, Type::field()); let typevars = Some(vec![element_type.clone()]); From 7ccf8e901776b90883b373ea5bc0a89933117b53 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Fri, 15 Sep 2023 19:36:37 +0000 Subject: [PATCH 03/14] working non-homogenous arr accesses for block params --- .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 89 +++++++- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 214 ++++++++++++++---- .../noirc_evaluator/src/ssa/ir/instruction.rs | 3 + .../nested_array_dynamic/Prover.toml | 28 ++- .../nested_array_dynamic/src/main.nr | 45 ++-- 5 files changed, 314 insertions(+), 65 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index b5461269f4a..46eb0907c96 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -61,6 +61,11 @@ impl AcirType { AcirType::NumericType(NumericType::NativeField) } + /// Returns an unsigned type of the specified bit size + pub(crate) fn unsigned(bit_size: u32) -> Self { + AcirType::NumericType(NumericType::Unsigned { bit_size }) + } + /// Returns a boolean type fn boolean() -> Self { AcirType::NumericType(NumericType::Unsigned { bit_size: 1 }) @@ -74,6 +79,16 @@ impl AcirType { }; matches!(numeric_type, NumericType::Signed { .. }) } + + pub(crate) fn from_slice(value: &SsaType, size: usize) -> Self { + match value { + SsaType::Slice(elements) => { + let elements = elements.iter().map(|e| e.into()).collect(); + AcirType::Array(elements, size) + } + _ => unreachable!("Attempted to use {value} where a slice was expected"), + } + } } impl From for AcirType { @@ -90,7 +105,7 @@ impl<'a> From<&'a SsaType> for AcirType { let elements = elements.iter().map(|e| e.into()).collect(); AcirType::Array(elements, *size) } - _ => unreachable!("The type {value} cannot be represented in ACIR"), + _ => unreachable!("The type {value} cannot be represented in ACIR"), } } } @@ -1220,6 +1235,8 @@ impl AcirContext { if let Ok(some_value) = value.clone().into_var() { values.push(self.var_to_witness(some_value)?); } else { + dbg!(optional_values.clone()); + dbg!("got here"); nested = true; break; } @@ -1228,10 +1245,74 @@ impl AcirContext { } }; // we do not initialize nested arrays. This means that non-const indexes are not supported for nested arrays - if !nested { - self.acir_ir.push_opcode(Opcode::MemoryInit { block_id, init: initialized_values }); - } + // if !nested { + // self.acir_ir.push_opcode(Opcode::MemoryInit { block_id, init: initialized_values }); + // } + dbg!(nested); + self.acir_ir.push_opcode(Opcode::MemoryInit { block_id, init: initialized_values }); + + + Ok(()) + } + + pub(crate) fn initialize_array_new( + &mut self, + block_id: BlockId, + len: usize, + optional_value: Option, + ) -> Result<(), InternalError> { + let initialized_values = match optional_value { + None => { + let zero = self.add_constant(FieldElement::zero()); + let zero_witness = self.var_to_witness(zero)?; + vec![zero_witness; len] + } + Some(optional_value) => { + let mut values = Vec::new(); + self.initialize_array_inner(&mut values, optional_value)?; + values + } + }; + self.acir_ir.push_opcode(Opcode::MemoryInit { block_id, init: initialized_values }); + + Ok(()) + } + + fn initialize_array_inner( + &mut self, + witnesses: &mut Vec, + input: AcirValue, + ) -> Result<(), InternalError> { + match input { + AcirValue::Var(var, _) => { + witnesses.push(self.var_to_witness(var)?); + } + AcirValue::Array(values) => { + for value in values { + self.initialize_array_inner(witnesses, value)?; + } + } + // AcirValue::DynamicArray(AcirDynamicArray { block_id, len }) => { + AcirValue::DynamicArray(_) => { + panic!("dyn array should already be initialized"); + } + // AcirValue::DynamicArray(AcirDynamicArray { block_id, len }) => { + // for i in 0..len { + // // We generate witnesses corresponding to the array values + // let index = AcirValue::Var( + // self.add_constant(FieldElement::from(i as u128)), + // AcirType::NumericType(NumericType::NativeField), + // ); + + // let index_var = index.into_var()?; + // let value_read_var = + // self.read_from_memory(block_id, &index_var)?; + + // witnesses.push(self.var_to_witness(value_read_var)?); + // } + // } + } Ok(()) } } diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 39d56706990..ef62d1593fc 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -244,8 +244,11 @@ impl Context { AcirValue::Var(_, _) => (), AcirValue::Array(values) => { let block_id = self.block_id(param_id); + dbg!(block_id.0); + dbg!(values.len()); let v = vecmap(values, |v| v.clone()); - self.initialize_array(block_id, values.len(), Some(&v))?; + // self.initialize_array(block_id, values.len(), Some(&v))?; + self.initialize_array_new(block_id, values.len(), Some(value.clone()))?; } AcirValue::DynamicArray(_) => unreachable!( "The dynamic array type is created in Acir gen and therefore cannot be a block parameter" @@ -257,6 +260,26 @@ impl Context { Ok(start_witness..=end_witness) } + // fn initialize_nested_array( + // &mut self, + // array: &ValueId, + // acir_value: &AcirValue, + // ) -> Result<(), RuntimeError> { + // match acir_value { + // AcirValue::Var(_, _) => (), + // AcirValue::Array(values) => { + // let block_id = self.block_id(array); + // dbg!(block_id.0); + // let v = vecmap(values, |v| v.clone()); + // self.initialize_array(block_id, values.len(), Some(&v))?; + // } + // AcirValue::DynamicArray(_) => unreachable!( + // "The dynamic array type is created in Acir gen and therefore cannot be a block parameter" + // ), + // } + // Ok(()) + // } + fn convert_ssa_block_param(&mut self, param_type: &Type) -> Result { self.create_value_from_type(param_type, &mut |this, typ| this.add_numeric_input_var(&typ)) } @@ -339,6 +362,8 @@ impl Context { rhs: AcirValue, read_from_index: &mut impl FnMut(BlockId, usize) -> Result, ) -> Result, InternalError> { + dbg!(lhs.clone()); + dbg!(rhs.clone()); match (lhs, rhs) { (AcirValue::Var(lhs, _), AcirValue::Var(rhs, _)) => Ok(vec![(lhs, rhs)]), (AcirValue::Array(lhs_values), AcirValue::Array(rhs_values)) => { @@ -545,36 +570,7 @@ impl Context { } }; - // We compute some AcirVars: - // - index_var is the index of the array - // - predicate_index is 0, or the index if the predicate is true - // - new_value is the optional value when the operation is an array_set - // When there is a predicate, it is predicate*value + (1-predicate)*dummy, where dummy is the value of the array at the requested index. - // It is a dummy value because in the case of a false predicate, the value stored at the requested index will be itself. let index_const = dfg.get_numeric_constant(index); - let index_var = self.convert_numeric_value(index, dfg)?; - let predicate_index = - self.acir_context.mul_var(index_var, self.current_side_effects_enabled_var)?; - let new_value = if let Some(store) = store_value { - let store_var = Some(self.convert_value(store, dfg).into_var()?); - if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) { - store_var - } else { - let dummy = self.array_get(instruction, array, predicate_index, dfg)?; - let true_pred = self - .acir_context - .mul_var(store_var.unwrap(), self.current_side_effects_enabled_var)?; - let one = self.acir_context.add_constant(FieldElement::one()); - let not_pred = - self.acir_context.sub_var(one, self.current_side_effects_enabled_var)?; - let false_pred = self.acir_context.mul_var(not_pred, dummy)?; - // predicate*value + (1-predicate)*dummy - Some(self.acir_context.add_var(true_pred, false_pred)?) - } - } else { - None - }; - // Handle constant index: if there is no predicate and we have the array values, we can perform the operation directly on the array match dfg.type_of_value(array) { Type::Array(_, _) => { @@ -641,12 +637,7 @@ impl Context { _ => unreachable!("ICE: expected array or slice type"), } - let new_index = if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) - { - index_var - } else { - predicate_index - }; + let (new_index, new_value) = self.convert_array_operation_inputs(instruction, dfg, array, index, store_value)?; let resolved_array = dfg.resolve(array); let map_array = last_array_uses.get(&resolved_array) == Some(&instruction); @@ -660,6 +651,54 @@ impl Context { Ok(()) } + /// We need to properly setup the inputs for array operations in ACIR. + /// From the original SSA values we compute the following AcirVars: + /// - index_var is the index of the array + /// - predicate_index is 0, or the index if the predicate is true + /// - new_value is the optional value when the operation is an array_set + /// When there is a predicate, it is predicate*value + (1-predicate)*dummy, where dummy is the value of the array at the requested index. + /// It is a dummy value because in the case of a false predicate, the value stored at the requested index will be itself. + fn convert_array_operation_inputs( + &mut self, + instruction: InstructionId, + dfg: &DataFlowGraph, + array: ValueId, + index: ValueId, + store_value: Option, + ) -> Result<(AcirVar, Option), RuntimeError> { + let index_var = self.convert_numeric_value(index, dfg)?; + let predicate_index = + self.acir_context.mul_var(index_var, self.current_side_effects_enabled_var)?; + let new_value = if let Some(store) = store_value { + let store_var = Some(self.convert_value(store, dfg).into_var()?); + if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) { + store_var + } else { + let dummy = self.array_get(instruction, array, predicate_index, dfg)?; + let true_pred = self + .acir_context + .mul_var(store_var.unwrap(), self.current_side_effects_enabled_var)?; + let one = self.acir_context.add_constant(FieldElement::one()); + let not_pred = + self.acir_context.sub_var(one, self.current_side_effects_enabled_var)?; + let false_pred = self.acir_context.mul_var(not_pred, dummy)?; + // predicate*value + (1-predicate)*dummy + Some(self.acir_context.add_var(true_pred, false_pred)?) + } + } else { + None + }; + + let new_index = if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) + { + index_var + } else { + predicate_index + }; + + return Ok((new_index, new_value)) + } + /// Generates a read opcode for the array fn array_get( &mut self, @@ -670,6 +709,12 @@ impl Context { ) -> Result { let array = dfg.resolve(array); let block_id = self.block_id(&array); + let results = dfg.instruction_results(instruction); + dbg!(results); + dbg!(dfg.resolve(results[0])); + let res_typ = dfg.type_of_value(results[0]); + dbg!(res_typ.clone()); + dbg!(block_id.0); if !self.initialized_arrays.contains(&block_id) { match &dfg[array] { Value::Array { array, .. } => { @@ -686,13 +731,91 @@ impl Context { } } + let array_typ = dfg.type_of_value(array); + // dbg!(array_typ.clone()); + let element_size = array_typ.element_size(); + dbg!(element_size); + let element_size_var = self.acir_context.add_constant(FieldElement::from(element_size as u128)); + // let var_index = self.acir_context. + let offset = self.acir_context.div_var(var_index, element_size_var, AcirType::unsigned(32), self.current_side_effects_enabled_var)?; + let mut is_first_elem = true; + let var_index = self.acir_context.add_var(var_index, offset)?; + + let mut values = Vector::new(); + self.array_get_type(dfg, &res_typ, var_index, &mut values, block_id, &mut is_first_elem)?; + dbg!(values.clone()); + + self.define_result(dfg, instruction, values[0].clone()); + + // TODO: update array_get to return an AcirValue let read = self.acir_context.read_from_memory(block_id, &var_index)?; - let typ = dfg.type_of_value(array); - let typ = AcirType::from(typ); - self.define_result(dfg, instruction, AcirValue::Var(read, typ)); Ok(read) } + // TOOD: switch this to use create_value_from_type + fn array_get_type( + &mut self, + dfg: &DataFlowGraph, + ssa_type: &Type, + // acir_types: &mut Vec, + mut var_index: AcirVar, + values: &mut Vector, + block_id: BlockId, + first_elem: &mut bool, + ) -> Result<(), RuntimeError> { + // dbg!(ssa_type.clone()); + let one = self.acir_context.add_constant(FieldElement::one()); + // TODO: update var_index + match ssa_type.clone() { + Type::Numeric(numeric_type) => { + if !*first_elem { + var_index = self.acir_context.add_var(var_index, one)?; + } + *first_elem = false; + // var_index = self.acir_context.add_var(var_index, one)?; + + let read = self.acir_context.read_from_memory(block_id, &var_index)?; + let typ = AcirType::NumericType(numeric_type); + let value = AcirValue::Var(read, typ); + values.push_back(value); + } + Type::Array(element_types, len) => { + // element_types[i].clone() + // dbg!(element_types.len()); + // dbg!(element_types.clone()); + // for _ in 0..len { + // for _ in element_types.as_ref() { + // // if !*first_elem { + // // var_index = self.acir_context.add_var(var_index, one)?; + // // } + // // var_index = self.acir_context.add_var(var_index, one)?; + // } + // } + let mut inner_vec = Vector::new(); + for _ in 0..len { + for typ in element_types.as_ref() { + self.array_get_type(dfg, typ, var_index, &mut inner_vec, block_id, first_elem)?; + // inner_vec.push_back(elem); + } + } + let array_value = AcirValue::Array(inner_vec); + values.push_back(array_value); + } + Type::Slice(element_types) => { + // element_types[i].clone() + dbg!(element_types.len()); + // dbg!(element_types.clone()); + dbg!("got here?"); + for typ in element_types.as_ref() { + // var_index = self.acir_context.add_var(var_index, one)?; + self.array_get_type(dfg, typ, var_index, values, block_id, first_elem)?; + } + } + _ => unreachable!("ICE - expected an array or slice"), + }; + Ok(()) + } + /// Copy the array and generates a write opcode on the new array /// /// Note: Copying the array is inefficient and is not the way we want to do it in the end. @@ -746,7 +869,7 @@ impl Context { } _ => unreachable!("ICE - expected an array"), }; - + dbg!(len); // Check if the array has already been initialized in ACIR gen // if not, we initialize it using the values from SSA let already_initialized = self.initialized_arrays.contains(&block_id); @@ -814,6 +937,17 @@ impl Context { Ok(()) } + fn initialize_array_new( + &mut self, + array: BlockId, + len: usize, + value: Option, + ) -> Result<(), InternalError> { + self.acir_context.initialize_array_new(array, len, value)?; + self.initialized_arrays.insert(array); + Ok(()) + } + /// Remember the result of an instruction returning a single value fn define_result( &mut self, diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 1dd2368b1a0..ae24db611c7 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -396,6 +396,9 @@ impl Instruction { if let (Some((array, _)), Some(index)) = (array, index) { let index = index.try_to_u64().expect("Expected array index to fit in u64") as usize; + dbg!(array.len()); + dbg!(array.clone()); + dbg!(index); if index < array.len() { return SimplifiedTo(array[index]); } diff --git a/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml b/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml index 00ffa6e4620..57603c24863 100644 --- a/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml +++ b/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml @@ -1,13 +1,29 @@ -y = "2" +y = "3" [[x]] a = "1" -b = "2" +b = ["2", "3"] [[x]] -a = "3" -b = "4" +a = "4" +b = ["5", "6"] [[x]] -a = "5" -b = "6" +a = "7" +b = ["8", "9"] + +[[x]] +a = "10" +b = ["11", "12"] + +# [[x]] +# a = "1" +# b = "2" + +# [[x]] +# a = "3" +# b = "4" + +# [[x]] +# a = "5" +# b = "6" 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 bda11468b84..1fbaef8b1eb 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 @@ -1,22 +1,37 @@ struct Foo { a: Field, - b: Field, + b: [Field; 2], } -fn main(mut x : [Foo; 3], y : pub Field) { - assert(x[y - 2].a == 1); - assert(x[y - 2].b == 2); - assert(x[y - 1].a == 3); - assert(x[y - 1].b == 4); - assert(x[y].a == 5); - assert(x[y].b == 6); +fn main(mut x : [Foo; 4], y : pub Field) { + dep::std::println(x[y - 3]); + dep::std::println(x[y - 2]); + dep::std::println(x[y - 1]); + dep::std::println(x[y]); - if y != 2 { - x[y].a = 50; - } else { - x[y].a = 100; - } - - dep::std::println(x); + assert(x[y - 3].a == 1); + assert(x[y - 3].b == [2, 3]); + assert(x[y - 2].a == 4); + assert(x[y - 2].b == [5, 6]); + assert(x[y - 1].a == 7); + assert(x[y - 1].b == [8, 9]); + assert(x[y].a == 10); + assert(x[y].b == [11, 12]); + + // if y != 2 { + // x[y].a = 50; + // } else { + // x[y].a = 100; + // } + // assert(x[y].a == 50); + + // dep::std::println(x[y].a); } + +// fn main(mut x : [Foo; 4], y : pub Field) { +// dep::std::println(x[0]); +// dep::std::println(x[1]); +// dep::std::println(x[2]); +// dep::std::println(x[3]); +// } From d5d16d7c06d948a9f8303fe541b633cd4e8a832a Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 19 Sep 2023 00:22:35 +0000 Subject: [PATCH 04/14] add array_set, broken, need to maintain structure from SSA for accurate index --- .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 27 +- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 353 +++++++++++++----- .../noirc_evaluator/src/ssa/ir/instruction.rs | 2 +- compiler/noirc_evaluator/src/ssa/ir/types.rs | 19 + .../src/ssa/opt/flatten_cfg.rs | 6 +- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 1 + .../nested_array_dynamic/Prover.toml | 32 +- .../nested_array_dynamic/src/main.nr | 70 ++-- 8 files changed, 366 insertions(+), 144 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index 46eb0907c96..be61dcaf022 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -273,6 +273,13 @@ impl AcirContext { } } + pub(crate) fn get_constant(&self, var: &AcirVar) -> Option { + match self.vars[var] { + AcirVarData::Const(field) => Some(field), + _ => None, + } + } + /// Adds a new Variable to context whose value will /// be constrained to be the negation of `var`. /// @@ -1273,7 +1280,7 @@ impl AcirContext { values } }; - + // dbg!(initialized_values.len()); self.acir_ir.push_opcode(Opcode::MemoryInit { block_id, init: initialized_values }); Ok(()) @@ -1293,25 +1300,11 @@ impl AcirContext { self.initialize_array_inner(witnesses, value)?; } } - // AcirValue::DynamicArray(AcirDynamicArray { block_id, len }) => { AcirValue::DynamicArray(_) => { panic!("dyn array should already be initialized"); } - // AcirValue::DynamicArray(AcirDynamicArray { block_id, len }) => { - // for i in 0..len { - // // We generate witnesses corresponding to the array values - // let index = AcirValue::Var( - // self.add_constant(FieldElement::from(i as u128)), - // AcirType::NumericType(NumericType::NativeField), - // ); - - // let index_var = index.into_var()?; - // let value_read_var = - // self.read_from_memory(block_id, &index_var)?; - - // witnesses.push(self.var_to_witness(value_read_var)?); - // } - // } + // TODO: I think it is correct that we should panic on dyn array + // but need to test and verify } Ok(()) } diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index ef62d1593fc..b70dee38910 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -95,10 +95,13 @@ impl AcirValue { fn into_var(self) -> Result { match self { AcirValue::Var(var, _) => Ok(var), - AcirValue::DynamicArray(_) | AcirValue::Array(_) => Err(InternalError::General { - message: "Called AcirValue::into_var on an array".to_string(), - call_stack: CallStack::new(), - }), + AcirValue::DynamicArray(_) | AcirValue::Array(_) => { + dbg!(self); + Err(InternalError::General { + message: "Called AcirValue::into_var on an array".to_string(), + call_stack: CallStack::new(), + }) + } } } @@ -246,7 +249,7 @@ impl Context { let block_id = self.block_id(param_id); dbg!(block_id.0); dbg!(values.len()); - let v = vecmap(values, |v| v.clone()); + // let v = vecmap(values, |v| v.clone()); // self.initialize_array(block_id, values.len(), Some(&v))?; self.initialize_array_new(block_id, values.len(), Some(value.clone()))?; } @@ -260,26 +263,6 @@ impl Context { Ok(start_witness..=end_witness) } - // fn initialize_nested_array( - // &mut self, - // array: &ValueId, - // acir_value: &AcirValue, - // ) -> Result<(), RuntimeError> { - // match acir_value { - // AcirValue::Var(_, _) => (), - // AcirValue::Array(values) => { - // let block_id = self.block_id(array); - // dbg!(block_id.0); - // let v = vecmap(values, |v| v.clone()); - // self.initialize_array(block_id, values.len(), Some(&v))?; - // } - // AcirValue::DynamicArray(_) => unreachable!( - // "The dynamic array type is created in Acir gen and therefore cannot be a block parameter" - // ), - // } - // Ok(()) - // } - fn convert_ssa_block_param(&mut self, param_type: &Type) -> Result { self.create_value_from_type(param_type, &mut |this, typ| this.add_numeric_input_var(&typ)) } @@ -362,8 +345,8 @@ impl Context { rhs: AcirValue, read_from_index: &mut impl FnMut(BlockId, usize) -> Result, ) -> Result, InternalError> { - dbg!(lhs.clone()); - dbg!(rhs.clone()); + // dbg!(lhs.clone()); + // dbg!(rhs.clone()); match (lhs, rhs) { (AcirValue::Var(lhs, _), AcirValue::Var(rhs, _)) => Ok(vec![(lhs, rhs)]), (AcirValue::Array(lhs_values), AcirValue::Array(rhs_values)) => { @@ -403,6 +386,8 @@ impl Context { self.acir_context.add_constant(FieldElement::from(array_index as u128)), AcirType::NumericType(NumericType::NativeField), ); + dbg!("inside read_dynamic_array_index"); + let index_var = index.into_var()?; self.acir_context.read_from_memory(block_id, &index_var) @@ -440,10 +425,11 @@ impl Context { assert_eq!(result_ids.len(), output_values.len(), "ICE: The number of Brillig output values should match the result ids in SSA"); for result in result_ids.iter().zip(output_values) { - if let AcirValue::Array(values) = &result.1 { + if let AcirValue::Array(values) = &result.1 { let block_id = self.block_id(&dfg.resolve(*result.0)); - let values: Vec = values.iter().cloned().collect(); - self.initialize_array(block_id, values.len(), Some(&values))?; + // let values: Vec = values.iter().cloned().collect(); + // self.initialize_array(block_id, values.len(), Some(&values))?; + self.initialize_array_new(block_id, values.len(), Some(result.1.clone()))?; } self.ssa_values.insert(*result.0, result.1); } @@ -466,7 +452,8 @@ impl Context { let block_id = self.block_id(result); let values = vecmap(values, |v| v.clone()); - self.initialize_array(block_id, values.len(), Some(&values))?; + self.initialize_array_new(block_id, values.len(), Some(output.clone()))?; + // self.initialize_array(block_id, values.len(), Some(&values))?; } AcirValue::DynamicArray(_) => { unreachable!("The output from an intrinsic call is expected to be a single value or an array but got {output:?}"); @@ -559,7 +546,10 @@ impl Context { // Pass the instruction between array methods rather than the internal fields themselves let (array, index, store_value) = match dfg[instruction] { Instruction::ArrayGet { array, index } => (array, index, None), - Instruction::ArraySet { array, index, value, .. } => (array, index, Some(value)), + Instruction::ArraySet { array, index, value, .. } => { + dbg!(&dfg[value]); + (array, index, Some(value)) + } _ => { return Err(InternalError::UnExpected { expected: "Instruction should be an ArrayGet or ArraySet".to_owned(), @@ -571,6 +561,7 @@ impl Context { }; let index_const = dfg.get_numeric_constant(index); + // dbg!(index_const.clone()); // Handle constant index: if there is no predicate and we have the array values, we can perform the operation directly on the array match dfg.type_of_value(array) { Type::Array(_, _) => { @@ -609,8 +600,11 @@ impl Context { call_stack, }); } else { + // dbg!("updating because index is known"); + // dbg!(index); let value = match store_value { Some(store_value) => { + // dbg!("got here"); let store_value = self.convert_value(store_value, dfg); AcirValue::Array(array.update(index, store_value)) } @@ -637,7 +631,8 @@ impl Context { _ => unreachable!("ICE: expected array or slice type"), } - let (new_index, new_value) = self.convert_array_operation_inputs(instruction, dfg, array, index, store_value)?; + // let (new_index, new_value) = self.convert_array_operation_inputs(instruction, dfg, array, index, store_value)?; + let (new_index, new_value) = self.convert_array_operation_inputs_new(instruction, dfg, array, index, store_value)?; let resolved_array = dfg.resolve(array); let map_array = last_array_uses.get(&resolved_array) == Some(&instruction); @@ -670,10 +665,17 @@ impl Context { let predicate_index = self.acir_context.mul_var(index_var, self.current_side_effects_enabled_var)?; let new_value = if let Some(store) = store_value { - let store_var = Some(self.convert_value(store, dfg).into_var()?); + dbg!("about to get store var"); + println!("store value id: {store}"); + let resolved_store = dfg.resolve(store); + println!("resolved store: {resolved_store}"); + let acir_value = self.convert_value(store, dfg); + let store_var = Some(acir_value.into_var()?); + dbg!("got store var"); if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) { store_var } else { + dbg!("need to check predicate"); let dummy = self.array_get(instruction, array, predicate_index, dfg)?; let true_pred = self .acir_context @@ -699,6 +701,86 @@ impl Context { return Ok((new_index, new_value)) } + fn convert_array_operation_inputs_new( + &mut self, + instruction: InstructionId, + dfg: &DataFlowGraph, + array: ValueId, + index: ValueId, + store_value: Option, + ) -> Result<(AcirVar, Option), RuntimeError> { + let index_var = self.convert_numeric_value(index, dfg)?; + let predicate_index = + self.acir_context.mul_var(index_var, self.current_side_effects_enabled_var)?; + + // dbg!("about to convert_value: "); + // dbg!(store_value.clone()); + let new_value = if let Some(store) = store_value { + dbg!(&dfg[store]); + let store_value = self.convert_value(store, dfg); + dbg!(store_value.clone()); + // let store_value = self.convert_array_set_store_value(&store_value, dummy)?; + if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) { + dbg!("got here"); + Some(store_value) + } else { + dbg!("using dummy"); + let dummy = self.array_get(instruction, array, predicate_index, dfg)?; + + // TODO: fix this it is a placeholder and wrong!!! + // Some(self.convert_value(store, dfg)) + Some(self.convert_array_set_store_value(&store_value, dummy)?) + // Some(store_value) + } + } else { + None + }; + + let new_index = if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) + { + index_var + } else { + predicate_index + }; + // dbg!(new_value.clone()); + Ok((new_index, new_value)) + } + + fn convert_array_set_store_value( + &mut self, + store_value: &AcirValue, + // TODO: probably want to change this dummy to be in here + dummy: AcirVar, + ) -> Result { + match store_value { + AcirValue::Var(store_var, _) => { + // let dummy = self.array_get(instruction, array, predicate_index, dfg)?; + let true_pred = self + .acir_context + .mul_var(*store_var, self.current_side_effects_enabled_var)?; + let one = self.acir_context.add_constant(FieldElement::one()); + let not_pred = + self.acir_context.sub_var(one, self.current_side_effects_enabled_var)?; + let false_pred = self.acir_context.mul_var(not_pred, dummy)?; + // predicate*value + (1-predicate)*dummy + let new_value = self.acir_context.add_var(true_pred, false_pred)?; + Ok(AcirValue::Var(new_value, AcirType::field())) + } + AcirValue::Array(values) => { + let mut elements = im::Vector::new(); + + for val in values { + elements.push_back(self.convert_array_set_store_value(val, dummy)?); + } + + Ok(AcirValue::Array(elements)) + } + AcirValue::DynamicArray(_) => { + panic!("ahhh") + } + } + } + /// Generates a read opcode for the array fn array_get( &mut self, @@ -708,21 +790,24 @@ impl Context { dfg: &DataFlowGraph, ) -> Result { let array = dfg.resolve(array); + // dbg!(array); let block_id = self.block_id(&array); let results = dfg.instruction_results(instruction); - dbg!(results); - dbg!(dfg.resolve(results[0])); + // dbg!(results); + // dbg!(dfg.resolve(results[0])); let res_typ = dfg.type_of_value(results[0]); dbg!(res_typ.clone()); dbg!(block_id.0); if !self.initialized_arrays.contains(&block_id) { match &dfg[array] { Value::Array { array, .. } => { - let values: Vec = + let values: Vector = array.iter().map(|i| self.convert_value(*i, dfg)).collect(); - self.initialize_array(block_id, array.len(), Some(&values))?; + self.initialize_array_new(block_id, values.len(), Some(AcirValue::Array(values)))?; + // self.initialize_array(block_id, array.len(), Some(&values))?; } _ => { + dbg!(&dfg[array]); return Err(RuntimeError::UnInitialized { name: "array".to_string(), call_stack: self.acir_context.get_call_stack(), @@ -731,19 +816,46 @@ impl Context { } } + // let res_type_size = res_typ.element_size(); + // dbg!(res_type_size); + let res_type_flat_size = res_typ.flattened_element_size(); + dbg!(res_type_flat_size); + let array_typ = dfg.type_of_value(array); // dbg!(array_typ.clone()); let element_size = array_typ.element_size(); - dbg!(element_size); + dbg!(element_size); + let flat_array_size = array_typ.flattened_element_size(); + dbg!(flat_array_size); + let array_len = dfg.try_get_array_length(array).expect("ICE: need to have an array, slices not implemented"); + let flat_elem_size = flat_array_size / array_len; + let flat_element_size_var = self.acir_context.add_constant(FieldElement::from(flat_elem_size as u128)); + let element_size_var = self.acir_context.add_constant(FieldElement::from(element_size as u128)); - // let var_index = self.acir_context. - let offset = self.acir_context.div_var(var_index, element_size_var, AcirType::unsigned(32), self.current_side_effects_enabled_var)?; - let mut is_first_elem = true; - let var_index = self.acir_context.add_var(var_index, offset)?; + // TODO: switch to use euclidean_division_var + 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 = self.acir_context.modulo_var(var_index, element_size_var, 32, self.current_side_effects_enabled_var)?; + let var_index = self.acir_context.mul_var(outer_offset, flat_element_size_var)?; + let var_index = self.acir_context.add_var(var_index, inner_offset)?; + + // Work with index + let one = self.acir_context.add_constant(FieldElement::one()); + let pred = self.acir_context.less_than_var(one, inner_offset, 32, self.current_side_effects_enabled_var)?; + let not_pred = self.acir_context.sub_var(one, pred)?; + let var_index_plus_one = self.acir_context.add_var(var_index, one)?; + let true_pred = self.acir_context.mul_var(pred, var_index_plus_one)?; + let false_pred = self.acir_context.mul_var(not_pred, var_index)?; + let var_index = self.acir_context.add_var(true_pred, false_pred)?; + // Had to add one here once we added a value to preceding element type in program + // TODO: make this general as this was simply for testing + let mut var_index = self.acir_context.add_var(var_index, one)?; + let mut is_first_elem = true; let mut values = Vector::new(); - self.array_get_type(dfg, &res_typ, var_index, &mut values, block_id, &mut is_first_elem)?; - dbg!(values.clone()); + self.array_get_type(dfg, &res_typ, &mut var_index, &mut values, block_id, &mut is_first_elem)?; + // dbg!(values.clone()); + assert_eq!(values.len(), 1); + // dbg!(values.len()); self.define_result(dfg, instruction, values[0].clone()); @@ -758,21 +870,19 @@ impl Context { dfg: &DataFlowGraph, ssa_type: &Type, // acir_types: &mut Vec, - mut var_index: AcirVar, + var_index: &mut AcirVar, values: &mut Vector, block_id: BlockId, first_elem: &mut bool, ) -> Result<(), RuntimeError> { - // dbg!(ssa_type.clone()); let one = self.acir_context.add_constant(FieldElement::one()); - // TODO: update var_index match ssa_type.clone() { Type::Numeric(numeric_type) => { + dbg!(*first_elem); if !*first_elem { - var_index = self.acir_context.add_var(var_index, one)?; + *var_index = self.acir_context.add_var(*var_index, one)?; } *first_elem = false; - // var_index = self.acir_context.add_var(var_index, one)?; let read = self.acir_context.read_from_memory(block_id, &var_index)?; let typ = AcirType::NumericType(numeric_type); @@ -780,36 +890,30 @@ impl Context { values.push_back(value); } Type::Array(element_types, len) => { - // element_types[i].clone() - // dbg!(element_types.len()); - // dbg!(element_types.clone()); - // for _ in 0..len { - // for _ in element_types.as_ref() { - // // if !*first_elem { - // // var_index = self.acir_context.add_var(var_index, one)?; - // // } - // // var_index = self.acir_context.add_var(var_index, one)?; - // } - // } let mut inner_vec = Vector::new(); - for _ in 0..len { + dbg!(len); + for i in 0..len { + dbg!(i); for typ in element_types.as_ref() { self.array_get_type(dfg, typ, var_index, &mut inner_vec, block_id, first_elem)?; - // inner_vec.push_back(elem); } } let array_value = AcirValue::Array(inner_vec); values.push_back(array_value); } Type::Slice(element_types) => { - // element_types[i].clone() dbg!(element_types.len()); - // dbg!(element_types.clone()); dbg!("got here?"); - for typ in element_types.as_ref() { - // var_index = self.acir_context.add_var(var_index, one)?; - self.array_get_type(dfg, typ, var_index, values, block_id, first_elem)?; - } + panic!("ICE: Non-const indices for nested arrays of slices is not allowed"); + // TODO: need values here to fetch the len like for a Type::Array, not obvious how to incorporate it yet + // let mut inner_vec = Vector::new(); + // for _ in 0..len { + // for typ in element_types.as_ref() { + // self.array_get_type(dfg, typ, var_index, &mut inner_vec, block_id, first_elem)?; + // } + // } + // let array_value = AcirValue::Array(inner_vec); + // values.push_back(array_value); } _ => unreachable!("ICE - expected an array or slice"), }; @@ -823,10 +927,12 @@ impl Context { &mut self, instruction: InstructionId, var_index: AcirVar, - store_value: AcirVar, + store_value: AcirValue, dfg: &DataFlowGraph, map_array: bool, - ) -> Result<(), InternalError> { + ) -> Result<(), RuntimeError> { + dbg!("INSIDE ARRAY_SET"); + // let x = self.acir_context. // Pass the instruction between array methods rather than the internal fields themselves let (array, length) = match dfg[instruction] { Instruction::ArraySet { array, length, .. } => (array, length), @@ -835,15 +941,16 @@ impl Context { expected: "Instruction should be an ArraySet".to_owned(), found: format!("Instead got {:?}", dfg[instruction]), call_stack: self.acir_context.get_call_stack(), - }) + }.into()) } }; // Fetch the internal SSA ID for the array - let array = dfg.resolve(array); + let array_id = dfg.resolve(array); + dbg!(array_id); // Use the SSA ID to get or create its block ID - let block_id = self.block_id(&array); + let block_id = self.block_id(&array_id); // Every array has a length in its type, so we fetch that from // the SSA IR. @@ -852,7 +959,8 @@ 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 len = match dfg.type_of_value(array) { + let array_typ = dfg.type_of_value(array_id); + let len = match &array_typ { Type::Array(typ, len) => { // Flatten the array length to handle arrays of complex types len * typ.len() @@ -869,22 +977,26 @@ impl Context { } _ => unreachable!("ICE - expected an array"), }; - dbg!(len); + // Check if the array has already been initialized in ACIR gen // if not, we initialize it using the values from SSA let already_initialized = self.initialized_arrays.contains(&block_id); + dbg!(already_initialized); if !already_initialized { - match &dfg[array] { + let value = &dfg[array_id]; + match value { Value::Array { array, .. } => { - let values: Vec = - array.iter().map(|i| self.convert_value(*i, dfg)).collect(); - self.initialize_array(block_id, array.len(), Some(&values))?; + // let values: Vector = + // array.iter().map(|i| self.convert_value(*i, dfg)).collect(); + let value = self.convert_value(array_id, dfg); + self.initialize_array_new(block_id, array.len(), Some(value))?; + // self.initialize_array(block_id, array.len(), Some(&values))?; } _ => { return Err(InternalError::General { message: format!("Array {array} should be initialized"), call_stack: self.acir_context.get_call_stack(), - }) + }.into()) } } } @@ -898,32 +1010,99 @@ impl Context { .expect("Array set does not have one result"); let result_block_id; if map_array { + dbg!(map_array); self.memory_blocks.insert(*result_id, block_id); result_block_id = block_id; } else { + dbg!("got here"); + // dbg!(len); + // let array_typ = dfg.type_of_value(array_id); + // let element_size = array_typ.element_size(); + // dbg!(element_size); + let flat_elem_size = array_typ.flattened_element_size(); + dbg!(flat_elem_size); // Initialize the new array with the values from the old array result_block_id = self.block_id(result_id); - let init_values = try_vecmap(0..len, |i| { + dbg!(result_block_id.0); + // TODO: probably need to fix how we read the old array to account for nested arrays + let init_values = try_vecmap(0..flat_elem_size, |i| { let index = AcirValue::Var( self.acir_context.add_constant(FieldElement::from(i as u128)), AcirType::NumericType(NumericType::NativeField), ); let var = index.into_var()?; let read = self.acir_context.read_from_memory(block_id, &var)?; - Ok(AcirValue::Var(read, AcirType::NumericType(NumericType::NativeField))) + Ok::(AcirValue::Var(read, AcirType::NumericType(NumericType::NativeField))) })?; - self.initialize_array(result_block_id, len, Some(&init_values))?; + dbg!(len); + self.initialize_array_new(result_block_id, flat_elem_size, Some(AcirValue::Array(init_values.into())))?; + // self.initialize_array(result_block_id, len, Some(&init_values))?; } - // Write the new value into the new array at the specified index - self.acir_context.write_to_memory(result_block_id, &var_index, &store_value)?; + // let index_const = self.acir_context.get_constant(&var_index); + // dbg!(index_const); + let element_size = array_typ.element_size(); + // dbg!(element_size); + let flat_array_size = array_typ.flattened_element_size(); + // dbg!(flat_array_size); + let array_len = dfg.try_get_array_length(array).expect("ICE: need to have an array, slices not implemented"); + // dbg!(array_len); + let flat_elem_size = flat_array_size / array_len; + let flat_element_size_var = self.acir_context.add_constant(FieldElement::from(flat_elem_size as u128)); + + let element_size_var = self.acir_context.add_constant(FieldElement::from(element_size as u128)); + // TODO: switch to use euclidean_division-var + 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 = self.acir_context.modulo_var(var_index, element_size_var, 32, self.current_side_effects_enabled_var)?; + let var_index = self.acir_context.mul_var(outer_offset, flat_element_size_var)?; + // let one = self.acir_context.add_constant(FieldElement::one()); + // let var_index = self.acir_context.add_var(var_index, one)?; + let mut var_index = self.acir_context.add_var(var_index, inner_offset)?; + + let index_const = self.acir_context.get_constant(&var_index); + dbg!(index_const); + let mut is_first_elem = true; + dbg!(result_block_id.0); + dbg!(store_value.clone()); + self.array_set_value(store_value, result_block_id, &mut var_index, &mut is_first_elem)?; + dbg!(len); + dbg!(result_block_id.0); let result_value = - AcirValue::DynamicArray(AcirDynamicArray { block_id: result_block_id, len }); + AcirValue::DynamicArray(AcirDynamicArray { block_id: result_block_id, len: flat_array_size }); self.define_result(dfg, instruction, result_value); Ok(()) } + fn array_set_value( + &mut self, + value: AcirValue, + block_id: BlockId, + var_index: &mut AcirVar, + first_elem: &mut bool, + ) -> Result<(), RuntimeError> { + let one = self.acir_context.add_constant(FieldElement::one()); + match value { + AcirValue::Var(store_var, _) => { + if !*first_elem { + *var_index = self.acir_context.add_var(*var_index, one)?; + } + *first_elem = false; + // Write the new value into the new array at the specified index + self.acir_context.write_to_memory(block_id, &var_index, &store_var)?; + } + AcirValue::Array(values) => { + for value in values { + self.array_set_value(value, block_id, var_index, first_elem)?; + } + } + AcirValue::DynamicArray(_) => { + panic!("ahhh got dyn array for set") + } + } + Ok(()) + } + /// Initializes an array with the given values and caches the fact that we /// have initialized this array. fn initialize_array( diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index ae24db611c7..33692ecbd23 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -397,7 +397,7 @@ impl Instruction { let index = index.try_to_u64().expect("Expected array index to fit in u64") as usize; dbg!(array.len()); - dbg!(array.clone()); + // dbg!(array.clone()); dbg!(index); if index < array.len() { return SimplifiedTo(array[index]); diff --git a/compiler/noirc_evaluator/src/ssa/ir/types.rs b/compiler/noirc_evaluator/src/ssa/ir/types.rs index e8110f0901b..a6e7785c07f 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/types.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/types.rs @@ -77,6 +77,25 @@ impl Type { other => panic!("element_size: Expected array or slice, found {other}"), } } + + pub(crate) fn flattened_element_size(&self) -> usize { + let mut size = 0; + match self { + Type::Array(elements, len) => { + // array.into_iter().flat_map(AcirValue::flatten).collect(), + // dbg!(elements.clone()); + for elem in elements.as_ref() { + size += elem.flattened_element_size() * len; + } + // let element_size = elements.len(); + // let x = elements.into_iter().flat_map(|elem| elem.flattened_element_size()).collect(); + } + _ => { + size += 1 + } + } + size + } } /// Composite Types are essentially flattened struct or tuple types. diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index d57c2cc7933..8c8b0bbd9f0 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -551,9 +551,13 @@ impl<'f> Context<'f> { _ => panic!("Expected array type"), }; + // dbg!(len); + dbg!(then_value); + dbg!(else_value); 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: FieldElement = ((i * element_types.len() + element_index) as u128).into(); + // dbg!(index.clone()); let index = self.inserter.function.dfg.make_constant(index, Type::field()); let typevars = Some(vec![element_type.clone()]); diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 652869bdc9d..952cec6f8b9 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -298,6 +298,7 @@ impl<'a> FunctionContext<'a> { ) -> Values { // base_index = index * type_size let type_size = Self::convert_type(element_type).size_of_type(); + dbg!(type_size); let type_size = self.builder.field_constant(type_size as u128); let base_index = self.builder.set_location(location).insert_binary(index, BinaryOp::Mul, type_size); diff --git a/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml b/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml index 57603c24863..849e32631c9 100644 --- a/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml +++ b/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml @@ -2,28 +2,28 @@ y = "3" [[x]] a = "1" -b = ["2", "3"] +b = ["2", "3", "20"] + +[x.bar] +inner = ["100", "101", "102"] [[x]] -a = "4" -b = ["5", "6"] +a = "4" # idx = 3, true start idx = 7 +b = ["5", "6", "21"] # idx = 4, start idx = 8 + +[x.bar] +inner = ["103", "104", "105"] # idx = 5, idx = 11 [[x]] a = "7" -b = ["8", "9"] +b = ["8", "9", "22"] + +[x.bar] +inner = ["106", "107", "108"] [[x]] a = "10" -b = ["11", "12"] - -# [[x]] -# a = "1" -# b = "2" - -# [[x]] -# a = "3" -# b = "4" +b = ["11", "12", "23"] -# [[x]] -# a = "5" -# b = "6" +[x.bar] +inner = ["109", "110", "111"] 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 1fbaef8b1eb..08e0977be88 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 @@ -1,37 +1,63 @@ +struct Bar { + inner: [Field; 3], +} struct Foo { a: Field, - b: [Field; 2], + b: [Field; 3], + bar: Bar, } -fn main(mut x : [Foo; 4], y : pub Field) { - dep::std::println(x[y - 3]); - dep::std::println(x[y - 2]); - dep::std::println(x[y - 1]); - dep::std::println(x[y]); - - assert(x[y - 3].a == 1); - assert(x[y - 3].b == [2, 3]); - assert(x[y - 2].a == 4); - assert(x[y - 2].b == [5, 6]); - assert(x[y - 1].a == 7); - assert(x[y - 1].b == [8, 9]); - assert(x[y].a == 10); - assert(x[y].b == [11, 12]); +// fn main(mut x : [Foo; 4], y : pub Field) { +// dep::std::println(x[3].bar.inner); +// } +fn main(mut x : [Foo; 4], y : pub Field) { + // assert(x[y - 3].a == 1); + dep::std::println(x[y - 3].bar.inner); + dep::std::println(x[y - 2].bar.inner); + dep::std::println(x[y - 1].bar.inner); + // assert(x[y - 3].b == [2, 3]); + // assert(x[y - 2].a == 4); + // assert(x[y - 2].b == [5, 6]); + // assert(x[y - 1].a == 7); + // assert(x[y - 1].b == [8, 9]); + // assert(x[y].a == 10); + // assert(x[y].b == [11, 12]); + // dep::std::println(x[y].bar.inner); + dep::std::println(x[y].bar.inner); + assert(x[y].bar.inner == [109, 110, 111]); + dep::std::println("show list"); + for i in 0..4 { + dep::std::println(x[i]); + } // if y != 2 { // x[y].a = 50; // } else { // x[y].a = 100; // } + // dep::std::println("new list"); + // for i in 0..4 { + // dep::std::println(x[i]); + // } + // x[y].a = 50; + // dep::std::println("new list"); + // for i in 0..4 { + // dep::std::println(x[i]); + // } // assert(x[y].a == 50); - // dep::std::println(x[y].a); + // if y == 2 { + // x[y - 1].b = [50, 51]; + // } else { + // x[y - 1].b = [100, 101]; + // } + // x[y - 1].b = [100, 101]; + // dep::std::println("new list"); + // for i in 0..4 { + // dep::std::println(x[i]); + // } + // dep::std::println(x[2].b); + // assert(x[2].b == [100, 101]); } -// fn main(mut x : [Foo; 4], y : pub Field) { -// dep::std::println(x[0]); -// dep::std::println(x[1]); -// dep::std::println(x[2]); -// dep::std::println(x[3]); -// } From e2eff9b0a69ad88ee42e6d59b78383b9a5a8bc8f Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 19 Sep 2023 03:40:43 +0000 Subject: [PATCH 05/14] working non-homogenous arrays using internal type element size array, heavy cleanup still needed --- Cargo.toml | 3 + .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 170 +++++++++++++----- .../nested_array_dynamic/src/main.nr | 64 +++---- 3 files changed, 146 insertions(+), 91 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index dac1c15e0a5..9bfadbded16 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -68,3 +68,6 @@ url = "2.2.0" wasm-bindgen = { version = "=0.2.86", features = ["serde-serialize"] } wasm-bindgen-test = "0.3.33" base64 = "0.21.2" + +# [patch.crates-io] +# acvm = { path = "/mnt/user-data/maxim/acvm/acvm" } diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index b70dee38910..ac455151d48 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -304,6 +304,19 @@ impl Context { block_id } + /// Get the next BlockId for internal memory + /// used during ACIR generation. + /// This is useful for referencing information that can + /// only be computed dynamically, such as the type structure + /// of non-homogenous arrays. + fn internal_block_id(&mut self) -> BlockId { + let block_id = BlockId(self.max_block_id); + self.max_block_id += 1; + // We do not insert into `self.memory_blocks` here as this BlockId + // does not have a corresponding ValueId + block_id + } + /// Creates an `AcirVar` corresponding to a parameter witness to appears in the abi. A range /// constraint is added if the numeric type requires it. /// @@ -547,7 +560,7 @@ impl Context { let (array, index, store_value) = match dfg[instruction] { Instruction::ArrayGet { array, index } => (array, index, None), Instruction::ArraySet { array, index, value, .. } => { - dbg!(&dfg[value]); + // dbg!(&dfg[value]); (array, index, Some(value)) } _ => { @@ -632,6 +645,8 @@ impl Context { } // let (new_index, new_value) = self.convert_array_operation_inputs(instruction, dfg, array, index, store_value)?; + // let new_value = new_value.map(|var| AcirValue::Var(var, AcirType::field())); + let (new_index, new_value) = self.convert_array_operation_inputs_new(instruction, dfg, array, index, store_value)?; let resolved_array = dfg.resolve(array); @@ -716,20 +731,22 @@ impl Context { // dbg!("about to convert_value: "); // dbg!(store_value.clone()); let new_value = if let Some(store) = store_value { - dbg!(&dfg[store]); + // dbg!(&dfg[store]); let store_value = self.convert_value(store, dfg); - dbg!(store_value.clone()); + // dbg!(store_value.clone()); // let store_value = self.convert_array_set_store_value(&store_value, dummy)?; if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) { - dbg!("got here"); + // dbg!("got here"); Some(store_value) } else { - dbg!("using dummy"); - let dummy = self.array_get(instruction, array, predicate_index, dfg)?; + // dbg!("using dummy"); + // TODO: accurately setup the dummy value + let one = self.acir_context.add_constant(FieldElement::one()); + // let dummy = self.array_get(instruction, array, predicate_index, dfg)?; // TODO: fix this it is a placeholder and wrong!!! // Some(self.convert_value(store, dfg)) - Some(self.convert_array_set_store_value(&store_value, dummy)?) + Some(self.convert_array_set_store_value(&store_value, one)?) // Some(store_value) } } else { @@ -796,13 +813,14 @@ impl Context { // dbg!(results); // dbg!(dfg.resolve(results[0])); let res_typ = dfg.type_of_value(results[0]); - dbg!(res_typ.clone()); - dbg!(block_id.0); + // dbg!(res_typ.clone()); + // dbg!(block_id.0); if !self.initialized_arrays.contains(&block_id) { match &dfg[array] { Value::Array { array, .. } => { let values: Vector = array.iter().map(|i| self.convert_value(*i, dfg)).collect(); + // TODO: this is not the correct len self.initialize_array_new(block_id, values.len(), Some(AcirValue::Array(values)))?; // self.initialize_array(block_id, array.len(), Some(&values))?; } @@ -815,40 +833,78 @@ impl Context { } } } - + // let x = + + let element_type_sizes = self.internal_block_id(); + // dbg!(element_type_sizes.0); + // TODO: doing this here but perhaps it should be done on array initialization automatically + match &dfg[array] { + Value::Array { typ, .. } | Value::Param { typ, .. } + | Value::Instruction { typ, .. } => { + let mut flat_elem_type_sizes = Vec::new(); + flat_elem_type_sizes.push(0); + match typ { + Type::Array(element_types, _) => { + for (i, typ) in element_types.as_ref().iter().enumerate() { + let mut flat_value_size = typ.flattened_element_size(); + flat_value_size += flat_elem_type_sizes[i]; + flat_elem_type_sizes.push(flat_value_size); + } + } + _ => panic!("ahhhh should only have an array"), + } + flat_elem_type_sizes.pop(); + // flat_elem_type_sizes + // dbg!(flat_elem_type_sizes.clone()); + 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()) + }); + self.initialize_array_new(element_type_sizes, init_values.len(), Some(AcirValue::Array(init_values.into())))?; + } + _ => { + dbg!(&dfg[array]); + return Err(RuntimeError::UnInitialized { + name: "array".to_string(), + call_stack: self.acir_context.get_call_stack(), + }); + } + } + // let res_type_size = res_typ.element_size(); // dbg!(res_type_size); let res_type_flat_size = res_typ.flattened_element_size(); - dbg!(res_type_flat_size); + // dbg!(res_type_flat_size); let array_typ = dfg.type_of_value(array); // dbg!(array_typ.clone()); let element_size = array_typ.element_size(); - dbg!(element_size); + // dbg!(element_size); let flat_array_size = array_typ.flattened_element_size(); - dbg!(flat_array_size); + // dbg!(flat_array_size); let array_len = dfg.try_get_array_length(array).expect("ICE: need to have an array, slices not implemented"); let flat_elem_size = flat_array_size / array_len; let flat_element_size_var = self.acir_context.add_constant(FieldElement::from(flat_elem_size as u128)); let element_size_var = self.acir_context.add_constant(FieldElement::from(element_size as u128)); - // TODO: switch to use euclidean_division_var 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 = self.acir_context.modulo_var(var_index, element_size_var, 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 var_index = self.acir_context.mul_var(outer_offset, flat_element_size_var)?; - let var_index = self.acir_context.add_var(var_index, inner_offset)?; + let mut var_index = self.acir_context.add_var(var_index, inner_offset)?; // Work with index - let one = self.acir_context.add_constant(FieldElement::one()); - let pred = self.acir_context.less_than_var(one, inner_offset, 32, self.current_side_effects_enabled_var)?; - let not_pred = self.acir_context.sub_var(one, pred)?; - let var_index_plus_one = self.acir_context.add_var(var_index, one)?; - let true_pred = self.acir_context.mul_var(pred, var_index_plus_one)?; - let false_pred = self.acir_context.mul_var(not_pred, var_index)?; - let var_index = self.acir_context.add_var(true_pred, false_pred)?; - // Had to add one here once we added a value to preceding element type in program - // TODO: make this general as this was simply for testing - let mut var_index = self.acir_context.add_var(var_index, one)?; + // let one = self.acir_context.add_constant(FieldElement::one()); + // let pred = self.acir_context.less_than_var(one, inner_offset, 32, self.current_side_effects_enabled_var)?; + // let not_pred = self.acir_context.sub_var(one, pred)?; + // let var_index_plus_one = self.acir_context.add_var(var_index, one)?; + // let true_pred = self.acir_context.mul_var(pred, var_index_plus_one)?; + // let false_pred = self.acir_context.mul_var(not_pred, var_index)?; + // let var_index = self.acir_context.add_var(true_pred, false_pred)?; + // // Had to add one here once we added a value to preceding element type in program + // // TODO: make this general as this was simply for testing + // let mut var_index = self.acir_context.add_var(var_index, one)?; let mut is_first_elem = true; let mut values = Vector::new(); @@ -878,7 +934,7 @@ impl Context { let one = self.acir_context.add_constant(FieldElement::one()); match ssa_type.clone() { Type::Numeric(numeric_type) => { - dbg!(*first_elem); + // dbg!(*first_elem); if !*first_elem { *var_index = self.acir_context.add_var(*var_index, one)?; } @@ -891,9 +947,9 @@ impl Context { } Type::Array(element_types, len) => { let mut inner_vec = Vector::new(); - dbg!(len); + // dbg!(len); for i in 0..len { - dbg!(i); + // dbg!(i); for typ in element_types.as_ref() { self.array_get_type(dfg, typ, var_index, &mut inner_vec, block_id, first_elem)?; } @@ -932,7 +988,6 @@ impl Context { map_array: bool, ) -> Result<(), RuntimeError> { dbg!("INSIDE ARRAY_SET"); - // let x = self.acir_context. // Pass the instruction between array methods rather than the internal fields themselves let (array, length) = match dfg[instruction] { Instruction::ArraySet { array, length, .. } => (array, length), @@ -947,7 +1002,7 @@ impl Context { // Fetch the internal SSA ID for the array let array_id = dfg.resolve(array); - dbg!(array_id); + // dbg!(array_id); // Use the SSA ID to get or create its block ID let block_id = self.block_id(&array_id); @@ -1001,6 +1056,39 @@ impl Context { } } + let element_type_sizes = self.internal_block_id(); + match &dfg[array_id] { + Value::Array { typ, .. } | Value::Param { typ, .. } + | Value::Instruction { typ, .. } => { + // dbg!(typ.clone()); + let mut flat_elem_type_sizes = Vec::new(); + flat_elem_type_sizes.push(0); + match typ { + Type::Array(element_types, _) => { + for (i, typ) in element_types.as_ref().iter().enumerate() { + let mut flat_value_size = typ.flattened_element_size(); + flat_value_size += flat_elem_type_sizes[i]; + flat_elem_type_sizes.push(flat_value_size); + } + } + _ => panic!("ahhhh should only have an array"), + } + flat_elem_type_sizes.pop(); + 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()) + }); + self.initialize_array_new(element_type_sizes, init_values.len(), Some(AcirValue::Array(init_values.into())))?; + } + _ => { + dbg!(&dfg[array]); + return Err(RuntimeError::UnInitialized { + name: "array".to_string(), + call_stack: self.acir_context.get_call_stack(), + }); + } + } + // Since array_set creates a new array, we create a new block ID for this // array, unless map_array is true. In that case, we operate directly on block_id // and we do not create a new block ID. @@ -1034,40 +1122,28 @@ impl Context { let read = self.acir_context.read_from_memory(block_id, &var)?; Ok::(AcirValue::Var(read, AcirType::NumericType(NumericType::NativeField))) })?; - dbg!(len); self.initialize_array_new(result_block_id, flat_elem_size, Some(AcirValue::Array(init_values.into())))?; - // self.initialize_array(result_block_id, len, Some(&init_values))?; } - // let index_const = self.acir_context.get_constant(&var_index); - // dbg!(index_const); - let element_size = array_typ.element_size(); // dbg!(element_size); let flat_array_size = array_typ.flattened_element_size(); // dbg!(flat_array_size); let array_len = dfg.try_get_array_length(array).expect("ICE: need to have an array, slices not implemented"); - // dbg!(array_len); let flat_elem_size = flat_array_size / array_len; let flat_element_size_var = self.acir_context.add_constant(FieldElement::from(flat_elem_size as u128)); let element_size_var = self.acir_context.add_constant(FieldElement::from(element_size as u128)); - // TODO: switch to use euclidean_division-var 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 = self.acir_context.modulo_var(var_index, element_size_var, 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 var_index = self.acir_context.mul_var(outer_offset, flat_element_size_var)?; - // let one = self.acir_context.add_constant(FieldElement::one()); - // let var_index = self.acir_context.add_var(var_index, one)?; let mut var_index = self.acir_context.add_var(var_index, inner_offset)?; - let index_const = self.acir_context.get_constant(&var_index); - dbg!(index_const); let mut is_first_elem = true; - dbg!(result_block_id.0); - dbg!(store_value.clone()); self.array_set_value(store_value, result_block_id, &mut var_index, &mut is_first_elem)?; - dbg!(len); - dbg!(result_block_id.0); + let result_value = AcirValue::DynamicArray(AcirDynamicArray { block_id: result_block_id, len: flat_array_size }); self.define_result(dfg, instruction, result_value); 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 08e0977be88..93bfecded26 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 @@ -8,56 +8,32 @@ struct Foo { bar: Bar, } -// fn main(mut x : [Foo; 4], y : pub Field) { -// dep::std::println(x[3].bar.inner); -// } - fn main(mut x : [Foo; 4], y : pub Field) { - // assert(x[y - 3].a == 1); - dep::std::println(x[y - 3].bar.inner); - dep::std::println(x[y - 2].bar.inner); - dep::std::println(x[y - 1].bar.inner); - // assert(x[y - 3].b == [2, 3]); - // assert(x[y - 2].a == 4); - // assert(x[y - 2].b == [5, 6]); - // assert(x[y - 1].a == 7); - // assert(x[y - 1].b == [8, 9]); - // assert(x[y].a == 10); - // assert(x[y].b == [11, 12]); - // dep::std::println(x[y].bar.inner); - dep::std::println(x[y].bar.inner); + assert(x[y - 3].a == 1); + assert(x[y - 3].b == [2, 3, 20]); + assert(x[y - 2].a == 4); + assert(x[y - 2].b == [5, 6, 21]); + assert(x[y - 1].a == 7); + assert(x[y - 1].b == [8, 9, 22]); + assert(x[y].a == 10); + assert(x[y].b == [11, 12, 23]); assert(x[y].bar.inner == [109, 110, 111]); dep::std::println("show list"); for i in 0..4 { dep::std::println(x[i]); } - // if y != 2 { - // x[y].a = 50; - // } else { - // x[y].a = 100; - // } - // dep::std::println("new list"); - // for i in 0..4 { - // dep::std::println(x[i]); - // } - // x[y].a = 50; - // dep::std::println("new list"); - // for i in 0..4 { - // dep::std::println(x[i]); - // } - // assert(x[y].a == 50); + if y != 2 { + x[y].a = 50; + } else { + x[y].a = 100; + } + assert(x[y].a == 50); - // if y == 2 { - // x[y - 1].b = [50, 51]; - // } else { - // x[y - 1].b = [100, 101]; - // } - // x[y - 1].b = [100, 101]; - // dep::std::println("new list"); - // for i in 0..4 { - // dep::std::println(x[i]); - // } - // dep::std::println(x[2].b); - // assert(x[2].b == [100, 101]); + if y == 2 { + x[y - 1].b = [50, 51, 52]; + } else { + x[y - 1].b = [100, 101, 102]; + } + assert(x[2].b == [100, 101, 102]); } From 219c8f2d7343d4960ac76672ddf16b5dc83d4963 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 19 Sep 2023 15:33:57 +0000 Subject: [PATCH 06/14] cleanup lots of debug and move const index access into its own method --- Cargo.toml | 3 - .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 66 +-- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 430 ++++++++---------- .../noirc_evaluator/src/ssa/ir/instruction.rs | 3 - compiler/noirc_evaluator/src/ssa/ir/types.rs | 16 +- .../src/ssa/opt/flatten_cfg.rs | 7 +- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 1 - .../nested_array_dynamic/src/main.nr | 11 +- 8 files changed, 200 insertions(+), 337 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9bfadbded16..dac1c15e0a5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -68,6 +68,3 @@ url = "2.2.0" wasm-bindgen = { version = "=0.2.86", features = ["serde-serialize"] } wasm-bindgen-test = "0.3.33" base64 = "0.21.2" - -# [patch.crates-io] -# acvm = { path = "/mnt/user-data/maxim/acvm/acvm" } diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index be61dcaf022..b5688763dae 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -61,7 +61,7 @@ impl AcirType { AcirType::NumericType(NumericType::NativeField) } - /// Returns an unsigned type of the specified bit size + /// Returns an unsigned type of the specified bit size pub(crate) fn unsigned(bit_size: u32) -> Self { AcirType::NumericType(NumericType::Unsigned { bit_size }) } @@ -79,16 +79,6 @@ impl AcirType { }; matches!(numeric_type, NumericType::Signed { .. }) } - - pub(crate) fn from_slice(value: &SsaType, size: usize) -> Self { - match value { - SsaType::Slice(elements) => { - let elements = elements.iter().map(|e| e.into()).collect(); - AcirType::Array(elements, size) - } - _ => unreachable!("Attempted to use {value} where a slice was expected"), - } - } } impl From for AcirType { @@ -273,13 +263,6 @@ impl AcirContext { } } - pub(crate) fn get_constant(&self, var: &AcirVar) -> Option { - match self.vars[var] { - AcirVarData::Const(field) => Some(field), - _ => None, - } - } - /// Adds a new Variable to context whose value will /// be constrained to be the negation of `var`. /// @@ -1222,47 +1205,6 @@ impl AcirContext { /// Initializes an array in memory with the given values `optional_values`. /// If `optional_values` is empty, then the array is initialized with zeros. pub(crate) fn initialize_array( - &mut self, - block_id: BlockId, - len: usize, - optional_values: Option<&[AcirValue]>, - ) -> Result<(), InternalError> { - // If the optional values are supplied, then we fill the initialized - // array with those values. If not, then we fill it with zeros. - let mut nested = false; - let initialized_values = match optional_values { - None => { - let zero = self.add_constant(FieldElement::zero()); - let zero_witness = self.var_to_witness(zero)?; - vec![zero_witness; len] - } - Some(optional_values) => { - let mut values = Vec::new(); - for value in optional_values { - if let Ok(some_value) = value.clone().into_var() { - values.push(self.var_to_witness(some_value)?); - } else { - dbg!(optional_values.clone()); - dbg!("got here"); - nested = true; - break; - } - } - values - } - }; - // we do not initialize nested arrays. This means that non-const indexes are not supported for nested arrays - // if !nested { - // self.acir_ir.push_opcode(Opcode::MemoryInit { block_id, init: initialized_values }); - // } - dbg!(nested); - self.acir_ir.push_opcode(Opcode::MemoryInit { block_id, init: initialized_values }); - - - Ok(()) - } - - pub(crate) fn initialize_array_new( &mut self, block_id: BlockId, len: usize, @@ -1280,7 +1222,7 @@ impl AcirContext { values } }; - // dbg!(initialized_values.len()); + self.acir_ir.push_opcode(Opcode::MemoryInit { block_id, init: initialized_values }); Ok(()) @@ -1301,10 +1243,8 @@ impl AcirContext { } } AcirValue::DynamicArray(_) => { - panic!("dyn array should already be initialized"); + unreachable!("Dynamic array should already be initialized"); } - // TODO: I think it is correct that we should panic on dyn array - // but need to test and verify } Ok(()) } diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index ac455151d48..bea747bf8ef 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -95,13 +95,10 @@ impl AcirValue { fn into_var(self) -> Result { match self { AcirValue::Var(var, _) => Ok(var), - AcirValue::DynamicArray(_) | AcirValue::Array(_) => { - dbg!(self); - Err(InternalError::General { - message: "Called AcirValue::into_var on an array".to_string(), - call_stack: CallStack::new(), - }) - } + AcirValue::DynamicArray(_) | AcirValue::Array(_) => Err(InternalError::General { + message: "Called AcirValue::into_var on an array".to_string(), + call_stack: CallStack::new(), + }), } } @@ -247,11 +244,7 @@ impl Context { AcirValue::Var(_, _) => (), AcirValue::Array(values) => { let block_id = self.block_id(param_id); - dbg!(block_id.0); - dbg!(values.len()); - // let v = vecmap(values, |v| v.clone()); - // self.initialize_array(block_id, values.len(), Some(&v))?; - self.initialize_array_new(block_id, values.len(), Some(value.clone()))?; + self.initialize_array(block_id, typ.flattened_size(), Some(value.clone()))?; } AcirValue::DynamicArray(_) => unreachable!( "The dynamic array type is created in Acir gen and therefore cannot be a block parameter" @@ -304,8 +297,8 @@ impl Context { block_id } - /// Get the next BlockId for internal memory - /// used during ACIR generation. + /// Get the next BlockId for internal memory + /// used during ACIR generation. /// This is useful for referencing information that can /// only be computed dynamically, such as the type structure /// of non-homogenous arrays. @@ -358,8 +351,6 @@ impl Context { rhs: AcirValue, read_from_index: &mut impl FnMut(BlockId, usize) -> Result, ) -> Result, InternalError> { - // dbg!(lhs.clone()); - // dbg!(rhs.clone()); match (lhs, rhs) { (AcirValue::Var(lhs, _), AcirValue::Var(rhs, _)) => Ok(vec![(lhs, rhs)]), (AcirValue::Array(lhs_values), AcirValue::Array(rhs_values)) => { @@ -399,8 +390,6 @@ impl Context { self.acir_context.add_constant(FieldElement::from(array_index as u128)), AcirType::NumericType(NumericType::NativeField), ); - dbg!("inside read_dynamic_array_index"); - let index_var = index.into_var()?; self.acir_context.read_from_memory(block_id, &index_var) @@ -438,11 +427,11 @@ impl Context { assert_eq!(result_ids.len(), output_values.len(), "ICE: The number of Brillig output values should match the result ids in SSA"); for result in result_ids.iter().zip(output_values) { - if let AcirValue::Array(values) = &result.1 { - let block_id = self.block_id(&dfg.resolve(*result.0)); - // let values: Vec = values.iter().cloned().collect(); - // self.initialize_array(block_id, values.len(), Some(&values))?; - self.initialize_array_new(block_id, values.len(), Some(result.1.clone()))?; + if let AcirValue::Array(_) = &result.1 { + let array_id = dfg.resolve(*result.0); + let block_id = self.block_id(&array_id); + let array_typ = dfg.type_of_value(array_id); + self.initialize_array(block_id, array_typ.flattened_size(), Some(result.1.clone()))?; } self.ssa_values.insert(*result.0, result.1); } @@ -465,7 +454,11 @@ impl Context { let block_id = self.block_id(result); let values = vecmap(values, |v| v.clone()); - self.initialize_array_new(block_id, values.len(), Some(output.clone()))?; + self.initialize_array( + block_id, + values.len(), + Some(output.clone()), + )?; // self.initialize_array(block_id, values.len(), Some(&values))?; } AcirValue::DynamicArray(_) => { @@ -559,10 +552,7 @@ impl Context { // Pass the instruction between array methods rather than the internal fields themselves let (array, index, store_value) = match dfg[instruction] { Instruction::ArrayGet { array, index } => (array, index, None), - Instruction::ArraySet { array, index, value, .. } => { - // dbg!(&dfg[value]); - (array, index, Some(value)) - } + Instruction::ArraySet { array, index, value, .. } => (array, index, Some(value)), _ => { return Err(InternalError::UnExpected { expected: "Instruction should be an ArrayGet or ArraySet".to_owned(), @@ -573,9 +563,38 @@ impl Context { } }; + let accessed_constant_index = + self.handle_constant_index(instruction, dfg, index, array, store_value)?; + if accessed_constant_index { + return Ok(()); + } + + let (new_index, new_value) = + self.convert_array_operation_inputs(dfg, index, store_value)?; + + let resolved_array = dfg.resolve(array); + let map_array = last_array_uses.get(&resolved_array) == Some(&instruction); + + if let Some(new_value) = new_value { + self.array_set(instruction, new_index, new_value, dfg, map_array)?; + } else { + self.array_get(instruction, array, new_index, dfg)?; + } + + Ok(()) + } + + /// Handle constant index: if there is no predicate and we have the array values, + /// we can perform the operation directly on the array + fn handle_constant_index( + &mut self, + instruction: InstructionId, + dfg: &DataFlowGraph, + index: ValueId, + array: ValueId, + store_value: Option, + ) -> Result { let index_const = dfg.get_numeric_constant(index); - // dbg!(index_const.clone()); - // Handle constant index: if there is no predicate and we have the array values, we can perform the operation directly on the array match dfg.type_of_value(array) { Type::Array(_, _) => { match self.convert_value(array, dfg) { @@ -613,11 +632,8 @@ impl Context { call_stack, }); } else { - // dbg!("updating because index is known"); - // dbg!(index); let value = match store_value { Some(store_value) => { - // dbg!("got here"); let store_value = self.convert_value(store_value, dfg); AcirValue::Array(array.update(index, store_value)) } @@ -625,13 +641,13 @@ impl Context { }; self.define_result(dfg, instruction, value); - return Ok(()); + return Ok(true); } } // If there is a predicate and the index is not out of range, we can directly perform the read else if index < array_size && store_value.is_none() { self.define_result(dfg, instruction, array[index].clone()); - return Ok(()); + return Ok(true); } } } @@ -639,88 +655,24 @@ impl Context { } } Type::Slice(_) => { - // Do nothing we only want dynamic checks here + // Do nothing we only want dynamic checks for slices } _ => unreachable!("ICE: expected array or slice type"), } - - // let (new_index, new_value) = self.convert_array_operation_inputs(instruction, dfg, array, index, store_value)?; - // let new_value = new_value.map(|var| AcirValue::Var(var, AcirType::field())); - - let (new_index, new_value) = self.convert_array_operation_inputs_new(instruction, dfg, array, index, store_value)?; - - let resolved_array = dfg.resolve(array); - let map_array = last_array_uses.get(&resolved_array) == Some(&instruction); - - if let Some(new_value) = new_value { - self.array_set(instruction, new_index, new_value, dfg, map_array)?; - } else { - self.array_get(instruction, array, new_index, dfg)?; - } - - Ok(()) + Ok(false) } - /// We need to properly setup the inputs for array operations in ACIR. + /// We need to properly setup the inputs for array operations in ACIR. /// From the original SSA values we compute the following AcirVars: /// - index_var is the index of the array /// - predicate_index is 0, or the index if the predicate is true /// - new_value is the optional value when the operation is an array_set - /// When there is a predicate, it is predicate*value + (1-predicate)*dummy, where dummy is the value of the array at the requested index. - /// It is a dummy value because in the case of a false predicate, the value stored at the requested index will be itself. + /// When there is a predicate, it is predicate*value + (1-predicate)*dummy, where dummy is zero. + /// It is a dummy value because in the case of a false predicate, the value stored at the requested index is zeroed out + /// and will never be used elsewhere in the program. fn convert_array_operation_inputs( &mut self, - instruction: InstructionId, - dfg: &DataFlowGraph, - array: ValueId, - index: ValueId, - store_value: Option, - ) -> Result<(AcirVar, Option), RuntimeError> { - let index_var = self.convert_numeric_value(index, dfg)?; - let predicate_index = - self.acir_context.mul_var(index_var, self.current_side_effects_enabled_var)?; - let new_value = if let Some(store) = store_value { - dbg!("about to get store var"); - println!("store value id: {store}"); - let resolved_store = dfg.resolve(store); - println!("resolved store: {resolved_store}"); - let acir_value = self.convert_value(store, dfg); - let store_var = Some(acir_value.into_var()?); - dbg!("got store var"); - if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) { - store_var - } else { - dbg!("need to check predicate"); - let dummy = self.array_get(instruction, array, predicate_index, dfg)?; - let true_pred = self - .acir_context - .mul_var(store_var.unwrap(), self.current_side_effects_enabled_var)?; - let one = self.acir_context.add_constant(FieldElement::one()); - let not_pred = - self.acir_context.sub_var(one, self.current_side_effects_enabled_var)?; - let false_pred = self.acir_context.mul_var(not_pred, dummy)?; - // predicate*value + (1-predicate)*dummy - Some(self.acir_context.add_var(true_pred, false_pred)?) - } - } else { - None - }; - - let new_index = if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) - { - index_var - } else { - predicate_index - }; - - return Ok((new_index, new_value)) - } - - fn convert_array_operation_inputs_new( - &mut self, - instruction: InstructionId, dfg: &DataFlowGraph, - array: ValueId, index: ValueId, store_value: Option, ) -> Result<(AcirVar, Option), RuntimeError> { @@ -728,26 +680,15 @@ impl Context { let predicate_index = self.acir_context.mul_var(index_var, self.current_side_effects_enabled_var)?; - // dbg!("about to convert_value: "); - // dbg!(store_value.clone()); let new_value = if let Some(store) = store_value { - // dbg!(&dfg[store]); let store_value = self.convert_value(store, dfg); - // dbg!(store_value.clone()); - // let store_value = self.convert_array_set_store_value(&store_value, dummy)?; if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) { - // dbg!("got here"); Some(store_value) } else { - // dbg!("using dummy"); - // TODO: accurately setup the dummy value - let one = self.acir_context.add_constant(FieldElement::one()); - // let dummy = self.array_get(instruction, array, predicate_index, dfg)?; - - // TODO: fix this it is a placeholder and wrong!!! - // Some(self.convert_value(store, dfg)) - Some(self.convert_array_set_store_value(&store_value, one)?) - // Some(store_value) + // We use zero as the dummy value. As the dummy value will never be accessed + // it is ok to zero out the value at the requested index + let zero = self.acir_context.add_constant(FieldElement::zero()); + Some(self.convert_array_set_store_value(&store_value, zero)?) } } else { None @@ -759,27 +700,24 @@ impl Context { } else { predicate_index }; - // dbg!(new_value.clone()); + Ok((new_index, new_value)) } fn convert_array_set_store_value( &mut self, store_value: &AcirValue, - // TODO: probably want to change this dummy to be in here dummy: AcirVar, ) -> Result { match store_value { AcirValue::Var(store_var, _) => { - // let dummy = self.array_get(instruction, array, predicate_index, dfg)?; - let true_pred = self - .acir_context - .mul_var(*store_var, self.current_side_effects_enabled_var)?; + let true_pred = + self.acir_context.mul_var(*store_var, self.current_side_effects_enabled_var)?; let one = self.acir_context.add_constant(FieldElement::one()); let not_pred = self.acir_context.sub_var(one, self.current_side_effects_enabled_var)?; let false_pred = self.acir_context.mul_var(not_pred, dummy)?; - // predicate*value + (1-predicate)*dummy + // predicate*value + (1-predicate)*dummy let new_value = self.acir_context.add_var(true_pred, false_pred)?; Ok(AcirValue::Var(new_value, AcirType::field())) } @@ -807,25 +745,21 @@ impl Context { dfg: &DataFlowGraph, ) -> Result { let array = dfg.resolve(array); - // dbg!(array); + + let array_typ = dfg.type_of_value(array); let block_id = self.block_id(&array); - let results = dfg.instruction_results(instruction); - // dbg!(results); - // dbg!(dfg.resolve(results[0])); - let res_typ = dfg.type_of_value(results[0]); - // dbg!(res_typ.clone()); - // dbg!(block_id.0); if !self.initialized_arrays.contains(&block_id) { match &dfg[array] { Value::Array { array, .. } => { let values: Vector = array.iter().map(|i| self.convert_value(*i, dfg)).collect(); - // TODO: this is not the correct len - self.initialize_array_new(block_id, values.len(), Some(AcirValue::Array(values)))?; - // self.initialize_array(block_id, array.len(), Some(&values))?; + self.initialize_array( + block_id, + array_typ.flattened_size(), + Some(AcirValue::Array(values)), + )?; } _ => { - dbg!(&dfg[array]); return Err(RuntimeError::UnInitialized { name: "array".to_string(), call_stack: self.acir_context.get_call_stack(), @@ -833,34 +767,36 @@ impl Context { } } } - // let x = let element_type_sizes = self.internal_block_id(); - // dbg!(element_type_sizes.0); // TODO: doing this here but perhaps it should be done on array initialization automatically match &dfg[array] { - Value::Array { typ, .. } | Value::Param { typ, .. } + Value::Array { typ, .. } + | Value::Param { typ, .. } | Value::Instruction { typ, .. } => { let mut flat_elem_type_sizes = Vec::new(); flat_elem_type_sizes.push(0); match typ { Type::Array(element_types, _) => { for (i, typ) in element_types.as_ref().iter().enumerate() { - let mut flat_value_size = typ.flattened_element_size(); + let mut flat_value_size = typ.flattened_size(); flat_value_size += flat_elem_type_sizes[i]; flat_elem_type_sizes.push(flat_value_size); } } _ => panic!("ahhhh should only have an array"), } + // We do not have to initialize the last elem size value as that is the maximum array size flat_elem_type_sizes.pop(); - // flat_elem_type_sizes - // dbg!(flat_elem_type_sizes.clone()); 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()) }); - self.initialize_array_new(element_type_sizes, init_values.len(), Some(AcirValue::Array(init_values.into())))?; + self.initialize_array( + element_type_sizes, + init_values.len(), + Some(AcirValue::Array(init_values.into())), + )?; } _ => { dbg!(&dfg[array]); @@ -870,62 +806,63 @@ impl Context { }); } } - - // let res_type_size = res_typ.element_size(); - // dbg!(res_type_size); - let res_type_flat_size = res_typ.flattened_element_size(); - // dbg!(res_type_flat_size); - let array_typ = dfg.type_of_value(array); - // dbg!(array_typ.clone()); let element_size = array_typ.element_size(); - // dbg!(element_size); - let flat_array_size = array_typ.flattened_element_size(); - // dbg!(flat_array_size); - let array_len = dfg.try_get_array_length(array).expect("ICE: need to have an array, slices not implemented"); + let flat_array_size = array_typ.flattened_size(); + let array_len = dfg + .try_get_array_length(array) + .expect("ICE: need to have an array, slices not implemented"); let flat_elem_size = flat_array_size / array_len; - let flat_element_size_var = self.acir_context.add_constant(FieldElement::from(flat_elem_size as u128)); - - 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 flat_element_size_var = + self.acir_context.add_constant(FieldElement::from(flat_elem_size as u128)); + + 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 var_index = self.acir_context.mul_var(outer_offset, flat_element_size_var)?; let mut var_index = self.acir_context.add_var(var_index, inner_offset)?; - // Work with index - // let one = self.acir_context.add_constant(FieldElement::one()); - // let pred = self.acir_context.less_than_var(one, inner_offset, 32, self.current_side_effects_enabled_var)?; - // let not_pred = self.acir_context.sub_var(one, pred)?; - // let var_index_plus_one = self.acir_context.add_var(var_index, one)?; - // let true_pred = self.acir_context.mul_var(pred, var_index_plus_one)?; - // let false_pred = self.acir_context.mul_var(not_pred, var_index)?; - // let var_index = self.acir_context.add_var(true_pred, false_pred)?; - // // Had to add one here once we added a value to preceding element type in program - // // TODO: make this general as this was simply for testing - // let mut var_index = self.acir_context.add_var(var_index, one)?; + let results = dfg.instruction_results(instruction); + let res_typ = dfg.type_of_value(results[0]); let mut is_first_elem = true; let mut values = Vector::new(); - self.array_get_type(dfg, &res_typ, &mut var_index, &mut values, block_id, &mut is_first_elem)?; - // dbg!(values.clone()); + self.array_get_type( + dfg, + &res_typ, + &mut var_index, + &mut values, + block_id, + &mut is_first_elem, + )?; assert_eq!(values.len(), 1); - // dbg!(values.len()); self.define_result(dfg, instruction, values[0].clone()); // TODO: update array_get to return an AcirValue + // or remove that array_get returns anything let read = self.acir_context.read_from_memory(block_id, &var_index)?; Ok(read) } - // TOOD: switch this to use create_value_from_type fn array_get_type( &mut self, dfg: &DataFlowGraph, ssa_type: &Type, - // acir_types: &mut Vec, var_index: &mut AcirVar, values: &mut Vector, block_id: BlockId, @@ -934,7 +871,6 @@ impl Context { let one = self.acir_context.add_constant(FieldElement::one()); match ssa_type.clone() { Type::Numeric(numeric_type) => { - // dbg!(*first_elem); if !*first_elem { *var_index = self.acir_context.add_var(*var_index, one)?; } @@ -947,29 +883,24 @@ impl Context { } Type::Array(element_types, len) => { let mut inner_vec = Vector::new(); - // dbg!(len); - for i in 0..len { - // dbg!(i); + for _ in 0..len { for typ in element_types.as_ref() { - self.array_get_type(dfg, typ, var_index, &mut inner_vec, block_id, first_elem)?; + self.array_get_type( + dfg, + typ, + var_index, + &mut inner_vec, + block_id, + first_elem, + )?; } } let array_value = AcirValue::Array(inner_vec); values.push_back(array_value); } - Type::Slice(element_types) => { - dbg!(element_types.len()); - dbg!("got here?"); - panic!("ICE: Non-const indices for nested arrays of slices is not allowed"); + Type::Slice(_) => { // TODO: need values here to fetch the len like for a Type::Array, not obvious how to incorporate it yet - // let mut inner_vec = Vector::new(); - // for _ in 0..len { - // for typ in element_types.as_ref() { - // self.array_get_type(dfg, typ, var_index, &mut inner_vec, block_id, first_elem)?; - // } - // } - // let array_value = AcirValue::Array(inner_vec); - // values.push_back(array_value); + unimplemented!("ICE: Non-const indices for nested arrays of slices is not allowed"); } _ => unreachable!("ICE - expected an array or slice"), }; @@ -987,7 +918,6 @@ impl Context { dfg: &DataFlowGraph, map_array: bool, ) -> Result<(), RuntimeError> { - dbg!("INSIDE ARRAY_SET"); // Pass the instruction between array methods rather than the internal fields themselves let (array, length) = match dfg[instruction] { Instruction::ArraySet { array, length, .. } => (array, length), @@ -996,13 +926,13 @@ impl Context { expected: "Instruction should be an ArraySet".to_owned(), found: format!("Instead got {:?}", dfg[instruction]), call_stack: self.acir_context.get_call_stack(), - }.into()) + } + .into()) } }; // Fetch the internal SSA ID for the array let array_id = dfg.resolve(array); - // dbg!(array_id); // Use the SSA ID to get or create its block ID let block_id = self.block_id(&array_id); @@ -1018,7 +948,7 @@ impl Context { let len = match &array_typ { Type::Array(typ, len) => { // Flatten the array length to handle arrays of complex types - len * typ.len() + *len } Type::Slice(typ) => { // Fetch the true length of the slice from the array_set instruction @@ -1028,7 +958,7 @@ impl Context { let len = self.acir_context.var_to_expression(length_acir_var)?.to_const(); let len = len .expect("ICE: slice length should be fully tracked and constant by ACIR gen"); - len.to_u128() as usize * typ.len() + len.to_u128() as usize } _ => unreachable!("ICE - expected an array"), }; @@ -1036,37 +966,34 @@ impl Context { // Check if the array has already been initialized in ACIR gen // if not, we initialize it using the values from SSA let already_initialized = self.initialized_arrays.contains(&block_id); - dbg!(already_initialized); if !already_initialized { let value = &dfg[array_id]; match value { - Value::Array { array, .. } => { - // let values: Vector = - // array.iter().map(|i| self.convert_value(*i, dfg)).collect(); + Value::Array { .. } => { let value = self.convert_value(array_id, dfg); - self.initialize_array_new(block_id, array.len(), Some(value))?; - // self.initialize_array(block_id, array.len(), Some(&values))?; + self.initialize_array(block_id, array_typ.flattened_size(), Some(value))?; } _ => { return Err(InternalError::General { message: format!("Array {array} should be initialized"), call_stack: self.acir_context.get_call_stack(), - }.into()) + } + .into()) } } } let element_type_sizes = self.internal_block_id(); match &dfg[array_id] { - Value::Array { typ, .. } | Value::Param { typ, .. } + Value::Array { typ, .. } + | Value::Param { typ, .. } | Value::Instruction { typ, .. } => { - // dbg!(typ.clone()); let mut flat_elem_type_sizes = Vec::new(); flat_elem_type_sizes.push(0); match typ { Type::Array(element_types, _) => { for (i, typ) in element_types.as_ref().iter().enumerate() { - let mut flat_value_size = typ.flattened_element_size(); + let mut flat_value_size = typ.flattened_size(); flat_value_size += flat_elem_type_sizes[i]; flat_elem_type_sizes.push(flat_value_size); } @@ -1078,7 +1005,11 @@ impl Context { let var = self.acir_context.add_constant(FieldElement::from(type_size as u128)); AcirValue::Var(var, AcirType::field()) }); - self.initialize_array_new(element_type_sizes, init_values.len(), Some(AcirValue::Array(init_values.into())))?; + self.initialize_array( + element_type_sizes, + init_values.len(), + Some(AcirValue::Array(init_values.into())), + )?; } _ => { dbg!(&dfg[array]); @@ -1102,41 +1033,53 @@ impl Context { self.memory_blocks.insert(*result_id, block_id); result_block_id = block_id; } else { - dbg!("got here"); - // dbg!(len); - // let array_typ = dfg.type_of_value(array_id); - // let element_size = array_typ.element_size(); - // dbg!(element_size); - let flat_elem_size = array_typ.flattened_element_size(); - dbg!(flat_elem_size); + let flat_array_size = array_typ.flattened_size(); // Initialize the new array with the values from the old array result_block_id = self.block_id(result_id); - dbg!(result_block_id.0); - // TODO: probably need to fix how we read the old array to account for nested arrays - let init_values = try_vecmap(0..flat_elem_size, |i| { + let init_values = try_vecmap(0..flat_array_size, |i| { let index = AcirValue::Var( self.acir_context.add_constant(FieldElement::from(i as u128)), AcirType::NumericType(NumericType::NativeField), ); let var = index.into_var()?; let read = self.acir_context.read_from_memory(block_id, &var)?; - Ok::(AcirValue::Var(read, AcirType::NumericType(NumericType::NativeField))) + Ok::(AcirValue::Var( + read, + AcirType::NumericType(NumericType::NativeField), + )) })?; - self.initialize_array_new(result_block_id, flat_elem_size, Some(AcirValue::Array(init_values.into())))?; + self.initialize_array( + result_block_id, + flat_array_size, + Some(AcirValue::Array(init_values.into())), + )?; } let element_size = array_typ.element_size(); - // dbg!(element_size); - let flat_array_size = array_typ.flattened_element_size(); - // dbg!(flat_array_size); - let array_len = dfg.try_get_array_length(array).expect("ICE: need to have an array, slices not implemented"); + let flat_array_size = array_typ.flattened_size(); + let array_len = dfg + .try_get_array_length(array) + .expect("ICE: need to have an array, slices not implemented"); let flat_elem_size = flat_array_size / array_len; - let flat_element_size_var = self.acir_context.add_constant(FieldElement::from(flat_elem_size as u128)); - - 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 flat_element_size_var = + self.acir_context.add_constant(FieldElement::from(flat_elem_size as u128)); + + 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 var_index = self.acir_context.mul_var(outer_offset, flat_element_size_var)?; let mut var_index = self.acir_context.add_var(var_index, inner_offset)?; @@ -1144,8 +1087,10 @@ impl Context { let mut is_first_elem = true; self.array_set_value(store_value, result_block_id, &mut var_index, &mut is_first_elem)?; - let result_value = - AcirValue::DynamicArray(AcirDynamicArray { block_id: result_block_id, len: flat_array_size }); + let result_value = AcirValue::DynamicArray(AcirDynamicArray { + block_id: result_block_id, + len: flat_array_size, + }); self.define_result(dfg, instruction, result_value); Ok(()) } @@ -1182,23 +1127,12 @@ impl Context { /// Initializes an array with the given values and caches the fact that we /// have initialized this array. fn initialize_array( - &mut self, - array: BlockId, - len: usize, - values: Option<&[AcirValue]>, - ) -> Result<(), InternalError> { - self.acir_context.initialize_array(array, len, values)?; - self.initialized_arrays.insert(array); - Ok(()) - } - - fn initialize_array_new( &mut self, array: BlockId, len: usize, value: Option, - ) -> Result<(), InternalError> { - self.acir_context.initialize_array_new(array, len, value)?; + ) -> Result<(), InternalError> { + self.acir_context.initialize_array(array, len, value)?; self.initialized_arrays.insert(array); Ok(()) } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 33692ecbd23..1dd2368b1a0 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -396,9 +396,6 @@ impl Instruction { if let (Some((array, _)), Some(index)) = (array, index) { let index = index.try_to_u64().expect("Expected array index to fit in u64") as usize; - dbg!(array.len()); - // dbg!(array.clone()); - dbg!(index); if index < array.len() { return SimplifiedTo(array[index]); } diff --git a/compiler/noirc_evaluator/src/ssa/ir/types.rs b/compiler/noirc_evaluator/src/ssa/ir/types.rs index a6e7785c07f..b576ee12d45 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/types.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/types.rs @@ -78,21 +78,17 @@ impl Type { } } - pub(crate) fn flattened_element_size(&self) -> usize { + /// Returns the flattened size of a Type + pub(crate) fn flattened_size(&self) -> usize { let mut size = 0; match self { Type::Array(elements, len) => { - // array.into_iter().flat_map(AcirValue::flatten).collect(), - // dbg!(elements.clone()); - for elem in elements.as_ref() { - size += elem.flattened_element_size() * len; - } - // let element_size = elements.len(); - // let x = elements.into_iter().flat_map(|elem| elem.flattened_element_size()).collect(); + size = elements.iter().fold(size, |sum, elem| sum + (elem.flattened_size() * len)); } - _ => { - size += 1 + Type::Slice(_) => { + unimplemented!("ICE: cannot fetch flattened slice size"); } + _ => size += 1, } size } diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 8c8b0bbd9f0..496c092393c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -551,13 +551,10 @@ impl<'f> Context<'f> { _ => panic!("Expected array type"), }; - // dbg!(len); - dbg!(then_value); - dbg!(else_value); 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(); - // dbg!(index.clone()); + 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()]); diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 99b1a318dd8..20250f5470e 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -298,7 +298,6 @@ impl<'a> FunctionContext<'a> { ) -> Values { // base_index = index * type_size let type_size = Self::convert_type(element_type).size_of_type(); - dbg!(type_size); let type_size = self.builder.field_constant(type_size as u128); let base_index = self.builder.set_location(location).insert_binary(index, BinaryOp::Mul, type_size); 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 93bfecded26..076c2b68f11 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 @@ -18,10 +18,8 @@ fn main(mut x : [Foo; 4], y : pub Field) { assert(x[y].a == 10); assert(x[y].b == [11, 12, 23]); assert(x[y].bar.inner == [109, 110, 111]); - dep::std::println("show list"); - for i in 0..4 { - dep::std::println(x[i]); - } + + // Check dynamic array set if y != 2 { x[y].a = 50; } else { @@ -35,5 +33,10 @@ fn main(mut x : [Foo; 4], y : pub Field) { x[y - 1].b = [100, 101, 102]; } assert(x[2].b == [100, 101, 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]); } From 3195325717a14cacb6d01eb14f81e6818c875f5a Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 19 Sep 2023 15:52:10 +0000 Subject: [PATCH 07/14] updating array_get and array_set to reduce repeated code --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 204 +++++++----------- .../nested_array_dynamic/Prover.toml | 12 +- 2 files changed, 84 insertions(+), 132 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index bea747bf8ef..f6df507e7f0 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -768,6 +768,74 @@ impl Context { } } + let element_type_sizes = self.init_element_types_size_array(array, dfg)?; + let array_len = dfg + .try_get_array_length(array) + .expect("ICE: need to have an array, slices not implemented"); + + let mut var_index = self.get_flattened_index(&array_typ, array_len, var_index, element_type_sizes)?; + let results = dfg.instruction_results(instruction); + let res_typ = dfg.type_of_value(results[0]); + + let mut is_first_elem = true; + let mut values = Vector::new(); + self.array_get_type( + dfg, + &res_typ, + &mut var_index, + &mut values, + block_id, + &mut is_first_elem, + )?; + assert_eq!(values.len(), 1); + + self.define_result(dfg, instruction, values[0].clone()); + + // TODO: update array_get to return an AcirValue + // or remove that array_get returns anything + let read = self.acir_context.read_from_memory(block_id, &var_index)?; + Ok(read) + } + + fn get_flattened_index( + &mut self, + array_typ: &Type, + array_len: usize, + var_index: AcirVar, + element_type_sizes: BlockId, + ) -> Result { + let element_size = array_typ.element_size(); + let flat_array_size = array_typ.flattened_size(); + let flat_elem_size = flat_array_size / array_len; + let flat_element_size_var = + self.acir_context.add_constant(FieldElement::from(flat_elem_size as u128)); + + 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 var_index = self.acir_context.mul_var(outer_offset, flat_element_size_var)?; + Ok(self.acir_context.add_var(var_index, inner_offset)?) + } + + fn init_element_types_size_array( + &mut self, + array: ValueId, + dfg: &DataFlowGraph, + ) -> Result { let element_type_sizes = self.internal_block_id(); // TODO: doing this here but perhaps it should be done on array initialization automatically match &dfg[array] { @@ -779,12 +847,10 @@ impl Context { match typ { Type::Array(element_types, _) => { for (i, typ) in element_types.as_ref().iter().enumerate() { - let mut flat_value_size = typ.flattened_size(); - flat_value_size += flat_elem_type_sizes[i]; - flat_elem_type_sizes.push(flat_value_size); + flat_elem_type_sizes.push(typ.flattened_size() + flat_elem_type_sizes[i]); } } - _ => panic!("ahhhh should only have an array"), + _ => unreachable!("ICE: Expected an array type but got {typ}"), } // We do not have to initialize the last elem size value as that is the maximum array size flat_elem_type_sizes.pop(); @@ -799,64 +865,13 @@ impl Context { )?; } _ => { - dbg!(&dfg[array]); return Err(RuntimeError::UnInitialized { name: "array".to_string(), call_stack: self.acir_context.get_call_stack(), }); } } - - let element_size = array_typ.element_size(); - let flat_array_size = array_typ.flattened_size(); - let array_len = dfg - .try_get_array_length(array) - .expect("ICE: need to have an array, slices not implemented"); - let flat_elem_size = flat_array_size / array_len; - let flat_element_size_var = - self.acir_context.add_constant(FieldElement::from(flat_elem_size as u128)); - - 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 var_index = self.acir_context.mul_var(outer_offset, flat_element_size_var)?; - let mut var_index = self.acir_context.add_var(var_index, inner_offset)?; - - let results = dfg.instruction_results(instruction); - let res_typ = dfg.type_of_value(results[0]); - - let mut is_first_elem = true; - let mut values = Vector::new(); - self.array_get_type( - dfg, - &res_typ, - &mut var_index, - &mut values, - block_id, - &mut is_first_elem, - )?; - assert_eq!(values.len(), 1); - - self.define_result(dfg, instruction, values[0].clone()); - - // TODO: update array_get to return an AcirValue - // or remove that array_get returns anything - let read = self.acir_context.read_from_memory(block_id, &var_index)?; - Ok(read) + Ok(element_type_sizes) } fn array_get_type( @@ -945,12 +960,12 @@ impl Context { // 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_typ = dfg.type_of_value(array_id); - let len = match &array_typ { - Type::Array(typ, len) => { + let array_len = match &array_typ { + Type::Array(_, len) => { // Flatten the array length to handle arrays of complex types *len } - Type::Slice(typ) => { + Type::Slice(_) => { // Fetch the true length of the slice from the array_set instruction let length = length .expect("ICE: array set on slice must have a length associated with the call"); @@ -983,42 +998,8 @@ impl Context { } } - let element_type_sizes = self.internal_block_id(); - match &dfg[array_id] { - Value::Array { typ, .. } - | Value::Param { typ, .. } - | Value::Instruction { typ, .. } => { - let mut flat_elem_type_sizes = Vec::new(); - flat_elem_type_sizes.push(0); - match typ { - Type::Array(element_types, _) => { - for (i, typ) in element_types.as_ref().iter().enumerate() { - let mut flat_value_size = typ.flattened_size(); - flat_value_size += flat_elem_type_sizes[i]; - flat_elem_type_sizes.push(flat_value_size); - } - } - _ => panic!("ahhhh should only have an array"), - } - flat_elem_type_sizes.pop(); - 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()) - }); - self.initialize_array( - element_type_sizes, - init_values.len(), - Some(AcirValue::Array(init_values.into())), - )?; - } - _ => { - dbg!(&dfg[array]); - return Err(RuntimeError::UnInitialized { - name: "array".to_string(), - call_stack: self.acir_context.get_call_stack(), - }); - } - } + let element_type_sizes = self.init_element_types_size_array(array, dfg)?; + let flat_array_size = array_typ.flattened_size(); // Since array_set creates a new array, we create a new block ID for this // array, unless map_array is true. In that case, we operate directly on block_id @@ -1029,11 +1010,9 @@ impl Context { .expect("Array set does not have one result"); let result_block_id; if map_array { - dbg!(map_array); self.memory_blocks.insert(*result_id, block_id); result_block_id = block_id; } else { - let flat_array_size = array_typ.flattened_size(); // Initialize the new array with the values from the old array result_block_id = self.block_id(result_id); let init_values = try_vecmap(0..flat_array_size, |i| { @@ -1054,35 +1033,8 @@ impl Context { Some(AcirValue::Array(init_values.into())), )?; } - - let element_size = array_typ.element_size(); - let flat_array_size = array_typ.flattened_size(); - let array_len = dfg - .try_get_array_length(array) - .expect("ICE: need to have an array, slices not implemented"); - let flat_elem_size = flat_array_size / array_len; - let flat_element_size_var = - self.acir_context.add_constant(FieldElement::from(flat_elem_size as u128)); - - 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 var_index = self.acir_context.mul_var(outer_offset, flat_element_size_var)?; - let mut var_index = self.acir_context.add_var(var_index, inner_offset)?; + + let mut var_index = self.get_flattened_index(&array_typ, array_len, var_index, element_type_sizes)?; let mut is_first_elem = true; self.array_set_value(store_value, result_block_id, &mut var_index, &mut is_first_elem)?; diff --git a/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml b/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml index 849e32631c9..6c7e77b581d 100644 --- a/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml +++ b/tooling/nargo_cli/tests/execution_success/nested_array_dynamic/Prover.toml @@ -8,11 +8,11 @@ b = ["2", "3", "20"] inner = ["100", "101", "102"] [[x]] -a = "4" # idx = 3, true start idx = 7 -b = ["5", "6", "21"] # idx = 4, start idx = 8 +a = "4" # idx = 3, flattened start idx = 7 +b = ["5", "6", "21"] # idx = 4, flattened start idx = 8 [x.bar] -inner = ["103", "104", "105"] # idx = 5, idx = 11 +inner = ["103", "104", "105"] # idx = 5, flattened start idx = 11 [[x]] a = "7" @@ -22,8 +22,8 @@ b = ["8", "9", "22"] inner = ["106", "107", "108"] [[x]] -a = "10" -b = ["11", "12", "23"] +a = "10" # idx = 9, flattened start idx = 21 +b = ["11", "12", "23"] # idx = 10, flattened start idx = 22 [x.bar] -inner = ["109", "110", "111"] +inner = ["109", "110", "111"] # idx = 11, flattened start idx = 25 From 8fb3bb13b48323ba48219f4565ad6f26d7462f48 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 19 Sep 2023 17:55:01 +0000 Subject: [PATCH 08/14] restrict dynamic indices for non-homogenous slices --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 188 +++++++++--------- 1 file changed, 92 insertions(+), 96 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index f6df507e7f0..acd738c723e 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -244,7 +244,12 @@ impl Context { AcirValue::Var(_, _) => (), AcirValue::Array(values) => { let block_id = self.block_id(param_id); - self.initialize_array(block_id, typ.flattened_size(), Some(value.clone()))?; + let len = if matches!(typ, Type::Array(_, _)) { + typ.flattened_size() + } else { + values.len() + }; + self.initialize_array(block_id, len, Some(value.clone()))?; } AcirValue::DynamicArray(_) => unreachable!( "The dynamic array type is created in Acir gen and therefore cannot be a block parameter" @@ -453,13 +458,13 @@ impl Context { AcirValue::Array(values) => { let block_id = self.block_id(result); let values = vecmap(values, |v| v.clone()); - - self.initialize_array( - block_id, - values.len(), - Some(output.clone()), - )?; - // self.initialize_array(block_id, values.len(), Some(&values))?; + let array_typ = dfg.type_of_value(*result); + let len = if matches!(array_typ, Type::Array(_, _)) { + array_typ.flattened_size() + } else { + values.len() + }; + self.initialize_array(block_id, len, Some(output.clone()))?; } AcirValue::DynamicArray(_) => { unreachable!("The output from an intrinsic call is expected to be a single value or an array but got {output:?}"); @@ -731,7 +736,7 @@ impl Context { Ok(AcirValue::Array(elements)) } AcirValue::DynamicArray(_) => { - panic!("ahhh") + unimplemented!("ICE: setting a dynamic array not supported"); } } } @@ -744,20 +749,20 @@ impl Context { var_index: AcirVar, dfg: &DataFlowGraph, ) -> Result { - let array = dfg.resolve(array); + let array_id = dfg.resolve(array); - let array_typ = dfg.type_of_value(array); - let block_id = self.block_id(&array); + let array_typ = dfg.type_of_value(array_id); + let block_id = self.block_id(&array_id); if !self.initialized_arrays.contains(&block_id) { - match &dfg[array] { + match &dfg[array_id] { Value::Array { array, .. } => { - let values: Vector = - array.iter().map(|i| self.convert_value(*i, dfg)).collect(); - self.initialize_array( - block_id, - array_typ.flattened_size(), - Some(AcirValue::Array(values)), - )?; + let value = self.convert_value(array_id, dfg); + let len = if matches!(array_typ, Type::Array(_, _)) { + array_typ.flattened_size() + } else { + array.len() + }; + self.initialize_array(block_id, len, Some(value))?; } _ => { return Err(RuntimeError::UnInitialized { @@ -768,25 +773,19 @@ impl Context { } } - let element_type_sizes = self.init_element_types_size_array(array, dfg)?; - let array_len = dfg - .try_get_array_length(array) - .expect("ICE: need to have an array, slices not implemented"); + let mut var_index = if matches!(array_typ, Type::Array(_, _)) { + let array_len = dfg.try_get_array_length(array_id).expect("ICE: expected an array"); + self.get_flattened_index(&array_typ, array_len, var_index)? + } else { + var_index + }; - let mut var_index = self.get_flattened_index(&array_typ, array_len, var_index, element_type_sizes)?; let results = dfg.instruction_results(instruction); let res_typ = dfg.type_of_value(results[0]); let mut is_first_elem = true; let mut values = Vector::new(); - self.array_get_type( - dfg, - &res_typ, - &mut var_index, - &mut values, - block_id, - &mut is_first_elem, - )?; + self.array_get_type(&res_typ, &mut var_index, &mut values, block_id, &mut is_first_elem)?; assert_eq!(values.len(), 1); self.define_result(dfg, instruction, values[0].clone()); @@ -802,8 +801,9 @@ impl Context { array_typ: &Type, array_len: usize, var_index: AcirVar, - element_type_sizes: BlockId, ) -> Result { + let element_type_sizes = self.init_element_types_size_array(array_typ)?; + let element_size = array_typ.element_size(); let flat_array_size = array_typ.flattened_size(); let flat_elem_size = flat_array_size / array_len; @@ -828,55 +828,45 @@ impl Context { self.acir_context.read_from_memory(element_type_sizes, &inner_offset_index)?; let var_index = self.acir_context.mul_var(outer_offset, flat_element_size_var)?; - Ok(self.acir_context.add_var(var_index, inner_offset)?) + self.acir_context.add_var(var_index, inner_offset) } - fn init_element_types_size_array( - &mut self, - array: ValueId, - dfg: &DataFlowGraph, - ) -> Result { + fn init_element_types_size_array(&mut self, array_typ: &Type) -> Result { let element_type_sizes = self.internal_block_id(); - // TODO: doing this here but perhaps it should be done on array initialization automatically - match &dfg[array] { - Value::Array { typ, .. } - | Value::Param { typ, .. } - | Value::Instruction { typ, .. } => { - let mut flat_elem_type_sizes = Vec::new(); - flat_elem_type_sizes.push(0); - match 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]); - } - } - _ => unreachable!("ICE: Expected an array type but got {typ}"), + 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]); } - // We do not have to initialize the last elem size value as that is the maximum array size - flat_elem_type_sizes.pop(); - 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()) - }); - self.initialize_array( - element_type_sizes, - init_values.len(), - Some(AcirValue::Array(init_values.into())), - )?; } _ => { - return Err(RuntimeError::UnInitialized { - name: "array".to_string(), + return Err(InternalError::UnExpected { + expected: "array".to_owned(), + found: array_typ.to_string(), call_stack: self.acir_context.get_call_stack(), - }); + } + .into()) } } + // We do not have to initialize the last elem size value as that is the maximum array size + flat_elem_type_sizes.pop(); + 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()) + }); + self.initialize_array( + element_type_sizes, + init_values.len(), + Some(AcirValue::Array(init_values.into())), + )?; + Ok(element_type_sizes) } fn array_get_type( &mut self, - dfg: &DataFlowGraph, ssa_type: &Type, var_index: &mut AcirVar, values: &mut Vector, @@ -891,7 +881,7 @@ impl Context { } *first_elem = false; - let read = self.acir_context.read_from_memory(block_id, &var_index)?; + let read = self.acir_context.read_from_memory(block_id, var_index)?; let typ = AcirType::NumericType(numeric_type); let value = AcirValue::Var(read, typ); values.push_back(value); @@ -900,22 +890,20 @@ impl Context { let mut inner_vec = Vector::new(); for _ in 0..len { for typ in element_types.as_ref() { - self.array_get_type( - dfg, - typ, - var_index, - &mut inner_vec, - block_id, - first_elem, - )?; + self.array_get_type(typ, var_index, &mut inner_vec, block_id, first_elem)?; } } let array_value = AcirValue::Array(inner_vec); values.push_back(array_value); } Type::Slice(_) => { - // TODO: need values here to fetch the len like for a Type::Array, not obvious how to incorporate it yet - unimplemented!("ICE: Non-const indices for nested arrays of slices is not allowed"); + // TODO: need SSA values here to fetch the len like for a Type::Array, not obvious how to incorporate it yet + return Err(InternalError::UnExpected { + expected: "array".to_owned(), + found: ssa_type.to_string(), + call_stack: self.acir_context.get_call_stack(), + } + .into()); } _ => unreachable!("ICE - expected an array or slice"), }; @@ -961,9 +949,9 @@ impl Context { // a separate SSA value and restrictions on slice indices should be generated elsewhere in the SSA. let array_typ = dfg.type_of_value(array_id); let array_len = match &array_typ { - Type::Array(_, len) => { + Type::Array(_, _) => { // Flatten the array length to handle arrays of complex types - *len + array_typ.flattened_size() } Type::Slice(_) => { // Fetch the true length of the slice from the array_set instruction @@ -984,9 +972,14 @@ impl Context { if !already_initialized { let value = &dfg[array_id]; match value { - Value::Array { .. } => { + Value::Array { array, .. } => { let value = self.convert_value(array_id, dfg); - self.initialize_array(block_id, array_typ.flattened_size(), Some(value))?; + let len = if matches!(array_typ, Type::Array(_, _)) { + array_typ.flattened_size() + } else { + array.len() + }; + self.initialize_array(block_id, len, Some(value))?; } _ => { return Err(InternalError::General { @@ -998,9 +991,6 @@ impl Context { } } - let element_type_sizes = self.init_element_types_size_array(array, dfg)?; - let flat_array_size = array_typ.flattened_size(); - // Since array_set creates a new array, we create a new block ID for this // array, unless map_array is true. In that case, we operate directly on block_id // and we do not create a new block ID. @@ -1015,7 +1005,7 @@ impl Context { } else { // Initialize the new array with the values from the old array result_block_id = self.block_id(result_id); - let init_values = try_vecmap(0..flat_array_size, |i| { + let init_values = try_vecmap(0..array_len, |i| { let index = AcirValue::Var( self.acir_context.add_constant(FieldElement::from(i as u128)), AcirType::NumericType(NumericType::NativeField), @@ -1029,20 +1019,26 @@ impl Context { })?; self.initialize_array( result_block_id, - flat_array_size, + array_len, Some(AcirValue::Array(init_values.into())), )?; } - - let mut var_index = self.get_flattened_index(&array_typ, array_len, var_index, element_type_sizes)?; + + // TODO: Need to add support for dynamic indices with non-homogenous slices + let mut var_index = if matches!(array_typ, Type::Array(_, _)) { + // This is the user facing array len without the element types flattened + let true_array_len = + dfg.try_get_array_length(array_id).expect("ICE: expected an array"); + self.get_flattened_index(&array_typ, true_array_len, var_index)? + } else { + var_index + }; let mut is_first_elem = true; self.array_set_value(store_value, result_block_id, &mut var_index, &mut is_first_elem)?; - let result_value = AcirValue::DynamicArray(AcirDynamicArray { - block_id: result_block_id, - len: flat_array_size, - }); + let result_value = + AcirValue::DynamicArray(AcirDynamicArray { block_id: result_block_id, len: array_len }); self.define_result(dfg, instruction, result_value); Ok(()) } @@ -1062,7 +1058,7 @@ impl Context { } *first_elem = false; // Write the new value into the new array at the specified index - self.acir_context.write_to_memory(block_id, &var_index, &store_var)?; + self.acir_context.write_to_memory(block_id, var_index, &store_var)?; } AcirValue::Array(values) => { for value in values { From 469c3339b61a26bb932760858495934624b56898 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 19 Sep 2023 18:30:54 +0000 Subject: [PATCH 09/14] cleanup array_get_value --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 180 +++++++++--------- 1 file changed, 86 insertions(+), 94 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index acd738c723e..ba318f445a2 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -748,7 +748,7 @@ impl Context { array: ValueId, var_index: AcirVar, dfg: &DataFlowGraph, - ) -> Result { + ) -> Result<(), RuntimeError> { let array_id = dfg.resolve(array); let array_typ = dfg.type_of_value(array_id); @@ -783,96 +783,21 @@ impl Context { let results = dfg.instruction_results(instruction); let res_typ = dfg.type_of_value(results[0]); - let mut is_first_elem = true; - let mut values = Vector::new(); - self.array_get_type(&res_typ, &mut var_index, &mut values, block_id, &mut is_first_elem)?; - assert_eq!(values.len(), 1); - - self.define_result(dfg, instruction, values[0].clone()); - - // TODO: update array_get to return an AcirValue - // or remove that array_get returns anything - let read = self.acir_context.read_from_memory(block_id, &var_index)?; - Ok(read) - } - - fn get_flattened_index( - &mut self, - array_typ: &Type, - array_len: usize, - var_index: AcirVar, - ) -> Result { - let element_type_sizes = self.init_element_types_size_array(array_typ)?; - - let element_size = array_typ.element_size(); - let flat_array_size = array_typ.flattened_size(); - let flat_elem_size = flat_array_size / array_len; - let flat_element_size_var = - self.acir_context.add_constant(FieldElement::from(flat_elem_size as u128)); - - 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 var_index = self.acir_context.mul_var(outer_offset, flat_element_size_var)?; - self.acir_context.add_var(var_index, inner_offset) - } + let mut first_elem = true; + let value = self.array_get_value(&res_typ, block_id, &mut var_index, &mut first_elem)?; - fn init_element_types_size_array(&mut self, array_typ: &Type) -> Result { - let element_type_sizes = self.internal_block_id(); - 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]); - } - } - _ => { - return Err(InternalError::UnExpected { - expected: "array".to_owned(), - found: array_typ.to_string(), - call_stack: self.acir_context.get_call_stack(), - } - .into()) - } - } - // We do not have to initialize the last elem size value as that is the maximum array size - flat_elem_type_sizes.pop(); - 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()) - }); - self.initialize_array( - element_type_sizes, - init_values.len(), - Some(AcirValue::Array(init_values.into())), - )?; + self.define_result(dfg, instruction, value); - Ok(element_type_sizes) + Ok(()) } - fn array_get_type( + fn array_get_value( &mut self, ssa_type: &Type, - var_index: &mut AcirVar, - values: &mut Vector, block_id: BlockId, + var_index: &mut AcirVar, first_elem: &mut bool, - ) -> Result<(), RuntimeError> { + ) -> Result { let one = self.acir_context.add_constant(FieldElement::one()); match ssa_type.clone() { Type::Numeric(numeric_type) => { @@ -883,31 +808,29 @@ impl Context { let read = self.acir_context.read_from_memory(block_id, var_index)?; let typ = AcirType::NumericType(numeric_type); - let value = AcirValue::Var(read, typ); - values.push_back(value); + Ok(AcirValue::Var(read, typ)) } Type::Array(element_types, len) => { - let mut inner_vec = Vector::new(); + let mut values = Vector::new(); for _ in 0..len { for typ in element_types.as_ref() { - self.array_get_type(typ, var_index, &mut inner_vec, block_id, first_elem)?; + values + .push_back(self.array_get_value(typ, block_id, var_index, first_elem)?); } } - let array_value = AcirValue::Array(inner_vec); - values.push_back(array_value); + Ok(AcirValue::Array(values)) } Type::Slice(_) => { - // TODO: need SSA values here to fetch the len like for a Type::Array, not obvious how to incorporate it yet - return Err(InternalError::UnExpected { + // TODO: need SSA values here to fetch the len like we do for a Type::Array + Err(InternalError::UnExpected { expected: "array".to_owned(), found: ssa_type.to_string(), call_stack: self.acir_context.get_call_stack(), } - .into()); + .into()) } _ => unreachable!("ICE - expected an array or slice"), - }; - Ok(()) + } } /// Copy the array and generates a write opcode on the new array @@ -1072,6 +995,75 @@ impl Context { Ok(()) } + fn init_element_types_size_array(&mut self, array_typ: &Type) -> Result { + let element_type_sizes = self.internal_block_id(); + 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]); + } + } + _ => { + return Err(InternalError::UnExpected { + expected: "array".to_owned(), + found: array_typ.to_string(), + call_stack: self.acir_context.get_call_stack(), + } + .into()) + } + } + // We do not have to initialize the last elem size value as that is the maximum array size + flat_elem_type_sizes.pop(); + 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()) + }); + self.initialize_array( + element_type_sizes, + init_values.len(), + Some(AcirValue::Array(init_values.into())), + )?; + + Ok(element_type_sizes) + } + + fn get_flattened_index( + &mut self, + array_typ: &Type, + array_len: usize, + var_index: AcirVar, + ) -> Result { + let element_type_sizes = self.init_element_types_size_array(array_typ)?; + + let element_size = array_typ.element_size(); + let flat_array_size = array_typ.flattened_size(); + let flat_elem_size = flat_array_size / array_len; + let flat_element_size_var = + self.acir_context.add_constant(FieldElement::from(flat_elem_size as u128)); + + 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 var_index = self.acir_context.mul_var(outer_offset, flat_element_size_var)?; + self.acir_context.add_var(var_index, inner_offset) + } + /// Initializes an array with the given values and caches the fact that we /// have initialized this array. fn initialize_array( From 0648142a768ff462e7701ace4b8355d3b677fed1 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 19 Sep 2023 19:24:42 +0000 Subject: [PATCH 10/14] add internal_memory_blocks map --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 34 ++++++++++++++----- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index ba318f445a2..fe4173d3cef 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -65,6 +65,12 @@ struct Context { /// Each acir memory block corresponds to a different SSA array. memory_blocks: HashMap, BlockId>, + /// Maps SSA values to a BlockId used internally + /// A BlockId is an ACIR structure which identifies a memory block + /// Each memory blocks corresponds to a different SSA value + /// which utilizes this internal memory for ACIR generation. + internal_memory_blocks: HashMap, BlockId>, + /// Number of the next BlockId, it is used to construct /// a new BlockId max_block_id: u32, @@ -154,6 +160,7 @@ impl Context { acir_context, initialized_arrays: HashSet::new(), memory_blocks: HashMap::default(), + internal_memory_blocks: HashMap::default(), max_block_id: 0, } } @@ -307,11 +314,13 @@ impl Context { /// This is useful for referencing information that can /// only be computed dynamically, such as the type structure /// of non-homogenous arrays. - fn internal_block_id(&mut self) -> BlockId { + fn internal_block_id(&mut self, value: &ValueId) -> BlockId { + if let Some(block_id) = self.internal_memory_blocks.get(value) { + return *block_id; + } let block_id = BlockId(self.max_block_id); self.max_block_id += 1; - // We do not insert into `self.memory_blocks` here as this BlockId - // does not have a corresponding ValueId + self.internal_memory_blocks.insert(*value, block_id); block_id } @@ -775,7 +784,7 @@ impl Context { let mut var_index = if matches!(array_typ, Type::Array(_, _)) { let array_len = dfg.try_get_array_length(array_id).expect("ICE: expected an array"); - self.get_flattened_index(&array_typ, array_len, var_index)? + self.get_flattened_index(&array_typ, array_id, array_len, var_index)? } else { var_index }; @@ -952,7 +961,7 @@ impl Context { // This is the user facing array len without the element types flattened let true_array_len = dfg.try_get_array_length(array_id).expect("ICE: expected an array"); - self.get_flattened_index(&array_typ, true_array_len, var_index)? + self.get_flattened_index(&array_typ, array_id, true_array_len, var_index)? } else { var_index }; @@ -995,8 +1004,16 @@ impl Context { Ok(()) } - fn init_element_types_size_array(&mut self, array_typ: &Type) -> Result { - let element_type_sizes = self.internal_block_id(); + fn init_element_types_size_array( + &mut self, + array_typ: &Type, + array_id: ValueId, + ) -> Result { + let element_type_sizes = self.internal_block_id(&array_id); + // Check whether an internal type sizes array has already been initialized + if self.initialized_arrays.contains(&element_type_sizes) { + return Ok(element_type_sizes); + } let mut flat_elem_type_sizes = Vec::new(); flat_elem_type_sizes.push(0); match array_typ { @@ -1032,10 +1049,11 @@ impl Context { fn get_flattened_index( &mut self, array_typ: &Type, + array_id: ValueId, array_len: usize, var_index: AcirVar, ) -> Result { - let element_type_sizes = self.init_element_types_size_array(array_typ)?; + let element_type_sizes = self.init_element_types_size_array(array_typ, array_id)?; let element_size = array_typ.element_size(); let flat_array_size = array_typ.flattened_size(); From b99f0e2a5ded6fd4c86ae0319313061778c76a67 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 20 Sep 2023 19:13:19 +0000 Subject: [PATCH 11/14] do not make separate bool for handle_constant_index --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index fe4173d3cef..4404c905204 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -577,9 +577,7 @@ impl Context { } }; - let accessed_constant_index = - self.handle_constant_index(instruction, dfg, index, array, store_value)?; - if accessed_constant_index { + if self.handle_constant_index(instruction, dfg, index, array, store_value)? { return Ok(()); } From 57841704c75a4cc7e2cf05f82460f7d84e50f8e5 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 20 Sep 2023 20:01:43 +0000 Subject: [PATCH 12/14] fetch dummy value with predicate index --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 155 +++++++++--------- 1 file changed, 81 insertions(+), 74 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 4404c905204..29592c5b03a 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -582,7 +582,7 @@ impl Context { } let (new_index, new_value) = - self.convert_array_operation_inputs(dfg, index, store_value)?; + self.convert_array_operation_inputs(array, dfg, index, store_value)?; let resolved_array = dfg.resolve(array); let map_array = last_array_uses.get(&resolved_array) == Some(&instruction); @@ -679,17 +679,17 @@ impl Context { /// - index_var is the index of the array /// - predicate_index is 0, or the index if the predicate is true /// - new_value is the optional value when the operation is an array_set - /// When there is a predicate, it is predicate*value + (1-predicate)*dummy, where dummy is zero. - /// It is a dummy value because in the case of a false predicate, the value stored at the requested index is zeroed out - /// and will never be used elsewhere in the program. + /// When there is a predicate, it is predicate*value + (1-predicate)*dummy, where dummy is the value of the array at the requested index. + /// It is a dummy value because in the case of a false predicate, the value stored at the requested index will be itself. fn convert_array_operation_inputs( &mut self, + array: ValueId, dfg: &DataFlowGraph, index: ValueId, store_value: Option, ) -> Result<(AcirVar, Option), RuntimeError> { let index_var = self.convert_numeric_value(index, dfg)?; - let predicate_index = + let mut predicate_index = self.acir_context.mul_var(index_var, self.current_side_effects_enabled_var)?; let new_value = if let Some(store) = store_value { @@ -697,10 +697,20 @@ impl Context { if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) { Some(store_value) } else { - // We use zero as the dummy value. As the dummy value will never be accessed - // it is ok to zero out the value at the requested index - let zero = self.acir_context.add_constant(FieldElement::zero()); - Some(self.convert_array_set_store_value(&store_value, zero)?) + let (_, _, block_id) = self.check_array_is_initialized(array, dfg)?; + + let store_type = dfg.type_of_value(store); + + // We must setup the dummy value to match the type of the value we wish to store + let mut is_first_elem = true; + let dummy = self.array_get_value( + &store_type, + block_id, + &mut predicate_index, + &mut is_first_elem, + )?; + + Some(self.convert_array_set_store_value(&store_value, &dummy)?) } } else { None @@ -719,32 +729,40 @@ impl Context { fn convert_array_set_store_value( &mut self, store_value: &AcirValue, - dummy: AcirVar, + dummy_value: &AcirValue, ) -> Result { - match store_value { - AcirValue::Var(store_var, _) => { + match (store_value, dummy_value) { + (AcirValue::Var(store_var, _), AcirValue::Var(dummy_var, _)) => { let true_pred = self.acir_context.mul_var(*store_var, self.current_side_effects_enabled_var)?; let one = self.acir_context.add_constant(FieldElement::one()); let not_pred = self.acir_context.sub_var(one, self.current_side_effects_enabled_var)?; - let false_pred = self.acir_context.mul_var(not_pred, dummy)?; + let false_pred = self.acir_context.mul_var(not_pred, *dummy_var)?; // predicate*value + (1-predicate)*dummy let new_value = self.acir_context.add_var(true_pred, false_pred)?; Ok(AcirValue::Var(new_value, AcirType::field())) } - AcirValue::Array(values) => { + (AcirValue::Array(values), AcirValue::Array(dummy_values)) => { let mut elements = im::Vector::new(); - for val in values { - elements.push_back(self.convert_array_set_store_value(val, dummy)?); + assert_eq!( + values.len(), + dummy_values.len(), + "ICE: The store value and dummy must have the same number of inner values" + ); + for (val, dummy_val) in values.iter().zip(dummy_values) { + elements.push_back(self.convert_array_set_store_value(val, dummy_val)?); } Ok(AcirValue::Array(elements)) } - AcirValue::DynamicArray(_) => { + (AcirValue::DynamicArray(_), AcirValue::DynamicArray(_)) => { unimplemented!("ICE: setting a dynamic array not supported"); } + _ => { + unreachable!("ICE: The store value and dummy value must match"); + } } } @@ -755,30 +773,8 @@ impl Context { array: ValueId, var_index: AcirVar, dfg: &DataFlowGraph, - ) -> Result<(), RuntimeError> { - let array_id = dfg.resolve(array); - - let array_typ = dfg.type_of_value(array_id); - let block_id = self.block_id(&array_id); - if !self.initialized_arrays.contains(&block_id) { - match &dfg[array_id] { - Value::Array { array, .. } => { - let value = self.convert_value(array_id, dfg); - let len = if matches!(array_typ, Type::Array(_, _)) { - array_typ.flattened_size() - } else { - array.len() - }; - self.initialize_array(block_id, len, Some(value))?; - } - _ => { - return Err(RuntimeError::UnInitialized { - name: "array".to_string(), - call_stack: self.acir_context.get_call_stack(), - }); - } - } - } + ) -> Result { + let (array_id, array_typ, block_id) = self.check_array_is_initialized(array, dfg)?; let mut var_index = if matches!(array_typ, Type::Array(_, _)) { let array_len = dfg.try_get_array_length(array_id).expect("ICE: expected an array"); @@ -793,9 +789,9 @@ impl Context { let mut first_elem = true; let value = self.array_get_value(&res_typ, block_id, &mut var_index, &mut first_elem)?; - self.define_result(dfg, instruction, value); + self.define_result(dfg, instruction, value.clone()); - Ok(()) + Ok(value) } fn array_get_value( @@ -864,11 +860,7 @@ impl Context { } }; - // Fetch the internal SSA ID for the array - let array_id = dfg.resolve(array); - - // Use the SSA ID to get or create its block ID - let block_id = self.block_id(&array_id); + let (array_id, array_typ, block_id) = self.check_array_is_initialized(array, dfg)?; // Every array has a length in its type, so we fetch that from // the SSA IR. @@ -877,7 +869,6 @@ 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_typ = dfg.type_of_value(array_id); let array_len = match &array_typ { Type::Array(_, _) => { // Flatten the array length to handle arrays of complex types @@ -896,31 +887,6 @@ impl Context { _ => unreachable!("ICE - expected an array"), }; - // Check if the array has already been initialized in ACIR gen - // if not, we initialize it using the values from SSA - let already_initialized = self.initialized_arrays.contains(&block_id); - if !already_initialized { - let value = &dfg[array_id]; - match value { - Value::Array { array, .. } => { - let value = self.convert_value(array_id, dfg); - let len = if matches!(array_typ, Type::Array(_, _)) { - array_typ.flattened_size() - } else { - array.len() - }; - self.initialize_array(block_id, len, Some(value))?; - } - _ => { - return Err(InternalError::General { - message: format!("Array {array} should be initialized"), - call_stack: self.acir_context.get_call_stack(), - } - .into()) - } - } - } - // Since array_set creates a new array, we create a new block ID for this // array, unless map_array is true. In that case, we operate directly on block_id // and we do not create a new block ID. @@ -1002,6 +968,47 @@ impl Context { Ok(()) } + fn check_array_is_initialized( + &mut self, + array: ValueId, + dfg: &DataFlowGraph, + ) -> Result<(ValueId, Type, BlockId), RuntimeError> { + // Fetch the internal SSA ID for the array + let array_id = dfg.resolve(array); + + let array_typ = dfg.type_of_value(array_id); + + // Use the SSA ID to get or create its block ID + let block_id = self.block_id(&array_id); + + // Check if the array has already been initialized in ACIR gen + // if not, we initialize it using the values from SSA + let already_initialized = self.initialized_arrays.contains(&block_id); + if !already_initialized { + let value = &dfg[array_id]; + match value { + Value::Array { array, .. } => { + let value = self.convert_value(array_id, dfg); + let len = if matches!(array_typ, Type::Array(_, _)) { + array_typ.flattened_size() + } else { + array.len() + }; + self.initialize_array(block_id, len, Some(value))?; + } + _ => { + return Err(InternalError::General { + message: format!("Array {array} should be initialized"), + call_stack: self.acir_context.get_call_stack(), + } + .into()) + } + } + } + + Ok((array_id, array_typ, block_id)) + } + fn init_element_types_size_array( &mut self, array_typ: &Type, From 05f35c644e387d29bb27818522c0e7624f282742 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 20 Sep 2023 23:47:38 +0000 Subject: [PATCH 13/14] flatten the index once for both array_set and array_get, do not modify modify the predicate index for the non-dummy val --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 44 ++++++++----------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 29592c5b03a..2637d3cf5ac 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -688,8 +688,18 @@ impl Context { index: ValueId, store_value: Option, ) -> Result<(AcirVar, Option), RuntimeError> { + let (array_id, array_typ, block_id) = self.check_array_is_initialized(array, dfg)?; + let index_var = self.convert_numeric_value(index, dfg)?; - let mut predicate_index = + // TODO(#2752): Need to add support for dynamic indices with non-homogenous slices + let index_var = if matches!(array_typ, Type::Array(_, _)) { + let array_len = dfg.try_get_array_length(array_id).expect("ICE: expected an array"); + self.get_flattened_index(&array_typ, array_id, array_len, index_var)? + } else { + index_var + }; + + let predicate_index = self.acir_context.mul_var(index_var, self.current_side_effects_enabled_var)?; let new_value = if let Some(store) = store_value { @@ -697,16 +707,15 @@ impl Context { if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) { Some(store_value) } else { - let (_, _, block_id) = self.check_array_is_initialized(array, dfg)?; - let store_type = dfg.type_of_value(store); + 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 mut is_first_elem = true; let dummy = self.array_get_value( &store_type, block_id, - &mut predicate_index, + &mut dummy_predicate_index, &mut is_first_elem, )?; @@ -771,17 +780,10 @@ impl Context { &mut self, instruction: InstructionId, array: ValueId, - var_index: AcirVar, + mut var_index: AcirVar, dfg: &DataFlowGraph, ) -> Result { - let (array_id, array_typ, block_id) = self.check_array_is_initialized(array, dfg)?; - - let mut var_index = if matches!(array_typ, Type::Array(_, _)) { - let array_len = dfg.try_get_array_length(array_id).expect("ICE: expected an array"); - self.get_flattened_index(&array_typ, array_id, array_len, var_index)? - } else { - var_index - }; + let (_, _, block_id) = self.check_array_is_initialized(array, dfg)?; let results = dfg.instruction_results(instruction); let res_typ = dfg.type_of_value(results[0]); @@ -824,7 +826,7 @@ impl Context { Ok(AcirValue::Array(values)) } Type::Slice(_) => { - // TODO: need SSA values here to fetch the len like we do for a Type::Array + // TODO(#2752): need SSA values here to fetch the len like we do for a Type::Array Err(InternalError::UnExpected { expected: "array".to_owned(), found: ssa_type.to_string(), @@ -842,7 +844,7 @@ impl Context { fn array_set( &mut self, instruction: InstructionId, - var_index: AcirVar, + mut var_index: AcirVar, store_value: AcirValue, dfg: &DataFlowGraph, map_array: bool, @@ -860,7 +862,7 @@ impl Context { } }; - let (array_id, array_typ, block_id) = self.check_array_is_initialized(array, dfg)?; + let (_, array_typ, block_id) = self.check_array_is_initialized(array, dfg)?; // Every array has a length in its type, so we fetch that from // the SSA IR. @@ -920,16 +922,6 @@ impl Context { )?; } - // TODO: Need to add support for dynamic indices with non-homogenous slices - let mut var_index = if matches!(array_typ, Type::Array(_, _)) { - // This is the user facing array len without the element types flattened - let true_array_len = - dfg.try_get_array_length(array_id).expect("ICE: expected an array"); - self.get_flattened_index(&array_typ, array_id, true_array_len, var_index)? - } else { - var_index - }; - let mut is_first_elem = true; self.array_set_value(store_value, result_block_id, &mut var_index, &mut is_first_elem)?; From f2dd702f84fbd3e6a21a8ffacab637736b3490e1 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 21 Sep 2023 14:32:59 +0000 Subject: [PATCH 14/14] remove first_elem param --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 40 +++++++------------ 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 2637d3cf5ac..10b5adc54c7 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -711,13 +711,8 @@ 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 mut is_first_elem = true; - let dummy = self.array_get_value( - &store_type, - block_id, - &mut dummy_predicate_index, - &mut is_first_elem, - )?; + let dummy = + self.array_get_value(&store_type, block_id, &mut dummy_predicate_index)?; Some(self.convert_array_set_store_value(&store_value, &dummy)?) } @@ -788,8 +783,7 @@ impl Context { let results = dfg.instruction_results(instruction); let res_typ = dfg.type_of_value(results[0]); - let mut first_elem = true; - let value = self.array_get_value(&res_typ, block_id, &mut var_index, &mut first_elem)?; + let value = self.array_get_value(&res_typ, block_id, &mut var_index)?; self.define_result(dfg, instruction, value.clone()); @@ -801,17 +795,16 @@ impl Context { ssa_type: &Type, block_id: BlockId, var_index: &mut AcirVar, - first_elem: &mut bool, ) -> Result { let one = self.acir_context.add_constant(FieldElement::one()); match ssa_type.clone() { Type::Numeric(numeric_type) => { - if !*first_elem { - *var_index = self.acir_context.add_var(*var_index, one)?; - } - *first_elem = false; - + // Read the value from the array at the specified index let read = self.acir_context.read_from_memory(block_id, var_index)?; + + // Incremement the var_index in case of a nested array + *var_index = self.acir_context.add_var(*var_index, one)?; + let typ = AcirType::NumericType(numeric_type); Ok(AcirValue::Var(read, typ)) } @@ -819,8 +812,7 @@ 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, first_elem)?); + values.push_back(self.array_get_value(typ, block_id, var_index)?); } } Ok(AcirValue::Array(values)) @@ -922,8 +914,7 @@ impl Context { )?; } - let mut is_first_elem = true; - self.array_set_value(store_value, result_block_id, &mut var_index, &mut is_first_elem)?; + self.array_set_value(store_value, result_block_id, &mut var_index)?; let result_value = AcirValue::DynamicArray(AcirDynamicArray { block_id: result_block_id, len: array_len }); @@ -936,25 +927,22 @@ impl Context { value: AcirValue, block_id: BlockId, var_index: &mut AcirVar, - first_elem: &mut bool, ) -> Result<(), RuntimeError> { let one = self.acir_context.add_constant(FieldElement::one()); match value { AcirValue::Var(store_var, _) => { - if !*first_elem { - *var_index = self.acir_context.add_var(*var_index, one)?; - } - *first_elem = false; // Write the new value into the new array at the specified index self.acir_context.write_to_memory(block_id, var_index, &store_var)?; + // Incremement the var_index in case of a nested array + *var_index = self.acir_context.add_var(*var_index, one)?; } AcirValue::Array(values) => { for value in values { - self.array_set_value(value, block_id, var_index, first_elem)?; + self.array_set_value(value, block_id, var_index)?; } } AcirValue::DynamicArray(_) => { - panic!("ahhh got dyn array for set") + unimplemented!("ICE: setting a dynamic array not supported"); } } Ok(())