From 6a4be880bdd1d8dd8a0f9850e372882b4620fb82 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 21 Nov 2024 11:52:12 -0600 Subject: [PATCH 1/8] Don't remove necessary RC instructions --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 8d3fa9cc615..0460b29753a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -127,8 +127,9 @@ impl Context { .push(instructions_len - instruction_index - 1); } } else { - use Instruction::*; - if matches!(instruction, IncrementRc { .. } | DecrementRc { .. }) { + // We can't remove rc instructions if they're loaded from a reference + // since we'd have no way of knowing whether the reference is still used. + if Self::is_inc_dec_instruction_on_known_array(instruction, &function.dfg) { self.rc_instructions.push((*instruction_id, block_id)); } else { instruction.for_each_value(|value| { @@ -337,6 +338,21 @@ impl Context { inserted_check } + + /// True if this is a `Instruction::IncrementRc` or `Instruction::DecrementRc` + /// operating on an array directly from a `Instruction::MakeArray`. + fn is_inc_dec_instruction_on_known_array( + instruction: &Instruction, + dfg: &DataFlowGraph, + ) -> bool { + use Instruction::*; + if let IncrementRc { value } | DecrementRc { value } = instruction { + if let Value::Instruction { instruction, .. } = &dfg[*value] { + return matches!(&dfg[*instruction], MakeArray { .. }); + } + } + false + } } fn instruction_might_result_in_out_of_bounds( From a750a7aba3fa8dfea0d9e30b61a2140cf3baf41e Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 21 Nov 2024 12:01:14 -0600 Subject: [PATCH 2/8] Disable rc_tracker --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 0460b29753a..584e852e01b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -108,7 +108,7 @@ impl Context { let instructions_len = block.instructions().len(); - let mut rc_tracker = RcTracker::default(); + // let mut rc_tracker = RcTracker::default(); // Indexes of instructions that might be out of bounds. // We'll remove those, but before that we'll insert bounds checks for them. @@ -138,11 +138,11 @@ impl Context { } } - rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); + // rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); } - self.instructions_to_remove.extend(rc_tracker.get_non_mutated_arrays()); - self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove); + // self.instructions_to_remove.extend(rc_tracker.get_non_mutated_arrays()); + // self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove); // If there are some instructions that might trigger an out of bounds error, // first add constrain checks. Then run the DIE pass again, which will remove those From 6543754566177a5c441dcb912893d8a660e51d0b Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 22 Nov 2024 10:26:15 -0600 Subject: [PATCH 3/8] Narrow down issue to non_mutated_arrays tracking --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 584e852e01b..3d1be5578ae 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -108,7 +108,7 @@ impl Context { let instructions_len = block.instructions().len(); - // let mut rc_tracker = RcTracker::default(); + let mut rc_tracker = RcTracker::default(); // Indexes of instructions that might be out of bounds. // We'll remove those, but before that we'll insert bounds checks for them. @@ -138,11 +138,11 @@ impl Context { } } - // rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); + rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); } // self.instructions_to_remove.extend(rc_tracker.get_non_mutated_arrays()); - // self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove); + self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove); // If there are some instructions that might trigger an out of bounds error, // first add constrain checks. Then run the DIE pass again, which will remove those From 4b3950ad88a0fa4836ed4999d9c08b7652b98a79 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 22 Nov 2024 13:17:33 -0600 Subject: [PATCH 4/8] Disable test --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 58 ++++++++++----------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 3d1be5578ae..b3bd34a3199 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -845,33 +845,33 @@ mod test { assert!(matches!(&main.dfg[instructions[3]], Instruction::ArrayGet { .. })); } - #[test] - fn remove_inc_rcs_that_are_never_mutably_borrowed() { - let src = " - acir(inline) fn main f0 { - b0(v0: [Field; 2]): - inc_rc v0 - inc_rc v0 - inc_rc v0 - v2 = array_get v0, index u32 0 -> Field - inc_rc v0 - return v2 - } - "; - let ssa = Ssa::from_str(src).unwrap(); - let main = ssa.main(); - - // The instruction count never includes the terminator instruction - assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5); - - let expected = " - acir(inline) fn main f0 { - b0(v0: [Field; 2]): - v2 = array_get v0, index u32 0 -> Field - return v2 - } - "; - let ssa = ssa.dead_instruction_elimination(); - assert_normalized_ssa_equals(ssa, expected); - } + // #[test] + // fn remove_inc_rcs_that_are_never_mutably_borrowed() { + // let src = " + // acir(inline) fn main f0 { + // b0(v0: [Field; 2]): + // inc_rc v0 + // inc_rc v0 + // inc_rc v0 + // v2 = array_get v0, index u32 0 -> Field + // inc_rc v0 + // return v2 + // } + // "; + // let ssa = Ssa::from_str(src).unwrap(); + // let main = ssa.main(); + + // // The instruction count never includes the terminator instruction + // assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5); + + // let expected = " + // acir(inline) fn main f0 { + // b0(v0: [Field; 2]): + // v2 = array_get v0, index u32 0 -> Field + // return v2 + // } + // "; + // let ssa = ssa.dead_instruction_elimination(); + // assert_normalized_ssa_equals(ssa, expected); + // } } From 602e1e6b8e1be02ea9d6b25a22e7dd3426ed1471 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 22 Nov 2024 14:43:48 -0600 Subject: [PATCH 5/8] Reintroduce optimization; it is per type now --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 77 +++++++++++---------- 1 file changed, 40 insertions(+), 37 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index b3bd34a3199..ce8577f07dd 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -141,7 +141,7 @@ impl Context { rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); } - // self.instructions_to_remove.extend(rc_tracker.get_non_mutated_arrays()); + self.instructions_to_remove.extend(rc_tracker.get_non_mutated_arrays(&function.dfg)); self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove); // If there are some instructions that might trigger an out of bounds error, @@ -529,7 +529,7 @@ struct RcTracker { // We also separately track all IncrementRc instructions and all arrays which have been mutably borrowed. // If an array has not been mutably borrowed we can then safely remove all IncrementRc instructions on that array. inc_rcs: HashMap>, - mut_borrowed_arrays: HashSet, + mutated_array_types: HashSet, // The SSA often creates patterns where after simplifications we end up with repeat // IncrementRc instructions on the same value. We track whether the previous instruction was an IncrementRc, // and if the current instruction is also an IncrementRc on the same value we remove the current instruction. @@ -583,25 +583,28 @@ impl RcTracker { } } - self.mut_borrowed_arrays.insert(*array); + self.mutated_array_types.insert(typ); } Instruction::Store { value, .. } => { - // We are very conservative and say that any store of an array value means it has the potential - // to be mutated. This is done due to the tracking of mutable borrows still being per block. + // We are very conservative and say that any store of an array value means that any + // array of that type has the potential to be mutated. This is done due to the + // tracking of mutable borrows still being per block and that we don't have the + // aliasing information from mem2reg. let typ = function.dfg.type_of_value(*value); if matches!(&typ, Type::Array(..) | Type::Slice(..)) { - self.mut_borrowed_arrays.insert(*value); + self.mutated_array_types.insert(typ); } } _ => {} } } - fn get_non_mutated_arrays(&self) -> HashSet { + fn get_non_mutated_arrays(&self, dfg: &DataFlowGraph) -> HashSet { self.inc_rcs .keys() .filter_map(|value| { - if !self.mut_borrowed_arrays.contains(value) { + let typ = dfg.type_of_value(*value); + if !self.mutated_array_types.contains(&typ) { Some(&self.inc_rcs[value]) } else { None @@ -845,33 +848,33 @@ mod test { assert!(matches!(&main.dfg[instructions[3]], Instruction::ArrayGet { .. })); } - // #[test] - // fn remove_inc_rcs_that_are_never_mutably_borrowed() { - // let src = " - // acir(inline) fn main f0 { - // b0(v0: [Field; 2]): - // inc_rc v0 - // inc_rc v0 - // inc_rc v0 - // v2 = array_get v0, index u32 0 -> Field - // inc_rc v0 - // return v2 - // } - // "; - // let ssa = Ssa::from_str(src).unwrap(); - // let main = ssa.main(); - - // // The instruction count never includes the terminator instruction - // assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5); - - // let expected = " - // acir(inline) fn main f0 { - // b0(v0: [Field; 2]): - // v2 = array_get v0, index u32 0 -> Field - // return v2 - // } - // "; - // let ssa = ssa.dead_instruction_elimination(); - // assert_normalized_ssa_equals(ssa, expected); - // } + #[test] + fn remove_inc_rcs_that_are_never_mutably_borrowed() { + let src = " + acir(inline) fn main f0 { + b0(v0: [Field; 2]): + inc_rc v0 + inc_rc v0 + inc_rc v0 + v2 = array_get v0, index u32 0 -> Field + inc_rc v0 + return v2 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let main = ssa.main(); + + // The instruction count never includes the terminator instruction + assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5); + + let expected = " + acir(inline) fn main f0 { + b0(v0: [Field; 2]): + v2 = array_get v0, index u32 0 -> Field + return v2 + } + "; + let ssa = ssa.dead_instruction_elimination(); + assert_normalized_ssa_equals(ssa, expected); + } } From fdc8dd07e49f5c645ec4d6699d8e83df00954f57 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 22 Nov 2024 14:57:16 -0600 Subject: [PATCH 6/8] Consider arrays returned by intrinsics to be fresh as well --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index ce8577f07dd..fc1cc0b15ef 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -340,7 +340,8 @@ impl Context { } /// True if this is a `Instruction::IncrementRc` or `Instruction::DecrementRc` - /// operating on an array directly from a `Instruction::MakeArray`. + /// operating on an array directly from a `Instruction::MakeArray` or an + /// intrinsic known to return a fresh array. fn is_inc_dec_instruction_on_known_array( instruction: &Instruction, dfg: &DataFlowGraph, @@ -348,7 +349,13 @@ impl Context { use Instruction::*; if let IncrementRc { value } | DecrementRc { value } = instruction { if let Value::Instruction { instruction, .. } = &dfg[*value] { - return matches!(&dfg[*instruction], MakeArray { .. }); + return match &dfg[*instruction] { + MakeArray { .. } => true, + Call { func, .. } => { + matches!(&dfg[*func], Value::Intrinsic(_) | Value::ForeignFunction(_)) + } + _ => false, + }; } } false From 0d7960d4ec8ffa1fc3d9ff02496687643a01a65f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 27 Nov 2024 14:53:14 -0300 Subject: [PATCH 7/8] Add a test --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index fc1cc0b15ef..5f81a82accc 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -884,4 +884,25 @@ mod test { let ssa = ssa.dead_instruction_elimination(); assert_normalized_ssa_equals(ssa, expected); } + + #[test] + fn does_not_remove_inc_or_dec_rc_of_if_they_are_loaded_from_a_reference() { + let src = " + brillig(inline) fn borrow_mut f0 { + b0(v0: &mut [Field; 3]): + v1 = load v0 -> [Field; 3] + inc_rc v1 // this one shouldn't be removed + v2 = load v0 -> [Field; 3] + inc_rc v2 // this one shouldn't be removed + v3 = load v0 -> [Field; 3] + v6 = array_set v3, index u32 0, value Field 5 + store v6 at v0 + dec_rc v6 + return + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let ssa = ssa.dead_instruction_elimination(); + assert_normalized_ssa_equals(ssa, src); + } } From 5fd09eda6384750f4d2f6551aca921da3741f686 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 2 Dec 2024 19:28:31 +0000 Subject: [PATCH 8/8] bring back the rc reload state --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 53a31ae57c1..1750ecb453a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -142,6 +142,10 @@ struct PerFunctionContext<'f> { /// instruction that aliased that reference. /// If that store has been set for removal, we can also remove this instruction. aliased_references: HashMap>, + + /// Track whether the last instruction is an inc_rc/dec_rc instruction. + /// If it is we should not remove any repeat last loads. + inside_rc_reload: bool, } impl<'f> PerFunctionContext<'f> { @@ -158,6 +162,7 @@ impl<'f> PerFunctionContext<'f> { last_loads: HashMap::default(), calls_reference_input: HashSet::default(), aliased_references: HashMap::default(), + inside_rc_reload: false, } } @@ -435,7 +440,7 @@ impl<'f> PerFunctionContext<'f> { let result = self.inserter.function.dfg.instruction_results(instruction)[0]; let previous_result = self.inserter.function.dfg.instruction_results(*last_load)[0]; - if *previous_address == address { + if *previous_address == address && !self.inside_rc_reload { self.inserter.map_value(result, previous_result); self.instructions_to_remove.insert(instruction); } @@ -553,6 +558,18 @@ impl<'f> PerFunctionContext<'f> { } _ => (), } + + self.track_rc_reload_state(instruction); + } + + fn track_rc_reload_state(&mut self, instruction: InstructionId) { + match &self.inserter.function.dfg[instruction] { + // We just had an increment or decrement to an array's reference counter + Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. } => { + self.inside_rc_reload = true; + } + _ => self.inside_rc_reload = false, + } } fn contains_references(typ: &Type) -> bool {