From 5ac5bb091403e5b068011576ddd122f09646a665 Mon Sep 17 00:00:00 2001 From: Tom French Date: Tue, 9 Jul 2024 01:17:11 +0100 Subject: [PATCH 1/2] chore: unbundle `check_array_is_initialized` --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 101 +++++++++--------- 1 file changed, 51 insertions(+), 50 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 3b7d2c1025f..fada49c0758 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -975,6 +975,8 @@ impl<'a> Context<'a> { .into()) } }; + // Ensure that array id is fully resolved. + let array = dfg.resolve(array); if self.handle_constant_index(instruction, dfg, index, array, store_value)? { return Ok(()); @@ -984,8 +986,7 @@ 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 array_id = dfg.resolve(array); - let array_typ = dfg.type_of_value(array_id); + let array_typ = dfg.type_of_value(array); // For simplicity we compute the offset only for simple arrays let is_simple_array = dfg.instruction_results(instruction).len() == 1 && can_omit_element_sizes_array(&array_typ); @@ -1108,13 +1109,14 @@ impl<'a> Context<'a> { /// 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, + array_id: ValueId, 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 array_typ = dfg.type_of_value(array_id); + let block_id = self.ensure_array_is_initialized(array_id, dfg)?; let index_var = self.convert_numeric_value(index, dfg)?; let index_var = self.get_flattened_index(&array_typ, array_id, index_var, dfg)?; @@ -1233,22 +1235,22 @@ impl<'a> Context<'a> { dfg: &DataFlowGraph, mut index_side_effect: bool, ) -> Result { - let (array_id, _, block_id) = self.check_array_is_initialized(array, dfg)?; + let block_id = self.ensure_array_is_initialized(array, dfg)?; let results = dfg.instruction_results(instruction); let res_typ = dfg.type_of_value(results[0]); // Get operations to call-data parameters are replaced by a get to the call-data-bus array if let Some(call_data) = self.data_bus.call_data { - if self.data_bus.call_data_map.contains_key(&array_id) { + if self.data_bus.call_data_map.contains_key(&array) { // TODO: the block_id of call-data must be notified to the backend // TODO: should we do the same for return-data? let type_size = res_typ.flattened_size(); let type_size = self.acir_context.add_constant(FieldElement::from(type_size as i128)); let offset = self.acir_context.mul_var(var_index, type_size)?; - let bus_index = self.acir_context.add_constant(FieldElement::from( - self.data_bus.call_data_map[&array_id] as i128, - )); + let bus_index = self + .acir_context + .add_constant(FieldElement::from(self.data_bus.call_data_map[&array] as i128)); let new_index = self.acir_context.add_var(offset, bus_index)?; return self.array_get(instruction, call_data, new_index, dfg, index_side_effect); } @@ -1262,8 +1264,7 @@ impl<'a> Context<'a> { let mut value = self.array_get_value(&res_typ, block_id, &mut var_index)?; if let AcirValue::Var(value_var, typ) = &value { - let array_id = dfg.resolve(array_id); - let array_typ = dfg.type_of_value(array_id); + let array_typ = dfg.type_of_value(array); if let (Type::Numeric(numeric_type), AcirType::NumericType(num)) = (array_typ.first(), typ) { @@ -1347,7 +1348,7 @@ impl<'a> Context<'a> { } }; - let (array_id, array_typ, block_id) = self.check_array_is_initialized(array, dfg)?; + let block_id = self.ensure_array_is_initialized(array, dfg)?; // Every array has a length in its type, so we fetch that from // the SSA IR. @@ -1356,10 +1357,11 @@ impl<'a> Context<'a> { // 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); let array_len = if !array_typ.contains_slice_element() { array_typ.flattened_size() } else { - self.flattened_slice_size(array_id, dfg) + self.flattened_slice_size(array, dfg) }; // Since array_set creates a new array, we create a new block ID for this @@ -1382,18 +1384,13 @@ impl<'a> Context<'a> { self.array_set_value(&store_value, result_block_id, &mut var_index)?; let element_type_sizes = if !can_omit_element_sizes_array(&array_typ) { - let acir_value = self.convert_value(array_id, dfg); - Some(self.init_element_type_sizes_array( - &array_typ, - array_id, - Some(&acir_value), - dfg, - )?) + let acir_value = self.convert_value(array, dfg); + Some(self.init_element_type_sizes_array(&array_typ, array, Some(&acir_value), dfg)?) } else { None }; - let value_types = self.convert_value(array_id, dfg).flat_numeric_types(); + let value_types = self.convert_value(array, dfg).flat_numeric_types(); // Compiler sanity check assert_eq!(value_types.len(), array_len, "ICE: The length of the flattened type array should match the length of the dynamic array"); @@ -1439,37 +1436,33 @@ impl<'a> Context<'a> { Ok(()) } - fn check_array_is_initialized( + fn ensure_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); - + ) -> Result { // Use the SSA ID to get or create its block ID - let block_id = self.block_id(&array_id); + let block_id = self.block_id(&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]; + let value = &dfg[array]; match value { Value::Array { .. } | Value::Instruction { .. } => { - let value = self.convert_value(array_id, dfg); + let value = self.convert_value(array, dfg); + let array_typ = dfg.type_of_value(array); let len = if !array_typ.contains_slice_element() { array_typ.flattened_size() } else { - self.flattened_slice_size(array_id, dfg) + self.flattened_slice_size(array, dfg) }; self.initialize_array(block_id, len, Some(value))?; } _ => { return Err(InternalError::General { - message: format!("Array {array_id} should be initialized"), + message: format!("Array {array} should be initialized"), call_stack: self.acir_context.get_call_stack(), } .into()); @@ -1477,7 +1470,7 @@ impl<'a> Context<'a> { } } - Ok((array_id, array_typ, block_id)) + Ok(block_id) } fn init_element_type_sizes_array( @@ -1785,7 +1778,7 @@ impl<'a> Context<'a> { has_constant_return |= self.acir_context.is_constant(&acir_var); if is_databus { // We do not return value for the data bus. - self.check_array_is_initialized( + self.ensure_array_is_initialized( self.data_bus.return_data.expect( "`is_databus == true` implies `data_bus.return_data` is `Some`", ), @@ -2152,8 +2145,9 @@ impl<'a> Context<'a> { Ok(vec![AcirValue::Var(self.acir_context.add_constant(len), AcirType::field())]) } Intrinsic::AsSlice => { - let (slice_contents, slice_typ, block_id) = - self.check_array_is_initialized(arguments[0], dfg)?; + let slice_contents = arguments[0]; + let slice_typ = dfg.type_of_value(slice_contents); + let block_id = self.ensure_array_is_initialized(slice_contents, dfg)?; assert!(!slice_typ.is_nested_slice(), "ICE: Nested slice used in ACIR generation"); let result_block_id = self.block_id(&result_ids[1]); @@ -2197,8 +2191,9 @@ impl<'a> Context<'a> { Intrinsic::SlicePushBack => { // arguments = [slice_length, slice_contents, ...elements_to_push] let slice_length = self.convert_value(arguments[0], dfg).into_var()?; - let (slice_contents, slice_typ, _) = - self.check_array_is_initialized(arguments[1], dfg)?; + let slice_contents = arguments[1]; + let slice_typ = dfg.type_of_value(slice_contents); + assert!(!slice_typ.is_nested_slice(), "ICE: Nested slice used in ACIR generation"); let slice = self.convert_value(slice_contents, dfg); @@ -2264,9 +2259,8 @@ impl<'a> Context<'a> { Intrinsic::SlicePushFront => { // arguments = [slice_length, slice_contents, ...elements_to_push] let slice_length = self.convert_value(arguments[0], dfg).into_var()?; - - let (slice_contents, slice_typ, _) = - self.check_array_is_initialized(arguments[1], dfg)?; + let slice_contents = arguments[1]; + let slice_typ = dfg.type_of_value(slice_contents); assert!(!slice_typ.is_nested_slice(), "ICE: Nested slice used in ACIR generation"); let slice: AcirValue = self.convert_value(slice_contents, dfg); @@ -2329,6 +2323,7 @@ impl<'a> Context<'a> { Intrinsic::SlicePopBack => { // arguments = [slice_length, slice_contents] let slice_length = self.convert_value(arguments[0], dfg).into_var()?; + let slice_contents = arguments[1]; let one = self.acir_context.add_constant(FieldElement::one()); let new_slice_length = self.acir_context.sub_var(slice_length, one)?; @@ -2337,8 +2332,8 @@ impl<'a> Context<'a> { // the elements stored at that index will no longer be able to be accessed. let mut var_index = new_slice_length; - let (slice_contents, slice_typ, block_id) = - self.check_array_is_initialized(arguments[1], dfg)?; + let slice_typ = dfg.type_of_value(slice_contents); + let block_id = self.ensure_array_is_initialized(slice_contents, dfg)?; assert!(!slice_typ.is_nested_slice(), "ICE: Nested slice used in ACIR generation"); let mut popped_elements = Vec::new(); @@ -2363,9 +2358,11 @@ impl<'a> Context<'a> { Intrinsic::SlicePopFront => { // arguments = [slice_length, slice_contents] let slice_length = self.convert_value(arguments[0], dfg).into_var()?; + let slice_contents = arguments[1]; + + let slice_typ = dfg.type_of_value(slice_contents); + let block_id = self.ensure_array_is_initialized(slice_contents, dfg)?; - let (slice_contents, slice_typ, block_id) = - self.check_array_is_initialized(arguments[1], dfg)?; assert!(!slice_typ.is_nested_slice(), "ICE: Nested slice used in ACIR generation"); let one = self.acir_context.add_constant(FieldElement::one()); @@ -2404,9 +2401,11 @@ impl<'a> Context<'a> { Intrinsic::SliceInsert => { // arguments = [slice_length, slice_contents, insert_index, ...elements_to_insert] let slice_length = self.convert_value(arguments[0], dfg).into_var()?; + let slice_contents = arguments[1]; + + let slice_typ = dfg.type_of_value(slice_contents); + let block_id = self.ensure_array_is_initialized(slice_contents, dfg)?; - let (slice_contents, slice_typ, block_id) = - self.check_array_is_initialized(arguments[1], dfg)?; assert!(!slice_typ.is_nested_slice(), "ICE: Nested slice used in ACIR generation"); let slice = self.convert_value(slice_contents, dfg); @@ -2543,9 +2542,11 @@ impl<'a> Context<'a> { Intrinsic::SliceRemove => { // arguments = [slice_length, slice_contents, remove_index] let slice_length = self.convert_value(arguments[0], dfg).into_var()?; + let slice_contents = arguments[1]; + + let slice_typ = dfg.type_of_value(slice_contents); + let block_id = self.ensure_array_is_initialized(slice_contents, dfg)?; - let (slice_contents, slice_typ, block_id) = - self.check_array_is_initialized(arguments[1], dfg)?; assert!(!slice_typ.is_nested_slice(), "ICE: Nested slice used in ACIR generation"); let slice = self.convert_value(slice_contents, dfg); From 57cace170e42c67db9f2ee2ba62d77d5206635f3 Mon Sep 17 00:00:00 2001 From: Tom French Date: Tue, 9 Jul 2024 10:35:53 +0100 Subject: [PATCH 2/2] chore: remove unnecessary `mut` --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index fada49c0758..ac85952c0bd 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1724,7 +1724,7 @@ impl<'a> Context<'a> { /// Converts an SSA terminator's return values into their ACIR representations fn get_num_return_witnesses( - &mut self, + &self, terminator: &TerminatorInstruction, dfg: &DataFlowGraph, ) -> usize {