From af617c3bc25e9245b1671bb5af62f32ec962d210 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 3 Aug 2023 10:23:07 +0000 Subject: [PATCH 1/7] Do not create new MemoryBlock when not needed --- crates/noirc_evaluator/src/ssa.rs | 3 +- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 68 +++++++++++++------ .../noirc_evaluator/src/ssa/opt/array_use.rs | 51 ++++++++++++++ crates/noirc_evaluator/src/ssa/opt/mod.rs | 1 + 4 files changed, 100 insertions(+), 23 deletions(-) create mode 100644 crates/noirc_evaluator/src/ssa/opt/array_use.rs diff --git a/crates/noirc_evaluator/src/ssa.rs b/crates/noirc_evaluator/src/ssa.rs index c57bb330b09..93e7f730661 100644 --- a/crates/noirc_evaluator/src/ssa.rs +++ b/crates/noirc_evaluator/src/ssa.rs @@ -62,7 +62,8 @@ pub(crate) fn optimize_into_acir( .dead_instruction_elimination() .print(print_ssa_passes, "After Dead Instruction Elimination:"); } - ssa.into_acir(brillig, abi_distinctness) + let array_use = ssa.array_use(); + ssa.into_acir(brillig, abi_distinctness, &array_use) } /// Compiles the Program into ACIR and applies optimizations to the arithmetic gates diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs index f473becd966..24d7d498e16 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -112,9 +112,10 @@ impl Ssa { self, brillig: Brillig, abi_distinctness: AbiDistinctness, + array_use: &HashMap, ) -> Result { let context = Context::new(); - let mut generated_acir = context.convert_ssa(self, brillig)?; + let mut generated_acir = context.convert_ssa(self, brillig, array_use)?; match abi_distinctness { AbiDistinctness::Distinct => { @@ -154,10 +155,15 @@ impl Context { } /// Converts SSA into ACIR - fn convert_ssa(self, ssa: Ssa, brillig: Brillig) -> Result { + fn convert_ssa( + self, + ssa: Ssa, + brillig: Brillig, + array_use: &HashMap, + ) -> Result { let main_func = ssa.main(); match main_func.runtime() { - RuntimeType::Acir => self.convert_acir_main(main_func, &ssa, brillig), + RuntimeType::Acir => self.convert_acir_main(main_func, &ssa, brillig, array_use), RuntimeType::Brillig => self.convert_brillig_main(main_func, brillig), } } @@ -167,13 +173,14 @@ impl Context { main_func: &Function, ssa: &Ssa, brillig: Brillig, + array_use: &HashMap, ) -> Result { let dfg = &main_func.dfg; let entry_block = &dfg[main_func.entry_block()]; let input_witness = self.convert_ssa_block_params(entry_block.parameters(), dfg)?; for instruction_id in entry_block.instructions() { - self.convert_ssa_instruction(*instruction_id, dfg, ssa, &brillig)?; + self.convert_ssa_instruction(*instruction_id, dfg, ssa, &brillig, array_use)?; } self.convert_ssa_return(entry_block.unwrap_terminator(), dfg)?; @@ -310,6 +317,7 @@ impl Context { dfg: &DataFlowGraph, ssa: &Ssa, brillig: &Brillig, + array_use: &HashMap, ) -> Result<(), RuntimeError> { let instruction = &dfg[instruction_id]; self.acir_context.set_location(dfg.get_location(&instruction_id)); @@ -389,10 +397,17 @@ impl Context { self.current_side_effects_enabled_var = acir_var; } Instruction::ArrayGet { array, index } => { - self.handle_array_operation(instruction_id, *array, *index, None, dfg)?; + self.handle_array_operation(instruction_id, *array, *index, None, dfg, array_use)?; } Instruction::ArraySet { array, index, value } => { - self.handle_array_operation(instruction_id, *array, *index, Some(*value), dfg)?; + self.handle_array_operation( + instruction_id, + *array, + *index, + Some(*value), + dfg, + array_use, + )?; } Instruction::Allocate => { unreachable!("Expected all allocate instructions to be removed before acir_gen") @@ -447,6 +462,7 @@ impl Context { index: ValueId, store_value: Option, dfg: &DataFlowGraph, + array_use: &HashMap, ) -> Result<(), RuntimeError> { let index_const = dfg.get_numeric_constant(index); @@ -504,9 +520,10 @@ impl Context { } AcirValue::DynamicArray(_) => (), } - + let resolved_array = dfg.resolve(array); + let map_array = array_use.get(&resolved_array) == Some(&instruction); if let Some(store) = store_value { - self.array_set(instruction, array, index, store, dfg)?; + self.array_set(instruction, array, index, store, dfg, map_array)?; } else { self.array_get(instruction, array, index, dfg)?; } @@ -568,6 +585,7 @@ impl Context { index: ValueId, store_value: ValueId, dfg: &DataFlowGraph, + map_array: bool, ) -> Result<(), InternalError> { // Fetch the internal SSA ID for the array let array = dfg.resolve(array); @@ -602,24 +620,30 @@ impl Context { } // Since array_set creates a new array, we create a new block ID for this - // array. + // array, unless map_array is true. In that case, we operate directly on block_id + // and we do not create a new block ID. let result_id = dfg .instruction_results(instruction) .first() .expect("Array set does not have one result"); - let result_block_id = self.block_id(result_id); - - // Initialize the new array with the values from the old array - let init_values = try_vecmap(0..len, |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))) - })?; - self.initialize_array(result_block_id, len, Some(&init_values))?; + let result_block_id; + if map_array { + self.memory_blocks.insert(*result_id, block_id); + result_block_id = block_id; + } 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..len, |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))) + })?; + self.initialize_array(result_block_id, len, Some(&init_values))?; + } // Write the new value into the new array at the specified index let index_var = self.convert_value(index, dfg).into_var()?; diff --git a/crates/noirc_evaluator/src/ssa/opt/array_use.rs b/crates/noirc_evaluator/src/ssa/opt/array_use.rs new file mode 100644 index 00000000000..6dc2d595081 --- /dev/null +++ b/crates/noirc_evaluator/src/ssa/opt/array_use.rs @@ -0,0 +1,51 @@ +use std::collections::HashMap; + +use crate::ssa::{ + ir::{ + basic_block::BasicBlockId, + dfg::DataFlowGraph, + instruction::{Instruction, InstructionId}, + post_order::PostOrder, + value::ValueId, + }, + ssa_gen::Ssa, +}; + +impl Ssa { + /// Map arrays with the last instruction that uses it + /// For this we simply process all the instructions in execution order + /// and update the map whenever there is a match + pub(crate) fn array_use(&self) -> HashMap { + let mut array_use = HashMap::new(); + for func in self.functions.values() { + let mut reverse_post_order = Vec::new(); + reverse_post_order.extend_from_slice(PostOrder::with_function(func).as_slice()); + reverse_post_order.reverse(); + + for block in reverse_post_order { + last_use(block, &func.dfg, &mut array_use); + } + } + array_use + } +} + +/// Updates the array_def map when an instructions is using an array +fn last_use( + block_id: BasicBlockId, + dfg: &DataFlowGraph, + array_def: &mut HashMap, +) { + let block = &dfg[block_id]; + for instruction_id in block.instructions() { + match &dfg[*instruction_id] { + Instruction::ArrayGet { array, .. } | Instruction::ArraySet { array, .. } => { + let array = dfg.resolve(*array); + array_def.insert(array, *instruction_id); + } + _ => { + // Nothing to do + } + } + } +} diff --git a/crates/noirc_evaluator/src/ssa/opt/mod.rs b/crates/noirc_evaluator/src/ssa/opt/mod.rs index 0d4ad594486..23b4c726a6b 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mod.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mod.rs @@ -3,6 +3,7 @@ //! Each pass is generally expected to mutate the SSA IR into a gradually //! simpler form until the IR only has a single function remaining with 1 block within it. //! Generally, these passes are also expected to minimize the final amount of instructions. +mod array_use; mod constant_folding; mod defunctionalize; mod die; From 3d53f2d9003745a5d1bc795327035fa54d29bee7 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 3 Aug 2023 15:38:09 +0000 Subject: [PATCH 2/7] code review - handle function call --- crates/noirc_evaluator/src/ssa/opt/array_use.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/array_use.rs b/crates/noirc_evaluator/src/ssa/opt/array_use.rs index 6dc2d595081..8b09d883be9 100644 --- a/crates/noirc_evaluator/src/ssa/opt/array_use.rs +++ b/crates/noirc_evaluator/src/ssa/opt/array_use.rs @@ -6,7 +6,7 @@ use crate::ssa::{ dfg::DataFlowGraph, instruction::{Instruction, InstructionId}, post_order::PostOrder, - value::ValueId, + value::{Value, ValueId}, }, ssa_gen::Ssa, }; @@ -43,6 +43,14 @@ fn last_use( let array = dfg.resolve(*array); array_def.insert(array, *instruction_id); } + Instruction::Call { arguments, .. } => { + for argument in arguments { + if matches!(dfg[*argument], Value::Array { .. }) { + let array = dfg.resolve(*argument); + array_def.insert(array, *instruction_id); + } + } + } _ => { // Nothing to do } From 145f2c4b7f535b4a740eee36fedd0279aedbee74 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 3 Aug 2023 15:51:21 +0000 Subject: [PATCH 3/7] code review --- crates/noirc_evaluator/src/ssa/opt/array_use.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/array_use.rs b/crates/noirc_evaluator/src/ssa/opt/array_use.rs index 8b09d883be9..3ac6b663a37 100644 --- a/crates/noirc_evaluator/src/ssa/opt/array_use.rs +++ b/crates/noirc_evaluator/src/ssa/opt/array_use.rs @@ -18,10 +18,8 @@ impl Ssa { pub(crate) fn array_use(&self) -> HashMap { let mut array_use = HashMap::new(); for func in self.functions.values() { - let mut reverse_post_order = Vec::new(); - reverse_post_order.extend_from_slice(PostOrder::with_function(func).as_slice()); + let mut reverse_post_order = PostOrder::with_function(func).into_vec(); reverse_post_order.reverse(); - for block in reverse_post_order { last_use(block, &func.dfg, &mut array_use); } From 95d1d76bdac100826802d8fc796efba45e0561d8 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 3 Aug 2023 16:25:15 +0000 Subject: [PATCH 4/7] code review --- crates/noirc_evaluator/src/ssa/opt/array_use.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/array_use.rs b/crates/noirc_evaluator/src/ssa/opt/array_use.rs index 3ac6b663a37..bcb360de97b 100644 --- a/crates/noirc_evaluator/src/ssa/opt/array_use.rs +++ b/crates/noirc_evaluator/src/ssa/opt/array_use.rs @@ -43,9 +43,9 @@ fn last_use( } Instruction::Call { arguments, .. } => { for argument in arguments { - if matches!(dfg[*argument], Value::Array { .. }) { - let array = dfg.resolve(*argument); - array_def.insert(array, *instruction_id); + let resolved_arg = dfg.resolve(*argument); + if matches!(dfg[resolved_arg], Value::Array { .. }) { + array_def.insert(resolved_arg, *instruction_id); } } } From 6c102000b0979b7b7667af8e8eebce89d0f3baa3 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 3 Aug 2023 17:35:22 +0000 Subject: [PATCH 5/7] add missing commit --- crates/noirc_evaluator/src/ssa/ir/post_order.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/noirc_evaluator/src/ssa/ir/post_order.rs b/crates/noirc_evaluator/src/ssa/ir/post_order.rs index 202f5cff716..e3bdbd491df 100644 --- a/crates/noirc_evaluator/src/ssa/ir/post_order.rs +++ b/crates/noirc_evaluator/src/ssa/ir/post_order.rs @@ -27,6 +27,10 @@ impl PostOrder { PostOrder(Self::compute_post_order(func)) } + pub(crate) fn into_vec(self) -> Vec { + self.0 + } + // Computes the post-order of the function by doing a depth-first traversal of the // function's entry block's previously unvisited children. Each block is sequenced according // to when the traversal exits it. From 8a3bb7cada79cb77eabcd3d00aa5d4239210f100 Mon Sep 17 00:00:00 2001 From: guipublic Date: Fri, 4 Aug 2023 08:02:47 +0000 Subject: [PATCH 6/7] code review --- crates/noirc_evaluator/src/ssa.rs | 4 +-- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 29 ++++++++++++------- .../noirc_evaluator/src/ssa/opt/array_use.rs | 2 +- 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa.rs b/crates/noirc_evaluator/src/ssa.rs index 93e7f730661..fc36f7fe8d0 100644 --- a/crates/noirc_evaluator/src/ssa.rs +++ b/crates/noirc_evaluator/src/ssa.rs @@ -62,8 +62,8 @@ pub(crate) fn optimize_into_acir( .dead_instruction_elimination() .print(print_ssa_passes, "After Dead Instruction Elimination:"); } - let array_use = ssa.array_use(); - ssa.into_acir(brillig, abi_distinctness, &array_use) + let last_array_uses = ssa.find_last_array_uses(); + ssa.into_acir(brillig, abi_distinctness, &last_array_uses) } /// Compiles the Program into ACIR and applies optimizations to the arithmetic gates diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs index 24d7d498e16..c9b2e2feffa 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -112,10 +112,10 @@ impl Ssa { self, brillig: Brillig, abi_distinctness: AbiDistinctness, - array_use: &HashMap, + last_array_uses: &HashMap, ) -> Result { let context = Context::new(); - let mut generated_acir = context.convert_ssa(self, brillig, array_use)?; + let mut generated_acir = context.convert_ssa(self, brillig, last_array_uses)?; match abi_distinctness { AbiDistinctness::Distinct => { @@ -159,11 +159,11 @@ impl Context { self, ssa: Ssa, brillig: Brillig, - array_use: &HashMap, + last_array_uses: &HashMap, ) -> Result { let main_func = ssa.main(); match main_func.runtime() { - RuntimeType::Acir => self.convert_acir_main(main_func, &ssa, brillig, array_use), + RuntimeType::Acir => self.convert_acir_main(main_func, &ssa, brillig, last_array_uses), RuntimeType::Brillig => self.convert_brillig_main(main_func, brillig), } } @@ -173,14 +173,14 @@ impl Context { main_func: &Function, ssa: &Ssa, brillig: Brillig, - array_use: &HashMap, + last_array_uses: &HashMap, ) -> Result { let dfg = &main_func.dfg; let entry_block = &dfg[main_func.entry_block()]; let input_witness = self.convert_ssa_block_params(entry_block.parameters(), dfg)?; for instruction_id in entry_block.instructions() { - self.convert_ssa_instruction(*instruction_id, dfg, ssa, &brillig, array_use)?; + self.convert_ssa_instruction(*instruction_id, dfg, ssa, &brillig, last_array_uses)?; } self.convert_ssa_return(entry_block.unwrap_terminator(), dfg)?; @@ -317,7 +317,7 @@ impl Context { dfg: &DataFlowGraph, ssa: &Ssa, brillig: &Brillig, - array_use: &HashMap, + last_array_uses: &HashMap, ) -> Result<(), RuntimeError> { let instruction = &dfg[instruction_id]; self.acir_context.set_location(dfg.get_location(&instruction_id)); @@ -397,7 +397,14 @@ impl Context { self.current_side_effects_enabled_var = acir_var; } Instruction::ArrayGet { array, index } => { - self.handle_array_operation(instruction_id, *array, *index, None, dfg, array_use)?; + self.handle_array_operation( + instruction_id, + *array, + *index, + None, + dfg, + last_array_uses, + )?; } Instruction::ArraySet { array, index, value } => { self.handle_array_operation( @@ -406,7 +413,7 @@ impl Context { *index, Some(*value), dfg, - array_use, + last_array_uses, )?; } Instruction::Allocate => { @@ -462,7 +469,7 @@ impl Context { index: ValueId, store_value: Option, dfg: &DataFlowGraph, - array_use: &HashMap, + last_array_uses: &HashMap, ) -> Result<(), RuntimeError> { let index_const = dfg.get_numeric_constant(index); @@ -521,7 +528,7 @@ impl Context { AcirValue::DynamicArray(_) => (), } let resolved_array = dfg.resolve(array); - let map_array = array_use.get(&resolved_array) == Some(&instruction); + let map_array = last_array_uses.get(&resolved_array) == Some(&instruction); if let Some(store) = store_value { self.array_set(instruction, array, index, store, dfg, map_array)?; } else { diff --git a/crates/noirc_evaluator/src/ssa/opt/array_use.rs b/crates/noirc_evaluator/src/ssa/opt/array_use.rs index bcb360de97b..5b45d768e1f 100644 --- a/crates/noirc_evaluator/src/ssa/opt/array_use.rs +++ b/crates/noirc_evaluator/src/ssa/opt/array_use.rs @@ -15,7 +15,7 @@ impl Ssa { /// Map arrays with the last instruction that uses it /// For this we simply process all the instructions in execution order /// and update the map whenever there is a match - pub(crate) fn array_use(&self) -> HashMap { + pub(crate) fn find_last_array_uses(&self) -> HashMap { let mut array_use = HashMap::new(); for func in self.functions.values() { let mut reverse_post_order = PostOrder::with_function(func).into_vec(); From 1505aee718e7b47a864d2a308e0cc457f4aea232 Mon Sep 17 00:00:00 2001 From: guipublic Date: Fri, 4 Aug 2023 08:21:19 +0000 Subject: [PATCH 7/7] fix failing test case --- crates/noirc_evaluator/src/ssa/acir_gen/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs index c9b2e2feffa..aca809a85fa 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1121,7 +1121,7 @@ impl Context { #[cfg(test)] mod tests { - use std::rc::Rc; + use std::{rc::Rc, collections::HashMap}; use acvm::{ acir::{ @@ -1161,7 +1161,7 @@ mod tests { let ssa = builder.finish(); let context = Context::new(); - let acir = context.convert_ssa(ssa, Brillig::default()).unwrap(); + let acir = context.convert_ssa(ssa, Brillig::default(), &HashMap::new()).unwrap(); let expected_opcodes = vec![Opcode::Arithmetic(&Expression::one() - &Expression::from(Witness(1)))];