From 606acf7a4adc898ff5c90409fb5a8b60d61544ea Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 7 Mar 2025 10:54:59 -0600 Subject: [PATCH 01/11] Change decrement behavior --- .../src/brillig/brillig_gen/brillig_block.rs | 51 ++++---- .../src/ssa/ssa_gen/context.rs | 6 +- .../Nargo.toml | 2 +- .../Prover.toml | 0 .../reference_counts_inliner_0/src/main.nr | 119 ++++++++++++++++++ .../reference_counts_inliner_max/Nargo.toml | 7 ++ .../reference_counts_inliner_max/Prover.toml | 2 + .../src/main.nr | 36 +++--- .../reference_counts_inliner_min/Nargo.toml | 7 ++ .../reference_counts_inliner_min/Prover.toml | 2 + .../reference_counts_inliner_min/src/main.nr | 117 +++++++++++++++++ tooling/nargo_cli/build.rs | 27 +++- 12 files changed, 327 insertions(+), 49 deletions(-) rename test_programs/execution_success/{reference_counts => reference_counts_inliner_0}/Nargo.toml (70%) rename test_programs/execution_success/{reference_counts => reference_counts_inliner_0}/Prover.toml (100%) create mode 100644 test_programs/execution_success/reference_counts_inliner_0/src/main.nr create mode 100644 test_programs/execution_success/reference_counts_inliner_max/Nargo.toml create mode 100644 test_programs/execution_success/reference_counts_inliner_max/Prover.toml rename test_programs/execution_success/{reference_counts => reference_counts_inliner_max}/src/main.nr (76%) create mode 100644 test_programs/execution_success/reference_counts_inliner_min/Nargo.toml create mode 100644 test_programs/execution_success/reference_counts_inliner_min/Prover.toml create mode 100644 test_programs/execution_success/reference_counts_inliner_min/src/main.nr diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index d8f1f9d0997..1caea44eb71 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -20,7 +20,6 @@ use crate::ssa::ir::{ value::{Value, ValueId}, }; use acvm::acir::brillig::{MemoryAddress, ValueOrArray}; -use acvm::brillig_vm::MEMORY_ADDRESSING_BIT_SIZE; use acvm::{FieldElement, acir::AcirField}; use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use iter_extended::vecmap; @@ -872,12 +871,12 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { .store_instruction(array_or_vector.extract_register(), rc_register); self.brillig_context.deallocate_register(rc_register); } - Instruction::DecrementRc { value, original } => { + Instruction::DecrementRc { value, original: _ } => { let array_or_vector = self.convert_ssa_value(*value, dfg); - let orig_array_or_vector = self.convert_ssa_value(*original, dfg); + // let orig_array_or_vector = self.convert_ssa_value(*original, dfg); let array_register = array_or_vector.extract_register(); - let orig_array_register = orig_array_or_vector.extract_register(); + // let orig_array_register = orig_array_or_vector.extract_register(); let codegen_dec_rc = |ctx: &mut BrilligContext| { let rc_register = ctx.allocate_register(); @@ -909,28 +908,28 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { ctx.deallocate_register(rc_register); }; - if array_register == orig_array_register { - codegen_dec_rc(self.brillig_context); - } else { - // Check if the current and original register point at the same memory. - // If they don't, it means that an array-copy procedure has created a - // new array, which includes decrementing the RC of the original, - // which means we don't have to do the decrement here. - let condition = - SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); - // We will use a binary op to interpret the pointer as a number. - let bit_size: u32 = MEMORY_ADDRESSING_BIT_SIZE.into(); - let array_var = SingleAddrVariable::new(array_register, bit_size); - let orig_array_var = SingleAddrVariable::new(orig_array_register, bit_size); - self.brillig_context.binary_instruction( - array_var, - orig_array_var, - condition, - BrilligBinaryOp::Equals, - ); - self.brillig_context.codegen_if(condition.address, codegen_dec_rc); - self.brillig_context.deallocate_single_addr(condition); - } + // if array_register == orig_array_register { + codegen_dec_rc(self.brillig_context); + // } else { + // // Check if the current and original register point at the same memory. + // // If they don't, it means that an array-copy procedure has created a + // // new array, which includes decrementing the RC of the original, + // // which means we don't have to do the decrement here. + // let condition = + // SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); + // // We will use a binary op to interpret the pointer as a number. + // let bit_size: u32 = MEMORY_ADDRESSING_BIT_SIZE.into(); + // let array_var = SingleAddrVariable::new(array_register, bit_size); + // let orig_array_var = SingleAddrVariable::new(orig_array_register, bit_size); + // self.brillig_context.binary_instruction( + // array_var, + // orig_array_var, + // condition, + // BrilligBinaryOp::Equals, + // ); + // self.brillig_context.codegen_if(condition.address, codegen_dec_rc); + // self.brillig_context.deallocate_single_addr(condition); + // } } Instruction::EnableSideEffectsIf { .. } => { unreachable!("enable_side_effects not supported by brillig") diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 5db3ecb91b7..d0c1245a679 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -999,11 +999,15 @@ impl<'a> FunctionContext<'a> { mut incremented_params: Vec<(ValueId, ValueId)>, terminator_args: &[ValueId], ) { + // TODO: This check likely leads to unsoundness. + // It is here to avoid decrementing the RC of a parameter we're returning but we + // only check the exact ValueId which can be easily circumvented by storing to and + // loading from a temporary reference. incremented_params.retain(|(parameter, _)| !terminator_args.contains(parameter)); for (parameter, original) in incremented_params { if self.builder.current_function.dfg.value_is_reference(parameter) { - self.builder.decrement_array_reference_count(parameter, original); + self.builder.decrement_array_reference_count(original, original); } } } diff --git a/test_programs/execution_success/reference_counts/Nargo.toml b/test_programs/execution_success/reference_counts_inliner_0/Nargo.toml similarity index 70% rename from test_programs/execution_success/reference_counts/Nargo.toml rename to test_programs/execution_success/reference_counts_inliner_0/Nargo.toml index ae787e0ccb9..06967e1b893 100644 --- a/test_programs/execution_success/reference_counts/Nargo.toml +++ b/test_programs/execution_success/reference_counts_inliner_0/Nargo.toml @@ -1,5 +1,5 @@ [package] -name = "reference_counts" +name = "reference_counts_inliner_0" type = "bin" authors = [""] compiler_version = ">=0.35.0" diff --git a/test_programs/execution_success/reference_counts/Prover.toml b/test_programs/execution_success/reference_counts_inliner_0/Prover.toml similarity index 100% rename from test_programs/execution_success/reference_counts/Prover.toml rename to test_programs/execution_success/reference_counts_inliner_0/Prover.toml 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 new file mode 100644 index 00000000000..420f4fb8ca5 --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_0/src/main.nr @@ -0,0 +1,119 @@ +use std::mem::array_refcount; + +fn main() { + let mut array = [0, 1, 2]; + assert_refcount(array, 1); + + borrow(array, array_refcount(array)); + borrow_mut(&mut array, array_refcount(array)); + let _ = copy_mut(array, array_refcount(array)); + + borrow_mut_two(&mut array, &mut array, array_refcount(array)); + + let mut u32_array = [0, 1, 2]; + let rc1 = array_refcount(array); + let rc2 = array_refcount(u32_array); + borrow_mut_two_separate(&mut array, &mut u32_array, rc1, rc2); + + // Safety: test + regression_7297(); +} + +fn borrow(array: [Field; 3], rc_before_call: u32) { + assert_refcount(array, rc_before_call); + println(array[0]); +} + +fn borrow_mut(array: &mut [Field; 3], rc_before_call: u32) { + // Optimization: inc_rc isn't needed since there is only one array (`array`) + // of the same type that `array` can be modified through + assert_refcount(*array, rc_before_call + 0); + array[0] = 3; + println(array[0]); +} + +// Returning a copy of the array, otherwise the SSA can end up optimizing away +// the `array_set`, with the whole body just becoming basically `println(4);`. +fn copy_mut(mut array: [Field; 3], rc_before_call: u32) -> [Field; 3] { + assert_refcount(array, rc_before_call + 1); + array[0] = 4; + println(array[0]); + array +} + +/// Borrow the same array mutably through both parameters, inc_rc is necessary here, although +/// only one is needed to bring the rc from 1 to 2. +fn borrow_mut_two(array1: &mut [Field; 3], array2: &mut [Field; 3], rc_before_call: u32) { + // With an inliner value of 0, `borrow_mut_two` is inlined and the mutation below is optimized + // out. Afterward, the rc inc+dec gets removed since there is no longer a mutation in between. + assert_refcount(*array1, rc_before_call + 0); + assert_refcount(*array2, rc_before_call + 0); + array1[0] = 5; + array2[0] = 6; + println(array1[0]); // array1 & 2 alias, so this should also print 6 + println(array2[0]); +} + +/// Borrow a different array: we should be able to reason that these types cannot be mutably +/// aliased since they're different types so we don't need any inc_rc instructions. +fn borrow_mut_two_separate( + array1: &mut [Field; 3], + array2: &mut [u32; 3], + rc_before_call1: u32, + rc_before_call2: u32, +) { + assert_refcount(*array1, rc_before_call1 + 0); + assert_refcount(*array2, rc_before_call2 + 0); + array1[0] = 7; + array2[0] = 8; + println(array1[0]); + println(array2[0]); +} + +fn assert_refcount(array: [T; 3], expected: u32) { + let count = array_refcount(array); + + // All ref counts are zero when running this as a constrained program + if std::runtime::is_unconstrained() { + if count != expected { + // Brillig doesn't print the actual & expected arguments on assertion failure + println(f"actual = {count}, expected = {expected}"); + } + assert_eq(count, expected); + } else { + assert_eq(count, 0); + } +} + +fn regression_7297() { + let mut array = [0, 1, 2]; + + let refcount_0 = array_refcount(array); + borrow_mut_two(&mut array, &mut array, refcount_0); + let refcount_1 = array_refcount(array); + let array_2 = copy_mut(array, refcount_1); + let refcount_2 = array_refcount(array); + + println([refcount_0, refcount_1, refcount_2]); + + // Mutation of the original could occur if we double decremented the RC and then went back to 1 by accident. + // For this to come out we have to run the test with `--inliner-aggressiveness -9223372036854775808` + assert_eq(array[0], 6, "the original should not be mutated by copy_mut, only borrow_mut_two"); + assert_eq(array_2[0], 4, "the copy should have the expected content"); + + if std::runtime::is_unconstrained() { + // Double decrementing the RC could occur if we don't realize that array mutation made a copy, + // which decreases the RC of the original and sets the new one to 1. + // This assertion is redundant with the one following it, but it's here because `assert_eq` doesn't print + // what actual values that cause it to fail, so this is a way to highlight the bug about the refcount of + // still live arrays going to zero, without any doubt that it's just not 1, as it should be. + assert(refcount_1 != 0, "borrow_mut_two should create a fresh array and not decrease its RC"); + assert_eq(refcount_1, 1, "borrow_mut_two should create a fresh array with an RC of 1"); + // XXX: This assertion is here to demonstrate that this is happening, not because it must be like this + assert_eq( + refcount_2, + refcount_1 + 1, + "not ideal, but original RC permanently increased by copy_mut", + ); + } +} diff --git a/test_programs/execution_success/reference_counts_inliner_max/Nargo.toml b/test_programs/execution_success/reference_counts_inliner_max/Nargo.toml new file mode 100644 index 00000000000..654bcb5281d --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_max/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "reference_counts_inliner_max" +type = "bin" +authors = [""] +compiler_version = ">=0.35.0" + +[dependencies] diff --git a/test_programs/execution_success/reference_counts_inliner_max/Prover.toml b/test_programs/execution_success/reference_counts_inliner_max/Prover.toml new file mode 100644 index 00000000000..c01dd9462d8 --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_max/Prover.toml @@ -0,0 +1,2 @@ +x = 5 +b = true diff --git a/test_programs/execution_success/reference_counts/src/main.nr b/test_programs/execution_success/reference_counts_inliner_max/src/main.nr similarity index 76% rename from test_programs/execution_success/reference_counts/src/main.nr rename to test_programs/execution_success/reference_counts_inliner_max/src/main.nr index 2ee8a13f7a4..d062132e374 100644 --- a/test_programs/execution_success/reference_counts/src/main.nr +++ b/test_programs/execution_success/reference_counts_inliner_max/src/main.nr @@ -16,9 +16,7 @@ fn main() { borrow_mut_two_separate(&mut array, &mut u32_array, rc1, rc2); // Safety: test - unsafe { - regression_7297(); - } + regression_7297(); } fn borrow(array: [Field; 3], rc_before_call: u32) { @@ -85,8 +83,7 @@ fn assert_refcount(array: [T; 3], expected: u32) { } } -unconstrained fn regression_7297() { - println("regression_7297"); +fn regression_7297() { let mut array = [0, 1, 2]; let refcount_0 = array_refcount(array); @@ -101,17 +98,20 @@ unconstrained fn regression_7297() { // For this to come out we have to run the test with `--inliner-aggressiveness -9223372036854775808` assert_eq(array[0], 6, "the original should not be mutated by copy_mut, only borrow_mut_two"); assert_eq(array_2[0], 4, "the copy should have the expected content"); - // Double decrementing the RC could occur if we don't realize that array mutation made a copy, - // which decreases the RC of the original and sets the new one to 1. - // This assertion is redundant with the one following it, but it's here because `assert_eq` doesn't print - // what actual values that cause it to fail, so this is a way to highlight the bug about the refcount of - // still live arrays going to zero, without any doubt that it's just not 1, as it should be. - assert(refcount_1 != 0, "borrow_mut_two should create a fresh array and not decrease its RC"); - assert_eq(refcount_1, 1, "borrow_mut_two should create a fresh array with an RC of 1"); - // XXX: This assertion is here to demonstrate that this is happening, not because it must be like this - assert_eq( - refcount_2, - refcount_1 + 1, - "not ideal, but original RC permanently increased by copy_mut", - ); + + if std::runtime::is_unconstrained() { + // Double decrementing the RC could occur if we don't realize that array mutation made a copy, + // which decreases the RC of the original and sets the new one to 1. + // This assertion is redundant with the one following it, but it's here because `assert_eq` doesn't print + // what actual values that cause it to fail, so this is a way to highlight the bug about the refcount of + // still live arrays going to zero, without any doubt that it's just not 1, as it should be. + assert(refcount_1 != 0, "borrow_mut_two should create a fresh array and not decrease its RC"); + assert_eq(refcount_1, 1, "borrow_mut_two should create a fresh array with an RC of 1"); + // XXX: This assertion is here to demonstrate that this is happening, not because it must be like this + assert_eq( + refcount_2, + refcount_1 + 1, + "not ideal, but original RC permanently increased by copy_mut", + ); + } } diff --git a/test_programs/execution_success/reference_counts_inliner_min/Nargo.toml b/test_programs/execution_success/reference_counts_inliner_min/Nargo.toml new file mode 100644 index 00000000000..92e22d8529c --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_min/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "reference_counts_inliner_min" +type = "bin" +authors = [""] +compiler_version = ">=0.35.0" + +[dependencies] diff --git a/test_programs/execution_success/reference_counts_inliner_min/Prover.toml b/test_programs/execution_success/reference_counts_inliner_min/Prover.toml new file mode 100644 index 00000000000..c01dd9462d8 --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_min/Prover.toml @@ -0,0 +1,2 @@ +x = 5 +b = true 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 new file mode 100644 index 00000000000..d062132e374 --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_min/src/main.nr @@ -0,0 +1,117 @@ +use std::mem::array_refcount; + +fn main() { + let mut array = [0, 1, 2]; + assert_refcount(array, 1); + + borrow(array, array_refcount(array)); + borrow_mut(&mut array, array_refcount(array)); + let _ = copy_mut(array, array_refcount(array)); + + borrow_mut_two(&mut array, &mut array, array_refcount(array)); + + let mut u32_array = [0, 1, 2]; + let rc1 = array_refcount(array); + let rc2 = array_refcount(u32_array); + borrow_mut_two_separate(&mut array, &mut u32_array, rc1, rc2); + + // Safety: test + regression_7297(); +} + +fn borrow(array: [Field; 3], rc_before_call: u32) { + assert_refcount(array, rc_before_call); + println(array[0]); +} + +fn borrow_mut(array: &mut [Field; 3], rc_before_call: u32) { + // Optimization: inc_rc isn't needed since there is only one array (`array`) + // of the same type that `array` can be modified through + assert_refcount(*array, rc_before_call + 0); + array[0] = 3; + println(array[0]); +} + +// Returning a copy of the array, otherwise the SSA can end up optimizing away +// the `array_set`, with the whole body just becoming basically `println(4);`. +fn copy_mut(mut array: [Field; 3], rc_before_call: u32) -> [Field; 3] { + assert_refcount(array, rc_before_call + 1); + array[0] = 4; + println(array[0]); + array +} + +/// Borrow the same array mutably through both parameters, inc_rc is necessary here, although +/// only one is needed to bring the rc from 1 to 2. +fn borrow_mut_two(array1: &mut [Field; 3], array2: &mut [Field; 3], rc_before_call: u32) { + assert_refcount(*array1, rc_before_call + 1); + assert_refcount(*array2, rc_before_call + 1); + array1[0] = 5; + array2[0] = 6; + println(array1[0]); // array1 & 2 alias, so this should also print 6 + println(array2[0]); +} + +/// Borrow a different array: we should be able to reason that these types cannot be mutably +/// aliased since they're different types so we don't need any inc_rc instructions. +fn borrow_mut_two_separate( + array1: &mut [Field; 3], + array2: &mut [u32; 3], + rc_before_call1: u32, + rc_before_call2: u32, +) { + assert_refcount(*array1, rc_before_call1 + 0); + assert_refcount(*array2, rc_before_call2 + 0); + array1[0] = 7; + array2[0] = 8; + println(array1[0]); + println(array2[0]); +} + +fn assert_refcount(array: [T; 3], expected: u32) { + let count = array_refcount(array); + + // All ref counts are zero when running this as a constrained program + if std::runtime::is_unconstrained() { + if count != expected { + // Brillig doesn't print the actual & expected arguments on assertion failure + println(f"actual = {count}, expected = {expected}"); + } + assert_eq(count, expected); + } else { + assert_eq(count, 0); + } +} + +fn regression_7297() { + let mut array = [0, 1, 2]; + + let refcount_0 = array_refcount(array); + borrow_mut_two(&mut array, &mut array, refcount_0); + let refcount_1 = array_refcount(array); + let array_2 = copy_mut(array, refcount_1); + let refcount_2 = array_refcount(array); + + println([refcount_0, refcount_1, refcount_2]); + + // Mutation of the original could occur if we double decremented the RC and then went back to 1 by accident. + // For this to come out we have to run the test with `--inliner-aggressiveness -9223372036854775808` + assert_eq(array[0], 6, "the original should not be mutated by copy_mut, only borrow_mut_two"); + assert_eq(array_2[0], 4, "the copy should have the expected content"); + + if std::runtime::is_unconstrained() { + // Double decrementing the RC could occur if we don't realize that array mutation made a copy, + // which decreases the RC of the original and sets the new one to 1. + // This assertion is redundant with the one following it, but it's here because `assert_eq` doesn't print + // what actual values that cause it to fail, so this is a way to highlight the bug about the refcount of + // still live arrays going to zero, without any doubt that it's just not 1, as it should be. + assert(refcount_1 != 0, "borrow_mut_two should create a fresh array and not decrease its RC"); + assert_eq(refcount_1, 1, "borrow_mut_two should create a fresh array with an RC of 1"); + // XXX: This assertion is here to demonstrate that this is happening, not because it must be like this + assert_eq( + refcount_2, + refcount_1 + 1, + "not ideal, but original RC permanently increased by copy_mut", + ); + } +} diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index b9faf018dfd..ab850312904 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -60,10 +60,20 @@ const IGNORED_BRILLIG_TESTS: [&str; 10] = [ "is_unconstrained", ]; -/// Tests which aren't expected to work with the default inliner cases. -const INLINER_MIN_OVERRIDES: [(&str, i64); 1] = [ +/// Tests which aren't expected to work with the default minimum inliner cases. +const INLINER_MIN_OVERRIDES: [(&str, i64); 4] = [ // 0 works if PoseidonHasher::write is tagged as `inline_always`, otherwise 22. ("eddsa", 0), + ("reference_counts_inliner_0", 0), + ("reference_counts_inliner_min", i64::MIN), + ("reference_counts_inliner_max", i64::MAX), +]; + +/// Tests which aren't expected to work with the default maximum inliner cases. +const INLINER_MAX_OVERRIDES: [(&str, i64); 3] = [ + ("reference_counts_inliner_0", 0), + ("reference_counts_inliner_min", i64::MIN), + ("reference_counts_inliner_max", i64::MAX), ]; /// Some tests are expected to have warnings @@ -113,6 +123,8 @@ struct MatrixConfig { vary_inliner: bool, // If there is a non-default minimum inliner aggressiveness to use with the brillig tests. min_inliner: i64, + // If there is a non-default maximum inliner aggressiveness to use with the brillig tests. + max_inliner: i64, } // Enum to be able to preserve readable test labels and also compare to numbers. @@ -161,6 +173,9 @@ fn generate_test_cases( if !cases.iter().any(|c| c.value() == matrix_config.min_inliner) { cases.push(Inliner::Custom(matrix_config.min_inliner)); } + if !cases.iter().any(|c| c.value() == matrix_config.max_inliner) { + cases.push(Inliner::Custom(matrix_config.max_inliner)); + } cases } else { vec![Inliner::Default] @@ -171,7 +186,8 @@ fn generate_test_cases( let mut test_cases = Vec::new(); for brillig in &brillig_cases { for inliner in &inliner_cases { - if *brillig && inliner.value() < matrix_config.min_inliner { + let inliner_range = matrix_config.min_inliner..=matrix_config.max_inliner; + if *brillig && !inliner_range.contains(&inliner.value()) { continue; } test_cases.push(format!( @@ -246,6 +262,11 @@ fn generate_execution_success_tests(test_file: &mut File, test_data_dir: &Path) .find(|(n, _)| *n == test_name.as_str()) .map(|(_, i)| *i) .unwrap_or(i64::MIN), + max_inliner: INLINER_MAX_OVERRIDES + .iter() + .find(|(n, _)| *n == test_name.as_str()) + .map(|(_, i)| *i) + .unwrap_or(i64::MAX), }, ); } From 11ff841904f47bb47332f32068e5895d088da657 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 7 Mar 2025 11:06:06 -0600 Subject: [PATCH 02/11] fmt --- .../execution_success/reference_counts_inliner_0/src/main.nr | 5 ++++- .../reference_counts_inliner_max/src/main.nr | 5 ++++- .../reference_counts_inliner_min/src/main.nr | 5 ++++- 3 files changed, 12 insertions(+), 3 deletions(-) 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 420f4fb8ca5..aab730eef06 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 @@ -107,7 +107,10 @@ fn regression_7297() { // This assertion is redundant with the one following it, but it's here because `assert_eq` doesn't print // what actual values that cause it to fail, so this is a way to highlight the bug about the refcount of // still live arrays going to zero, without any doubt that it's just not 1, as it should be. - assert(refcount_1 != 0, "borrow_mut_two should create a fresh array and not decrease its RC"); + assert( + refcount_1 != 0, + "borrow_mut_two should create a fresh array and not decrease its RC", + ); assert_eq(refcount_1, 1, "borrow_mut_two should create a fresh array with an RC of 1"); // XXX: This assertion is here to demonstrate that this is happening, not because it must be like this assert_eq( diff --git a/test_programs/execution_success/reference_counts_inliner_max/src/main.nr b/test_programs/execution_success/reference_counts_inliner_max/src/main.nr index d062132e374..7eda848f851 100644 --- a/test_programs/execution_success/reference_counts_inliner_max/src/main.nr +++ b/test_programs/execution_success/reference_counts_inliner_max/src/main.nr @@ -105,7 +105,10 @@ fn regression_7297() { // This assertion is redundant with the one following it, but it's here because `assert_eq` doesn't print // what actual values that cause it to fail, so this is a way to highlight the bug about the refcount of // still live arrays going to zero, without any doubt that it's just not 1, as it should be. - assert(refcount_1 != 0, "borrow_mut_two should create a fresh array and not decrease its RC"); + assert( + refcount_1 != 0, + "borrow_mut_two should create a fresh array and not decrease its RC", + ); assert_eq(refcount_1, 1, "borrow_mut_two should create a fresh array with an RC of 1"); // XXX: This assertion is here to demonstrate that this is happening, not because it must be like this assert_eq( 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 d062132e374..7eda848f851 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 @@ -105,7 +105,10 @@ fn regression_7297() { // This assertion is redundant with the one following it, but it's here because `assert_eq` doesn't print // what actual values that cause it to fail, so this is a way to highlight the bug about the refcount of // still live arrays going to zero, without any doubt that it's just not 1, as it should be. - assert(refcount_1 != 0, "borrow_mut_two should create a fresh array and not decrease its RC"); + assert( + refcount_1 != 0, + "borrow_mut_two should create a fresh array and not decrease its RC", + ); assert_eq(refcount_1, 1, "borrow_mut_two should create a fresh array with an RC of 1"); // XXX: This assertion is here to demonstrate that this is happening, not because it must be like this assert_eq( From 19c19c0e244377a8bf1d677af7825a631637f8f3 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 7 Mar 2025 11:33:42 -0600 Subject: [PATCH 03/11] Update ignored dbg tests --- tooling/debugger/ignored-tests.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tooling/debugger/ignored-tests.txt b/tooling/debugger/ignored-tests.txt index 1515e58a90f..3f26d0c1c12 100644 --- a/tooling/debugger/ignored-tests.txt +++ b/tooling/debugger/ignored-tests.txt @@ -2,7 +2,9 @@ brillig_references debug_logs is_unconstrained macros -reference_counts +reference_counts_inliner_0 +reference_counts_inliner_min +reference_counts_inliner_max references regression_4709 regression_7323 From b1dd380edb28893b808785c84ec1ed8474c7fb59 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 7 Mar 2025 11:42:07 -0600 Subject: [PATCH 04/11] Change expected rcs --- .../reference_counts_inliner_0/src/main.nr | 6 ++--- .../reference_counts_inliner_max/src/main.nr | 23 +++++++++++-------- 2 files changed, 15 insertions(+), 14 deletions(-) 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 aab730eef06..7eda848f851 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 @@ -44,10 +44,8 @@ fn copy_mut(mut array: [Field; 3], rc_before_call: u32) -> [Field; 3] { /// Borrow the same array mutably through both parameters, inc_rc is necessary here, although /// only one is needed to bring the rc from 1 to 2. fn borrow_mut_two(array1: &mut [Field; 3], array2: &mut [Field; 3], rc_before_call: u32) { - // With an inliner value of 0, `borrow_mut_two` is inlined and the mutation below is optimized - // out. Afterward, the rc inc+dec gets removed since there is no longer a mutation in between. - assert_refcount(*array1, rc_before_call + 0); - assert_refcount(*array2, rc_before_call + 0); + assert_refcount(*array1, rc_before_call + 1); + assert_refcount(*array2, rc_before_call + 1); array1[0] = 5; array2[0] = 6; println(array1[0]); // array1 & 2 alias, so this should also print 6 diff --git a/test_programs/execution_success/reference_counts_inliner_max/src/main.nr b/test_programs/execution_success/reference_counts_inliner_max/src/main.nr index 7eda848f851..4af6ba9d8b0 100644 --- a/test_programs/execution_success/reference_counts_inliner_max/src/main.nr +++ b/test_programs/execution_success/reference_counts_inliner_max/src/main.nr @@ -89,15 +89,15 @@ fn regression_7297() { let refcount_0 = array_refcount(array); borrow_mut_two(&mut array, &mut array, refcount_0); let refcount_1 = array_refcount(array); - let array_2 = copy_mut(array, refcount_1); - let refcount_2 = array_refcount(array); + // let array_2 = copy_mut(array, refcount_1); + // let refcount_2 = array_refcount(array); - println([refcount_0, refcount_1, refcount_2]); + // println([refcount_0, refcount_1, refcount_2]); // Mutation of the original could occur if we double decremented the RC and then went back to 1 by accident. // For this to come out we have to run the test with `--inliner-aggressiveness -9223372036854775808` assert_eq(array[0], 6, "the original should not be mutated by copy_mut, only borrow_mut_two"); - assert_eq(array_2[0], 4, "the copy should have the expected content"); + // assert_eq(array_2[0], 4, "the copy should have the expected content"); if std::runtime::is_unconstrained() { // Double decrementing the RC could occur if we don't realize that array mutation made a copy, @@ -109,12 +109,15 @@ fn regression_7297() { refcount_1 != 0, "borrow_mut_two should create a fresh array and not decrease its RC", ); - assert_eq(refcount_1, 1, "borrow_mut_two should create a fresh array with an RC of 1"); + + // Difference from other inliner cases. With inliner max we fail to remove a inc_ref + assert_eq(refcount_1, 2); + // XXX: This assertion is here to demonstrate that this is happening, not because it must be like this - assert_eq( - refcount_2, - refcount_1 + 1, - "not ideal, but original RC permanently increased by copy_mut", - ); + // assert_eq( + // refcount_2, + // refcount_1 + 1, + // "not ideal, but original RC permanently increased by copy_mut", + // ); } } From 9dbb50c461732a9c91990e3c584e47b895d7aa2f Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 7 Mar 2025 13:55:19 -0600 Subject: [PATCH 05/11] Uncomment lines --- .../reference_counts_inliner_max/src/main.nr | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test_programs/execution_success/reference_counts_inliner_max/src/main.nr b/test_programs/execution_success/reference_counts_inliner_max/src/main.nr index 4af6ba9d8b0..0c9eb0d873b 100644 --- a/test_programs/execution_success/reference_counts_inliner_max/src/main.nr +++ b/test_programs/execution_success/reference_counts_inliner_max/src/main.nr @@ -89,15 +89,15 @@ fn regression_7297() { let refcount_0 = array_refcount(array); borrow_mut_two(&mut array, &mut array, refcount_0); let refcount_1 = array_refcount(array); - // let array_2 = copy_mut(array, refcount_1); - // let refcount_2 = array_refcount(array); + let array_2 = copy_mut(array, refcount_1); + let refcount_2 = array_refcount(array); - // println([refcount_0, refcount_1, refcount_2]); + println([refcount_0, refcount_1, refcount_2]); // Mutation of the original could occur if we double decremented the RC and then went back to 1 by accident. // For this to come out we have to run the test with `--inliner-aggressiveness -9223372036854775808` assert_eq(array[0], 6, "the original should not be mutated by copy_mut, only borrow_mut_two"); - // assert_eq(array_2[0], 4, "the copy should have the expected content"); + assert_eq(array_2[0], 4, "the copy should have the expected content"); if std::runtime::is_unconstrained() { // Double decrementing the RC could occur if we don't realize that array mutation made a copy, @@ -114,10 +114,10 @@ fn regression_7297() { assert_eq(refcount_1, 2); // XXX: This assertion is here to demonstrate that this is happening, not because it must be like this - // assert_eq( - // refcount_2, - // refcount_1 + 1, - // "not ideal, but original RC permanently increased by copy_mut", - // ); + assert_eq( + refcount_2, + refcount_1 + 1, + "not ideal, but original RC permanently increased by copy_mut", + ); } } From 6bba6121779125d43021e434c2fe80b5f8d3381f Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 7 Mar 2025 14:09:46 -0600 Subject: [PATCH 06/11] Remove commented code --- .../src/brillig/brillig_gen/brillig_block.rs | 83 +++++++------------ .../src/ssa/function_builder/mod.rs | 28 +++---- .../noirc_evaluator/src/ssa/ir/instruction.rs | 19 ++--- .../noirc_evaluator/src/ssa/ir/printer.rs | 4 +- .../src/ssa/ssa_gen/context.rs | 2 +- 5 files changed, 48 insertions(+), 88 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 1caea44eb71..63250ff65bb 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -871,65 +871,40 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { .store_instruction(array_or_vector.extract_register(), rc_register); self.brillig_context.deallocate_register(rc_register); } - Instruction::DecrementRc { value, original: _ } => { + Instruction::DecrementRc { value } => { let array_or_vector = self.convert_ssa_value(*value, dfg); - // let orig_array_or_vector = self.convert_ssa_value(*original, dfg); - let array_register = array_or_vector.extract_register(); - // let orig_array_register = orig_array_or_vector.extract_register(); - - let codegen_dec_rc = |ctx: &mut BrilligContext| { - let rc_register = ctx.allocate_register(); - - ctx.load_instruction(rc_register, array_register); - - ctx.codegen_usize_op_in_place(rc_register, BrilligBinaryOp::Sub, 1); - // Check that the refcount didn't go below 1. If we allow it to underflow - // and become 0 or usize::MAX, and then return to 1, then it will indicate - // an array as mutable when it probably shouldn't be. - if ctx.enable_debug_assertions() { - let min_addr = ReservedRegisters::usize_one(); - let condition = SingleAddrVariable::new(ctx.allocate_register(), 1); - ctx.memory_op_instruction( - min_addr, - rc_register, - condition.address, - BrilligBinaryOp::LessThanEquals, - ); - ctx.codegen_constrain( - condition, - Some("array ref-count went to zero".to_owned()), - ); - ctx.deallocate_single_addr(condition); - } + let rc_register = self.brillig_context.allocate_register(); + self.brillig_context.load_instruction(rc_register, array_register); - ctx.store_instruction(array_register, rc_register); - ctx.deallocate_register(rc_register); - }; + // Check that the refcount isn't already 0 before we decrement. If we allow it to underflow + // and become usize::MAX, and then return to 1, then it will indicate + // an array as mutable when it probably shouldn't be. + if self.brillig_context.enable_debug_assertions() { + let min_addr = ReservedRegisters::usize_one(); + let condition = + SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); + self.brillig_context.memory_op_instruction( + min_addr, + rc_register, + condition.address, + BrilligBinaryOp::LessThan, + ); + self.brillig_context.codegen_constrain( + condition, + Some("array ref-count underflowed below zero".to_owned()), + ); + self.brillig_context.deallocate_single_addr(condition); + } - // if array_register == orig_array_register { - codegen_dec_rc(self.brillig_context); - // } else { - // // Check if the current and original register point at the same memory. - // // If they don't, it means that an array-copy procedure has created a - // // new array, which includes decrementing the RC of the original, - // // which means we don't have to do the decrement here. - // let condition = - // SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); - // // We will use a binary op to interpret the pointer as a number. - // let bit_size: u32 = MEMORY_ADDRESSING_BIT_SIZE.into(); - // let array_var = SingleAddrVariable::new(array_register, bit_size); - // let orig_array_var = SingleAddrVariable::new(orig_array_register, bit_size); - // self.brillig_context.binary_instruction( - // array_var, - // orig_array_var, - // condition, - // BrilligBinaryOp::Equals, - // ); - // self.brillig_context.codegen_if(condition.address, codegen_dec_rc); - // self.brillig_context.deallocate_single_addr(condition); - // } + self.brillig_context.codegen_usize_op_in_place( + rc_register, + BrilligBinaryOp::Sub, + 1, + ); + self.brillig_context.store_instruction(array_register, rc_register); + self.brillig_context.deallocate_register(rc_register); } Instruction::EnableSideEffectsIf { .. } => { unreachable!("enable_side_effects not supported by brillig") diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 95944129c4e..66643fc3beb 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -394,8 +394,8 @@ impl FunctionBuilder { /// Insert an instruction to decrement an array's reference count. This only has an effect /// in unconstrained code where arrays are reference counted and copy on write. - pub(crate) fn insert_dec_rc(&mut self, value: ValueId, original: ValueId) { - self.insert_instruction(Instruction::DecrementRc { value, original }, None); + pub(crate) fn insert_dec_rc(&mut self, value: ValueId) { + self.insert_instruction(Instruction::DecrementRc { value }, None); } /// Insert an enable_side_effects_if instruction. These are normally only automatically @@ -497,7 +497,7 @@ impl FunctionBuilder { /// /// Returns the ID of the array that was affected, if any. pub(crate) fn increment_array_reference_count(&mut self, value: ValueId) -> Option { - self.update_array_reference_count(value, None) + self.update_array_reference_count(value, true) } /// Insert instructions to decrement the reference count of any array(s) stored @@ -505,12 +505,8 @@ impl FunctionBuilder { /// any arrays, this does nothing. /// /// Returns the ID of the array that was affected, if any. - pub(crate) fn decrement_array_reference_count( - &mut self, - value: ValueId, - original: ValueId, - ) -> Option { - self.update_array_reference_count(value, Some(original)) + pub(crate) fn decrement_array_reference_count(&mut self, value: ValueId) -> Option { + self.update_array_reference_count(value, false) } /// Increment or decrement the given value's reference count if it is an array. @@ -522,11 +518,7 @@ impl FunctionBuilder { /// the meantime, which in itself would make sure its RC is decremented. /// /// Returns the ID of the array that was affected, if any. - fn update_array_reference_count( - &mut self, - value: ValueId, - original: Option, - ) -> Option { + fn update_array_reference_count(&mut self, value: ValueId, increment: bool) -> Option { if self.current_function.runtime().is_acir() { return None; } @@ -536,7 +528,7 @@ impl FunctionBuilder { if element.contains_an_array() { let reference = value; let value = self.insert_load(reference, element.as_ref().clone()); - self.update_array_reference_count(value, original); + self.update_array_reference_count(value, increment); Some(value) } else { None @@ -545,10 +537,10 @@ impl FunctionBuilder { Type::Array(..) | Type::Slice(..) => { // If there are nested arrays or slices, we wait until ArrayGet // is issued to increment the count of that array. - if let Some(original) = original { - self.insert_dec_rc(value, original); - } else { + if increment { self.insert_inc_rc(value); + } else { + self.insert_dec_rc(value); } Some(value) } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 1bef9079eb8..dce437ac2a4 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -325,9 +325,7 @@ pub(crate) enum Instruction { /// This currently only has an effect in Brillig code where array sharing and copy on write is /// implemented via reference counting. In ACIR code this is done with im::Vector and these /// DecrementRc instructions are ignored. - /// - /// The `original` contains the value of the array which was incremented by the pair of this decrement. - DecrementRc { value: ValueId, original: ValueId }, + DecrementRc { value: ValueId }, /// Merge two values returned from opposite branches of a conditional into one. /// @@ -724,9 +722,7 @@ impl Instruction { mutable: *mutable, }, Instruction::IncrementRc { value } => Instruction::IncrementRc { value: f(*value) }, - Instruction::DecrementRc { value, original } => { - Instruction::DecrementRc { value: f(*value), original: f(*original) } - } + Instruction::DecrementRc { value } => Instruction::DecrementRc { value: f(*value) }, Instruction::RangeCheck { value, max_bit_size, assert_message } => { Instruction::RangeCheck { value: f(*value), @@ -797,9 +793,8 @@ impl Instruction { *value = f(*value); } Instruction::IncrementRc { value } => *value = f(*value), - Instruction::DecrementRc { value, original } => { + Instruction::DecrementRc { value } => { *value = f(*value); - *original = f(*original); } Instruction::RangeCheck { value, max_bit_size: _, assert_message: _ } => { *value = f(*value); @@ -866,12 +861,10 @@ impl Instruction { Instruction::EnableSideEffectsIf { condition } => { f(*condition); } - Instruction::IncrementRc { value } | Instruction::RangeCheck { value, .. } => { - f(*value); - } - Instruction::DecrementRc { value, original } => { + Instruction::IncrementRc { value } + | Instruction::DecrementRc { value } + | Instruction::RangeCheck { value, .. } => { f(*value); - f(*original); } Instruction::IfElse { then_condition, then_value, else_condition, else_value } => { f(*then_condition); diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 0fac2242abd..2809139da45 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -250,8 +250,8 @@ fn display_instruction_inner( Instruction::IncrementRc { value } => { writeln!(f, "inc_rc {}", show(*value)) } - Instruction::DecrementRc { value, original } => { - writeln!(f, "dec_rc {} {}", show(*value), show(*original)) + Instruction::DecrementRc { value } => { + writeln!(f, "dec_rc {}", show(*value)) } Instruction::RangeCheck { value, max_bit_size, .. } => { writeln!(f, "range_check {} to {} bits", show(*value), *max_bit_size,) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index d0c1245a679..1314781d0f3 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -1007,7 +1007,7 @@ impl<'a> FunctionContext<'a> { for (parameter, original) in incremented_params { if self.builder.current_function.dfg.value_is_reference(parameter) { - self.builder.decrement_array_reference_count(original, original); + self.builder.decrement_array_reference_count(original); } } } From 4e4b3bf55125c9e560c14a78636c9612c86d3238 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 7 Mar 2025 14:22:38 -0600 Subject: [PATCH 07/11] Update ssa parser & tests --- .../src/ssa/checks/check_for_underconstrained_values.rs | 4 ++-- compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs | 2 +- compiler/noirc_evaluator/src/ssa/opt/die.rs | 6 +++--- compiler/noirc_evaluator/src/ssa/opt/rc.rs | 6 +++--- compiler/noirc_evaluator/src/ssa/opt/unrolling.rs | 4 ++-- compiler/noirc_evaluator/src/ssa/parser/ast.rs | 1 - compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs | 5 ++--- compiler/noirc_evaluator/src/ssa/parser/mod.rs | 3 +-- compiler/noirc_evaluator/src/ssa/parser/tests.rs | 2 +- 9 files changed, 15 insertions(+), 18 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 752b2c82d09..d1e673773e0 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -1043,8 +1043,8 @@ mod test { v19 = call f1(v5) -> u32 v20 = add v8, v19 constrain v6 == v20 - dec_rc v4 v4 - dec_rc v5 v5 + dec_rc v4 + dec_rc v5 return } diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 3bde1d7530f..c46fc85d5ef 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -1446,7 +1446,7 @@ mod test { v2 = array_get v0, index u32 0 -> Field v4 = array_get v0, index u32 1 -> Field v5 = add v2, v4 - dec_rc v0 v0 + dec_rc v0 return v5 } "; diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index e742ad4aa5d..842946deb9e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -838,7 +838,7 @@ mod test { b0(v0: [Field; 2]): inc_rc v0 v2 = array_get v0, index u32 0 -> Field - dec_rc v0 v0 + dec_rc v0 return v2 } "; @@ -862,7 +862,7 @@ mod test { b0(v0: [Field; 2]): inc_rc v0 v2 = array_set v0, index u32 0, value u32 0 - dec_rc v0 v0 + dec_rc v0 return v2 } "; @@ -970,7 +970,7 @@ mod test { v3 = load v0 -> [Field; 3] v6 = array_set v3, index u32 0, value Field 5 store v6 at v0 - dec_rc v6 v1 + dec_rc v6 return } "; diff --git a/compiler/noirc_evaluator/src/ssa/opt/rc.rs b/compiler/noirc_evaluator/src/ssa/opt/rc.rs index 13137c94046..5fa60c7a3ec 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/rc.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/rc.rs @@ -190,7 +190,7 @@ mod test { b0(v0: [Field; 2]): inc_rc v0 inc_rc v0 - dec_rc v0 v0 + dec_rc v0 v1 = make_array [v0] : [[Field; 2]; 1] return v1 } @@ -218,7 +218,7 @@ mod test { v2 = load v1 -> [Field; 2] v5 = array_set v2, index u64 0, value Field 5 store v5 at v1 - dec_rc v0 v0 + dec_rc v0 return } "; @@ -250,7 +250,7 @@ mod test { v5 = array_set v2, index u64 0, value Field 5 store v5 at v0 v6 = load v0 -> [Field; 2] - dec_rc v6 v1 + dec_rc v1 store v6 at v0 return } diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 43afb9fa41a..5e47e056260 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -1287,7 +1287,7 @@ mod tests { jmp b1() b1(): v20 = load v3 -> [u64; 6] - dec_rc v0 v0 + dec_rc v0 return v20 } "; @@ -1463,7 +1463,7 @@ mod tests { jmp b1(v16) b2(): v8 = load v4 -> [u64; 6] - dec_rc v0 v0 + dec_rc v0 return v8 }} " diff --git a/compiler/noirc_evaluator/src/ssa/parser/ast.rs b/compiler/noirc_evaluator/src/ssa/parser/ast.rs index 7bc88a1fde1..05743ffd7ca 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/ast.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/ast.rs @@ -112,7 +112,6 @@ pub(crate) enum ParsedInstruction { }, DecrementRc { value: ParsedValue, - original: ParsedValue, }, EnableSideEffectsIf { condition: ParsedValue, diff --git a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs index d7ca5832f06..3fc26ce45ef 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs @@ -267,10 +267,9 @@ impl Translator { }; self.builder.insert_constrain(lhs, rhs, assert_message); } - ParsedInstruction::DecrementRc { value, original } => { + ParsedInstruction::DecrementRc { value } => { let value = self.translate_value(value)?; - let original = self.translate_value(original)?; - self.builder.decrement_array_reference_count(value, original); + self.builder.decrement_array_reference_count(value); } ParsedInstruction::EnableSideEffectsIf { condition } => { let condition = self.translate_value(condition)?; diff --git a/compiler/noirc_evaluator/src/ssa/parser/mod.rs b/compiler/noirc_evaluator/src/ssa/parser/mod.rs index 9f4d38648e2..97ecbc1635d 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/mod.rs @@ -396,8 +396,7 @@ impl<'a> Parser<'a> { } let value = self.parse_value_or_error()?; - let original = self.parse_value_or_error()?; - Ok(Some(ParsedInstruction::DecrementRc { value, original })) + Ok(Some(ParsedInstruction::DecrementRc { value })) } fn parse_enable_side_effects(&mut self) -> ParseResult> { diff --git a/compiler/noirc_evaluator/src/ssa/parser/tests.rs b/compiler/noirc_evaluator/src/ssa/parser/tests.rs index a865a31a060..e14510fe2a1 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/tests.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/tests.rs @@ -445,7 +445,7 @@ fn test_dec_rc() { let src = " brillig(inline) fn main f0 { b0(v0: [Field; 3]): - dec_rc v0 v0 + dec_rc v0 return } "; From e421f3bc63e25073c084613debf66570ca4e0720 Mon Sep 17 00:00:00 2001 From: jfecher Date: Tue, 18 Mar 2025 07:48:42 -0500 Subject: [PATCH 08/11] Update compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs Co-authored-by: Maxim Vezenov --- .../noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 63250ff65bb..8ae4eddf1f5 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -893,7 +893,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { ); self.brillig_context.codegen_constrain( condition, - Some("array ref-count underflowed below zero".to_owned()), + Some("array ref-count underflow detected".to_owned()), ); self.brillig_context.deallocate_single_addr(condition); } From 1aaa27108632fa33033c83354c6666c6eb4f874b Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 18 Mar 2025 09:34:48 -0500 Subject: [PATCH 09/11] Add stdout.txt --- .../reference_counts_inliner_0/src/main.nr | 2 -- .../reference_counts_inliner_0/stdout.txt | 10 ++++++++++ .../reference_counts_inliner_max/src/main.nr | 2 -- .../reference_counts_inliner_max/stdout.txt | 10 ++++++++++ .../reference_counts_inliner_min/src/main.nr | 2 -- .../reference_counts_inliner_min/stdout.txt | 10 ++++++++++ 6 files changed, 30 insertions(+), 6 deletions(-) create mode 100644 test_programs/execution_success/reference_counts_inliner_0/stdout.txt create mode 100644 test_programs/execution_success/reference_counts_inliner_max/stdout.txt create mode 100644 test_programs/execution_success/reference_counts_inliner_min/stdout.txt 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 7eda848f851..0af6e0e79a0 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 @@ -92,8 +92,6 @@ fn regression_7297() { let array_2 = copy_mut(array, refcount_1); let refcount_2 = array_refcount(array); - println([refcount_0, refcount_1, refcount_2]); - // Mutation of the original could occur if we double decremented the RC and then went back to 1 by accident. // For this to come out we have to run the test with `--inliner-aggressiveness -9223372036854775808` assert_eq(array[0], 6, "the original should not be mutated by copy_mut, only borrow_mut_two"); diff --git a/test_programs/execution_success/reference_counts_inliner_0/stdout.txt b/test_programs/execution_success/reference_counts_inliner_0/stdout.txt new file mode 100644 index 00000000000..7e020b1074f --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_0/stdout.txt @@ -0,0 +1,10 @@ +0x00 +0x03 +0x04 +0x06 +0x06 +0x07 +8 +0x06 +0x06 +0x04 diff --git a/test_programs/execution_success/reference_counts_inliner_max/src/main.nr b/test_programs/execution_success/reference_counts_inliner_max/src/main.nr index 0c9eb0d873b..f0c34ced4d1 100644 --- a/test_programs/execution_success/reference_counts_inliner_max/src/main.nr +++ b/test_programs/execution_success/reference_counts_inliner_max/src/main.nr @@ -92,8 +92,6 @@ fn regression_7297() { let array_2 = copy_mut(array, refcount_1); let refcount_2 = array_refcount(array); - println([refcount_0, refcount_1, refcount_2]); - // Mutation of the original could occur if we double decremented the RC and then went back to 1 by accident. // For this to come out we have to run the test with `--inliner-aggressiveness -9223372036854775808` assert_eq(array[0], 6, "the original should not be mutated by copy_mut, only borrow_mut_two"); diff --git a/test_programs/execution_success/reference_counts_inliner_max/stdout.txt b/test_programs/execution_success/reference_counts_inliner_max/stdout.txt new file mode 100644 index 00000000000..7e020b1074f --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_max/stdout.txt @@ -0,0 +1,10 @@ +0x00 +0x03 +0x04 +0x06 +0x06 +0x07 +8 +0x06 +0x06 +0x04 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 7eda848f851..0af6e0e79a0 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 @@ -92,8 +92,6 @@ fn regression_7297() { let array_2 = copy_mut(array, refcount_1); let refcount_2 = array_refcount(array); - println([refcount_0, refcount_1, refcount_2]); - // Mutation of the original could occur if we double decremented the RC and then went back to 1 by accident. // For this to come out we have to run the test with `--inliner-aggressiveness -9223372036854775808` assert_eq(array[0], 6, "the original should not be mutated by copy_mut, only borrow_mut_two"); diff --git a/test_programs/execution_success/reference_counts_inliner_min/stdout.txt b/test_programs/execution_success/reference_counts_inliner_min/stdout.txt new file mode 100644 index 00000000000..7e020b1074f --- /dev/null +++ b/test_programs/execution_success/reference_counts_inliner_min/stdout.txt @@ -0,0 +1,10 @@ +0x00 +0x03 +0x04 +0x06 +0x06 +0x07 +8 +0x06 +0x06 +0x04 From 1db42192715e33897b873d07ba447b2fa8370f25 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 18 Mar 2025 10:30:31 -0500 Subject: [PATCH 10/11] Remove removed test from build.rs --- tooling/nargo_cli/build.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index b26e66e8777..4ba587f0ca3 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -89,10 +89,7 @@ const TESTS_WITH_EXPECTED_WARNINGS: [&str; 4] = [ ]; /// Tests for which we don't check that stdout matches the expected output. -const TESTS_WITHOUT_STDOUT_CHECK: [&str; 1] = [ - // The output changes depending on whether `--force-brillig` is passed or not - "reference_counts", -]; +const TESTS_WITHOUT_STDOUT_CHECK: [&str; 0] = []; fn read_test_cases( test_data_dir: &Path, From 470be75843582e5affff59d833aaca396c254c0e Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 18 Mar 2025 10:38:32 -0500 Subject: [PATCH 11/11] Add another inliner build array --- tooling/nargo_cli/build.rs | 40 ++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index 4ba587f0ca3..bc743ef1ed0 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -61,16 +61,16 @@ const IGNORED_BRILLIG_TESTS: [&str; 10] = [ ]; /// Tests which aren't expected to work with the default minimum inliner cases. -const INLINER_MIN_OVERRIDES: [(&str, i64); 4] = [ +const INLINER_MIN_OVERRIDES: [(&str, i64); 1] = [ // 0 works if PoseidonHasher::write is tagged as `inline_always`, otherwise 22. ("eddsa", 0), - ("reference_counts_inliner_0", 0), - ("reference_counts_inliner_min", i64::MIN), - ("reference_counts_inliner_max", i64::MAX), ]; /// Tests which aren't expected to work with the default maximum inliner cases. -const INLINER_MAX_OVERRIDES: [(&str, i64); 3] = [ +const INLINER_MAX_OVERRIDES: [(&str, i64); 0] = []; + +/// These tests should only be run on exactly 1 inliner setting (the one given here) +const INLINER_OVERRIDES: [(&str, i64); 3] = [ ("reference_counts_inliner_0", 0), ("reference_counts_inliner_min", i64::MIN), ("reference_counts_inliner_max", i64::MAX), @@ -294,22 +294,32 @@ fn generate_execution_success_tests(test_file: &mut File, test_data_dir: &Path) &MatrixConfig { vary_brillig: !IGNORED_BRILLIG_TESTS.contains(&test_name.as_str()), vary_inliner: true, - min_inliner: INLINER_MIN_OVERRIDES - .iter() - .find(|(n, _)| *n == test_name.as_str()) - .map(|(_, i)| *i) - .unwrap_or(i64::MIN), - max_inliner: INLINER_MAX_OVERRIDES - .iter() - .find(|(n, _)| *n == test_name.as_str()) - .map(|(_, i)| *i) - .unwrap_or(i64::MAX), + min_inliner: min_inliner(&test_name), + max_inliner: max_inliner(&test_name), }, ); } writeln!(test_file, "}}").unwrap(); } +fn max_inliner(test_name: &str) -> i64 { + INLINER_MAX_OVERRIDES + .iter() + .chain(&INLINER_OVERRIDES) + .find(|(n, _)| *n == test_name) + .map(|(_, i)| *i) + .unwrap_or(i64::MAX) +} + +fn min_inliner(test_name: &str) -> i64 { + INLINER_MIN_OVERRIDES + .iter() + .chain(&INLINER_OVERRIDES) + .find(|(n, _)| *n == test_name) + .map(|(_, i)| *i) + .unwrap_or(i64::MIN) +} + fn generate_execution_failure_tests(test_file: &mut File, test_data_dir: &Path) { let test_type = "execution_failure"; let test_cases = read_test_cases(test_data_dir, test_type);