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

chore(ssa): Skip array_set pass for Brillig functions #6513

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Nov 14, 2024

Description

Problem*

Resolves #6486

Summary*

The PR changes the array_set pass to skip Brilling functions completely and remove all code related to is_brillig_runtime as well.

Additional Context

This is undoing the changes made in #6463 to fix a Brillig bug that made arrays shared via function parameters with the caller mutable. Apparently ACIR never had a problem with this, and not being able to do that optimisation makes it less efficient.

I suppose we could make this conditional on the already present is_brillig_runtime flag, however it was decided that the array_set pass was only ever meant to be used by ACIR, and Brillig should use refcounts exclusively to decide whether to mutate or not.

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 and/or cargo fmt on default settings.

Copy link
Contributor

Changes to Brillig bytecode sizes

Generated at commit: d8065ad5a1a6e0fb6588407883ada9d63ff7381f, compared to commit: 0fc0c53ec183890370c69aa4148952b3123cb055

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
brillig_array_to_slice +24 ❌ +26.67%
reference_only_used_as_alias +33 ❌ +13.31%
pedersen_hash +24 ❌ +6.70%
regression_struct_array_conditional +30 ❌ +5.73%
merkle_insert +36 ❌ +4.63%
pedersen_check +27 ❌ +3.62%
brillig_pedersen +27 ❌ +3.62%

Full diff report 👇
Program Brillig opcodes (+/-) %
brillig_array_to_slice 114 (+24) +26.67%
reference_only_used_as_alias 281 (+33) +13.31%
pedersen_hash 382 (+24) +6.70%
regression_struct_array_conditional 554 (+30) +5.73%
merkle_insert 814 (+36) +4.63%
pedersen_check 772 (+27) +3.62%
brillig_pedersen 772 (+27) +3.62%
simple_shield 897 (+27) +3.10%
array_to_slice 1,043 (+30) +2.96%
schnorr 1,492 (+36) +2.47%
fold_complex_outputs 573 (+12) +2.14%
import 149 (+3) +2.05%
array_sort 302 (+6) +2.03%
sha2_byte 3,388 (+66) +1.99%
brillig_rc_regression_6123 175 (+3) +1.74%
hashmap 26,656 (+411) +1.57%
nested_array_dynamic 2,181 (+33) +1.54%
pedersen_commitment 200 (+3) +1.52%
brillig_cow_regression 2,410 (+30) +1.26%
databus_two_calldata 270 (+3) +1.12%
nested_array_in_slice 1,202 (+12) +1.01%
array_dynamic 320 (+3) +0.95%
conditional_1 1,337 (+12) +0.91%
regression_capacity_tracker 231 (+2) +0.87%
regression 729 (+6) +0.83%
slice_dynamic_index 2,736 (+22) +0.81%
keccak256 1,778 (+12) +0.68%
brillig_keccak 1,778 (+12) +0.68%
uhashmap 16,310 (+102) +0.63%
array_dynamic_nested_blackbox_input 994 (+6) +0.61%
array_dynamic_blackbox_input 1,176 (+6) +0.51%
6 1,367 (+6) +0.44%
sha256 2,053 (+9) +0.44%
sha256_var_witness_const_regression 1,403 (+6) +0.43%
conditional_regression_short_circuit 1,437 (+6) +0.42%
fold_numeric_generic_poseidon 770 (+3) +0.39%
no_predicates_numeric_generic_poseidon 770 (+3) +0.39%
brillig_sha256 796 (+3) +0.38%
regression_4449 859 (+3) +0.35%
sha256_var_padding_regression 5,248 (+18) +0.34%
ram_blowup_regression 1,009 (+3) +0.30%
ecdsa_secp256k1 1,013 (+3) +0.30%
sha256_var_size_regression 2,046 (+6) +0.29%
strings 1,042 (+3) +0.29%
sha256_regression 7,272 (+18) +0.25%
poseidonsponge_x5_254 4,502 (+6) +0.13%
regression_5252 4,879 (+6) +0.12%
poseidon_bn254_hash_width_3 5,692 (+6) +0.11%
poseidon_bn254_hash 5,692 (+6) +0.11%
u128 2,911 (+3) +0.10%
eddsa 10,894 (+9) +0.08%

Copy link
Contributor

Changes to circuit sizes

