Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ACIR gen / ACVM: Handle non-inlined ACIR functions #4428

Closed
Tracked by #4426
vezenovm opened this issue Feb 26, 2024 · 1 comment · Fixed by AztecProtocol/aztec-packages#5341 or AztecProtocol/aztec-packages#5380
Labels
acir-gen enhancement New feature or request ssa

Comments

@vezenovm
Copy link
Contributor

vezenovm commented Feb 26, 2024

Problem

After #4427 we will have an opcode that tells us whether we should call a separate ACIR. We have no ability in the compiler to codegen separate ACIR functions as we currently inline all ACIR functions earlier during SSA gen.

Happy Case

TLDR; Jump to number (2) for the selected solution forward. I have kept the full initial comment for record keeping purpsoes.

We will need a new function attribute such as #[fold] that tells the compiler whether a certain ACIR function should be inlined during SSA gen. We then need ACIR gen to appropriately handle generating ACIR for multiple function calls and storing each ACIR as part of our main Circuit struct that gets sent to the backend. We could either do this with a recursive structure or a flat vector of circuits:

  1. Recursive Circuit type

The new circuit struct should look something like such:

pub struct Circuit {
    pub opcodes: Vec<Opcode>,
    pub circuits: Vec<Circuit>, // or Vec<Vec<Opcode>> as we may not want a recursive `Circuit` type
    // ... other fields
}

Each id in a Call opcode will point to an ACIR function. We want IDs as we want one ACIR per function. ACIR gen and the execution environment should be able to handle this requirement.

Making sure we appropriately codegen the function calls will not be trivial and will also require updates from the backend to appropriately handle the new Circuit type. Initially a backend can just perform ACIR function inlining on its own until it is ready to integrate more advanced features such as folding.

  1. Alternative flat list of circuits

Ultimately the backend needs two things to handle a stack of function calls:

  1. A list of circuits in the order to be executed
  2. An accumulated witness. The executor will be responsible for correctly offsetting witness values so that a function can fetch the appropriate input scalars.

Rather than a recursive circuit structure we should have the following:

pub struct Program {
    // Each ACIR circuit with the main function representing the first program
    pub functions: Vec<Circuit>,
    // other relevant info such as ABIs
}

We should initially construct a singular witness map for the entire call stack execution. It will then be up to the executor to set up the appropriate final call stack to send to the backend with the respective witness for each function to execute. It is important to note that the same circuit can be called multiple times. For these cases we will have the same ACIR, but a larger witness which needs to be appropriately handled.

I could see the structure getting transformed into something like this:

pub struct Program {
     // Each ACIR circuit with the main function representing the first program
     pub functions: Vec<Circuit>,
     // A call stack of the witness to use for the respective ACIR. The size of this witness stack should be 
     // the same as the number of `Call` instruction in 
     pub witnesses: Vec<WitnessMap>,  
     // other relevant info such as ABIs
}

From this list of function calls and witness maps the backend can appropriate construct multiple circuits to be folded together.

Project Impact

None

Impact Context

No response

Workaround

None

Workaround Description

No response

Additional Context

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@vezenovm vezenovm added enhancement New feature or request ssa acir-gen labels Feb 26, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Feb 26, 2024
@vezenovm vezenovm changed the title ACIR gen: Handle non-inlined ACIR functions ACIR gen / ACVM: Handle non-inlined ACIR functions Feb 26, 2024
@vezenovm
Copy link
Contributor Author

vezenovm commented Mar 5, 2024

I'm going forward with the flat list of circuits approach.

To expand the initial proposal listed above:

We probably need to store some index with the list of witnesses to more accurately represent a function stack.

Program {
    functions: Vec<Circuit>,
}

WitnessStack {
    stack: Vec<StackItem>,
}

StackItem {
    // Index into the program's function list for which we have an associated witness
    index: u32,
    // Function witness
    witness: WitnessVector,
    // potentially other information such as inputs/outputs to be copy constrained by the backend between stack items
}

The backend will then do something like such:

auto constraint_systems: vector<acir_format::AcirFormat> = get_program(bytecode);
auto witness_stack: vector<(index, WitnessVector)> = get_witness_stack(bytecode);

while (!witness_stack.empty()) {
    let witness = witness_stack.pop_back();
    let constraint_system = constraint_systems[witness.0];

    // create foldable circuit
    // accumulate and prove per loop step
    // verify after the loop is finished
}

Some pseudocode showing how the final program and stack passed to the backend may look:

main(x) -> out {
    let out = A(x);
    A(out + 2)
    // Not enough to predicate just the call, read below for more
    // B(out) * predicate
    // C(out) * !predicate
    if x == 1 {
        B(out, predicate)
    } else {
        C(out, predicate)
    }
}

C(input) -> out {
    assert(input == 5) * predicate
    input + 1 * predicate
}

B(input) -> out {
    assert(input == 5) * predicate
    (input + 2) * predicate
}

B ACIR: 
    AssertZero(input - 5 == 0)
    x0 + 2

A(input) -> out {
    B(input)
}

