From bbbae8a821a602116cc6fdcf24da3e724b7fb483 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 26 Jan 2024 12:05:31 -0800 Subject: [PATCH 1/4] Add missing IncRc to assign statements --- compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 9d9635fed34..bbae05ae51c 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -678,6 +678,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()) } From ba6c9b1171025d7a4ce6c23c7c1e67267fc1885c Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 30 Jan 2024 12:14:17 -0600 Subject: [PATCH 2/4] Fix test and add test case --- .../src/ssa/function_builder/mod.rs | 24 +++++++++++++++++-- .../brillig_cow_assign/Nargo.toml | 7 ++++++ .../brillig_cow_assign/Prover.toml | 2 ++ .../brillig_cow_assign/src/main.nr | 23 ++++++++++++++++++ 4 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 44be423be10..c9dec342558 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -486,10 +486,30 @@ 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. + match &self.current_function.dfg[value] { + Value::Instruction { instruction, .. } => { + match &self.current_function.dfg[*instruction] { + Instruction::Load { address } => { + // 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/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); +} From 0c22691badbf4affeea3ecdf10ca86a29a04e577 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 30 Jan 2024 12:24:38 -0600 Subject: [PATCH 3/4] Satisfy Clippy --- .../src/ssa/function_builder/mod.rs | 23 ++++++++----------- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 2 +- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index c9dec342558..e7e8f00bd42 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -494,21 +494,16 @@ impl FunctionBuilder { // 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. - match &self.current_function.dfg[value] { - Value::Instruction { instruction, .. } => { - match &self.current_function.dfg[*instruction] { - Instruction::Load { address } => { - // 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); - } - _ => (), - } + 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 bbae05ae51c..259aab9f228 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -680,7 +680,7 @@ impl<'a> FunctionContext<'a> { rhs.clone().for_each(|value| { let value = value.eval(self); - self.builder.increment_array_reference_count(value) + self.builder.increment_array_reference_count(value); }); self.assign_new_value(lhs, rhs); From d331ee27e63df73e7e294fba7031a4c0ab073294 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 5 Feb 2024 18:22:23 +0000 Subject: [PATCH 4/4] fix: Remove hack in increment_array_reference_count (#4265) # Description ## Problem\* A followup to https://github.com/noir-lang/noir/pull/4210 ## Summary\* ## Additional Context ## Documentation\* Check one: - [ ] 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 | 17 +---------------- .../noirc_evaluator/src/ssa/ssa_gen/context.rs | 1 + 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index e7e8f00bd42..6e2d49b25c7 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -486,25 +486,10 @@ impl FunctionBuilder { self.increment_array_reference_count(value); } } - typ @ Type::Array(..) | typ @ Type::Slice(..) => { + Type::Array(..) | 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/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 0e155776545..56fe365cc25 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -997,6 +997,7 @@ impl<'a> FunctionContext<'a> { new_value.for_each(|value| { let value = value.eval(self); array = self.builder.insert_array_set(array, index, value); + self.builder.increment_array_reference_count(array); index = self.builder.insert_binary(index, BinaryOp::Add, one); }); array