From 3d8741a19f18498e4ab27604e443539b4946af25 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 12 Dec 2024 03:15:47 +0000 Subject: [PATCH 1/5] enable MakeArray in loop invariant code motion by inserting an inc rc in its place --- .../src/ssa/opt/loop_invariant.rs | 116 ++++++++++++++++++ .../gates_report_brillig_execution.sh | 2 - tooling/nargo_cli/build.rs | 4 +- 3 files changed, 117 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 87e7f8bcff3..9920ec2e682 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -9,6 +9,7 @@ //! We also check that we are not hoisting instructions with side effects. use acvm::{acir::AcirField, FieldElement}; use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; +use iter_extended::vecmap; use crate::ssa::{ ir::{ @@ -113,6 +114,29 @@ impl<'f> LoopInvariantContext<'f> { if hoist_invariant { self.inserter.push_instruction(instruction_id, pre_header); + + // We track whether we may mutate MakeArray instructions before we deduplicate + // them but we still need to issue an extra inc_rc in case they're mutated afterward. + if matches!( + self.inserter.function.dfg[instruction_id], + Instruction::MakeArray { .. } + ) { + let results = + self.inserter.function.dfg.instruction_results(instruction_id).to_vec(); + let results = vecmap(results, |value| self.inserter.resolve(value)); + assert_eq!( + results.len(), + 1, + "ICE: We expect only a single result from an `Instruction::MakeArray`" + ); + dbg!(results.clone()); + let inc_rc = Instruction::IncrementRc { value: results[0] }; + let call_stack = self.inserter.function.dfg.get_call_stack(instruction_id); + self.inserter + .function + .dfg + .insert_instruction_and_results(inc_rc, *block, None, call_stack); + } } else { self.inserter.push_instruction(instruction_id, *block); } @@ -190,6 +214,7 @@ impl<'f> LoopInvariantContext<'f> { }); let can_be_deduplicated = instruction.can_be_deduplicated(self.inserter.function, false) + || matches!(instruction, Instruction::MakeArray { .. }) || self.can_be_deduplicated_from_upper_bound(&instruction); is_loop_invariant && can_be_deduplicated @@ -559,4 +584,95 @@ mod test { let ssa = ssa.loop_invariant_code_motion(); assert_normalized_ssa_equals(ssa, expected); } + + #[test] + fn insert_inc_rc_when_moving_make_array() { + // SSA for the following program: + // + // unconstrained fn main(x: u32, y: u32) { + // let mut a1 = [1, 2, 3, 4, 5]; + // a1[x] = 64; + // for i in 0 .. 5 { + // let mut a2 = [1, 2, 3, 4, 5]; + // a2[y + i] = 128; + // foo(a2); + // } + // foo(a1); + // } + // + // We want to make sure move a loop invariant make_array instruction, + // to account for whether that array has been marked as mutable. + // To do so, we increment the reference counter on the array we are moving. + // In the SSA below, we want to move `v42` out of the loop. + let src = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v8 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] + v9 = allocate -> &mut [Field; 5] + v11 = array_set v8, index v0, value Field 64 + v13 = add v0, u32 1 + store v11 at v9 + jmp b1(u32 0) + b1(v2: u32): + v16 = lt v2, u32 5 + jmpif v16 then: b3, else: b2 + b2(): + v17 = load v9 -> [Field; 5] + call f1(v17) + return + b3(): + v19 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] + v20 = allocate -> &mut [Field; 5] + v21 = add v1, v2 + v23 = array_set v19, index v21, value Field 128 + call f1(v23) + v25 = add v2, u32 1 + jmp b1(v25) + } + + brillig(inline) fn foo f1 { + b0(v0: [Field; 5]): + return + } + "; + + let ssa = Ssa::from_str(src).unwrap(); + + // We expect the `make_array` at the top of `b3` to be replaced with an `inc_rc` + // of the newly hoisted `make_array` at the end of `b0`. + let expected = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v32 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] + v33 = allocate -> &mut [Field; 5] + v34 = array_set v32, index v0, value Field 64 + v35 = add v0, u32 1 + store v34 at v33 + v36 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] + jmp b1(u32 0) + b1(v2: u32): + v37 = lt v2, u32 5 + jmpif v37 then: b3, else: b2 + b2(): + v38 = load v33 -> [Field; 5] + call f1(v38) + return + b3(): + inc_rc v36 + v39 = allocate -> &mut [Field; 5] + v40 = add v1, v2 + v41 = array_set v36, index v40, value Field 128 + call f1(v41) + v42 = add v2, u32 1 + jmp b1(v42) + } + brillig(inline) fn foo f1 { + b0(v0: [Field; 5]): + return + } + "; + + let ssa = ssa.loop_invariant_code_motion(); + assert_normalized_ssa_equals(ssa, expected); + } } diff --git a/test_programs/gates_report_brillig_execution.sh b/test_programs/gates_report_brillig_execution.sh index 024c7612541..b3f4a8bda98 100755 --- a/test_programs/gates_report_brillig_execution.sh +++ b/test_programs/gates_report_brillig_execution.sh @@ -8,8 +8,6 @@ excluded_dirs=( "double_verify_nested_proof" "overlapping_dep_and_mod" "comptime_println" - # Takes a very long time to execute as large loops do not get simplified. - "regression_4709" # bit sizes for bigint operation doesn't match up. "bigint" # Expected to fail as test asserts on which runtime it is in. diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index db6177366d3..8db2c1786d8 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -42,9 +42,7 @@ fn main() { /// Some tests are explicitly ignored in brillig due to them failing. /// These should be fixed and removed from this list. -const IGNORED_BRILLIG_TESTS: [&str; 11] = [ - // Takes a very long time to execute as large loops do not get simplified. - "regression_4709", +const IGNORED_BRILLIG_TESTS: [&str; 10] = [ // bit sizes for bigint operation doesn't match up. "bigint", // ICE due to looking for function which doesn't exist. From c87c924e35f75915c4feda0bf2c57a6ba5eb7bf5 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 11 Dec 2024 22:27:21 -0500 Subject: [PATCH 2/5] Update compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs --- compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 9920ec2e682..2fa9d837d26 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -129,7 +129,6 @@ impl<'f> LoopInvariantContext<'f> { 1, "ICE: We expect only a single result from an `Instruction::MakeArray`" ); - dbg!(results.clone()); let inc_rc = Instruction::IncrementRc { value: results[0] }; let call_stack = self.inserter.function.dfg.get_call_stack(instruction_id); self.inserter From 456aabe4b4331ff9aed7745c1c4508d1a24c03e5 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 12 Dec 2024 03:29:49 +0000 Subject: [PATCH 3/5] fix weird formatting in test --- .../src/ssa/opt/loop_invariant.rs | 121 +++++++++--------- 1 file changed, 61 insertions(+), 60 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 2fa9d837d26..23d2c5b5172 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -604,72 +604,73 @@ mod test { // To do so, we increment the reference counter on the array we are moving. // In the SSA below, we want to move `v42` out of the loop. let src = " - brillig(inline) fn main f0 { - b0(v0: u32, v1: u32): - v8 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] - v9 = allocate -> &mut [Field; 5] - v11 = array_set v8, index v0, value Field 64 - v13 = add v0, u32 1 - store v11 at v9 - jmp b1(u32 0) - b1(v2: u32): - v16 = lt v2, u32 5 - jmpif v16 then: b3, else: b2 - b2(): - v17 = load v9 -> [Field; 5] - call f1(v17) - return - b3(): - v19 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] - v20 = allocate -> &mut [Field; 5] - v21 = add v1, v2 - v23 = array_set v19, index v21, value Field 128 - call f1(v23) - v25 = add v2, u32 1 - jmp b1(v25) - } - - brillig(inline) fn foo f1 { - b0(v0: [Field; 5]): - return - } - "; + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v8 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] + v9 = allocate -> &mut [Field; 5] + v11 = array_set v8, index v0, value Field 64 + v13 = add v0, u32 1 + store v11 at v9 + jmp b1(u32 0) + b1(v2: u32): + v16 = lt v2, u32 5 + jmpif v16 then: b3, else: b2 + b2(): + v17 = load v9 -> [Field; 5] + call f1(v17) + return + b3(): + v19 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] + v20 = allocate -> &mut [Field; 5] + v21 = add v1, v2 + v23 = array_set v19, index v21, value Field 128 + call f1(v23) + v25 = add v2, u32 1 + jmp b1(v25) + } + + brillig(inline) fn foo f1 { + b0(v0: [Field; 5]): + return + } + "; let ssa = Ssa::from_str(src).unwrap(); // We expect the `make_array` at the top of `b3` to be replaced with an `inc_rc` // of the newly hoisted `make_array` at the end of `b0`. let expected = " - brillig(inline) fn main f0 { - b0(v0: u32, v1: u32): - v32 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] - v33 = allocate -> &mut [Field; 5] - v34 = array_set v32, index v0, value Field 64 - v35 = add v0, u32 1 - store v34 at v33 - v36 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] - jmp b1(u32 0) - b1(v2: u32): - v37 = lt v2, u32 5 - jmpif v37 then: b3, else: b2 - b2(): - v38 = load v33 -> [Field; 5] - call f1(v38) - return - b3(): - inc_rc v36 - v39 = allocate -> &mut [Field; 5] - v40 = add v1, v2 - v41 = array_set v36, index v40, value Field 128 - call f1(v41) - v42 = add v2, u32 1 - jmp b1(v42) - } - brillig(inline) fn foo f1 { - b0(v0: [Field; 5]): - return - } - "; + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v32 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] + v33 = allocate -> &mut [Field; 5] + v34 = array_set v32, index v0, value Field 64 + v35 = add v0, u32 1 + store v34 at v33 + v36 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] + jmp b1(u32 0) + b1(v2: u32): + v37 = lt v2, u32 5 + jmpif v37 then: b3, else: b2 + b2(): + v38 = load v33 -> [Field; 5] + call f1(v38) + return + b3(): + inc_rc v36 + v39 = allocate -> &mut [Field; 5] + v40 = add v1, v2 + v41 = array_set v36, index v40, value Field 128 + call f1(v41) + v42 = add v2, u32 1 + jmp b1(v42) + } + + brillig(inline) fn foo f1 { + b0(v0: [Field; 5]): + return + } + "; let ssa = ssa.loop_invariant_code_motion(); assert_normalized_ssa_equals(ssa, expected); From 1777215fe1cf4bd4fdecc5f78a43e4e0ad8231d2 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 12 Dec 2024 03:53:21 +0000 Subject: [PATCH 4/5] fix expected to be normalized --- .../src/ssa/opt/loop_invariant.rs | 36 +++++++++---------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 23d2c5b5172..23617390d2f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -628,7 +628,6 @@ mod test { v25 = add v2, u32 1 jmp b1(v25) } - brillig(inline) fn foo f1 { b0(v0: [Field; 5]): return @@ -642,30 +641,29 @@ mod test { let expected = " brillig(inline) fn main f0 { b0(v0: u32, v1: u32): - v32 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] - v33 = allocate -> &mut [Field; 5] - v34 = array_set v32, index v0, value Field 64 - v35 = add v0, u32 1 - store v34 at v33 - v36 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] + v8 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] + v9 = allocate -> &mut [Field; 5] + v11 = array_set v8, index v0, value Field 64 + v13 = add v0, u32 1 + store v11 at v9 + v14 = make_array [Field 1, Field 2, Field 3, Field 4, Field 5] : [Field; 5] jmp b1(u32 0) b1(v2: u32): - v37 = lt v2, u32 5 - jmpif v37 then: b3, else: b2 + v17 = lt v2, u32 5 + jmpif v17 then: b3, else: b2 b2(): - v38 = load v33 -> [Field; 5] - call f1(v38) + v18 = load v9 -> [Field; 5] + call f1(v18) return b3(): - inc_rc v36 - v39 = allocate -> &mut [Field; 5] - v40 = add v1, v2 - v41 = array_set v36, index v40, value Field 128 - call f1(v41) - v42 = add v2, u32 1 - jmp b1(v42) + inc_rc v14 + v20 = allocate -> &mut [Field; 5] + v21 = add v1, v2 + v23 = array_set v14, index v21, value Field 128 + call f1(v23) + v25 = add v2, u32 1 + jmp b1(v25) } - brillig(inline) fn foo f1 { b0(v0: [Field; 5]): return From 67052835ae0a73064f2b912f9edab63720071596 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 12 Dec 2024 19:22:08 +0000 Subject: [PATCH 5/5] pr comments --- .../src/ssa/opt/loop_invariant.rs | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 23617390d2f..86d256de1cd 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -9,7 +9,6 @@ //! We also check that we are not hoisting instructions with side effects. use acvm::{acir::AcirField, FieldElement}; use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; -use iter_extended::vecmap; use crate::ssa::{ ir::{ @@ -115,21 +114,15 @@ impl<'f> LoopInvariantContext<'f> { if hoist_invariant { self.inserter.push_instruction(instruction_id, pre_header); - // We track whether we may mutate MakeArray instructions before we deduplicate - // them but we still need to issue an extra inc_rc in case they're mutated afterward. + // If we are hoisting a MakeArray instruction, + // we need to issue an extra inc_rc in case they are mutated afterward. if matches!( self.inserter.function.dfg[instruction_id], Instruction::MakeArray { .. } ) { - let results = - self.inserter.function.dfg.instruction_results(instruction_id).to_vec(); - let results = vecmap(results, |value| self.inserter.resolve(value)); - assert_eq!( - results.len(), - 1, - "ICE: We expect only a single result from an `Instruction::MakeArray`" - ); - let inc_rc = Instruction::IncrementRc { value: results[0] }; + let result = + self.inserter.function.dfg.instruction_results(instruction_id)[0]; + let inc_rc = Instruction::IncrementRc { value: result }; let call_stack = self.inserter.function.dfg.get_call_stack(instruction_id); self.inserter .function