From e0ad0a9d3054d41ae441c081cc2a412e96847ba5 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 29 Aug 2024 21:04:41 +0000 Subject: [PATCH 01/19] remove trivial stores --- compiler/noirc_evaluator/src/ssa.rs | 5 +++++ compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 12 +++++++++--- .../noirc_evaluator/src/ssa/opt/mem2reg/block.rs | 5 ++++- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 2d138c13f7f..d3d625964e9 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -117,6 +117,11 @@ pub(crate) fn optimize_into_acir( .run_pass(Ssa::remove_enable_side_effects, "After EnableSideEffectsIf removal:") .run_pass(Ssa::fold_constants_using_constraints, "After Constraint Folding:") .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") + // TODO: mem2reg and DIE were put here to delete any stores that can be removes following die removing loads. + // Decide whether we want to run mem2reg once more or just have it able to handle this case on its own + .run_pass(Ssa::mem2reg, "After Mem2Reg:") + // The extra mem2reg requires another DIE as we can have allocates we do not want + .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") .run_pass(Ssa::array_set_optimization, "After Array Set Optimizations:") .finish(); diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 9d6582c0db7..1701df03502 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -195,7 +195,7 @@ impl<'f> PerFunctionContext<'f> { } self.handle_terminator(block, &mut references); - + // If there's only 1 block in the function total, we can remove any remaining last stores // as well. We can't do this if there are multiple blocks since subsequent blocks may // reference these stores. @@ -253,7 +253,6 @@ impl<'f> PerFunctionContext<'f> { // If the load is known, replace it with the known value and remove the load if let Some(value) = references.get_known_value(address) { - let result = self.inserter.function.dfg.instruction_results(instruction)[0]; self.inserter.map_value(result, value); self.instructions_to_remove.insert(instruction); } else { @@ -272,10 +271,17 @@ impl<'f> PerFunctionContext<'f> { // function calls in-between, we can remove the previous store. if let Some(last_store) = references.last_stores.get(&address) { self.instructions_to_remove.insert(*last_store); + } + + if let Some(last_load) = self.last_loads.get(&address) { + let load_result = self.inserter.function.dfg.instruction_results(*last_load)[0]; + if load_result == value { + self.instructions_to_remove.insert(instruction); + } } references.set_known_value(address, value); - references.last_stores.insert(address, instruction); + references.last_stores.insert(address, instruction); } Instruction::Allocate => { // Register the new reference diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs index 532785d2928..54a7eb58067 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs @@ -163,7 +163,10 @@ impl Block { // even if we don't have a known address? If not we'll have to invalidate all // known references if this reference is ever stored to. } - } + } + // else { + // self.references.insert(result, ReferenceValue::Known(address)); + // } } /// Iterate through each known alias of the given address and apply the function `f` to each. From 5bbaca85ecc267fd3602c138ace66e5388e9ca1d Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 29 Aug 2024 21:09:14 +0000 Subject: [PATCH 02/19] fmt --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 6 +++--- compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 1701df03502..aebf9abbe50 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -195,7 +195,7 @@ impl<'f> PerFunctionContext<'f> { } self.handle_terminator(block, &mut references); - + // If there's only 1 block in the function total, we can remove any remaining last stores // as well. We can't do this if there are multiple blocks since subsequent blocks may // reference these stores. @@ -271,7 +271,7 @@ impl<'f> PerFunctionContext<'f> { // function calls in-between, we can remove the previous store. if let Some(last_store) = references.last_stores.get(&address) { self.instructions_to_remove.insert(*last_store); - } + } if let Some(last_load) = self.last_loads.get(&address) { let load_result = self.inserter.function.dfg.instruction_results(*last_load)[0]; @@ -281,7 +281,7 @@ impl<'f> PerFunctionContext<'f> { } references.set_known_value(address, value); - references.last_stores.insert(address, instruction); + references.last_stores.insert(address, instruction); } Instruction::Allocate => { // Register the new reference diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs index 54a7eb58067..ca11a4ec57f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs @@ -163,7 +163,7 @@ impl Block { // even if we don't have a known address? If not we'll have to invalidate all // known references if this reference is ever stored to. } - } + } // else { // self.references.insert(result, ReferenceValue::Known(address)); // } From a66009ad8588d6bc6bfc70b67bbd9f545ca9f025 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 29 Aug 2024 21:37:35 +0000 Subject: [PATCH 03/19] don't include arrays in last store --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 10 ++++++---- compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs | 3 --- .../brillig_loop_size_regression/src/main.nr | 3 ++- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index aebf9abbe50..2149043094a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -116,7 +116,7 @@ struct PerFunctionContext<'f> { /// Track a value's last load across all blocks. /// If a value is not used in anymore loads we can remove the last store to that value. - last_loads: HashMap, + last_loads: HashMap, } impl<'f> PerFunctionContext<'f> { @@ -258,7 +258,7 @@ impl<'f> PerFunctionContext<'f> { } else { references.mark_value_used(address, self.inserter.function); - self.last_loads.insert(address, instruction); + self.last_loads.insert(address, (instruction, block_id)); } } Instruction::Store { address, value } => { @@ -273,9 +273,11 @@ impl<'f> PerFunctionContext<'f> { self.instructions_to_remove.insert(*last_store); } - if let Some(last_load) = self.last_loads.get(&address) { + if let Some((last_load, last_load_block)) = self.last_loads.get(&address) { let load_result = self.inserter.function.dfg.instruction_results(*last_load)[0]; - if load_result == value { + let contains_array = + self.inserter.function.dfg.type_of_value(value).contains_an_array(); + if load_result == value && *last_load_block == block_id && !contains_array { self.instructions_to_remove.insert(instruction); } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs index ca11a4ec57f..532785d2928 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs @@ -164,9 +164,6 @@ impl Block { // known references if this reference is ever stored to. } } - // else { - // self.references.insert(result, ReferenceValue::Known(address)); - // } } /// Iterate through each known alias of the given address and apply the function `f` to each. diff --git a/test_programs/execution_success/brillig_loop_size_regression/src/main.nr b/test_programs/execution_success/brillig_loop_size_regression/src/main.nr index 488304114b9..ad9d49debeb 100644 --- a/test_programs/execution_success/brillig_loop_size_regression/src/main.nr +++ b/test_programs/execution_success/brillig_loop_size_regression/src/main.nr @@ -7,8 +7,9 @@ struct EnumEmulation { unconstrained fn main() -> pub Field { let mut emulated_enum = EnumEmulation { a: Option::some(1), b: Option::none(), c: Option::none() }; + let copy_enum = emulated_enum; for _ in 0..1 { - assert_eq(emulated_enum.a.unwrap(), 1); + assert_eq(copy_enum.a.unwrap(), 1); } emulated_enum.a = Option::some(2); From ce31cb0208837e40ee1c9441882dcc052cb2b572 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 30 Aug 2024 14:14:35 +0000 Subject: [PATCH 04/19] switch to using get_known_value and set_known_value --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 23 +++++++++++++++---- .../brillig_loop_size_regression/src/main.nr | 14 +++++++++-- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 2149043094a..fc70e8c01c0 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -258,6 +258,10 @@ impl<'f> PerFunctionContext<'f> { } else { references.mark_value_used(address, self.inserter.function); + references.expressions.insert(result, Expression::Other(result)); + references.aliases.insert(Expression::Other(result), AliasSet::known(result)); + references.set_known_value(result, address); + self.last_loads.insert(address, (instruction, block_id)); } } @@ -273,11 +277,20 @@ impl<'f> PerFunctionContext<'f> { self.instructions_to_remove.insert(*last_store); } - if let Some((last_load, last_load_block)) = self.last_loads.get(&address) { - let load_result = self.inserter.function.dfg.instruction_results(*last_load)[0]; - let contains_array = - self.inserter.function.dfg.type_of_value(value).contains_an_array(); - if load_result == value && *last_load_block == block_id && !contains_array { + // NOTE: This causes issues for brillig_cow_assign, and uhashmap + // if let Some((last_load, last_load_block)) = self.last_loads.get(&address) { + // let load_result = self.inserter.function.dfg.instruction_results(*last_load)[0]; + // NOTE: Checking !contains_array fixes brillig_cow_assign but not uhashmap + // // let contains_array = + // // self.inserter.function.dfg.type_of_value(value).contains_an_array(); + // if load_result == value && *last_load_block == block_id { + // self.instructions_to_remove.insert(instruction); + // } + // } + + // NOTE: This causes the same failures as the last_loads approach + if let Some(known_value) = references.get_known_value(value) { + if known_value == address { self.instructions_to_remove.insert(instruction); } } diff --git a/test_programs/execution_success/brillig_loop_size_regression/src/main.nr b/test_programs/execution_success/brillig_loop_size_regression/src/main.nr index ad9d49debeb..b367702abf8 100644 --- a/test_programs/execution_success/brillig_loop_size_regression/src/main.nr +++ b/test_programs/execution_success/brillig_loop_size_regression/src/main.nr @@ -7,11 +7,21 @@ struct EnumEmulation { unconstrained fn main() -> pub Field { let mut emulated_enum = EnumEmulation { a: Option::some(1), b: Option::none(), c: Option::none() }; - let copy_enum = emulated_enum; + // Do this to optimize out loads in the loop + // let copy_enum = emulated_enum; for _ in 0..1 { - assert_eq(copy_enum.a.unwrap(), 1); + assert_eq(emulated_enum.a.unwrap(), 1); } + // let foo = &mut 0; + // let alias = foo; + + // let foo_load1 = *foo; + // *alias = 7; + // *foo = foo_load1; // This looks like it'd be optimized out + + // assert_eq(*foo, 7); + emulated_enum.a = Option::some(2); emulated_enum.a.unwrap() } From 45db90b97cfb23ad55b32d24cb42cc5bbfbad3e2 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 30 Aug 2024 16:52:12 +0000 Subject: [PATCH 05/19] add from_rc flag to store --- .../src/brillig/brillig_gen/brillig_block.rs | 2 +- compiler/noirc_evaluator/src/ssa.rs | 1 + .../noirc_evaluator/src/ssa/function_builder/mod.rs | 8 ++++++-- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 8 ++++---- compiler/noirc_evaluator/src/ssa/ir/printer.rs | 4 ++-- compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs | 8 ++++---- .../src/ssa/opt/flatten_cfg/capacity_tracker.rs | 2 +- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 12 ++++++------ .../execution_success/brillig_cow_assign/src/main.nr | 5 +++++ .../brillig_loop_size_regression/src/main.nr | 9 --------- 10 files changed, 30 insertions(+), 29 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 1e672eeea3c..d9dd27e5668 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -315,7 +315,7 @@ impl<'block> BrilligBlock<'block> { } } } - Instruction::Store { address, value } => { + Instruction::Store { address, value , .. } => { let address_var = self.convert_ssa_single_addr_value(*address, dfg); let source_variable = self.convert_ssa_value(*value, dfg); diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index d3d625964e9..db82186f1d1 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -96,6 +96,7 @@ pub(crate) fn optimize_into_acir( .run_pass(Ssa::inline_functions, "After Inlining:") // Run mem2reg with the CFG separated into blocks .run_pass(Ssa::mem2reg, "After Mem2Reg:") + // .run_pass(Ssa::remove_paired_rc, "After Removing Paired rc_inc & rc_decs:") .run_pass(Ssa::as_slice_optimization, "After `as_slice` optimization") .try_run_pass( Ssa::evaluate_static_assert_and_assert_constant, diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index bf6430c36d7..78958add25e 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -219,7 +219,11 @@ impl FunctionBuilder { /// at the given address. Expects that the address points somewhere /// within a previous Allocate instruction. pub(crate) fn insert_store(&mut self, address: ValueId, value: ValueId) { - self.insert_instruction(Instruction::Store { address, value }, None); + self.insert_instruction(Instruction::Store { address, value, from_rc: false }, None); + } + + pub(crate) fn insert_store_from_rc(&mut self, address: ValueId, value: ValueId) { + self.insert_instruction(Instruction::Store { address, value, from_rc: true }, None); } /// Insert a binary instruction at the end of the current block. @@ -478,7 +482,7 @@ impl FunctionBuilder { let address = *address; let new_load = self.insert_load(address, typ); update_rc(self, new_load); - self.insert_store(address, new_load); + self.insert_store_from_rc(address, new_load); } } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 36069f17933..fa3a422837b 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -215,7 +215,7 @@ pub(crate) enum Instruction { Load { address: ValueId }, /// Writes a value to memory. - Store { address: ValueId, value: ValueId }, + Store { address: ValueId, value: ValueId, from_rc: bool }, /// Provides a context for all instructions that follow up until the next /// `EnableSideEffectsIf` is encountered, for stating a condition that determines whether @@ -484,8 +484,8 @@ impl Instruction { }, Instruction::Allocate => Instruction::Allocate, Instruction::Load { address } => Instruction::Load { address: f(*address) }, - Instruction::Store { address, value } => { - Instruction::Store { address: f(*address), value: f(*value) } + Instruction::Store { address, value, from_rc } => { + Instruction::Store { address: f(*address), value: f(*value), from_rc: *from_rc } } Instruction::EnableSideEffectsIf { condition } => { Instruction::EnableSideEffectsIf { condition: f(*condition) } @@ -548,7 +548,7 @@ impl Instruction { } } - Instruction::Store { address, value } => { + Instruction::Store { address, value, .. } => { f(*address); f(*value); } diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index e8c9d01988e..a2a21b2d53e 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -173,8 +173,8 @@ fn display_instruction_inner( } Instruction::Allocate => writeln!(f, "allocate"), Instruction::Load { address } => writeln!(f, "load {}", show(*address)), - Instruction::Store { address, value } => { - writeln!(f, "store {} at {}", show(*value), show(*address)) + Instruction::Store { address, value, from_rc } => { + writeln!(f, "store {} at {}, from_rc {}", show(*value), show(*address), *from_rc) } Instruction::EnableSideEffectsIf { condition } => { writeln!(f, "enable_side_effects {}", show(*condition)) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index d5fb98c7adc..3c0ff02d223 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -647,7 +647,7 @@ impl<'f> Context<'f> { let value = new_values[address]; let address = *address; self.insert_instruction_with_typevars( - Instruction::Store { address, value }, + Instruction::Store { address, value, from_rc: false }, None, call_stack.clone(), ); @@ -740,9 +740,9 @@ impl<'f> Context<'f> { Instruction::Constrain(lhs, rhs, message) } - Instruction::Store { address, value } => { + Instruction::Store { address, value, from_rc } => { self.remember_store(address, value, call_stack); - Instruction::Store { address, value } + Instruction::Store { address, value, from_rc } } Instruction::RangeCheck { value, max_bit_size, assert_message } => { // Replace value with `value * predicate` to zero out value when predicate is inactive. @@ -869,7 +869,7 @@ impl<'f> Context<'f> { for (address, store) in store_values { let address = *address; let value = store.old_value; - let instruction = Instruction::Store { address, value }; + let instruction = Instruction::Store { address, value, from_rc: false, }; // Considering the location of undoing a store to be the same as the original store. self.insert_instruction_with_typevars(instruction, None, store.call_stack.clone()); } diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs index f0760f29006..f50acb4696d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs @@ -133,7 +133,7 @@ impl<'a> SliceCapacityTracker<'a> { } } } - Instruction::Store { address, value } => { + Instruction::Store { address, value, .. } => { let value_typ = self.dfg.type_of_value(*value); if value_typ.contains_slice_element() { self.compute_slice_capacity(*value, slice_sizes); diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index fc70e8c01c0..831534afb55 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -265,7 +265,7 @@ impl<'f> PerFunctionContext<'f> { self.last_loads.insert(address, (instruction, block_id)); } } - Instruction::Store { address, value } => { + Instruction::Store { address, value, from_rc } => { let address = self.inserter.function.dfg.resolve(*address); let value = self.inserter.function.dfg.resolve(*value); @@ -280,17 +280,17 @@ impl<'f> PerFunctionContext<'f> { // NOTE: This causes issues for brillig_cow_assign, and uhashmap // if let Some((last_load, last_load_block)) = self.last_loads.get(&address) { // let load_result = self.inserter.function.dfg.instruction_results(*last_load)[0]; - // NOTE: Checking !contains_array fixes brillig_cow_assign but not uhashmap + // // NOTE: Checking !contains_array fixes brillig_cow_assign but not uhashmap // // let contains_array = // // self.inserter.function.dfg.type_of_value(value).contains_an_array(); - // if load_result == value && *last_load_block == block_id { - // self.instructions_to_remove.insert(instruction); - // } + // if load_result == value && *last_load_block == block_id { + // self.instructions_to_remove.insert(instruction); + // } // } // NOTE: This causes the same failures as the last_loads approach if let Some(known_value) = references.get_known_value(value) { - if known_value == address { + if known_value == address && !from_rc { self.instructions_to_remove.insert(instruction); } } diff --git a/test_programs/execution_success/brillig_cow_assign/src/main.nr b/test_programs/execution_success/brillig_cow_assign/src/main.nr index e5c3e2bd2f5..9bcee9e96de 100644 --- a/test_programs/execution_success/brillig_cow_assign/src/main.nr +++ b/test_programs/execution_success/brillig_cow_assign/src/main.nr @@ -6,8 +6,13 @@ unconstrained fn main() { for i in 0..N { if i == N / 2 { + // println("inside if"); + // println(arr); mid_change = arr; + // println(mid_change); } + // println(i); + // println(mid_change); arr[i] = 27; } diff --git a/test_programs/execution_success/brillig_loop_size_regression/src/main.nr b/test_programs/execution_success/brillig_loop_size_regression/src/main.nr index b367702abf8..c5c8f87f450 100644 --- a/test_programs/execution_success/brillig_loop_size_regression/src/main.nr +++ b/test_programs/execution_success/brillig_loop_size_regression/src/main.nr @@ -13,15 +13,6 @@ unconstrained fn main() -> pub Field { assert_eq(emulated_enum.a.unwrap(), 1); } - // let foo = &mut 0; - // let alias = foo; - - // let foo_load1 = *foo; - // *alias = 7; - // *foo = foo_load1; // This looks like it'd be optimized out - - // assert_eq(*foo, 7); - emulated_enum.a = Option::some(2); emulated_enum.a.unwrap() } From 96b70bdc8ec8365849963c4ec489ea8a75198b57 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 30 Aug 2024 16:54:19 +0000 Subject: [PATCH 06/19] cargo fmt --- .../noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs | 2 +- compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs | 2 +- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index d9dd27e5668..ef447ac5a15 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -315,7 +315,7 @@ impl<'block> BrilligBlock<'block> { } } } - Instruction::Store { address, value , .. } => { + Instruction::Store { address, value, .. } => { let address_var = self.convert_ssa_single_addr_value(*address, dfg); let source_variable = self.convert_ssa_value(*value, dfg); diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 3c0ff02d223..d462a600503 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -869,7 +869,7 @@ impl<'f> Context<'f> { for (address, store) in store_values { let address = *address; let value = store.old_value; - let instruction = Instruction::Store { address, value, from_rc: false, }; + let instruction = Instruction::Store { address, value, from_rc: false }; // Considering the location of undoing a store to be the same as the original store. self.insert_instruction_with_typevars(instruction, None, store.call_stack.clone()); } diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 831534afb55..645095c4383 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -277,7 +277,7 @@ impl<'f> PerFunctionContext<'f> { self.instructions_to_remove.insert(*last_store); } - // NOTE: This causes issues for brillig_cow_assign, and uhashmap + // NOTE: This causes issues for brillig_cow_assign, and uhashmap // if let Some((last_load, last_load_block)) = self.last_loads.get(&address) { // let load_result = self.inserter.function.dfg.instruction_results(*last_load)[0]; // // NOTE: Checking !contains_array fixes brillig_cow_assign but not uhashmap From 82bc2ea0126dce2e7bc7e2a07605e7dec63d3da1 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 30 Aug 2024 19:23:10 +0000 Subject: [PATCH 07/19] don't call mem2reg and die again to fix uhashmap --- compiler/noirc_evaluator/src/ssa.rs | 4 ++-- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- tooling/nargo/src/errors.rs | 13 +++++++------ tooling/nargo/src/ops/execute.rs | 8 ++++++++ 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index db82186f1d1..fb0c6d50277 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -120,9 +120,9 @@ pub(crate) fn optimize_into_acir( .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") // TODO: mem2reg and DIE were put here to delete any stores that can be removes following die removing loads. // Decide whether we want to run mem2reg once more or just have it able to handle this case on its own - .run_pass(Ssa::mem2reg, "After Mem2Reg:") + // .run_pass(Ssa::mem2reg, "After Mem2Reg:") // The extra mem2reg requires another DIE as we can have allocates we do not want - .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") + // .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") .run_pass(Ssa::array_set_optimization, "After Array Set Optimizations:") .finish(); diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 645095c4383..56ba4dbbf85 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -288,7 +288,7 @@ impl<'f> PerFunctionContext<'f> { // } // } - // NOTE: This causes the same failures as the last_loads approach + // // NOTE: This causes the same failures as the last_loads approach if let Some(known_value) = references.get_known_value(value) { if known_value == address && !from_rc { self.instructions_to_remove.insert(instruction); diff --git a/tooling/nargo/src/errors.rs b/tooling/nargo/src/errors.rs index b5571ff7758..93e36d4728b 100644 --- a/tooling/nargo/src/errors.rs +++ b/tooling/nargo/src/errors.rs @@ -2,8 +2,7 @@ use std::collections::BTreeMap; use acvm::{ acir::circuit::{ - ErrorSelector, OpcodeLocation, RawAssertionPayload, ResolvedAssertionPayload, - ResolvedOpcodeLocation, + brillig::BrilligFunctionId, ErrorSelector, OpcodeLocation, RawAssertionPayload, ResolvedAssertionPayload, ResolvedOpcodeLocation }, pwg::{ErrorLocation, OpcodeResolutionError}, AcirField, FieldElement, @@ -66,7 +65,7 @@ impl NargoError { ) -> Option { match self { NargoError::ExecutionError(error) => match error { - ExecutionError::AssertionFailed(payload, _) => match payload { + ExecutionError::AssertionFailed(payload, _, _) => match payload { ResolvedAssertionPayload::String(message) => Some(message.to_string()), ResolvedAssertionPayload::Raw(raw) => { let abi_type = error_types.get(&raw.selector)?; @@ -90,7 +89,7 @@ impl NargoError { #[derive(Debug, Error)] pub enum ExecutionError { #[error("Failed assertion")] - AssertionFailed(ResolvedAssertionPayload, Vec), + AssertionFailed(ResolvedAssertionPayload, Vec, Option), #[error("Failed to solve program: '{}'", .0)] SolvingError(OpcodeResolutionError, Option>), @@ -106,7 +105,7 @@ fn extract_locations_from_error( OpcodeResolutionError::BrilligFunctionFailed { .. }, acir_call_stack, ) => acir_call_stack.clone(), - ExecutionError::AssertionFailed(_, call_stack) => Some(call_stack.clone()), + ExecutionError::AssertionFailed(_, call_stack, _) => Some(call_stack.clone()), ExecutionError::SolvingError( OpcodeResolutionError::IndexOutOfBounds { opcode_location: error_location, .. }, acir_call_stack, @@ -148,6 +147,7 @@ fn extract_locations_from_error( OpcodeResolutionError::BrilligFunctionFailed { function_id, .. }, _, ) => Some(*function_id), + ExecutionError::AssertionFailed(_, _, function_id) => *function_id, _ => None, }; @@ -186,7 +186,7 @@ fn extract_message_from_error( match nargo_err { NargoError::ExecutionError(ExecutionError::AssertionFailed( ResolvedAssertionPayload::String(message), - _, + _, _, )) => { format!("Assertion failed: '{message}'") } @@ -226,6 +226,7 @@ pub fn try_to_diagnose_runtime_error( } _ => return None, }; + // The location of the error itself will be the location at the top // of the call stack (the last item in the Vec). let location = source_locations.last()?; diff --git a/tooling/nargo/src/ops/execute.rs b/tooling/nargo/src/ops/execute.rs index 5a43b1c2f9c..f0a30f2de57 100644 --- a/tooling/nargo/src/ops/execute.rs +++ b/tooling/nargo/src/ops/execute.rs @@ -117,10 +117,18 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver, E: ForeignCallExecutor> _ => None, }; + let brillig_function_id = match &error { + OpcodeResolutionError::BrilligFunctionFailed { function_id, .. } => { + Some(*function_id) + } + _ => None, + }; + return Err(NargoError::ExecutionError(match assertion_payload { Some(payload) => ExecutionError::AssertionFailed( payload, call_stack.expect("Should have call stack for an assertion failure"), + brillig_function_id, ), None => ExecutionError::SolvingError(error, call_stack), })); From 6ce03eb7f20cfa099dfebde04307f7c3780c7ba2 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 30 Aug 2024 19:33:25 +0000 Subject: [PATCH 08/19] cargo fmt --- compiler/noirc_evaluator/src/ssa.rs | 5 +++-- tooling/nargo/src/errors.rs | 12 +++++++++--- tooling/nargo/src/ops/execute.rs | 2 +- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index fb0c6d50277..3f933814ba8 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -120,9 +120,10 @@ pub(crate) fn optimize_into_acir( .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") // TODO: mem2reg and DIE were put here to delete any stores that can be removes following die removing loads. // Decide whether we want to run mem2reg once more or just have it able to handle this case on its own - // .run_pass(Ssa::mem2reg, "After Mem2Reg:") + // NOTE: Running these two passes causes uhashmap to fail with a type mismatch: + .run_pass(Ssa::mem2reg, "After Mem2Reg:") // The extra mem2reg requires another DIE as we can have allocates we do not want - // .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") + .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") .run_pass(Ssa::array_set_optimization, "After Array Set Optimizations:") .finish(); diff --git a/tooling/nargo/src/errors.rs b/tooling/nargo/src/errors.rs index 93e36d4728b..05474195600 100644 --- a/tooling/nargo/src/errors.rs +++ b/tooling/nargo/src/errors.rs @@ -2,7 +2,8 @@ use std::collections::BTreeMap; use acvm::{ acir::circuit::{ - brillig::BrilligFunctionId, ErrorSelector, OpcodeLocation, RawAssertionPayload, ResolvedAssertionPayload, ResolvedOpcodeLocation + brillig::BrilligFunctionId, ErrorSelector, OpcodeLocation, RawAssertionPayload, + ResolvedAssertionPayload, ResolvedOpcodeLocation, }, pwg::{ErrorLocation, OpcodeResolutionError}, AcirField, FieldElement, @@ -89,7 +90,11 @@ impl NargoError { #[derive(Debug, Error)] pub enum ExecutionError { #[error("Failed assertion")] - AssertionFailed(ResolvedAssertionPayload, Vec, Option), + AssertionFailed( + ResolvedAssertionPayload, + Vec, + Option, + ), #[error("Failed to solve program: '{}'", .0)] SolvingError(OpcodeResolutionError, Option>), @@ -186,7 +191,8 @@ fn extract_message_from_error( match nargo_err { NargoError::ExecutionError(ExecutionError::AssertionFailed( ResolvedAssertionPayload::String(message), - _, _, + _, + _, )) => { format!("Assertion failed: '{message}'") } diff --git a/tooling/nargo/src/ops/execute.rs b/tooling/nargo/src/ops/execute.rs index f0a30f2de57..59d554d7ca5 100644 --- a/tooling/nargo/src/ops/execute.rs +++ b/tooling/nargo/src/ops/execute.rs @@ -117,7 +117,7 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver, E: ForeignCallExecutor> _ => None, }; - let brillig_function_id = match &error { + let brillig_function_id = match &error { OpcodeResolutionError::BrilligFunctionFailed { function_id, .. } => { Some(*function_id) } From 5c41302b72d9ecf72e1aa41891f22fdcc14a14e4 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 30 Aug 2024 19:33:49 +0000 Subject: [PATCH 09/19] comment out extra mem2reg and die --- compiler/noirc_evaluator/src/ssa.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 3f933814ba8..513933c6759 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -121,9 +121,9 @@ pub(crate) fn optimize_into_acir( // TODO: mem2reg and DIE were put here to delete any stores that can be removes following die removing loads. // Decide whether we want to run mem2reg once more or just have it able to handle this case on its own // NOTE: Running these two passes causes uhashmap to fail with a type mismatch: - .run_pass(Ssa::mem2reg, "After Mem2Reg:") + // .run_pass(Ssa::mem2reg, "After Mem2Reg:") // The extra mem2reg requires another DIE as we can have allocates we do not want - .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") + // .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") .run_pass(Ssa::array_set_optimization, "After Array Set Optimizations:") .finish(); From 568c083ce5383d5441c36690bdc340c1eb9db7f6 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 30 Aug 2024 19:35:25 +0000 Subject: [PATCH 10/19] remove last_loads approach and add some comments --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 56ba4dbbf85..fbb9733e9b9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -277,18 +277,6 @@ impl<'f> PerFunctionContext<'f> { self.instructions_to_remove.insert(*last_store); } - // NOTE: This causes issues for brillig_cow_assign, and uhashmap - // if let Some((last_load, last_load_block)) = self.last_loads.get(&address) { - // let load_result = self.inserter.function.dfg.instruction_results(*last_load)[0]; - // // NOTE: Checking !contains_array fixes brillig_cow_assign but not uhashmap - // // let contains_array = - // // self.inserter.function.dfg.type_of_value(value).contains_an_array(); - // if load_result == value && *last_load_block == block_id { - // self.instructions_to_remove.insert(instruction); - // } - // } - - // // NOTE: This causes the same failures as the last_loads approach if let Some(known_value) = references.get_known_value(value) { if known_value == address && !from_rc { self.instructions_to_remove.insert(instruction); From c1c2b61ef2038b230a07e17122f23bb1f4e4a13b Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 30 Aug 2024 19:40:06 +0000 Subject: [PATCH 11/19] cleanup --- compiler/noirc_evaluator/src/ssa.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 513933c6759..c6c4b464ca5 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -96,7 +96,6 @@ pub(crate) fn optimize_into_acir( .run_pass(Ssa::inline_functions, "After Inlining:") // Run mem2reg with the CFG separated into blocks .run_pass(Ssa::mem2reg, "After Mem2Reg:") - // .run_pass(Ssa::remove_paired_rc, "After Removing Paired rc_inc & rc_decs:") .run_pass(Ssa::as_slice_optimization, "After `as_slice` optimization") .try_run_pass( Ssa::evaluate_static_assert_and_assert_constant, @@ -121,6 +120,13 @@ pub(crate) fn optimize_into_acir( // TODO: mem2reg and DIE were put here to delete any stores that can be removes following die removing loads. // Decide whether we want to run mem2reg once more or just have it able to handle this case on its own // NOTE: Running these two passes causes uhashmap to fail with a type mismatch: + // ``` + // error: Assertion failed: 'Bit size for lhs 0 does not match op bit size 32' + // ┌─ std/collections/umap.nr:358:12 + // │ + // 358 │ if self.len() + 1 >= self.capacity() / 2 { + // ``` + // // .run_pass(Ssa::mem2reg, "After Mem2Reg:") // The extra mem2reg requires another DIE as we can have allocates we do not want // .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") From dae5a44c03268d6f378405f06748cdf39d9bb645 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 3 Sep 2024 14:34:50 +0000 Subject: [PATCH 12/19] cleanup --- .../execution_success/brillig_cow_assign/src/main.nr | 5 ----- .../brillig_loop_size_regression/src/main.nr | 6 +++--- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/test_programs/execution_success/brillig_cow_assign/src/main.nr b/test_programs/execution_success/brillig_cow_assign/src/main.nr index 9bcee9e96de..e5c3e2bd2f5 100644 --- a/test_programs/execution_success/brillig_cow_assign/src/main.nr +++ b/test_programs/execution_success/brillig_cow_assign/src/main.nr @@ -6,13 +6,8 @@ unconstrained fn main() { for i in 0..N { if i == N / 2 { - // println("inside if"); - // println(arr); mid_change = arr; - // println(mid_change); } - // println(i); - // println(mid_change); arr[i] = 27; } diff --git a/test_programs/execution_success/brillig_loop_size_regression/src/main.nr b/test_programs/execution_success/brillig_loop_size_regression/src/main.nr index c5c8f87f450..f9a5adbdc40 100644 --- a/test_programs/execution_success/brillig_loop_size_regression/src/main.nr +++ b/test_programs/execution_success/brillig_loop_size_regression/src/main.nr @@ -7,10 +7,10 @@ struct EnumEmulation { unconstrained fn main() -> pub Field { let mut emulated_enum = EnumEmulation { a: Option::some(1), b: Option::none(), c: Option::none() }; - // Do this to optimize out loads in the loop - // let copy_enum = emulated_enum; + // Do a copy to optimize out loads in the loop + let copy_enum = emulated_enum; for _ in 0..1 { - assert_eq(emulated_enum.a.unwrap(), 1); + assert_eq(copy_enum.a.unwrap(), 1); } emulated_enum.a = Option::some(2); From 4962db133a870c3a510d48c1cc504d2ac120cf5e Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 3 Sep 2024 14:49:19 +0000 Subject: [PATCH 13/19] reduce diff --- tooling/nargo/src/errors.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tooling/nargo/src/errors.rs b/tooling/nargo/src/errors.rs index 05474195600..0b8378702cb 100644 --- a/tooling/nargo/src/errors.rs +++ b/tooling/nargo/src/errors.rs @@ -232,7 +232,6 @@ pub fn try_to_diagnose_runtime_error( } _ => return None, }; - // The location of the error itself will be the location at the top // of the call stack (the last item in the Vec). let location = source_locations.last()?; From 31224eb3cdc121501eedf5a7b852c414885169f8 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 3 Sep 2024 16:18:07 +0000 Subject: [PATCH 14/19] missed push --- compiler/noirc_evaluator/src/ssa.rs | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 02b502478bd..57bd76d4f78 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -117,19 +117,6 @@ pub(crate) fn optimize_into_acir( .run_pass(Ssa::remove_enable_side_effects, "After EnableSideEffectsIf removal:") .run_pass(Ssa::fold_constants_using_constraints, "After Constraint Folding:") .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") - // TODO: mem2reg and DIE were put here to delete any stores that can be removes following die removing loads. - // Decide whether we want to run mem2reg once more or just have it able to handle this case on its own - // NOTE: Running these two passes causes uhashmap to fail with a type mismatch: - // ``` - // error: Assertion failed: 'Bit size for lhs 0 does not match op bit size 32' - // ┌─ std/collections/umap.nr:358:12 - // │ - // 358 │ if self.len() + 1 >= self.capacity() / 2 { - // ``` - // - // .run_pass(Ssa::mem2reg, "After Mem2Reg:") - // The extra mem2reg requires another DIE as we can have allocates we do not want - // .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") .run_pass(Ssa::array_set_optimization, "After Array Set Optimizations:") .finish(); From adacab46d64beeca39adc356848bd56b14bac4a0 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 3 Sep 2024 16:18:40 +0000 Subject: [PATCH 15/19] cleanup --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index fbb9733e9b9..237e38f5d34 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -116,7 +116,7 @@ struct PerFunctionContext<'f> { /// Track a value's last load across all blocks. /// If a value is not used in anymore loads we can remove the last store to that value. - last_loads: HashMap, + last_loads: HashMap, } impl<'f> PerFunctionContext<'f> { @@ -262,7 +262,7 @@ impl<'f> PerFunctionContext<'f> { references.aliases.insert(Expression::Other(result), AliasSet::known(result)); references.set_known_value(result, address); - self.last_loads.insert(address, (instruction, block_id)); + self.last_loads.insert(address, instruction); } } Instruction::Store { address, value, from_rc } => { From 20d20afc2fe282e7799444c4b7c886d04db8b19d Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 3 Sep 2024 21:53:45 +0000 Subject: [PATCH 16/19] remove from_rc from Store and track in mem2reg --- .../src/ssa/function_builder/mod.rs | 8 +-- .../noirc_evaluator/src/ssa/ir/instruction.rs | 6 +- .../noirc_evaluator/src/ssa/ir/printer.rs | 4 +- .../src/ssa/opt/flatten_cfg.rs | 8 +-- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 59 ++++++++++++++++++- 5 files changed, 67 insertions(+), 18 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index ae60f771606..0edb7daf530 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -219,11 +219,7 @@ impl FunctionBuilder { /// at the given address. Expects that the address points somewhere /// within a previous Allocate instruction. pub(crate) fn insert_store(&mut self, address: ValueId, value: ValueId) { - self.insert_instruction(Instruction::Store { address, value, from_rc: false }, None); - } - - pub(crate) fn insert_store_from_rc(&mut self, address: ValueId, value: ValueId) { - self.insert_instruction(Instruction::Store { address, value, from_rc: true }, None); + self.insert_instruction(Instruction::Store { address, value }, None); } /// Insert a binary instruction at the end of the current block. @@ -482,7 +478,7 @@ impl FunctionBuilder { let address = *address; let new_load = self.insert_load(address, typ); update_rc(self, new_load); - self.insert_store_from_rc(address, new_load); + self.insert_store(address, new_load); } } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index fdd06592a08..f0a2506a786 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -215,7 +215,7 @@ pub(crate) enum Instruction { Load { address: ValueId }, /// Writes a value to memory. - Store { address: ValueId, value: ValueId, from_rc: bool }, + Store { address: ValueId, value: ValueId }, /// Provides a context for all instructions that follow up until the next /// `EnableSideEffectsIf` is encountered, for stating a condition that determines whether @@ -482,8 +482,8 @@ impl Instruction { }, Instruction::Allocate => Instruction::Allocate, Instruction::Load { address } => Instruction::Load { address: f(*address) }, - Instruction::Store { address, value, from_rc } => { - Instruction::Store { address: f(*address), value: f(*value), from_rc: *from_rc } + Instruction::Store { address, value } => { + Instruction::Store { address: f(*address), value: f(*value) } } Instruction::EnableSideEffectsIf { condition } => { Instruction::EnableSideEffectsIf { condition: f(*condition) } diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 6fce661dec3..2b564c14aa7 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -173,8 +173,8 @@ fn display_instruction_inner( } Instruction::Allocate => writeln!(f, "allocate"), Instruction::Load { address } => writeln!(f, "load {}", show(*address)), - Instruction::Store { address, value, from_rc } => { - writeln!(f, "store {} at {}, from_rc {}", show(*value), show(*address), *from_rc) + Instruction::Store { address, value } => { + writeln!(f, "store {} at {}", show(*value), show(*address)) } Instruction::EnableSideEffectsIf { condition } => { writeln!(f, "enable_side_effects {}", show(*condition)) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index d462a600503..d5fb98c7adc 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -647,7 +647,7 @@ impl<'f> Context<'f> { let value = new_values[address]; let address = *address; self.insert_instruction_with_typevars( - Instruction::Store { address, value, from_rc: false }, + Instruction::Store { address, value }, None, call_stack.clone(), ); @@ -740,9 +740,9 @@ impl<'f> Context<'f> { Instruction::Constrain(lhs, rhs, message) } - Instruction::Store { address, value, from_rc } => { + Instruction::Store { address, value } => { self.remember_store(address, value, call_stack); - Instruction::Store { address, value, from_rc } + Instruction::Store { address, value } } Instruction::RangeCheck { value, max_bit_size, assert_message } => { // Replace value with `value * predicate` to zero out value when predicate is inactive. @@ -869,7 +869,7 @@ impl<'f> Context<'f> { for (address, store) in store_values { let address = *address; let value = store.old_value; - let instruction = Instruction::Store { address, value, from_rc: false }; + let instruction = Instruction::Store { address, value }; // Considering the location of undoing a store to be the same as the original store. self.insert_instruction_with_typevars(instruction, None, store.call_stack.clone()); } diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 237e38f5d34..1d7672633ca 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -117,6 +117,24 @@ struct PerFunctionContext<'f> { /// Track a value's last load across all blocks. /// If a value is not used in anymore loads we can remove the last store to that value. last_loads: HashMap, + + /// Flag for tracking whether we had to perform a re-load as part of the Brillig CoW optimization. + /// Stores made as part of this optimization should not be removed. + /// We want to catch stores of this nature: + /// ```text + /// v3 = load v1 + // inc_rc v3 + // v4 = load v1 + // inc_rc v4 + // store v4 at v1 + // store v3 at v2 + /// ``` + /// + /// We keep track of a tuple as we go through instructions. + /// The first tuple field is the value being loaded, which would be `v1` in the example above. + /// The second tuple states whether we have only done the first load or we are doing a re-load. + /// `inside_rc_reload` is reset to `None` on every instructions that is not a load, inc_rc, or dec_rc. + inside_rc_reload: Option<(ValueId, bool)>, } impl<'f> PerFunctionContext<'f> { @@ -131,6 +149,7 @@ impl<'f> PerFunctionContext<'f> { blocks: BTreeMap::new(), instructions_to_remove: BTreeSet::new(), last_loads: HashMap::default(), + inside_rc_reload: None, } } @@ -192,6 +211,7 @@ impl<'f> PerFunctionContext<'f> { for instruction in instructions { self.analyze_instruction(block, &mut references, instruction); + self.track_rc_reload_state(instruction); } self.handle_terminator(block, &mut references); @@ -265,7 +285,7 @@ impl<'f> PerFunctionContext<'f> { self.last_loads.insert(address, instruction); } } - Instruction::Store { address, value, from_rc } => { + Instruction::Store { address, value } => { let address = self.inserter.function.dfg.resolve(*address); let value = self.inserter.function.dfg.resolve(*value); @@ -277,8 +297,17 @@ impl<'f> PerFunctionContext<'f> { self.instructions_to_remove.insert(*last_store); } - if let Some(known_value) = references.get_known_value(value) { - if known_value == address && !from_rc { + let known_value = references.get_known_value(value); + let from_rc = self + .inside_rc_reload + .map(|(rc_value, is_reload)| rc_value == value && is_reload); + if let Some(known_value) = known_value { + let known_value_is_address = known_value == address; + if let Some(from_rc) = from_rc { + if known_value_is_address && !from_rc { + self.instructions_to_remove.insert(instruction); + } + } else if known_value_is_address { self.instructions_to_remove.insert(instruction); } } @@ -339,6 +368,30 @@ impl<'f> PerFunctionContext<'f> { } } + /// Update the `inside_rc_reload` context variable. + /// This method should always be called after `analyze_instruction`. + fn track_rc_reload_state(&mut self, instruction: InstructionId) { + match &self.inserter.function.dfg[instruction] { + Instruction::Load { .. } => { + if self.inside_rc_reload.is_some() { + let result = self.inserter.function.dfg.instruction_results(instruction)[0]; + self.inside_rc_reload = Some((result, true)); + } else { + let result = self.inserter.function.dfg.instruction_results(instruction)[0]; + self.inside_rc_reload = Some((result, false)); + } + } + Instruction::IncrementRc { value } | Instruction::DecrementRc { value } => { + if let Some((previous_value, _)) = self.inside_rc_reload { + if *value != previous_value { + self.inside_rc_reload = None; + } + } + } + _ => self.inside_rc_reload = None, + } + } + fn check_array_aliasing(&self, references: &mut Block, array: ValueId) { if let Some((elements, typ)) = self.inserter.function.dfg.get_array_constant(array) { if Self::contains_references(&typ) { From 2d8f5e635cd0e095c9292db138241db69b23fa2c Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 3 Sep 2024 21:57:29 +0000 Subject: [PATCH 17/19] remove unnecessary --- .../noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs | 2 +- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 2 +- .../noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 0c363d296f4..d3d0e2231ad 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -317,7 +317,7 @@ impl<'block> BrilligBlock<'block> { } } } - Instruction::Store { address, value, .. } => { + Instruction::Store { address, value } => { let address_var = self.convert_ssa_single_addr_value(*address, dfg); let source_variable = self.convert_ssa_value(*value, dfg); diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index f0a2506a786..e30707effed 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -546,7 +546,7 @@ impl Instruction { } } - Instruction::Store { address, value, .. } => { + Instruction::Store { address, value } => { f(*address); f(*value); } diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs index 191d64742d2..836c812843e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs @@ -125,7 +125,7 @@ impl<'a> SliceCapacityTracker<'a> { } } } - Instruction::Store { address, value, .. } => { + Instruction::Store { address, value } => { let value_typ = self.dfg.type_of_value(*value); if value_typ.contains_slice_element() { self.compute_slice_capacity(*value, slice_sizes); From 90a4fbec656c9c8856a7f93885a270a3981a8bc6 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 3 Sep 2024 22:38:36 +0000 Subject: [PATCH 18/19] move rc reload state tracking --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 1d7672633ca..e71ed3dc4d8 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -211,7 +211,6 @@ impl<'f> PerFunctionContext<'f> { for instruction in instructions { self.analyze_instruction(block, &mut references, instruction); - self.track_rc_reload_state(instruction); } self.handle_terminator(block, &mut references); @@ -366,18 +365,23 @@ impl<'f> PerFunctionContext<'f> { Instruction::Call { arguments, .. } => self.mark_all_unknown(arguments, references), _ => (), } + + self.track_rc_reload_state(instruction); } /// Update the `inside_rc_reload` context variable. - /// This method should always be called after `analyze_instruction`. + /// To maintain the same value ids, we must run this method inside `analyze_instruction` so that + /// we operate on the newly pushed instruction id. + /// This method should also always come after running analysis on the new instruction. fn track_rc_reload_state(&mut self, instruction: InstructionId) { match &self.inserter.function.dfg[instruction] { Instruction::Load { .. } => { + let result = self.inserter.function.dfg.instruction_results(instruction)[0]; + // let result = self.inserter.function.dfg.resolve(result); if self.inside_rc_reload.is_some() { - let result = self.inserter.function.dfg.instruction_results(instruction)[0]; self.inside_rc_reload = Some((result, true)); } else { - let result = self.inserter.function.dfg.instruction_results(instruction)[0]; + // let result = self.inserter.function.dfg.instruction_results(instruction)[0]; self.inside_rc_reload = Some((result, false)); } } From 90d57153c6e20bf814082be57183d9273e805934 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 4 Sep 2024 02:10:29 +0000 Subject: [PATCH 19/19] improve handling of rc reload state and handle call when tracking --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 45 +++++++++++-------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index e71ed3dc4d8..0994a01f029 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -77,7 +77,7 @@ use crate::ssa::{ instruction::{Instruction, InstructionId, TerminatorInstruction}, post_order::PostOrder, types::Type, - value::ValueId, + value::{Value, ValueId}, }, ssa_gen::Ssa, }; @@ -130,11 +130,12 @@ struct PerFunctionContext<'f> { // store v3 at v2 /// ``` /// - /// We keep track of a tuple as we go through instructions. - /// The first tuple field is the value being loaded, which would be `v1` in the example above. - /// The second tuple states whether we have only done the first load or we are doing a re-load. - /// `inside_rc_reload` is reset to `None` on every instructions that is not a load, inc_rc, or dec_rc. - inside_rc_reload: Option<(ValueId, bool)>, + /// We keep track of an optional boolean flag as we go through instructions. + /// If the flag exists it means we have hit a load instruction. + /// If the flag is false it means we have processed a single load, while if the flag is true + /// it means we have performed a re-load. + /// The field is reset to `None` on every instruction that is not a load, inc_rc, dec_rc, or function call. + inside_rc_reload: Option, } impl<'f> PerFunctionContext<'f> { @@ -297,12 +298,9 @@ impl<'f> PerFunctionContext<'f> { } let known_value = references.get_known_value(value); - let from_rc = self - .inside_rc_reload - .map(|(rc_value, is_reload)| rc_value == value && is_reload); if let Some(known_value) = known_value { let known_value_is_address = known_value == address; - if let Some(from_rc) = from_rc { + if let Some(from_rc) = self.inside_rc_reload { if known_value_is_address && !from_rc { self.instructions_to_remove.insert(instruction); } @@ -376,22 +374,31 @@ impl<'f> PerFunctionContext<'f> { fn track_rc_reload_state(&mut self, instruction: InstructionId) { match &self.inserter.function.dfg[instruction] { Instruction::Load { .. } => { - let result = self.inserter.function.dfg.instruction_results(instruction)[0]; - // let result = self.inserter.function.dfg.resolve(result); if self.inside_rc_reload.is_some() { - self.inside_rc_reload = Some((result, true)); + self.inside_rc_reload = Some(true); } else { - // let result = self.inserter.function.dfg.instruction_results(instruction)[0]; - self.inside_rc_reload = Some((result, false)); + self.inside_rc_reload = Some(false); } } - Instruction::IncrementRc { value } | Instruction::DecrementRc { value } => { - if let Some((previous_value, _)) = self.inside_rc_reload { - if *value != previous_value { - self.inside_rc_reload = None; + Instruction::Call { arguments, .. } => { + for arg in arguments { + if let Value::Instruction { instruction, .. } = + &self.inserter.function.dfg[*arg] + { + let instruction = &self.inserter.function.dfg[*instruction]; + if let Instruction::Load { .. } = instruction { + if self.inside_rc_reload.is_some() { + self.inside_rc_reload = Some(true); + } else { + self.inside_rc_reload = Some(false); + } + } } } } + Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. } => { + // Do nothing. We want the reload state to remain the same. + } _ => self.inside_rc_reload = None, } }