A ACIR:
    Call { id: 2, inputs: [x0] },
    A witness: { x0 }

main ACIR:
    Call { id: 1, inputs: [x0], }
    x1 + 2 = x2;
    AssertZero(x2 - 2 == 0)
    Call { id: 1, inputs: [x2], .. }
    // Calls with side effects 
    Call { id: 2, inputs: [x2, predicate], .. }
    Call { id: 3, inputs: [x2, predicate], .. }

Stack if true:
B, A, B, A, B, main
2, 1, 2, 1, 2, 0

Stack if false:
B, A, B, A, C, main
2, 1, 2, 1, 0

How do we guarantee that a prover does not leave out `B` or `C` when folding?
We must maintain the same stack.

Stack must remain the same (use a predicate):
B, A, B, A, B, C, main

vezenovm added a commit to AztecProtocol/aztec-packages that referenced this issue Mar 29, 2024
…d ACIR (#5341)

Partially resolves noir-lang/noir#4428 but not
fully. Need ACVM execution work still.

This does the initial codegen work for having multiple ACIR functions. 

It does a few things:
1. Adds a new `InlineType` that is now part of `RuntimeType::Acir`. We
do not care about `RuntimeType::Brillig` as we do not inline any of
those calls.
2. Fetch the appropriate function signatures for all possible `Circuit`
functions inside of a `Program`. Previously we were just returning the
main function signature. This is needed to accurately set up the
`private_parameters` and `public_parameters` fields in a circuit.
3. Iterates over all SSA functions, determines which are entry points
and codegens them
4. Codegens the newly added `Call` opcode from
#4773

---------

Co-authored-by: Tom French <[email protected]>
Co-authored-by: Tom French <[email protected]>
Co-authored-by: jfecher <[email protected]>
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Mar 29, 2024
AztecBot added a commit that referenced this issue Mar 29, 2024
…d ACIR (AztecProtocol/aztec-packages#5341)

Partially resolves #4428 but not
fully. Need ACVM execution work still.

This does the initial codegen work for having multiple ACIR functions.

It does a few things:
1. Adds a new `InlineType` that is now part of `RuntimeType::Acir`. We
do not care about `RuntimeType::Brillig` as we do not inline any of
those calls.
2. Fetch the appropriate function signatures for all possible `Circuit`
functions inside of a `Program`. Previously we were just returning the
main function signature. This is needed to accurately set up the
`private_parameters` and `public_parameters` fields in a circuit.
3. Iterates over all SSA functions, determines which are entry points
and codegens them
4. Codegens the newly added `Call` opcode from
AztecProtocol/aztec-packages#4773

---------

Co-authored-by: Tom French <[email protected]>
Co-authored-by: Tom French <[email protected]>
Co-authored-by: jfecher <[email protected]>
vezenovm added a commit to AztecProtocol/aztec-packages that referenced this issue Mar 29, 2024
Resolves noir-lang/noir#4428

This is a followup to
#5341 which does the
initial ACIR generation work for multiple ACIR functions.

Execution is now done by moving `execute_circuit` to be part of a
stateful `ProgramExecutor` that builds a witness stack for every
completed `execute_circuit` call. An initial `execute_program` function
instantiates the `ProgramExecutor` and starts execution on our `main`
entry point circuit. When a `Call` opcode is reached we pause execution
and recursively call `execute_circuit`, which then returns the solved
witness for that call. We then resolve the outputs of that execution by
reading the return witnesses from the inner solved witness. We then push
the nested call solved witness onto the witness stack and continue
execution of our main ACVM instance. This is quite similar to the
process used by foreign calls with Brillig, except it is now done with
the main ACVM rather than the contained Brillig VM.

This witness stack and program (list of `Circuit` functions) then gets
passed to the backend. For now, I have only done an additive
`prove_and_verify_ultra_honk_program` to show the process working for
the basic non-inlined ACIR programs supplied here. I wanted to leave any
WASM exports or ACVM JS changes for a follow-up PR as they are quite a
bit of changes on their own.

---------

Co-authored-by: Tom French <[email protected]>
Co-authored-by: Tom French <[email protected]>
Co-authored-by: jfecher <[email protected]>
AztecBot added a commit that referenced this issue Mar 29, 2024
)

Resolves #4428

This is a followup to
AztecProtocol/aztec-packages#5341 which does the
initial ACIR generation work for multiple ACIR functions.

Execution is now done by moving `execute_circuit` to be part of a
stateful `ProgramExecutor` that builds a witness stack for every
completed `execute_circuit` call. An initial `execute_program` function
instantiates the `ProgramExecutor` and starts execution on our `main`
entry point circuit. When a `Call` opcode is reached we pause execution
and recursively call `execute_circuit`, which then returns the solved
witness for that call. We then resolve the outputs of that execution by
reading the return witnesses from the inner solved witness. We then push
the nested call solved witness onto the witness stack and continue
execution of our main ACVM instance. This is quite similar to the
process used by foreign calls with Brillig, except it is now done with
the main ACVM rather than the contained Brillig VM.

This witness stack and program (list of `Circuit` functions) then gets
passed to the backend. For now, I have only done an additive
`prove_and_verify_ultra_honk_program` to show the process working for
the basic non-inlined ACIR programs supplied here. I wanted to leave any
WASM exports or ACVM JS changes for a follow-up PR as they are quite a
bit of changes on their own.

---------

Co-authored-by: Tom French <[email protected]>
Co-authored-by: Tom French <[email protected]>
Co-authored-by: jfecher <[email protected]>
AztecBot added a commit that referenced this issue Mar 29, 2024
)

