From 4b3a0f0daa9ed99dee8d5567913eb9e61cf5ed10 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 30 Apr 2024 21:41:17 +0000 Subject: [PATCH 1/6] require for all calls to foldable functions to use distinct return witnesses --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 54 ++++++++++++------- .../fold_distinct_return/Nargo.toml | 7 +++ .../fold_distinct_return/Prover.toml | 2 + .../fold_distinct_return/src/main.nr | 10 ++++ 4 files changed, 53 insertions(+), 20 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 63d1c1b564f..f0d53021262 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -329,34 +329,46 @@ 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]; - match abi_distinctness { - Distinctness::Distinct => { - // Create a witness for each return witness we have - // to guarantee that the return witnesses are distinct - let distinct_return_witness: Vec<_> = main_func_acir - .return_witnesses - .clone() - .into_iter() - .map(|return_witness| { - main_func_acir - .create_witness_for_expression(&Expression::from(return_witness)) - }) - .collect(); + generate_distinct_return_witnesses(main_func_acir, abi_distinctness); - main_func_acir.return_witnesses = distinct_return_witness; + // Currently we require that all outputs from ACIR functions aside `main` must be unique. + // This removes the chance for return values to be re-ordered and thus reduce user confusion. + // + // TODO: Decide whether we want to let users specify distinctness for foldable functions. + // At the moment we specify Distinctness as part of the ABI exclusively rather than the function itself + // so this will need to be updated. + if acirs.len() > 1 { + for acir in acirs.iter_mut().skip(1) { + generate_distinct_return_witnesses(acir, Distinctness::Distinct) } - Distinctness::DuplicationAllowed => {} } + Ok((acirs, brillig)) } } +fn generate_distinct_return_witnesses(acir: &mut GeneratedAcir, distinctness: Distinctness) { + match distinctness { + Distinctness::Distinct => { + // Create a witness for each return witness we have + // to guarantee that the return witnesses are distinct + let distinct_return_witness: Vec<_> = acir + .return_witnesses + .clone() + .into_iter() + .map(|return_witness| { + acir + .create_witness_for_expression(&Expression::from(return_witness)) + }) + .collect(); + + acir.return_witnesses = distinct_return_witness; + } + Distinctness::DuplicationAllowed => {} + } +} + impl<'a> Context<'a> { fn new(shared_context: &'a mut SharedContext) -> Context<'a> { let mut acir_context = AcirContext::default(); @@ -714,6 +726,8 @@ impl<'a> Context<'a> { // TODO(https://github.com/noir-lang/noir/issues/4608): handle complex return types from ACIR functions let output_count = result_ids.iter().fold(0usize, |sum, result_id| { + dbg!(result_id); + dbg!(dfg.try_get_array_length(*result_id).unwrap_or(1)); sum + dfg.try_get_array_length(*result_id).unwrap_or(1) }); 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..e926aea58da --- /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 +} \ No newline at end of file From 573a5c376ddef01471f2cb92537e107192f539a5 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 30 Apr 2024 21:46:42 +0000 Subject: [PATCH 2/6] clippy --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index f0d53021262..933495329a9 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -334,13 +334,13 @@ impl Ssa { // Currently we require that all outputs from ACIR functions aside `main` must be unique. // This removes the chance for return values to be re-ordered and thus reduce user confusion. - // + // // TODO: Decide whether we want to let users specify distinctness for foldable functions. // At the moment we specify Distinctness as part of the ABI exclusively rather than the function itself // so this will need to be updated. if acirs.len() > 1 { for acir in acirs.iter_mut().skip(1) { - generate_distinct_return_witnesses(acir, Distinctness::Distinct) + generate_distinct_return_witnesses(acir, Distinctness::Distinct); } } @@ -358,12 +358,11 @@ fn generate_distinct_return_witnesses(acir: &mut GeneratedAcir, distinctness: Di .clone() .into_iter() .map(|return_witness| { - acir - .create_witness_for_expression(&Expression::from(return_witness)) + acir.create_witness_for_expression(&Expression::from(return_witness)) }) .collect(); - acir.return_witnesses = distinct_return_witness; + acir.return_witnesses = distinct_return_witness; } Distinctness::DuplicationAllowed => {} } @@ -726,8 +725,6 @@ impl<'a> Context<'a> { // TODO(https://github.com/noir-lang/noir/issues/4608): handle complex return types from ACIR functions let output_count = result_ids.iter().fold(0usize, |sum, result_id| { - dbg!(result_id); - dbg!(dfg.try_get_array_length(*result_id).unwrap_or(1)); sum + dfg.try_get_array_length(*result_id).unwrap_or(1) }); From 7c1f1a14593c264dd688df89aef887715b98473b Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 30 Apr 2024 22:00:33 +0000 Subject: [PATCH 3/6] nargo fmt --- .../execution_success/fold_distinct_return/src/main.nr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test_programs/execution_success/fold_distinct_return/src/main.nr b/test_programs/execution_success/fold_distinct_return/src/main.nr index e926aea58da..b0843a02b80 100644 --- a/test_programs/execution_success/fold_distinct_return/src/main.nr +++ b/test_programs/execution_success/fold_distinct_return/src/main.nr @@ -4,7 +4,7 @@ fn main(x: u32, y: pub u32) { } #[fold] -fn new_field_in_array(mut input: [u32; 3]) -> [u32 ; 3] { +fn new_field_in_array(mut input: [u32; 3]) -> [u32; 3] { input[0] = input[0] + 20; input -} \ No newline at end of file +} From 4c8c8a3cea486ead561a3d49366b5e852b351abf Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 1 May 2024 02:02:06 +0000 Subject: [PATCH 4/6] update debugger ignored tests --- tooling/debugger/ignored-tests.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tooling/debugger/ignored-tests.txt b/tooling/debugger/ignored-tests.txt index 5e6ce18be54..5888d4e23bc 100644 --- a/tooling/debugger/ignored-tests.txt +++ b/tooling/debugger/ignored-tests.txt @@ -19,3 +19,4 @@ fold_after_inlined_calls fold_numeric_generic_poseidon inline_never_basic regression_4709 +fold_distinct_return \ No newline at end of file From 5a66684b0d772906a55e4c8b969346f9f6d7c9d3 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 1 May 2024 13:33:19 +0000 Subject: [PATCH 5/6] update test after always unique return witness gen --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index de735d33825..cafc75884e7 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -2672,9 +2672,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] @@ -2917,9 +2917,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 From ea736f60528a2bbc3ff14bf08f25a23f086a3be0 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 1 May 2024 15:52:08 +0000 Subject: [PATCH 6/6] remove distinctness param --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index c78770bc647..10d58f327e5 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -326,11 +326,11 @@ impl Ssa { let brillig = vecmap(shared_context.generated_brillig, |brillig| BrilligBytecode { bytecode: brillig.byte_code, }); - + for acir in acirs.iter_mut() { - generate_distinct_return_witnesses(acir, Distinctness::Distinct); + generate_distinct_return_witnesses(acir); } - + Ok((acirs, brillig)) } }