Generated at commit: d8065ad5a1a6e0fb6588407883ada9d63ff7381f, compared to commit: 0fc0c53ec183890370c69aa4148952b3123cb055

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
array_dynamic_nested_blackbox_input -28 ✅ -10.18% -92 ✅ -1.24%
array_dynamic_main_output -11 ✅ -25.00% -42 ✅ -19.00%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
nested_array_dynamic 3,166 (-29) -0.91% 12,930 (-11) -0.09%
nested_dyn_array_regression_5782 32 (-3) -8.57% 2,859 (-4) -0.14%
array_dynamic 102 (-6) -5.56% 3,714 (-17) -0.46%
array_dynamic_nested_blackbox_input 247 (-28) -10.18% 7,315 (-92) -1.24%
array_dynamic_main_output 33 (-11) -25.00% 179 (-42) -19.00%

Copy link
Contributor

Changes to number of Brillig opcodes executed

Generated at commit: d8065ad5a1a6e0fb6588407883ada9d63ff7381f, compared to commit: 0fc0c53ec183890370c69aa4148952b3123cb055

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
reference_only_used_as_alias +163 ❌ +66.53%
regression_capacity_tracker +336 ❌ +58.95%
brillig_array_to_slice +31 ❌ +37.80%
regression_struct_array_conditional +270 ❌ +20.87%
pedersen_hash +99 ❌ +17.22%
merkle_insert +486 ❌ +14.48%

Full diff report 👇
Program Brillig opcodes (+/-) %
reference_only_used_as_alias 408 (+163) +66.53%
regression_capacity_tracker 906 (+336) +58.95%
brillig_array_to_slice 113 (+31) +37.80%
regression_struct_array_conditional 1,564 (+270) +20.87%
pedersen_hash 674 (+99) +17.22%
merkle_insert 3,842 (+486) +14.48%
simple_shield 2,885 (+306) +11.87%
sha2_byte 52,231 (+5,049) +10.70%
brillig_pedersen 1,252 (+117) +10.31%
pedersen_check 1,252 (+117) +10.31%
array_sort 538 (+36) +7.17%
pedersen_commitment 296 (+18) +6.47%
import 153 (+9) +6.25%
array_to_slice 2,475 (+133) +5.68%
fold_complex_outputs 889 (+36) +4.22%
hashmap 58,421 (+2,079) +3.69%
brillig_rc_regression_6123 289 (+9) +3.21%
slice_dynamic_index 4,355 (+132) +3.13%
array_dynamic_blackbox_input 19,955 (+594) +3.07%
schnorr 10,396 (+216) +2.12%
uhashmap 153,618 (+2,796) +1.85%
databus_two_calldata 500 (+9) +1.83%
nested_array_dynamic 3,116 (+54) +1.76%
nested_array_in_slice 1,469 (+24) +1.66%
array_dynamic_nested_blackbox_input 4,629 (+45) +0.98%
regression 2,703 (+18) +0.67%
poseidon_bn254_hash 169,552 (+1,053) +0.62%
poseidon_bn254_hash_width_3 169,552 (+1,053) +0.62%
conditional_1 5,860 (+36) +0.62%
regression_5252 951,206 (+5,400) +0.57%
poseidonsponge_x5_254 190,917 (+1,080) +0.57%
strings 1,897 (+9) +0.48%
regression_4449 219,762 (+630) +0.29%
u128 25,428 (+72) +0.28%
sha256_var_witness_const_regression 7,036 (+18) +0.26%
sha256 10,844 (+27) +0.25%
6 7,357 (+18) +0.25%
conditional_regression_short_circuit 7,432 (+18) +0.24%
eddsa 737,279 (+1,620) +0.22%
brillig_sha256 4,200 (+9) +0.21%
fold_numeric_generic_poseidon 5,239 (+9) +0.17%
no_predicates_numeric_generic_poseidon 5,239 (+9) +0.17%
ecdsa_secp256k1 7,075 (+9) +0.13%
sha256_var_size_regression 18,331 (+18) +0.10%
brillig_keccak 37,099 (+36) +0.10%
keccak256 37,099 (+36) +0.10%
brillig_cow_regression 583,839 (+252) +0.04%
sha256_regression 126,254 (+45) +0.04%
sha256_var_padding_regression 234,837 (+54) +0.02%
ram_blowup_regression 877,404 (+9) +0.00%

