Skip to content

Commit

Permalink
fix: Require for all foldable functions to use distinct return (#4949)
Browse files Browse the repository at this point in the history
…tnesses

# Description

## Problem\*

Found the issue while working on
#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>
  • Loading branch information
vezenovm and TomAFrench authored May 1, 2024
1 parent 5f1b584 commit d4c6806
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 9 deletions.
15 changes: 6 additions & 9 deletions compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "fold_distinct_return"
type = "bin"
authors = [""]
compiler_version = ">=0.28.0"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
x = "5"
y = "10"
10 changes: 10 additions & 0 deletions test_programs/execution_success/fold_distinct_return/src/main.nr
Original file line number Diff line number Diff line change
@@ -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
}
1 change: 1 addition & 0 deletions tooling/debugger/ignored-tests.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,5 @@ fold_numeric_generic_poseidon
no_predicates_basic
no_predicates_numeric_generic_poseidon
regression_4709
fold_distinct_return
fold_fibonacci

0 comments on commit d4c6806

Please sign in to comment.