Resolves #4428

This is a followup to
AztecProtocol/aztec-packages#5341 which does the
initial ACIR generation work for multiple ACIR functions.

Execution is now done by moving `execute_circuit` to be part of a
stateful `ProgramExecutor` that builds a witness stack for every
completed `execute_circuit` call. An initial `execute_program` function
instantiates the `ProgramExecutor` and starts execution on our `main`
entry point circuit. When a `Call` opcode is reached we pause execution
and recursively call `execute_circuit`, which then returns the solved
witness for that call. We then resolve the outputs of that execution by
reading the return witnesses from the inner solved witness. We then push
the nested call solved witness onto the witness stack and continue
execution of our main ACVM instance. This is quite similar to the
process used by foreign calls with Brillig, except it is now done with
the main ACVM rather than the contained Brillig VM.

This witness stack and program (list of `Circuit` functions) then gets
passed to the backend. For now, I have only done an additive
`prove_and_verify_ultra_honk_program` to show the process working for
the basic non-inlined ACIR programs supplied here. I wanted to leave any
WASM exports or ACVM JS changes for a follow-up PR as they are quite a
bit of changes on their own.

---------

Co-authored-by: Tom French <[email protected]>
Co-authored-by: Tom French <[email protected]>
Co-authored-by: jfecher <[email protected]>
AztecBot pushed a commit to AztecProtocol/barretenberg that referenced this issue Mar 30, 2024
Resolves noir-lang/noir#4428

This is a followup to
AztecProtocol/aztec-packages#5341 which does the
initial ACIR generation work for multiple ACIR functions.

Execution is now done by moving `execute_circuit` to be part of a
stateful `ProgramExecutor` that builds a witness stack for every
completed `execute_circuit` call. An initial `execute_program` function
instantiates the `ProgramExecutor` and starts execution on our `main`
entry point circuit. When a `Call` opcode is reached we pause execution
and recursively call `execute_circuit`, which then returns the solved
witness for that call. We then resolve the outputs of that execution by
reading the return witnesses from the inner solved witness. We then push
the nested call solved witness onto the witness stack and continue
execution of our main ACVM instance. This is quite similar to the
process used by foreign calls with Brillig, except it is now done with
the main ACVM rather than the contained Brillig VM.

This witness stack and program (list of `Circuit` functions) then gets
passed to the backend. For now, I have only done an additive
`prove_and_verify_ultra_honk_program` to show the process working for
the basic non-inlined ACIR programs supplied here. I wanted to leave any
WASM exports or ACVM JS changes for a follow-up PR as they are quite a
bit of changes on their own.

---------

Co-authored-by: Tom French <[email protected]>
Co-authored-by: Tom French <[email protected]>
Co-authored-by: jfecher <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue Apr 15, 2024
# Description

## Problem\*

Resolves leftover work from #4428 

We would error out on calls in non-inlined Acir calls, however, we would
only use the debug info from the main function.

## Summary\*

There is now a new `ResolvedOpcodeLocation` that wraps an acir function
index from the `Program` as well as its opcode location. We then build a
call stack during execution that is resolved upon execution failure.
Both `ExecutionError` variants now contain a call stack of
`ResolvedOpcodeLocation`.

For the `fold_basic_nested_call` if I make `assert(x == y)` fail I will
get the following now:
<img width="811" alt="Screenshot 2024-04-03 at 4 53 17 PM"
src="https://github.com/noir-lang/noir/assets/43554004/9c50bec5-4f85-4fe1-93ed-0527f9511f2c">

On master we would get an error in `main` at the first call to
`func_with_nested_foo_call`.

For the new
`test_programs/execution_failure/fold_nested_brillig_assert_fail` test
we have this output:
<img width="859" alt="Screenshot 2024-04-11 at 9 50 34 AM"
src="https://github.com/noir-lang/noir/assets/43554004/58ef8d8f-f67c-477d-9af1-cc1ccd89be5e">

I then also added support for call stacks with dynamic indices in
`test_programs/execution_failure/fold_dyn_index_fail`:
<img width="774" alt="Screenshot 2024-04-11 at 9 51 16 AM"
src="https://github.com/noir-lang/noir/assets/43554004/1066a05a-78db-4c84-83fd-77c17d83cc81">


## Additional Context



## Documentation\*

Check one:
- [] No documentation needed.
- [ ] Documentation included in this PR.
- [X] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [X] I have tested the changes locally.
- [X] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Tom French <[email protected]>
smanilov added a commit to blocksense-network/noir that referenced this issue Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment