-
Notifications
You must be signed in to change notification settings - Fork 235
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
Closed
Tracked by
#4426
ACIR gen / ACVM: Handle non-inlined ACIR functions #4428
vezenovm opened this issue
Feb 26, 2024
· 1 comment
· Fixed by AztecProtocol/aztec-packages#5341 or AztecProtocol/aztec-packages#5380
Labels
Comments
Closed
vezenovm
changed the title
ACIR gen: Handle non-inlined ACIR functions
ACIR gen / ACVM: Handle non-inlined ACIR functions
Feb 26, 2024
This was referenced Mar 4, 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:
Some pseudocode showing how the final program and stack passed to the backend may look:
|
This was referenced Mar 6, 2024
This was referenced Mar 21, 2024
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]>
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]>
This was referenced Apr 2, 2024
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
This was referenced Apr 16, 2024
This was referenced Apr 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 mainCircuit
struct that gets sent to the backend. We could either do this with a recursive structure or a flat vector of circuits:The new circuit struct should look something like such:
Each
id
in aCall
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.Ultimately the backend needs two things to handle a stack of function calls:
Rather than a recursive circuit structure we should have the following:
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:
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
The text was updated successfully, but these errors were encountered: