diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/tests/memory.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/tests/memory.rs index 32444bf9644..5f9d9c7f63b 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/tests/memory.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/tests/memory.rs @@ -136,11 +136,9 @@ fn brillig_array_with_rc_ops() { 24: sp[5] = u32 add sp[3], sp[1] 25: store sp[2] at sp[5] 26: sp[2] = load sp[4] - 27: sp[2] = u32 sub sp[2], @2 - 28: store sp[2] at sp[4] - 29: sp[4] = u32 add sp[3], sp[1] - 30: sp[2] = load sp[4] - 31: sp[1] = sp[2] - 32: return + 27: sp[4] = u32 add sp[3], sp[1] + 28: sp[2] = load sp[4] + 29: sp[1] = sp[2] + 30: return "); } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_memory.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_memory.rs index cf10d941f84..471334a749c 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_memory.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_memory.rs @@ -499,11 +499,17 @@ impl BrilligContext< /// The inputs are: /// * the `pointer` to the array/vector /// * the `rc` address of the vector where we have the current RC loaded already - pub(crate) fn codegen_decrement_rc(&mut self, pointer: MemoryAddress, rc: MemoryAddress) { - // Modify the RC (it's on the stack, or scratch space). - self.codegen_usize_op_in_place(rc, BrilligBinaryOp::Sub, 1); - // Write it back onto the heap. - self.store_instruction(pointer, rc); + pub(crate) fn codegen_decrement_rc(&mut self, _pointer: MemoryAddress, _rc: MemoryAddress) { + // In benchmarks having this on didn't have a noticeable performance benefit, + // but it does have a small increase in byte code size and the number of executed opcodes. + // When we disabled `dec_rc` in SSA, the performance improved, so for now we disabled this, + // in order to not deviate conceptually from SSA. The method is left in place as a reference + // to where we could re-enable them. + + // // Modify the RC (it's on the stack, or scratch space). + // self.codegen_usize_op_in_place(rc, BrilligBinaryOp::Sub, 1); + // // Write it back onto the heap. + // self.store_instruction(pointer, rc); } /// Increment the ref-count by 1. diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs index 1e04d83b927..3db327c8d6d 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs @@ -1095,7 +1095,7 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { Value::ArrayOrSlice(array.clone()) } else { if !is_rc_one { - *array.rc.borrow_mut() -= 1; + Self::decrement_rc(&array); } let mut elements = array.elements.borrow().to_vec(); elements[index as usize] = value; @@ -1113,6 +1113,13 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { Ok(()) } + /// Decrement the ref-count of an array by 1. + fn decrement_rc(_array: &ArrayValue) { + // The decrement of the ref-count is currently disabled in SSA as well as the Brillig codegen, + // but we might re-enable it in the future if the ownership optimizations change. + // *array.rc.borrow_mut() -= 1; + } + fn interpret_inc_rc(&self, value_id: ValueId) -> IResult<()> { if self.in_unconstrained_context() { let array = self.lookup_array_or_slice(value_id, "inc_rc")?; diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs b/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs index 163b85aabb6..797ab7371b0 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs @@ -960,7 +960,7 @@ fn array_set_with_offset() { let v0 = values[0].as_array_or_slice().unwrap(); let v1 = values[1].as_array_or_slice().unwrap(); - assert_eq!(*v0.rc.borrow(), 1, "1+1-1; the copy of v1 decreases the RC of v0"); + assert_eq!(*v0.rc.borrow(), 2, "1+1-0; the copy of v1 does not decrease the RC of v0"); assert_eq!(*v1.rc.borrow(), 1); let one = from_constant(1u32.into(), NumericType::NativeField); diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs index 0aa7f9999bd..d6fd07a3d79 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs @@ -2373,6 +2373,8 @@ mod test { fn do_not_deduplicate_call_with_inc_rc() { // This test ensures that a function which mutates an array pointer is marked impure. // This protects against future deduplication passes incorrectly assuming purity. + // The increasing RC numbers reflect the current expectation that the RC of the + // original array does not get decremented when a copy is made. let src = r#" brillig(inline) fn main f0 { b0(v0: u32): @@ -2381,10 +2383,10 @@ mod test { constrain v5 == u32 1 v8 = call f1(v3) -> [Field; 2] v9 = call array_refcount(v3) -> u32 - constrain v9 == u32 1 + constrain v9 == u32 2 v11 = call f1(v3) -> [Field; 2] v12 = call array_refcount(v3) -> u32 - constrain v12 == u32 1 + constrain v12 == u32 3 inc_rc v3 v15 = array_set v3, index v0, value Field 9 return v3, v15 diff --git a/test_programs/execution_success/reference_counts_inliner_0/src/main.nr b/test_programs/execution_success/reference_counts_inliner_0/src/main.nr index 16027589382..29edda80f9a 100644 --- a/test_programs/execution_success/reference_counts_inliner_0/src/main.nr +++ b/test_programs/execution_success/reference_counts_inliner_0/src/main.nr @@ -119,10 +119,13 @@ fn regression_7297() { "There is 1 clone after `borrow_mut_two` and before `refcount_1` is defined (cloned before array_refcount call)", ); + // The current expectation is that the original array does not get its RC decremented when a copy is made. + let decrement_count = 0; + assert_eq( refcount_2, - refcount_1 + 3 - 1, - "after refcount_1 we clone once in passing array to copy_mut, once to array_refcount after, and once within copy_mut, but we decrease in copy_mut when copying", + refcount_1 + 3 - decrement_count, + "after refcount_1 we clone once in passing array to copy_mut, once to array_refcount after, and once within copy_mut", ); } } diff --git a/test_programs/execution_success/reference_counts_inliner_min/src/main.nr b/test_programs/execution_success/reference_counts_inliner_min/src/main.nr index 16027589382..29edda80f9a 100644 --- a/test_programs/execution_success/reference_counts_inliner_min/src/main.nr +++ b/test_programs/execution_success/reference_counts_inliner_min/src/main.nr @@ -119,10 +119,13 @@ fn regression_7297() { "There is 1 clone after `borrow_mut_two` and before `refcount_1` is defined (cloned before array_refcount call)", ); + // The current expectation is that the original array does not get its RC decremented when a copy is made. + let decrement_count = 0; + assert_eq( refcount_2, - refcount_1 + 3 - 1, - "after refcount_1 we clone once in passing array to copy_mut, once to array_refcount after, and once within copy_mut, but we decrease in copy_mut when copying", + refcount_1 + 3 - decrement_count, + "after refcount_1 we clone once in passing array to copy_mut, once to array_refcount after, and once within copy_mut", ); } } diff --git a/test_programs/execution_success/reference_counts_slices_inliner_0/src/main.nr b/test_programs/execution_success/reference_counts_slices_inliner_0/src/main.nr index 70eef60ca08..e885d71efca 100644 --- a/test_programs/execution_success/reference_counts_slices_inliner_0/src/main.nr +++ b/test_programs/execution_success/reference_counts_slices_inliner_0/src/main.nr @@ -131,6 +131,7 @@ unconstrained fn regression_10335(i: u32) { let rc_a = slice_refcount(a); let rc_b = slice_refcount(b); - assert_eq(rc_a, 1); + // The current expectation is that the original array does not get its RC decremented. + assert_eq(rc_a, 2); assert_eq(rc_b, 1); } diff --git a/tooling/nargo_cli/tests/snapshots/execution_success/reference_counts_inliner_0/execute__tests__expanded.snap b/tooling/nargo_cli/tests/snapshots/execution_success/reference_counts_inliner_0/execute__tests__expanded.snap index a243bc41b77..43425cb046f 100644 --- a/tooling/nargo_cli/tests/snapshots/execution_success/reference_counts_inliner_0/execute__tests__expanded.snap +++ b/tooling/nargo_cli/tests/snapshots/execution_success/reference_counts_inliner_0/execute__tests__expanded.snap @@ -96,9 +96,10 @@ fn regression_7297() { refcount_1 == 2_u32, "There is 1 clone after `borrow_mut_two` and before `refcount_1` is defined (cloned before array_refcount call)", ); + let decrement_count: u32 = 0_u32; assert( - refcount_2 == ((refcount_1 + 3_u32) - 1_u32), - "after refcount_1 we clone once in passing array to copy_mut, once to array_refcount after, and once within copy_mut, but we decrease in copy_mut when copying", + refcount_2 == ((refcount_1 + 3_u32) - decrement_count), + "after refcount_1 we clone once in passing array to copy_mut, once to array_refcount after, and once within copy_mut", ); } } diff --git a/tooling/nargo_cli/tests/snapshots/execution_success/reference_counts_inliner_min/execute__tests__expanded.snap b/tooling/nargo_cli/tests/snapshots/execution_success/reference_counts_inliner_min/execute__tests__expanded.snap index a243bc41b77..43425cb046f 100644 --- a/tooling/nargo_cli/tests/snapshots/execution_success/reference_counts_inliner_min/execute__tests__expanded.snap +++ b/tooling/nargo_cli/tests/snapshots/execution_success/reference_counts_inliner_min/execute__tests__expanded.snap @@ -96,9 +96,10 @@ fn regression_7297() { refcount_1 == 2_u32, "There is 1 clone after `borrow_mut_two` and before `refcount_1` is defined (cloned before array_refcount call)", ); + let decrement_count: u32 = 0_u32; assert( - refcount_2 == ((refcount_1 + 3_u32) - 1_u32), - "after refcount_1 we clone once in passing array to copy_mut, once to array_refcount after, and once within copy_mut, but we decrease in copy_mut when copying", + refcount_2 == ((refcount_1 + 3_u32) - decrement_count), + "after refcount_1 we clone once in passing array to copy_mut, once to array_refcount after, and once within copy_mut", ); } } diff --git a/tooling/nargo_cli/tests/snapshots/execution_success/reference_counts_slices_inliner_0/execute__tests__expanded.snap b/tooling/nargo_cli/tests/snapshots/execution_success/reference_counts_slices_inliner_0/execute__tests__expanded.snap index b7b4339cd10..0bf20ca294b 100644 --- a/tooling/nargo_cli/tests/snapshots/execution_success/reference_counts_slices_inliner_0/execute__tests__expanded.snap +++ b/tooling/nargo_cli/tests/snapshots/execution_success/reference_counts_slices_inliner_0/execute__tests__expanded.snap @@ -108,6 +108,6 @@ unconstrained fn regression_10335(i: u32) { b[i] = 3_Field; let rc_a: u32 = slice_refcount(a); let rc_b: u32 = slice_refcount(b); - assert(rc_a == 1_u32); + assert(rc_a == 2_u32); assert(rc_b == 1_u32); }