-
Notifications
You must be signed in to change notification settings - Fork 200
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
chore(ssa refactor): blackbox array arg support #1484
chore(ssa refactor): blackbox array arg support #1484
Conversation
crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs
Outdated
Show resolved
Hide resolved
I'm not particularly happy about the inconsistent passing of
Suggestions welcome. |
if Self::value_is_array_address(*value_id, dfg) { | ||
let (array_id, index) = self | ||
.ssa_value_to_array_address | ||
.get(value_id) | ||
.expect("ICE: Call argument of undeclared array"); | ||
assert_eq!(index, &0, "ICE: Call arguments only accept arrays in their entirety"); | ||
let elements = self | ||
.acir_context | ||
.array_load_all(*array_id) | ||
.expect("add Result types to all methods so errors bubble up"); | ||
inputs.extend(elements); | ||
} else { | ||
inputs.push(self.convert_ssa_value(*value_id, dfg)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see what you're saying about this check being a bit odd. Can it be reconciled if we pass inputs
to convert_ssa_value
such that the function can push multiple inputs if needed? E.g. self.convert_ssa_value(*value_id, dfg, &mut inputs);
. Then that function could handle array addresses internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Currently call instructions are the only place where we need to gather the contents of an array. Until that changes, I figure it's better not to force that interface change on all the other places that use convert_ssa_value
.
crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/memory.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some nits -- Feel free to merge and address in a follow-up PR
* comment out tests for normal code path * chore(ssa refactor): Add method to handle black box functions in Acir gen (#1437) * add field mul and div * add code to process field mul and div * add assert example * add `is_equal` constraint * add `eq_var` method for AcirVar * process `Constrain` instruction and BinaryOp::Eq * add TODO for more than the maximum number of bits * add numeric_cast_var method which constrains a variable to be equal to a NumericType * implement casting for numeric types * add simple range constraint example * add constraints for `more_than_eq` * - add more_than_eq method - This method needs to know the bit size, so we cache this information whenever we do a range constraint. We should ideally also cache it for constants too since we can figure out their bit-sizes easily * add method to process less than binary operation * add example * assign result of cast operation * add `y` as an input value * return optimized circuit * refactor code so that mapping of the ValueIds to AcirVar is more explicit. This way we need to explicitly mark instructions as not returning anything. * Add function to push intrinsic into the IR * add method to call blackbox functions using a list of AcirVar * Addressed in Address GtEq extra opcodes #1444 * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs Co-authored-by: jfecher <jake@aztecprotocol.com> * fix clippy * add code to handle calls to intrinsics * add example of blackbox function * remove doc test * address TODO by moving most of the blackbox logic into GeneratedACIR module * refactor black box function logic -- also renamed it from intrinsics to blackbox functions * remove TODO --------- Co-authored-by: jfecher <jake@aztecprotocol.com> Co-authored-by: Joss <joss@aztecprotocol.com> * chore(ssa refactor): add range constraints for array param elements (#1460) * chore(ssa refactor): acir-gen arrays in main * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/memory.rs * chore(ssa refactor): PR suggestions * chore(ssa refactor): simplify test into something I know the input syntax for :-/ * chore(ssa refactor): rm todo (now has issue) * chore(ssa refactor): rm unused * chore(ssa refactor): abi_gen comment * chore(ssa refactor): split convert_ssa_load * chore(ssa refactor): more comments * chore(ssa refactor): more comments * chore(ssa refactor): add range constraints for array param elements * chore(ssa refactor): rename vars --------- Co-authored-by: kevaundray <kevtheappdev@gmail.com> * chore(ssa refactor): hookup alloc + store instructions (#1464) * chore(ssa refactor): acir-gen arrays in main * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/memory.rs * chore(ssa refactor): PR suggestions * chore(ssa refactor): simplify test into something I know the input syntax for :-/ * chore(ssa refactor): rm todo (now has issue) * chore(ssa refactor): rm unused * chore(ssa refactor): abi_gen comment * chore(ssa refactor): split convert_ssa_load * chore(ssa refactor): more comments * chore(ssa refactor): more comments * chore(ssa refactor): hookup alloc + store instructions * chore(ssa refactor): rm assert * chore(ssa refactor): rm double insert * Revert "chore(ssa refactor): rm double insert" This reverts commit f47a75f. * chore(ssa refactor): rm double insert --------- Co-authored-by: kevaundray <kevtheappdev@gmail.com> * chore(ssa refactor): bitwise ops (rebased) (#1477) * chore(ssa refactor): bitwise ops * Update crates/nargo_cli/tests/test_data_ssa_refactor/simple_bitwise/Prover.toml * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs * chore(ssa refactor): fix OR impl + tidy up * chore(ssa refactor): remove XOR & AND 1-bit opt; Assert equal bit size * chore(ssa refactor): PR feedback --------- Co-authored-by: kevaundray <kevtheappdev@gmail.com> * chore(ssa refactor): Adds a regression test for repeated op (#1470) * add test for adding same constant * chore(ssa refactor): add_data dupe fix * chore(ssa refactor): update regression test and remove redundant dupe check * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs * make test simpler --------- Co-authored-by: Joss <joss@aztecprotocol.com> * chore(ssa refactor): Add code to handle left and right shifts (#1471) * add shift left and shift right in AcirVar * call shl and shr in `convert_ssa_binary` * add example * remove TODO * update todos * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --------- Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> Co-authored-by: jfecher <jake@aztecprotocol.com> Co-authored-by: Joss <joss@aztecprotocol.com> * chore(ssa refactor): blackbox array arg support (#1484) * chore(ssa refactor): blackbox array arg support * chore(ssa refactor): fix post merge * chore(ssa refactor): push array flattening up * chore(ssa refactor): fix PR nits * chore(ssa refactor): ACIR-gen log directives (#1488) * chore(ssa refactor): blackbox array arg support * chore(ssa refactor): fix post merge * chore(ssa refactor): push array flattening up * chore(ssa refactor): ACIR-gen log directives * add back tests * chore: generate brillig opcodes for an empty function (#1448) * Brillig almost e2e for empty function * Code review * Donot inline brillig functions * decouple brillig from ssa * Add doc comments * convert only brillig functions into brillig * fix typo * add `new_function` and `new_brillig_function` * refactor code to match * fix clippy * call intrinsic directly via match --------- Co-authored-by: Kevaundray Wedderburn <kevtheappdev@gmail.com> * chore(ssa refactor): Remove dead blackboxes and handle pedersen domain separator (#1503) * chore(ssa refactor): Rm dead blackboxes and handle pedersen domain separator * chore(ssa refactor): variable keccak * chore(ssa refactor): switch between var/fixed keccak * use simpler case for now * remove Keccak message_size logic --------- Co-authored-by: Kevaundray Wedderburn <kevtheappdev@gmail.com> --------- Co-authored-by: jfecher <jake@aztecprotocol.com> Co-authored-by: Joss <joss@aztecprotocol.com> Co-authored-by: joss-aztec <94053499+joss-aztec@users.noreply.github.com> Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> Co-authored-by: guipublic <47281315+guipublic@users.noreply.github.com>
* comment out tests for normal code path * chore(ssa refactor): Add method to handle black box functions in Acir gen (#1437) * add field mul and div * add code to process field mul and div * add assert example * add `is_equal` constraint * add `eq_var` method for AcirVar * process `Constrain` instruction and BinaryOp::Eq * add TODO for more than the maximum number of bits * add numeric_cast_var method which constrains a variable to be equal to a NumericType * implement casting for numeric types * add simple range constraint example * add constraints for `more_than_eq` * - add more_than_eq method - This method needs to know the bit size, so we cache this information whenever we do a range constraint. We should ideally also cache it for constants too since we can figure out their bit-sizes easily * add method to process less than binary operation * add example * assign result of cast operation * add `y` as an input value * return optimized circuit * refactor code so that mapping of the ValueIds to AcirVar is more explicit. This way we need to explicitly mark instructions as not returning anything. * Add function to push intrinsic into the IR * add method to call blackbox functions using a list of AcirVar * Addressed in Address GtEq extra opcodes #1444 * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs Co-authored-by: jfecher <jake@aztecprotocol.com> * fix clippy * add code to handle calls to intrinsics * add example of blackbox function * remove doc test * address TODO by moving most of the blackbox logic into GeneratedACIR module * refactor black box function logic -- also renamed it from intrinsics to blackbox functions * remove TODO --------- Co-authored-by: jfecher <jake@aztecprotocol.com> Co-authored-by: Joss <joss@aztecprotocol.com> * chore(ssa refactor): add range constraints for array param elements (#1460) * chore(ssa refactor): acir-gen arrays in main * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/memory.rs * chore(ssa refactor): PR suggestions * chore(ssa refactor): simplify test into something I know the input syntax for :-/ * chore(ssa refactor): rm todo (now has issue) * chore(ssa refactor): rm unused * chore(ssa refactor): abi_gen comment * chore(ssa refactor): split convert_ssa_load * chore(ssa refactor): more comments * chore(ssa refactor): more comments * chore(ssa refactor): add range constraints for array param elements * chore(ssa refactor): rename vars --------- Co-authored-by: kevaundray <kevtheappdev@gmail.com> * chore(ssa refactor): hookup alloc + store instructions (#1464) * chore(ssa refactor): acir-gen arrays in main * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/memory.rs * chore(ssa refactor): PR suggestions * chore(ssa refactor): simplify test into something I know the input syntax for :-/ * chore(ssa refactor): rm todo (now has issue) * chore(ssa refactor): rm unused * chore(ssa refactor): abi_gen comment * chore(ssa refactor): split convert_ssa_load * chore(ssa refactor): more comments * chore(ssa refactor): more comments * chore(ssa refactor): hookup alloc + store instructions * chore(ssa refactor): rm assert * chore(ssa refactor): rm double insert * Revert "chore(ssa refactor): rm double insert" This reverts commit f47a75f. * chore(ssa refactor): rm double insert --------- Co-authored-by: kevaundray <kevtheappdev@gmail.com> * chore(ssa refactor): bitwise ops (rebased) (#1477) * chore(ssa refactor): bitwise ops * Update crates/nargo_cli/tests/test_data_ssa_refactor/simple_bitwise/Prover.toml * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs * chore(ssa refactor): fix OR impl + tidy up * chore(ssa refactor): remove XOR & AND 1-bit opt; Assert equal bit size * chore(ssa refactor): PR feedback --------- Co-authored-by: kevaundray <kevtheappdev@gmail.com> * chore(ssa refactor): Adds a regression test for repeated op (#1470) * add test for adding same constant * chore(ssa refactor): add_data dupe fix * chore(ssa refactor): update regression test and remove redundant dupe check * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs * make test simpler --------- Co-authored-by: Joss <joss@aztecprotocol.com> * chore(ssa refactor): Add code to handle left and right shifts (#1471) * add shift left and shift right in AcirVar * call shl and shr in `convert_ssa_binary` * add example * remove TODO * update todos * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> * Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --------- Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> Co-authored-by: jfecher <jake@aztecprotocol.com> Co-authored-by: Joss <joss@aztecprotocol.com> * chore(ssa refactor): blackbox array arg support (#1484) * chore(ssa refactor): blackbox array arg support * chore(ssa refactor): fix post merge * chore(ssa refactor): push array flattening up * chore(ssa refactor): fix PR nits * chore(ssa refactor): ACIR-gen log directives (#1488) * chore(ssa refactor): blackbox array arg support * chore(ssa refactor): fix post merge * chore(ssa refactor): push array flattening up * chore(ssa refactor): ACIR-gen log directives * chore(ssa refactor): to_radix * add back tests * chore: generate brillig opcodes for an empty function (#1448) * Brillig almost e2e for empty function * Code review * Donot inline brillig functions * decouple brillig from ssa * Add doc comments * convert only brillig functions into brillig * fix typo * add `new_function` and `new_brillig_function` * refactor code to match * fix clippy * call intrinsic directly via match --------- Co-authored-by: Kevaundray Wedderburn <kevtheappdev@gmail.com> * chore(ssa refactor): Remove dead blackboxes and handle pedersen domain separator (#1503) * chore(ssa refactor): Rm dead blackboxes and handle pedersen domain separator * chore(ssa refactor): variable keccak * chore(ssa refactor): switch between var/fixed keccak * use simpler case for now * remove Keccak message_size logic --------- Co-authored-by: Kevaundray Wedderburn <kevtheappdev@gmail.com> * chore(ssa refactor): finish radix impl * chore(ssa refactor): PR feedback --------- Co-authored-by: Kevaundray Wedderburn <kevtheappdev@gmail.com> Co-authored-by: jfecher <jake@aztecprotocol.com> Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> Co-authored-by: guipublic <47281315+guipublic@users.noreply.github.com>
Description
Problem*
ACIR gen didn't support array arguments to black boxes
Resolves
Summary*
acir_gen/mod
Forwards blackbox arguments toacir_variable
as a mix ofAcirVar
andArrayIds
.acir_variable
they take responsibility for rationalising the argumentsExample
compiles to
Documentation
This PR requires documentation updates when merged.
Additional Context
PR Checklist*
cargo fmt
on default settings.