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: Rework mem2reg load & store removal #6460

Closed
wants to merge 7 commits into from

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Nov 5, 2024

Description

Problem*

Resolves

Summary*

Draft so that I can compare opcode differences as I work on this PR. Currently I've removed all the extra load/store removals but haven't added them back so initial differences are expected to be bad, similar to #6434.

The goal is to rework the additions to mem2reg (notably all the different ways loads & stores are removed) to be more unified and to handle aliases better.

Additional Context

The end goal of this PR is not necessarily to improve brillig bytecode size (although that'd be a nice bonus) but to rework how we handle load & store removal in mem2reg currently to be less buggy so that #6434 can re-enable this feature without bugs and fix the opcode counts in that PR.

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

github-actions bot commented Nov 5, 2024

Changes to Brillig bytecode sizes

Generated at commit: fd530e96f9ab0f46fbc4b67e1e6f13e15b77520a, compared to commit: 713df69aad56fc5aaefd5d140275a3217de4d866

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
references -33 ✅ -10.61%
reference_only_used_as_alias -32 ✅ -11.39%
missing_closure_env -3 ✅ -12.00%
closures_mut_ref -7 ✅ -23.33%
brillig_slices -174 ✅ -24.34%
tuples -16 ✅ -30.19%
brillig_rc_regression_6123 -56 ✅ -30.94%
brillig_loop_size_regression -25 ✅ -33.78%

Full diff report 👇
Program Brillig opcodes (+/-) %
6_array 451 (+18) +4.16%
signed_div 180 (+6) +3.45%
import 153 (+4) +2.68%
pedersen_commitment 204 (+4) +2.00%
simple_shield 913 (+16) +1.78%
pedersen_check 785 (+13) +1.68%
brillig_pedersen 785 (+13) +1.68%
merkle_insert 822 (+8) +0.98%
pedersen_hash 384 (+2) +0.52%
6 1,375 (+6) +0.44%
conditional_regression_short_circuit 1,445 (+6) +0.42%
strings 1,046 (+4) +0.38%
brillig_sha256 800 (+3) +0.38%
regression_4449 863 (+3) +0.35%
array_dynamic_nested_blackbox_input 998 (+3) +0.30%
ecdsa_secp256k1 1,017 (+3) +0.30%
schnorr 1,496 (+4) +0.27%
conditional_1 1,345 (+3) +0.22%
sha256 2,057 (+1) +0.05%
nested_array_dynamic 2,144 (-1) -0.05%
u128 2,909 (-3) -0.10%
sha256_var_witness_const_regression 1,403 (-2) -0.14%
array_to_slice 1,041 (-3) -0.29%
brillig_cow_regression 2,408 (-8) -0.33%
check_large_field_bits 286 (-1) -0.35%
slice_regex 2,568 (-9) -0.35%
sha256_regression 7,248 (-30) -0.41%
sha2_byte 3,374 (-14) -0.41%
array_dynamic_blackbox_input 1,173 (-5) -0.42%
poseidonsponge_x5_254 4,474 (-20) -0.45%
databus_two_calldata 222 (-1) -0.45%
poseidon_bn254_hash_width_3 5,652 (-32) -0.56%
poseidon_bn254_hash 5,652 (-32) -0.56%
sha256_var_padding_regression 5,224 (-30) -0.57%
ram_blowup_regression 1,004 (-6) -0.59%
higher_order_functions 736 (-5) -0.67%
sha256_var_size_regression 2,034 (-14) -0.68%
regression_5252 4,852 (-40) -0.82%
slice_dynamic_index 2,783 (-25) -0.89%
7_function 636 (-7) -1.09%
regression_struct_array_conditional 549 (-9) -1.61%
slices 2,266 (-52) -2.24%
eddsa 10,835 (-285) -2.56%
brillig_cow 381 (-12) -3.05%
integer_array_indexing 59 (-3) -4.84%
fold_complex_outputs 558 (-30) -5.10%
derive 137 (-8) -5.52%
hashmap 26,010 (-1,584) -5.74%
signed_arithmetic 255 (-16) -5.90%
no_predicates_numeric_generic_poseidon 758 (-48) -5.96%
fold_numeric_generic_poseidon 758 (-48) -5.96%
3_add 47 (-3) -6.00%
uhashmap 15,614 (-1,102) -6.59%
poseidon2 337 (-24) -6.65%
bench_2_to_17 330 (-24) -6.78%
1_mul 62 (-5) -7.46%
fold_2_to_17 571 (-50) -8.05%
regression_6451 44 (-4) -8.33%
references 278 (-33) -10.61%
reference_only_used_as_alias 249 (-32) -11.39%
missing_closure_env 22 (-3) -12.00%
closures_mut_ref 23 (-7) -23.33%
brillig_slices 541 (-174) -24.34%
tuples 37 (-16) -30.19%
brillig_rc_regression_6123 125 (-56) -30.94%
brillig_loop_size_regression 49 (-25) -33.78%

Copy link
Contributor

github-actions bot commented Nov 5, 2024

Changes to circuit sizes

Generated at commit: 6cbc6ff3d5df777bb7a3b37d5cf3c955460a2aba, compared to commit: 35dedb54a0853ba0fa85038d832a520f9ba01a98

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
sha256_var_size_regression +20 ❌ +0.12% +19 ❌ +0.03%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
sha256_var_size_regression 16,181 (+20) +0.12% 70,980 (+19) +0.03%

Copy link
Contributor

github-actions bot commented Nov 5, 2024

Changes to number of Brillig opcodes executed

Generated at commit: 6cbc6ff3d5df777bb7a3b37d5cf3c955460a2aba, compared to commit: 35dedb54a0853ba0fa85038d832a520f9ba01a98

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
brillig_block_parameter_liveness +6,000 ❌ +206.19%
tuples +17 ❌ +53.13%
conditional_regression_661 +19 ❌ +16.10%
references +67 ❌ +15.91%
6 +858 ❌ +11.63%
conditional_regression_short_circuit +858 ❌ +11.52%
derive +13 ❌ +9.77%

Full diff report 👇
Program Brillig opcodes (+/-) %
brillig_block_parameter_liveness 8,910 (+6,000) +206.19%
tuples 49 (+17) +53.13%
conditional_regression_661 137 (+19) +16.10%
references 488 (+67) +15.91%
6 8,233 (+858) +11.63%
conditional_regression_short_circuit 8,308 (+858) +11.52%
derive 146 (+13) +9.77%
regression_4449 240,692 (+20,790) +9.45%
hashmap 61,304 (+4,113) +7.19%
nested_dyn_array_regression_5782 135 (+9) +7.14%
brillig_sha256 4,506 (+297) +7.06%
array_dynamic_nested_blackbox_input 4,811 (+303) +6.72%
sha256 11,477 (+619) +5.70%
conditional_1 6,142 (+297) +5.08%
higher_order_functions 1,590 (+72) +4.74%
uhashmap 159,571 (+7,087) +4.65%
sha256_var_witness_const_regression 7,363 (+322) +4.57%
ecdsa_secp256k1 7,381 (+297) +4.19%
6_array 1,922 (+77) +4.17%
brillig_slices 745 (+26) +3.62%
conditional_regression_421 90 (+3) +3.45%
if_else_chain 102 (+3) +3.03%
brillig_calls_conditionals 114 (+3) +2.70%
pedersen_commitment 284 (+6) +2.16%
import 147 (+3) +2.08%
poseidon2 720 (+12) +1.69%
brillig_pedersen 1,153 (+18) +1.59%
pedersen_check 1,153 (+18) +1.59%
simple_shield 2,618 (+39) +1.51%
slices 3,925 (+43) +1.11%
merkle_insert 3,392 (+36) +1.07%
pedersen_hash 581 (+6) +1.04%
array_to_slice 2,382 (+24) +1.02%
strings 1,907 (+19) +1.01%
7_function 2,839 (+27) +0.96%
brillig_cow 1,294 (+12) +0.94%
regression_capacity_tracker 576 (+4) +0.70%
sha2_byte 47,500 (+297) +0.63%
array_dynamic 502 (+3) +0.60%
regression 2,714 (+14) +0.52%
slice_dynamic_index 4,341 (+21) +0.49%
fold_numeric_generic_poseidon 5,284 (+24) +0.46%
no_predicates_numeric_generic_poseidon 5,284 (+24) +0.46%
sha256_regression 126,775 (+422) +0.33%
slice_regex 3,861 (+12) +0.31%
sha256_var_size_regression 18,404 (+50) +0.27%
array_dynamic_blackbox_input 19,437 (+50) +0.26%
schnorr 10,202 (+18) +0.18%
sha256_var_padding_regression 235,091 (+150) +0.06%
brillig_cow_regression 583,804 (+31) +0.01%
poseidon_bn254_hash 169,332 (+8) +0.00%
poseidon_bn254_hash_width_3 169,332 (+8) +0.00%
eddsa 737,929 (+31) +0.00%
regression_5252 951,276 (+30) +0.00%
ram_blowup_regression 877,687 (+25) +0.00%
fold_2_to_17 131,469,850 (+27) +0.00%
bench_2_to_17 144,690,675 (+15) +0.00%

@jfecher
Copy link
Contributor Author

jfecher commented Nov 5, 2024

Commenting to preserve the changes to brillig bytecode sizes comment, this is everything we lose from not doing load & store removal:

Changes to Brillig bytecode sizes

Generated at commit: da19e6da5118bf8acdf435a33ff45aef478d100c, compared to commit: 35dedb54a0853ba0fa85038d832a520f9ba01a98

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
brillig_block_parameter_liveness +16,000 ❌ +326.86%
regression_unsafe_no_predicates +28 ❌ +93.33%
tuples +28 ❌ +60.87%
regression_5045 +38 ❌ +44.71%
conditional_regression_661 +56 ❌ +43.08%
brillig_slices +207 ❌ +38.33%
references +95 ❌ +32.53%
bench_2_to_17 +56 ❌ +16.62%
Full diff report 👇

Program Brillig opcodes (+/-) %
brillig_block_parameter_liveness 20,895 (+16,000) +326.86%
regression_unsafe_no_predicates 58 (+28) +93.33%
tuples 74 (+28) +60.87%
regression_5045 123 (+38) +44.71%
conditional_regression_661 186 (+56) +43.08%
brillig_slices 747 (+207) +38.33%
references 387 (+95) +32.53%
bench_2_to_17 393 (+56) +16.62%
fold_2_to_17 683 (+96) +16.35%
hashmap 31,411 (+4,348) +16.07%
poseidon2 397 (+53) +15.41%
6_array 505 (+67) +15.30%
derive 156 (+20) +14.71%
no_predicates_numeric_generic_poseidon 862 (+93) +12.09%
fold_numeric_generic_poseidon 862 (+93) +12.09%
uhashmap 18,318 (+1,792) +10.84%
6 1,501 (+96) +6.83%
conditional_regression_short_circuit 1,575 (+100) +6.78%
nested_array_in_slice 1,431 (+87) +6.47%
nested_dyn_array_regression_5782 154 (+9) +6.21%
7_function 677 (+38) +5.95%
higher_order_functions 781 (+42) +5.68%
regression 771 (+39) +5.33%
slices 2,417 (+113) +4.90%
brillig_sha256 853 (+38) +4.66%
array_dynamic_nested_blackbox_input 1,053 (+46) +4.57%
sha256 2,207 (+96) +4.55%
regression_4449 916 (+38) +4.33%
regression_capacity_tracker 243 (+10) +4.29%
sha256_var_witness_const_regression 1,500 (+58) +4.02%
eddsa 11,592 (+430) +3.85%
brillig_cow 440 (+16) +3.77%
ecdsa_secp256k1 1,070 (+38) +3.68%
brillig_rc_regression_6123 179 (+6) +3.47%
conditional_regression_421 93 (+3) +3.33%
slice_dynamic_index 2,963 (+94) +3.28%
conditional_1 1,392 (+42) +3.11%
array_to_slice 1,048 (+29) +2.85%
import 150 (+4) +2.74%
fold_complex_outputs 581 (+15) +2.65%
if_else_chain 117 (+3) +2.63%
brillig_calls_conditionals 130 (+3) +2.36%
sha256_var_padding_regression 5,494 (+120) +2.23%
brillig_pedersen 761 (+16) +2.15%
pedersen_check 761 (+16) +2.15%
pedersen_commitment 201 (+4) +2.03%
ram_blowup_regression 1,050 (+20) +1.94%
strings 1,059 (+20) +1.92%
sha256_var_size_regression 2,129 (+40) +1.91%
sha256_regression 7,549 (+138) +1.86%
simple_shield 886 (+16) +1.84%
sha2_byte 3,406 (+59) +1.76%
array_dynamic_blackbox_input 1,215 (+21) +1.76%
regression_5252 4,941 (+63) +1.29%
array_dynamic 332 (+4) +1.22%
brillig_cow_regression 2,444 (+29) +1.20%
pedersen_hash 362 (+4) +1.12%
merkle_insert 786 (+8) +1.03%
regression_struct_array_conditional 550 (+4) +0.73%
array_sort 298 (+2) +0.68%
schnorr 1,468 (+8) +0.55%
slice_regex 2,589 (+12) +0.47%
brillig_keccak 1,774 (+4) +0.23%
keccak256 1,774 (+4) +0.23%
poseidon_bn254_hash_width_3 5,702 (+12) +0.21%
poseidon_bn254_hash 5,702 (+12) +0.21%
poseidonsponge_x5_254 4,504 (+4) +0.09%
u128 2,918 (+1) +0.03%

@jfecher
Copy link
Contributor Author

jfecher commented Nov 18, 2024

Closing this to work on an alternate solution

@jfecher jfecher closed this Nov 18, 2024
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.

1 participant