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

feat(ssa): Hoist add and mul binary ops using known induction variables #6910

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Dec 21, 2024

Description

Problem*

Followup to #6848 whose largest regression was in brillig opcodes executed #6848 (comment).

Summary*

In #6848 we marked certain binary ops as not being able to be hoisted due to the overflow checks inside of ACIR gen and Brillig gen. However, if a binary operation has one variable that is from an outer loop's induction variable we can determine whether or not there would be an overflow. This re-uses logic introduced in #6639.

This PR currently just tries to hoist add and mul instructions as they are simpler. For example, Sub overflows depends on if the induction variable is on the lhs or the rhs (e.g. if it is the rhs we should check the induction variable's upper bound but if it is the lhs we should check the lower bound). These can be done in follow-ups if we are comfortable with this approach.

Additional Context

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 Dec 21, 2024

Changes to Brillig bytecode sizes

Generated at commit: 6929ac3b93eab10080caa0ddbff637b8241edf12, compared to commit: 03b58fa2dfcc8acc8cf5198b1b23b55676fbdb02

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
poseidonsponge_x5_254 -10 ✅ -0.23%
sha2_byte -8 ✅ -0.29%

Full diff report 👇
Program Brillig opcodes (+/-) %
hashmap 21,798 (-12) -0.06%
uhashmap 14,001 (-15) -0.11%
poseidon_bn254_hash 5,425 (-10) -0.18%
poseidon_bn254_hash_width_3 5,425 (-10) -0.18%
regression_5252 4,594 (-10) -0.22%
poseidonsponge_x5_254 4,254 (-10) -0.23%
sha2_byte 2,767 (-8) -0.29%

Copy link
Contributor

github-actions bot commented Dec 21, 2024

Changes to number of Brillig opcodes executed

Generated at commit: 6929ac3b93eab10080caa0ddbff637b8241edf12, compared to commit: 03b58fa2dfcc8acc8cf5198b1b23b55676fbdb02

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
sha256_regression -9,606 ✅ -7.49%
sha256_var_size_regression -1,326 ✅ -7.49%
brillig_cow_regression -64,620 ✅ -11.07%
ram_blowup_regression -98,719 ✅ -11.25%

Full diff report 👇
Program Brillig opcodes (+/-) %
uhashmap 145,546 (-62) -0.04%
hashmap 54,140 (-62) -0.11%
poseidon_bn254_hash 162,594 (-1,599) -0.97%
poseidon_bn254_hash_width_3 162,594 (-1,599) -0.97%
poseidonsponge_x5_254 183,753 (-2,274) -1.22%
regression_5252 915,026 (-11,370) -1.23%
regression_6674_3 1,615 (-30) -1.82%
keccak256 34,777 (-700) -1.97%
conditional_1 5,717 (-201) -3.40%
loop_invariant_regression 1,164 (-42) -3.48%
array_dynamic_nested_blackbox_input 4,550 (-180) -3.81%
sha2_byte 47,309 (-2,223) -4.49%
sha256 14,996 (-720) -4.58%
sha256_var_witness_const_regression 7,200 (-360) -4.76%
conditional_regression_short_circuit 7,528 (-402) -5.07%
6 7,450 (-402) -5.12%
ecdsa_secp256k1 6,789 (-369) -5.16%
array_dynamic_blackbox_input 18,209 (-990) -5.16%
sha256_brillig_performance_regression 23,194 (-1,524) -6.17%
sha256_var_padding_regression 222,216 (-15,066) -6.35%
regression_4449 200,857 (-14,070) -6.55%
sha256_regression 118,704 (-9,606) -7.49%
sha256_var_size_regression 16,377 (-1,326) -7.49%
brillig_cow_regression 519,086 (-64,620) -11.07%
ram_blowup_regression 778,650 (-98,719) -11.25%

Copy link
Contributor

github-actions bot commented Dec 21, 2024

Compilation Sample

Program Compilation Time %
sha256_regression 1.373s 3%
regression_4709 0.780s 0%
ram_blowup_regression 15.472s 1%
rollup-base-public 116.085s 1%
rollup-base-private 95.825s -2%
private-kernel-tail 1.078s 5%
private-kernel-reset 7.137s -2%
private-kernel-inner 2.192s 4%
parity-root 0.713s -8%
noir-contracts 87.283s 1%

Copy link
Contributor

github-actions bot commented Dec 21, 2024

Execution Sample

Program Execution Time %
sha256_regression 0.618s 0%
regression_4709 0.391s 3%
ram_blowup_regression 4.451s 0%
rollup-base-public 21.356s 0%
rollup-base-private 19.487s 0%
private-kernel-tail 0.691s -1%
private-kernel-reset 1.468s -2%
private-kernel-inner 0.973s 1%
parity-root 0.527s -5%

Copy link
Contributor

github-actions bot commented Dec 21, 2024

Peak Memory Sample

Program Peak Memory
keccak256 78.48M
workspace 123.81M
regression_4709 422.91M
ram_blowup_regression 1.58G
private-kernel-tail 201.60M
private-kernel-reset 716.87M
private-kernel-inner 291.67M
parity-root 171.93M

@vezenovm vezenovm marked this pull request as ready for review December 21, 2024 01:34
@vezenovm vezenovm requested a review from a team December 21, 2024 01:34
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