From 2a53545f4238c9b8535e6bc5b0720fa15f44f946 Mon Sep 17 00:00:00 2001 From: jfecher Date: Mon, 11 Mar 2024 09:11:09 -0500 Subject: [PATCH] fix: Fix brillig slowdown when assigning arrays in loops (#4472) # Description ## Problem\* Resolves https://github.com/noir-lang/noir/issues/3795 ## Summary\* This is an older version of https://github.com/noir-lang/noir/pull/4210 which undoes the change in https://github.com/noir-lang/noir/pull/4210/commits/d331ee27e63df73e7e294fba7031a4c0ab073294 due to a regression https://github.com/noir-lang/noir/issues/4332. This PR is not yet confirmed to work since I do not have a test case for it! @sirasistant, do you mind seeing if this fixes the regression issue? ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [ ] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../src/ssa/function_builder/mod.rs | 19 +++++++++++++-- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 5 ++++ .../brillig_cow_assign/Nargo.toml | 7 ++++++ .../brillig_cow_assign/Prover.toml | 2 ++ .../brillig_cow_assign/src/main.nr | 23 +++++++++++++++++++ 5 files changed, 54 insertions(+), 2 deletions(-) create mode 100644 test_programs/execution_success/brillig_cow_assign/Nargo.toml create mode 100644 test_programs/execution_success/brillig_cow_assign/Prover.toml create mode 100644 test_programs/execution_success/brillig_cow_assign/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 9d27554dcaa..bf34a47485b 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -393,10 +393,25 @@ impl FunctionBuilder { self.increment_array_reference_count(value); } } - Type::Array(..) | Type::Slice(..) => { - self.insert_instruction(Instruction::IncrementRc { value }, None); + typ @ Type::Array(..) | typ @ Type::Slice(..) => { // If there are nested arrays or slices, we wait until ArrayGet // is issued to increment the count of that array. + self.insert_instruction(Instruction::IncrementRc { value }, None); + + // This is a bit odd, but in brillig the inc_rc instruction operates on + // a copy of the array's metadata, so we need to re-store a loaded array + // even if there have been no other changes to it. + if let Value::Instruction { instruction, .. } = &self.current_function.dfg[value] { + let instruction = &self.current_function.dfg[*instruction]; + if let Instruction::Load { address } = instruction { + // We can't re-use `value` in case the original address was stored + // to again in the meantime. So introduce another load. + let address = *address; + let value = self.insert_load(address, typ); + self.insert_instruction(Instruction::IncrementRc { value }, None); + self.insert_store(address, value); + } + } } } } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index d95295ae3c9..f3fa5d1d2f8 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -722,6 +722,11 @@ impl<'a> FunctionContext<'a> { let lhs = self.extract_current_value(&assign.lvalue)?; let rhs = self.codegen_expression(&assign.expression)?; + rhs.clone().for_each(|value| { + let value = value.eval(self); + self.builder.increment_array_reference_count(value); + }); + self.assign_new_value(lhs, rhs); Ok(Self::unit_value()) } diff --git a/test_programs/execution_success/brillig_cow_assign/Nargo.toml b/test_programs/execution_success/brillig_cow_assign/Nargo.toml new file mode 100644 index 00000000000..a878566a372 --- /dev/null +++ b/test_programs/execution_success/brillig_cow_assign/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "brillig_cow_assign" +type = "bin" +authors = [""] +compiler_version = ">=0.23.0" + +[dependencies] diff --git a/test_programs/execution_success/brillig_cow_assign/Prover.toml b/test_programs/execution_success/brillig_cow_assign/Prover.toml new file mode 100644 index 00000000000..882c73b83f8 --- /dev/null +++ b/test_programs/execution_success/brillig_cow_assign/Prover.toml @@ -0,0 +1,2 @@ +items_to_update = 10 +index = 6 diff --git a/test_programs/execution_success/brillig_cow_assign/src/main.nr b/test_programs/execution_success/brillig_cow_assign/src/main.nr new file mode 100644 index 00000000000..e5c3e2bd2f5 --- /dev/null +++ b/test_programs/execution_success/brillig_cow_assign/src/main.nr @@ -0,0 +1,23 @@ +global N = 10; + +unconstrained fn main() { + let mut arr = [0; N]; + let mut mid_change = arr; + + for i in 0..N { + if i == N / 2 { + mid_change = arr; + } + arr[i] = 27; + } + + // Expect: + // arr = [27, 27, 27, 27, 27, 27, 27, 27, 27, 27] + // mid_change = [27, 27, 27, 27, 27, 0, 0, 0, 0, 0] + + let modified_i = N / 2 + 1; + assert_eq(arr[modified_i], 27); + + // Fail here! + assert(mid_change[modified_i] != 27); +}