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 DIE pass to operate on a postorder SCC CFG #6490

Closed
wants to merge 7 commits into from

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Nov 8, 2024

Description

Problem*

This is currently preventing DIE from being able to remove side-effectful instructions like stores or increment_rcs directly. Instead we rely on either special checks in mem2reg (for stores) or special checks in DIE for inc/decrement rcs.

Summary*

This is part of a breakout change for #6460 since that PR changes mem2reg to use DIE to remove stores, but it needs this change to handle side-effectful instructions in the presence of loops. When loops are in the program and DIE is run, it previously could remove a store that would be used again next iteration of the loop.

This PR changes DIE to use a graph of strongly-connected components of blocks and to operate on every block within an SCC all at once. This requires a more traditional DIE algorithm which repeats until there are no more changes. This isn't necessary in ours currently since acir is always 1 block and normal scoping semantics prevent creating brillig programs which refer to the same instruction across multiple blocks (unless it is a store).

Additional Context

Currently a draft while I just verify it does work while merged into #6460, and to see if this has any brillig opcode regressions.

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 8, 2024

Changes to Brillig bytecode sizes

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

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
slice_regex +156 ❌ +6.05%
regression_4449 +36 ❌ +4.19%
brillig_hash_to_field +6 ❌ +4.03%
hash_to_field +6 ❌ +4.03%
check_large_field_bits -12 ✅ -4.18%
cast_and_shift_global -8 ✅ -11.27%

Full diff report 👇
Program Brillig opcodes (+/-) %
slice_regex 2,733 (+156) +6.05%
regression_4449 896 (+36) +4.19%
brillig_hash_to_field 155 (+6) +4.03%
hash_to_field 155 (+6) +4.03%
regression_5252 5,059 (+167) +3.41%
sha256_var_size_regression 2,114 (+66) +3.22%
wildcard_type 294 (+9) +3.16%
array_dynamic_blackbox_input 1,199 (+21) +1.78%
ram_blowup_regression 1,028 (+18) +1.78%
poseidonsponge_x5_254 4,565 (+71) +1.58%
sha256_var_padding_regression 5,326 (+72) +1.37%
keccak256 1,806 (+24) +1.35%
brillig_keccak 1,806 (+24) +1.35%
brillig_cow_regression 2,445 (+29) +1.20%
no_predicates_numeric_generic_poseidon 815 (+9) +1.12%
fold_numeric_generic_poseidon 815 (+9) +1.12%
sha256_regression 7,344 (+66) +0.91%
6 1,381 (+12) +0.88%
sha256 2,074 (+18) +0.88%
slice_loop 352 (+3) +0.86%
sha256_var_witness_const_regression 1,417 (+12) +0.85%
conditional_regression_short_circuit 1,451 (+12) +0.83%
brillig_sha256 803 (+6) +0.75%
merkle_insert 820 (+6) +0.74%
6_array 436 (+3) +0.69%
hashmap 27,784 (+190) +0.69%
poseidon_bn254_hash_width_3 5,722 (+38) +0.67%
poseidon_bn254_hash 5,722 (+38) +0.67%
array_dynamic_nested_blackbox_input 1,001 (+6) +0.60%
ecdsa_secp256k1 1,020 (+6) +0.59%
regression_4709 134,507 (+774) +0.58%
eddsa 11,180 (+60) +0.54%
sha2_byte 3,406 (+18) +0.53%
conditional_1 1,348 (+6) +0.45%
simple_shield 900 (+3) +0.33%
uhashmap 16,750 (+34) +0.20%
references 307 (-4) -1.29%
fold_complex_outputs 580 (-8) -1.36%
regression_struct_array_conditional 549 (-9) -1.61%
generics 204 (-4) -1.92%
side_effects_constrain_array 119 (-4) -3.25%
check_large_field_bits 275 (-12) -4.18%
cast_and_shift_global 63 (-8) -11.27%

Copy link
Contributor

github-actions bot commented Nov 8, 2024

Changes to number of Brillig opcodes executed

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

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
regression_4449 +23,240 ❌ +10.57%
slice_regex +261 ❌ +6.78%
wildcard_type +25 ❌ +5.09%
side_effects_constrain_array -6 ✅ -7.14%
check_large_field_bits -210 ✅ -7.67%
cast_and_shift_global -30 ✅ -14.71%

Full diff report 👇
Program Brillig opcodes (+/-) %
regression_4449 243,063 (+23,240) +10.57%
slice_regex 4,110 (+261) +6.78%
wildcard_type 516 (+25) +5.09%
sha256_var_witness_const_regression 7,134 (+96) +1.36%
fold_numeric_generic_poseidon 5,486 (+72) +1.33%
no_predicates_numeric_generic_poseidon 5,486 (+72) +1.33%
sha256 10,991 (+144) +1.33%
6 7,455 (+96) +1.30%
conditional_regression_short_circuit 7,530 (+96) +1.29%
brillig_sha256 4,249 (+48) +1.14%
array_dynamic_nested_blackbox_input 4,678 (+48) +1.04%
sha256_var_size_regression 18,489 (+156) +0.85%
6_array 1,855 (+15) +0.82%
conditional_1 5,949 (+48) +0.81%
slice_loop 1,225 (+9) +0.74%
ecdsa_secp256k1 7,124 (+48) +0.68%
brillig_hash_to_field 958 (+6) +0.63%
hash_to_field 958 (+6) +0.63%
regression_5252 985,891 (+6,122) +0.62%
array_dynamic_blackbox_input 20,894 (+126) +0.61%
poseidonsponge_x5_254 197,365 (+1,165) +0.59%
hashmap 60,320 (+318) +0.53%
poseidon_bn254_hash 178,792 (+867) +0.49%
poseidon_bn254_hash_width_3 178,792 (+867) +0.49%
merkle_insert 3,977 (+18) +0.45%
sha256_regression 126,722 (+462) +0.37%
simple_shield 2,873 (+9) +0.31%
eddsa 751,520 (+2,307) +0.31%
brillig_cow_regression 585,490 (+1,608) +0.28%
ram_blowup_regression 879,757 (+2,352) +0.27%
brillig_keccak 37,199 (+96) +0.26%
keccak256 37,199 (+96) +0.26%
sha256_var_padding_regression 235,407 (+564) +0.24%
sha2_byte 53,644 (+96) +0.18%
uhashmap 156,483 (+108) +0.07%
generics 234 (-2) -0.85%
regression_struct_array_conditional 1,573 (-14) -0.88%
references 423 (-4) -0.94%
fold_complex_outputs 890 (-14) -1.55%
side_effects_constrain_array 78 (-6) -7.14%
check_large_field_bits 2,528 (-210) -7.67%
cast_and_shift_global 174 (-30) -14.71%

@jfecher
Copy link
Contributor Author

jfecher commented Nov 18, 2024

Closing this to work on an alternate solution for mem2reg (this was only needed for #6460)

@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