From d4c68066ab35ce1c52510cf0c038fb627a0677c3 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 1 May 2024 17:11:57 +0100 Subject: [PATCH] fix: Require for all foldable functions to use distinct return (#4949) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …tnesses # Description ## Problem\* Found the issue while working on https://github.com/noir-lang/noir/issues/4608. ## Summary\* For this program: ```rust fn main(x: u32, y: pub u32) { let new_field = new_field_in_array([x, y, 3]); assert(new_field[0] == 25); } #[fold] fn new_field_in_array(mut input: [u32; 3]) -> [u32 ; 3] { input[0] = input[0] + 20; input } ``` We get the following ACIR: Old ACIR: ``` func 0 current witness index : 5 private parameters indices : [0] public parameters indices : [1] return value indices : [] BLACKBOX::RANGE [(_0, num_bits: 32)] [ ] BLACKBOX::RANGE [(_1, num_bits: 32)] [ ] EXPR [ (-1, _2) 3 ] CALL func 1: PREDICATE = %EXPR [ 1 ]% inputs: [Witness(0), Witness(1), Witness(2)], outputs: [Witness(3), Witness(4), Witness(5)] EXPR [ (1, _3) -25 ] func 1 current witness index : 3 private parameters indices : [0, 1, 2] public parameters indices : [] return value indices : [1, 2, 3] BLACKBOX::RANGE [(_0, num_bits: 32)] [ ] BLACKBOX::RANGE [(_1, num_bits: 32)] [ ] BLACKBOX::RANGE [(_2, num_bits: 32)] [ ] EXPR [ (1, _0) (-1, _3) 20 ] BLACKBOX::RANGE [(_3, num_bits: 32)] [ ] ``` One can see how simply the next witness (`_3` in this case) is being assigned to 20 directly and returned rather than returning it correctly according to the type. The constraint in the `main` function above will fail as the returned array will be `[5, 10, 20]` when we want `[20, 5, 10]`. This re-ordering can be mitigated by requiring all returns from foldable functions to be distinct for which we get the following ACIR: New ACIR: The main func is the same. ``` func 0 (same as above) func 1 current witness index : 6 private parameters indices : [0, 1, 2] public parameters indices : [] return value indices : [4, 5, 6] BLACKBOX::RANGE [(_0, num_bits: 32)] [ ] BLACKBOX::RANGE [(_1, num_bits: 32)] [ ] BLACKBOX::RANGE [(_2, num_bits: 32)] [ ] EXPR [ (1, _0) (-1, _3) 20 ] BLACKBOX::RANGE [(_3, num_bits: 32)] [ ] EXPR [ (1, _3) (-1, _4) 0 ] EXPR [ (1, _1) (-1, _5) 0 ] EXPR [ (1, _2) (-1, _6) 0 ] ``` I think just defaulting all return values from foldable functions to be distinct is going to be the least confusing for users (and we have discussed making it the default on `main` as well). If we want to add the ability for a user to not use `distinct` on foldable function return values that can come in a follow up. ## Additional Context ## Documentation\* Check one: - [ ] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** 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. --------- Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 15 ++++++--------- .../fold_distinct_return/Nargo.toml | 7 +++++++ .../fold_distinct_return/Prover.toml | 2 ++ .../fold_distinct_return/src/main.nr | 10 ++++++++++ tooling/debugger/ignored-tests.txt | 1 + 5 files changed, 26 insertions(+), 9 deletions(-) create mode 100644 test_programs/execution_success/fold_distinct_return/Nargo.toml create mode 100644 test_programs/execution_success/fold_distinct_return/Prover.toml create mode 100644 test_programs/execution_success/fold_distinct_return/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 4a513c7b804..10d58f327e5 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -327,13 +327,9 @@ impl Ssa { bytecode: brillig.byte_code, }); - // TODO: check whether doing this for a single circuit's return witnesses is correct. - // We probably need it for all foldable circuits, as any circuit being folded is essentially an entry point. However, I do not know how that - // plays a part when we potentially want not inlined functions normally as part of the compiler. - // Also at the moment we specify Distinctness as part of the ABI exclusively rather than the function itself - // so this will need to be updated. - let main_func_acir = &mut acirs[0]; - generate_distinct_return_witnesses(main_func_acir); + for acir in acirs.iter_mut() { + generate_distinct_return_witnesses(acir); + } Ok((acirs, brillig)) } @@ -2664,9 +2660,9 @@ mod test { #[test] #[should_panic] fn basic_calls_no_predicates() { + basic_call_with_outputs_assert(InlineType::NoPredicates); call_output_as_next_call_input(InlineType::NoPredicates); basic_nested_call(InlineType::NoPredicates); - basic_call_with_outputs_assert(InlineType::NoPredicates); } #[test] @@ -2909,9 +2905,10 @@ mod test { let func_with_nested_call_acir = &acir_functions[1]; let func_with_nested_call_opcodes = func_with_nested_call_acir.opcodes(); + assert_eq!( func_with_nested_call_opcodes.len(), - 2, + 3, "Should have an expression and a call to a nested `foo`" ); // Should call foo f2 diff --git a/test_programs/execution_success/fold_distinct_return/Nargo.toml b/test_programs/execution_success/fold_distinct_return/Nargo.toml new file mode 100644 index 00000000000..f18edb7e49d --- /dev/null +++ b/test_programs/execution_success/fold_distinct_return/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "fold_distinct_return" +type = "bin" +authors = [""] +compiler_version = ">=0.28.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_success/fold_distinct_return/Prover.toml b/test_programs/execution_success/fold_distinct_return/Prover.toml new file mode 100644 index 00000000000..f28f2f8cc48 --- /dev/null +++ b/test_programs/execution_success/fold_distinct_return/Prover.toml @@ -0,0 +1,2 @@ +x = "5" +y = "10" diff --git a/test_programs/execution_success/fold_distinct_return/src/main.nr b/test_programs/execution_success/fold_distinct_return/src/main.nr new file mode 100644 index 00000000000..b0843a02b80 --- /dev/null +++ b/test_programs/execution_success/fold_distinct_return/src/main.nr @@ -0,0 +1,10 @@ +fn main(x: u32, y: pub u32) { + let new_field = new_field_in_array([x, y, 3]); + assert(new_field[0] == 25); +} + +#[fold] +fn new_field_in_array(mut input: [u32; 3]) -> [u32; 3] { + input[0] = input[0] + 20; + input +} diff --git a/tooling/debugger/ignored-tests.txt b/tooling/debugger/ignored-tests.txt index eaeea488f3a..cbef395e65c 100644 --- a/tooling/debugger/ignored-tests.txt +++ b/tooling/debugger/ignored-tests.txt @@ -20,4 +20,5 @@ fold_numeric_generic_poseidon no_predicates_basic no_predicates_numeric_generic_poseidon regression_4709 +fold_distinct_return fold_fibonacci