From ed37142800704828b1a965e0439bcbc91e2c3ecb Mon Sep 17 00:00:00 2001 From: guipublic Date: Fri, 3 May 2024 13:23:46 +0000 Subject: [PATCH 1/8] set index and value to 0 for array_get with predicate --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 9 ++++++++- .../Nargo.toml | 7 +++++++ .../Prover.toml | 18 ++++++++++++++++++ .../src/main.nr | 11 +++++++++++ 4 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 test_programs/execution_success/regression_struct_array_conditional/Nargo.toml create mode 100644 test_programs/execution_success/regression_struct_array_conditional/Prover.toml create mode 100644 test_programs/execution_success/regression_struct_array_conditional/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 65cd9c05992..3bda7d0074f 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1200,7 +1200,14 @@ impl<'a> Context<'a> { !res_typ.contains_slice_element(), "ICE: Nested slice result found during ACIR generation" ); - let value = self.array_get_value(&res_typ, block_id, &mut var_index)?; + let mut value = self.array_get_value(&res_typ, block_id, &mut var_index)?; + if let AcirValue::Var(value_var, typ) = value { + // Set the value to 0 if current_side_effects is 0, to ensure it fits in any value type + value = AcirValue::Var( + self.acir_context.mul_var(value_var, self.current_side_effects_enabled_var)?, + typ, + ); + } self.define_result(dfg, instruction, value.clone()); diff --git a/test_programs/execution_success/regression_struct_array_conditional/Nargo.toml b/test_programs/execution_success/regression_struct_array_conditional/Nargo.toml new file mode 100644 index 00000000000..a0587210464 --- /dev/null +++ b/test_programs/execution_success/regression_struct_array_conditional/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_struct_array_conditional" +version = "0.1.0" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/execution_success/regression_struct_array_conditional/Prover.toml b/test_programs/execution_success/regression_struct_array_conditional/Prover.toml new file mode 100644 index 00000000000..ef97f9d482a --- /dev/null +++ b/test_programs/execution_success/regression_struct_array_conditional/Prover.toml @@ -0,0 +1,18 @@ +y = 1 +z = 1 + +[[x]] +value = "0x23de33be058ce5504e1ade738db8bdacfe268fa9dbde777092bf1d38519bdf59" +counter = "10" +dummy = "0" + +[[x]] +value = "3" +counter = "2" +dummy = "0" + +[[x]] +value = "2" +counter = "0" +dummy = "0" + diff --git a/test_programs/execution_success/regression_struct_array_conditional/src/main.nr b/test_programs/execution_success/regression_struct_array_conditional/src/main.nr new file mode 100644 index 00000000000..4f5973ff60c --- /dev/null +++ b/test_programs/execution_success/regression_struct_array_conditional/src/main.nr @@ -0,0 +1,11 @@ +struct foo { + value: Field, + counter: u8, + dummy: u8, +} +fn main(x: [foo; 3], y: u32, z: u32) -> pub u8 { + if z == 0 { + assert(y as u8 < x[y].counter); + } + x[0].counter +} From 6004ef985f463cc7e18f6ac9ffd15e4c8c9e9597 Mon Sep 17 00:00:00 2001 From: guipublic Date: Mon, 6 May 2024 11:56:12 +0000 Subject: [PATCH 2/8] avoid the multiplication when first element matches or if the array is simple --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 78 +++++++++++++++---- compiler/noirc_evaluator/src/ssa/ir/types.rs | 8 ++ 2 files changed, 73 insertions(+), 13 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index ae07e046334..0102783bfd5 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -956,14 +956,42 @@ impl<'a> Context<'a> { if self.handle_constant_index(instruction, dfg, index, array, store_value)? { return Ok(()); } - - let (new_index, new_value) = - self.convert_array_operation_inputs(array, dfg, index, store_value)?; + let mut offset = None; + let array_id = dfg.resolve(array); + let array_typ = dfg.type_of_value(array_id); + if dfg.instruction_results(instruction).len() == 1 { + let result_type = dfg.type_of_value(dfg.instruction_results(instruction)[0]); + if let Type::Array(item_type, _) = array_typ { + // For simplicity we compute the offset only for simple arrays + let mut contains_an_array = false; + for typ in item_type.iter() { + if typ.contains_an_array() { + contains_an_array = true; + break; + } + } + if !contains_an_array { + for (i, typ) in item_type.iter().enumerate() { + if result_type == *typ { + offset = Some(i); + break; + } + } + } + } + } + let (new_index, new_value) = self.convert_array_operation_inputs( + array, + dfg, + index, + store_value, + offset.unwrap_or_default(), + )?; if let Some(new_value) = new_value { self.array_set(instruction, new_index, new_value, dfg, mutable_array_set)?; } else { - self.array_get(instruction, array, new_index, dfg)?; + self.array_get(instruction, array, new_index, dfg, offset.is_some())?; } Ok(()) @@ -1054,6 +1082,7 @@ impl<'a> Context<'a> { /// in case we have a nested array. The index for SSA array operations only represents the flattened index of the current array. /// Thus internal array element type sizes need to be computed to accurately transform the index. /// - predicate_index is 0, or the index if the predicate is true + /// if an offset is specified, predicate_index is index*predicate + (1-predicate)*offset /// - 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. @@ -1063,14 +1092,21 @@ impl<'a> Context<'a> { dfg: &DataFlowGraph, index: ValueId, store_value: Option, + offset: usize, ) -> 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 index_var = self.get_flattened_index(&array_typ, array_id, index_var, dfg)?; - let predicate_index = - self.acir_context.mul_var(index_var, self.current_side_effects_enabled_var)?; + let predicate_index = if offset != 0 { + let offset = self.acir_context.add_constant(offset); + let sub = self.acir_context.sub_var(index_var, offset)?; + let pred = self.acir_context.mul_var(sub, self.current_side_effects_enabled_var)?; + self.acir_context.add_var(pred, offset)? + } else { + self.acir_context.mul_var(index_var, self.current_side_effects_enabled_var)? + }; let new_value = if let Some(store) = store_value { let store_value = self.convert_value(store, dfg); @@ -1171,12 +1207,14 @@ impl<'a> Context<'a> { } /// Generates a read opcode for the array + /// side_effect_free true means that we ensured index will have a type matching the value in the array fn array_get( &mut self, instruction: InstructionId, array: ValueId, mut var_index: AcirVar, dfg: &DataFlowGraph, + mut side_effect_free: bool, ) -> Result { let (array_id, _, block_id) = self.check_array_is_initialized(array, dfg)?; let results = dfg.instruction_results(instruction); @@ -1195,7 +1233,7 @@ impl<'a> Context<'a> { self.data_bus.call_data_map[&array_id] as i128, )); let new_index = self.acir_context.add_var(offset, bus_index)?; - return self.array_get(instruction, call_data, new_index, dfg); + return self.array_get(instruction, call_data, new_index, dfg, side_effect_free); } } @@ -1205,12 +1243,26 @@ impl<'a> Context<'a> { "ICE: Nested slice result found during ACIR generation" ); let mut value = self.array_get_value(&res_typ, block_id, &mut var_index)?; - if let AcirValue::Var(value_var, typ) = value { - // Set the value to 0 if current_side_effects is 0, to ensure it fits in any value type - value = AcirValue::Var( - self.acir_context.mul_var(value_var, self.current_side_effects_enabled_var)?, - typ, - ); + + if let AcirValue::Var(value_var, typ) = &value { + let array_id = dfg.resolve(array_id); + let array_typ = dfg.type_of_value(array_id); + if let Type::Numeric(numeric_type) = array_typ.first() { + if let AcirType::NumericType(num) = typ { + if numeric_type.bit_size() <= num.bit_size() { + // first element is compatible + side_effect_free = true; + } + } + } + // Fallback to multiplication if side_effect has not already been handled + if !side_effect_free { + // Set the value to 0 if current_side_effects is 0, to ensure it fits in any value type + value = AcirValue::Var( + self.acir_context.mul_var(*value_var, self.current_side_effects_enabled_var)?, + typ.clone(), + ); + } } self.define_result(dfg, instruction, value.clone()); diff --git a/compiler/noirc_evaluator/src/ssa/ir/types.rs b/compiler/noirc_evaluator/src/ssa/ir/types.rs index 48036580d29..d72ad487f66 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/types.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/types.rs @@ -166,6 +166,14 @@ impl Type { other => panic!("element_types: Expected array or slice, found {other}"), } } + + pub(crate) fn first(&self) -> Type { + match self { + Type::Numeric(_) | Type::Function => self.clone(), + Type::Reference(typ) => typ.first(), + Type::Slice(element_types) | Type::Array(element_types, _) => element_types[0].first(), + } + } } /// Composite Types are essentially flattened struct or tuple types. From c0c95a669d3f25322a1209a2de405578e30752da Mon Sep 17 00:00:00 2001 From: guipublic Date: Mon, 6 May 2024 14:47:19 +0000 Subject: [PATCH 3/8] update test case --- .../src/main.nr | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/test_programs/execution_success/regression_struct_array_conditional/src/main.nr b/test_programs/execution_success/regression_struct_array_conditional/src/main.nr index 4f5973ff60c..19a0834fc01 100644 --- a/test_programs/execution_success/regression_struct_array_conditional/src/main.nr +++ b/test_programs/execution_success/regression_struct_array_conditional/src/main.nr @@ -1,11 +1,38 @@ -struct foo { +struct foo { value: Field, counter: u8, dummy: u8, } +struct bar { + dummy: [u8;3], + value: Field, + counter: u8, +} +struct bar_field { + dummy: [Field;3], + value: Field, + counter: u8, +} fn main(x: [foo; 3], y: u32, z: u32) -> pub u8 { + let a = [y,z, x[y].counter as u32]; + let mut b = [bar{value: 0, counter: 0, dummy: [0;3]}; 3]; + let mut c = [bar_field{value: 0, counter: 0, dummy: [0;3]}; 3]; + for i in 0..3 { + b[i].value = x[i].value; + b[i].counter = x[i].counter; + b[i].dummy[0] = x[i].dummy; + c[i].value = x[i].value; + c[i].counter = x[i].counter; + c[i].dummy[0] = x[i].dummy as Field; + } if z == 0 { + // offset assert(y as u8 < x[y].counter); + assert(y <= a[y]); + // first element is compatible + assert(y as u8 < b[y].counter); + // fallback + assert(y as u8 < c[y].counter); } x[0].counter } From 7ac72654c24d6dd8b967a64b6a23921e719ef400 Mon Sep 17 00:00:00 2001 From: guipublic Date: Mon, 6 May 2024 17:26:19 +0000 Subject: [PATCH 4/8] Code review --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 51 +++++++++---------- 1 file changed, 24 insertions(+), 27 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 0102783bfd5..184c62cd0a7 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -956,28 +956,25 @@ impl<'a> Context<'a> { if self.handle_constant_index(instruction, dfg, index, array, store_value)? { return Ok(()); } + + // Get an offset such that the type of the array at the offset is the same as the type at the 'index' + // If we find one, we will use it when computing the index under the enable_side_effect predicate + // If not, array_get(..) will use a fallback costing one multiplication in the worst case. let mut offset = None; let array_id = dfg.resolve(array); let array_typ = dfg.type_of_value(array_id); - if dfg.instruction_results(instruction).len() == 1 { + // For simplicity we compute the offset only for simple arrays + if dfg.instruction_results(instruction).len() == 1 + && can_omit_element_sizes_array(&array_typ) + { let result_type = dfg.type_of_value(dfg.instruction_results(instruction)[0]); - if let Type::Array(item_type, _) = array_typ { - // For simplicity we compute the offset only for simple arrays - let mut contains_an_array = false; - for typ in item_type.iter() { - if typ.contains_an_array() { - contains_an_array = true; + if let Type::Array(item_type, _) | Type::Slice(item_type) = array_typ { + for (i, typ) in item_type.iter().enumerate() { + if result_type == *typ { + offset = Some(i); break; } } - if !contains_an_array { - for (i, typ) in item_type.iter().enumerate() { - if result_type == *typ { - offset = Some(i); - break; - } - } - } } } let (new_index, new_value) = self.convert_array_operation_inputs( @@ -991,7 +988,7 @@ impl<'a> Context<'a> { if let Some(new_value) = new_value { self.array_set(instruction, new_index, new_value, dfg, mutable_array_set)?; } else { - self.array_get(instruction, array, new_index, dfg, offset.is_some())?; + self.array_get(instruction, array, new_index, dfg, offset.is_none())?; } Ok(()) @@ -1207,14 +1204,14 @@ impl<'a> Context<'a> { } /// Generates a read opcode for the array - /// side_effect_free true means that we ensured index will have a type matching the value in the array + /// index_side_effect false means that we ensured index will have a type matching the value in the array fn array_get( &mut self, instruction: InstructionId, array: ValueId, mut var_index: AcirVar, dfg: &DataFlowGraph, - mut side_effect_free: bool, + mut index_side_effect: bool, ) -> Result { let (array_id, _, block_id) = self.check_array_is_initialized(array, dfg)?; let results = dfg.instruction_results(instruction); @@ -1233,7 +1230,7 @@ impl<'a> Context<'a> { self.data_bus.call_data_map[&array_id] as i128, )); let new_index = self.acir_context.add_var(offset, bus_index)?; - return self.array_get(instruction, call_data, new_index, dfg, side_effect_free); + return self.array_get(instruction, call_data, new_index, dfg, index_side_effect); } } @@ -1247,16 +1244,16 @@ impl<'a> Context<'a> { if let AcirValue::Var(value_var, typ) = &value { let array_id = dfg.resolve(array_id); let array_typ = dfg.type_of_value(array_id); - if let Type::Numeric(numeric_type) = array_typ.first() { - if let AcirType::NumericType(num) = typ { - if numeric_type.bit_size() <= num.bit_size() { - // first element is compatible - side_effect_free = true; - } + if let (Type::Numeric(numeric_type), AcirType::NumericType(num)) = + (array_typ.first(), typ) + { + if numeric_type.bit_size() <= num.bit_size() { + // first element is compatible + index_side_effect = false; } } - // Fallback to multiplication if side_effect has not already been handled - if !side_effect_free { + // Fallback to multiplication if the index side_effects have not already been handled + if index_side_effect { // Set the value to 0 if current_side_effects is 0, to ensure it fits in any value type value = AcirValue::Var( self.acir_context.mul_var(*value_var, self.current_side_effects_enabled_var)?, From 0ffbfb920500d2fe2f7b91dc2027a6cce39ece34 Mon Sep 17 00:00:00 2001 From: guipublic Date: Mon, 6 May 2024 17:28:55 +0000 Subject: [PATCH 5/8] add the PR in comment --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 184c62cd0a7..ab35f20f09c 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -960,6 +960,7 @@ impl<'a> Context<'a> { // Get an offset such that the type of the array at the offset is the same as the type at the 'index' // If we find one, we will use it when computing the index under the enable_side_effect predicate // If not, array_get(..) will use a fallback costing one multiplication in the worst case. + // cf. https://github.com/noir-lang/noir/pull/4971 let mut offset = None; let array_id = dfg.resolve(array); let array_typ = dfg.type_of_value(array_id); From 0a0cb7963d243ffcb73bf74aad89be2cbe13d3c3 Mon Sep 17 00:00:00 2001 From: guipublic Date: Mon, 6 May 2024 17:50:28 +0000 Subject: [PATCH 6/8] nargo fmt --- .../regression_struct_array_conditional/src/main.nr | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test_programs/execution_success/regression_struct_array_conditional/src/main.nr b/test_programs/execution_success/regression_struct_array_conditional/src/main.nr index 19a0834fc01..17502a9fe50 100644 --- a/test_programs/execution_success/regression_struct_array_conditional/src/main.nr +++ b/test_programs/execution_success/regression_struct_array_conditional/src/main.nr @@ -14,9 +14,9 @@ struct bar_field { counter: u8, } fn main(x: [foo; 3], y: u32, z: u32) -> pub u8 { - let a = [y,z, x[y].counter as u32]; - let mut b = [bar{value: 0, counter: 0, dummy: [0;3]}; 3]; - let mut c = [bar_field{value: 0, counter: 0, dummy: [0;3]}; 3]; + let a = [y, z, x[y].counter as u32]; + let mut b = [bar { value: 0, counter: 0, dummy: [0; 3] }; 3]; + let mut c = [bar_field { value: 0, counter: 0, dummy: [0; 3] }; 3]; for i in 0..3 { b[i].value = x[i].value; b[i].counter = x[i].counter; From 61e94a5710812660dfd2b29684f050f83c4133b1 Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 7 May 2024 08:29:27 +0000 Subject: [PATCH 7/8] code review --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index ab35f20f09c..a232727ae3c 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -961,23 +961,23 @@ impl<'a> Context<'a> { // If we find one, we will use it when computing the index under the enable_side_effect predicate // If not, array_get(..) will use a fallback costing one multiplication in the worst case. // cf. https://github.com/noir-lang/noir/pull/4971 - let mut offset = None; let array_id = dfg.resolve(array); let array_typ = dfg.type_of_value(array_id); // For simplicity we compute the offset only for simple arrays - if dfg.instruction_results(instruction).len() == 1 - && can_omit_element_sizes_array(&array_typ) - { + let is_simple_array = dfg.instruction_results(instruction).len() == 1 + && can_omit_element_sizes_array(&array_typ); + let offset = if is_simple_array { let result_type = dfg.type_of_value(dfg.instruction_results(instruction)[0]); - if let Type::Array(item_type, _) | Type::Slice(item_type) = array_typ { - for (i, typ) in item_type.iter().enumerate() { - if result_type == *typ { - offset = Some(i); - break; - } - } + match array_typ { + Type::Array(item_type, _) | Type::Slice(item_type) => item_type + .iter() + .enumerate() + .find_map(|(index, typ)| (result_type == *typ).then_some(index)), + _ => None, } - } + } else { + None + }; let (new_index, new_value) = self.convert_array_operation_inputs( array, dfg, @@ -1205,7 +1205,7 @@ impl<'a> Context<'a> { } /// Generates a read opcode for the array - /// index_side_effect false means that we ensured index will have a type matching the value in the array + /// `index_side_effect == false` means that we ensured `var_index` will have a type matching the value in the array fn array_get( &mut self, instruction: InstructionId, From c4884ca883c95eb8fea0ef84a838a85c6bea0281 Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 7 May 2024 08:36:04 +0000 Subject: [PATCH 8/8] code review --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index a232727ae3c..16334052d27 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1079,8 +1079,7 @@ impl<'a> Context<'a> { /// - new_index is the index of the array. ACIR memory operations work with a flat memory, so we fully flattened the specified index /// in case we have a nested array. The index for SSA array operations only represents the flattened index of the current array. /// Thus internal array element type sizes need to be computed to accurately transform the index. - /// - predicate_index is 0, or the index if the predicate is true - /// if an offset is specified, predicate_index is index*predicate + (1-predicate)*offset + /// - predicate_index is offset, 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. @@ -1097,14 +1096,11 @@ impl<'a> Context<'a> { let index_var = self.convert_numeric_value(index, dfg)?; let index_var = self.get_flattened_index(&array_typ, array_id, index_var, dfg)?; - let predicate_index = if offset != 0 { - let offset = self.acir_context.add_constant(offset); - let sub = self.acir_context.sub_var(index_var, offset)?; - let pred = self.acir_context.mul_var(sub, self.current_side_effects_enabled_var)?; - self.acir_context.add_var(pred, offset)? - } else { - self.acir_context.mul_var(index_var, self.current_side_effects_enabled_var)? - }; + // predicate_index = index*predicate + (1-predicate)*offset + let offset = self.acir_context.add_constant(offset); + let sub = self.acir_context.sub_var(index_var, offset)?; + let pred = self.acir_context.mul_var(sub, self.current_side_effects_enabled_var)?; + let predicate_index = self.acir_context.add_var(pred, offset)?; let new_value = if let Some(store) = store_value { let store_value = self.convert_value(store, dfg);