@TomAFrench TomAFrench added this pull request to the merge queue Nov 14, 2024
Merged via the queue into master with commit 61d10f2 Nov 14, 2024
49 checks passed
@TomAFrench TomAFrench deleted the 6486-ssa-brillig-no-array-mut branch November 14, 2024 11:28
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Nov 14, 2024
…g/noir#6449)

chore(ssa): Skip array_set pass for Brillig functions (noir-lang/noir#6513)
chore: Reverse ssa parser diff order (noir-lang/noir#6511)
chore: Parse negatives in SSA parser (noir-lang/noir#6510)
feat: avoid unnecessary ssa passes while loop unrolling (noir-lang/noir#6509)
fix(tests): Use a file lock as well as a mutex to isolate tests cases (noir-lang/noir#6508)
fix: set local_module before elaborating each trait (noir-lang/noir#6506)
fix: parse Slice type in SSa (noir-lang/noir#6507)
fix: perform arithmetic simplification through `CheckedCast` (noir-lang/noir#6502)
feat: SSA parser (noir-lang/noir#6489)
chore(test): Run test matrix on test_programs (noir-lang/noir#6429)
chore(ci): fix cargo deny (noir-lang/noir#6501)
feat: Deduplicate instructions across blocks (noir-lang/noir#6499)
chore: move tests for arithmetic generics closer to the code (noir-lang/noir#6497)
fix(docs): Fix broken links in oracles doc (noir-lang/noir#6488)
fix: Treat all parameters as possible aliases of each other (noir-lang/noir#6477)
chore: bump rust dependencies (noir-lang/noir#6482)
feat: use a full `BlackBoxFunctionSolver` implementation when execution brillig during acirgen (noir-lang/noir#6481)
chore(docs): Update How to Oracles (noir-lang/noir#5675)
chore: Release Noir(0.38.0) (noir-lang/noir#6422)
fix(ssa): Change array_set to not mutate slices coming from function inputs (noir-lang/noir#6463)
chore: update example to show how to split public inputs in bash (noir-lang/noir#6472)
fix: Discard optimisation that would change execution ordering or that is related to call outputs (noir-lang/noir#6461)
chore: proptest for `canonicalize` on infix type expressions (noir-lang/noir#6269)
fix: let formatter respect newlines between comments (noir-lang/noir#6458)
fix: check infix expression is valid in program input (noir-lang/noir#6450)
fix: don't crash on AsTraitPath with empty path (noir-lang/noir#6454)
fix(tests): Prevent EOF error while running test programs (noir-lang/noir#6455)
fix(sea): mem2reg to treat block input references as alias (noir-lang/noir#6452)
chore: revamp attributes (noir-lang/noir#6424)
feat!: Always Check Arithmetic Generics at Monomorphization (noir-lang/noir#6329)
chore: split path and import lookups (noir-lang/noir#6430)
fix(ssa): Resolve value IDs in terminator before comparing to array (noir-lang/noir#6448)
fix: right shift is not a regular division (noir-lang/noir#6400)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Nov 14, 2024
chore(ssa): Skip array_set pass for Brillig functions (noir-lang/noir#6513)
chore: Reverse ssa parser diff order (noir-lang/noir#6511)
chore: Parse negatives in SSA parser (noir-lang/noir#6510)
feat: avoid unnecessary ssa passes while loop unrolling (noir-lang/noir#6509)
fix(tests): Use a file lock as well as a mutex to isolate tests cases (noir-lang/noir#6508)
fix: set local_module before elaborating each trait (noir-lang/noir#6506)
fix: parse Slice type in SSa (noir-lang/noir#6507)
fix: perform arithmetic simplification through `CheckedCast` (noir-lang/noir#6502)
feat: SSA parser (noir-lang/noir#6489)
chore(test): Run test matrix on test_programs (noir-lang/noir#6429)
chore(ci): fix cargo deny (noir-lang/noir#6501)
feat: Deduplicate instructions across blocks (noir-lang/noir#6499)
chore: move tests for arithmetic generics closer to the code (noir-lang/noir#6497)
fix(docs): Fix broken links in oracles doc (noir-lang/noir#6488)
fix: Treat all parameters as possible aliases of each other (noir-lang/noir#6477)
chore: bump rust dependencies (noir-lang/noir#6482)
feat: use a full `BlackBoxFunctionSolver` implementation when execution brillig during acirgen (noir-lang/noir#6481)
chore(docs): Update How to Oracles (noir-lang/noir#5675)
chore: Release Noir(0.38.0) (noir-lang/noir#6422)
fix(ssa): Change array_set to not mutate slices coming from function inputs (noir-lang/noir#6463)
chore: update example to show how to split public inputs in bash (noir-lang/noir#6472)
fix: Discard optimisation that would change execution ordering or that is related to call outputs (noir-lang/noir#6461)
chore: proptest for `canonicalize` on infix type expressions (noir-lang/noir#6269)
fix: let formatter respect newlines between comments (noir-lang/noir#6458)
fix: check infix expression is valid in program input (noir-lang/noir#6450)
fix: don't crash on AsTraitPath with empty path (noir-lang/noir#6454)
fix(tests): Prevent EOF error while running test programs (noir-lang/noir#6455)
fix(sea): mem2reg to treat block input references as alias (noir-lang/noir#6452)
chore: revamp attributes (noir-lang/noir#6424)
feat!: Always Check Arithmetic Generics at Monomorphization (noir-lang/noir#6329)
chore: split path and import lookups (noir-lang/noir#6430)
fix(ssa): Resolve value IDs in terminator before comparing to array (noir-lang/noir#6448)
fix: right shift is not a regular division (noir-lang/noir#6400)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Nov 14, 2024
…me intepreter (noir-lang/noir#6514)

fix: disallow `#[test]` on associated functions (noir-lang/noir#6449)
chore(ssa): Skip array_set pass for Brillig functions (noir-lang/noir#6513)
chore: Reverse ssa parser diff order (noir-lang/noir#6511)
chore: Parse negatives in SSA parser (noir-lang/noir#6510)
feat: avoid unnecessary ssa passes while loop unrolling (noir-lang/noir#6509)
fix(tests): Use a file lock as well as a mutex to isolate tests cases (noir-lang/noir#6508)
fix: set local_module before elaborating each trait (noir-lang/noir#6506)
fix: parse Slice type in SSa (noir-lang/noir#6507)
fix: perform arithmetic simplification through `CheckedCast` (noir-lang/noir#6502)
feat: SSA parser (noir-lang/noir#6489)
chore(test): Run test matrix on test_programs (noir-lang/noir#6429)
chore(ci): fix cargo deny (noir-lang/noir#6501)
feat: Deduplicate instructions across blocks (noir-lang/noir#6499)
chore: move tests for arithmetic generics closer to the code (noir-lang/noir#6497)
fix(docs): Fix broken links in oracles doc (noir-lang/noir#6488)
fix: Treat all parameters as possible aliases of each other (noir-lang/noir#6477)
chore: bump rust dependencies (noir-lang/noir#6482)
feat: use a full `BlackBoxFunctionSolver` implementation when execution brillig during acirgen (noir-lang/noir#6481)
chore(docs): Update How to Oracles (noir-lang/noir#5675)
chore: Release Noir(0.38.0) (noir-lang/noir#6422)
fix(ssa): Change array_set to not mutate slices coming from function inputs (noir-lang/noir#6463)
chore: update example to show how to split public inputs in bash (noir-lang/noir#6472)
fix: Discard optimisation that would change execution ordering or that is related to call outputs (noir-lang/noir#6461)
chore: proptest for `canonicalize` on infix type expressions (noir-lang/noir#6269)
fix: let formatter respect newlines between comments (noir-lang/noir#6458)
fix: check infix expression is valid in program input (noir-lang/noir#6450)
fix: don't crash on AsTraitPath with empty path (noir-lang/noir#6454)
fix(tests): Prevent EOF error while running test programs (noir-lang/noir#6455)
fix(sea): mem2reg to treat block input references as alias (noir-lang/noir#6452)
chore: revamp attributes (noir-lang/noir#6424)
feat!: Always Check Arithmetic Generics at Monomorphization (noir-lang/noir#6329)
chore: split path and import lookups (noir-lang/noir#6430)
fix(ssa): Resolve value IDs in terminator before comparing to array (noir-lang/noir#6448)
fix: right shift is not a regular division (noir-lang/noir#6400)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove concept of array_set mut in brillig functions
2 participants