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: avoid branch condition negations in ValueMerger #6073

Closed
wants to merge 11 commits into from

Conversation

TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented Sep 17, 2024

Description

Problem*

Resolves

Summary*

This PR modifies how we merge values from pred * a + (1-pred) * b to b + pred * (a-b) which avoids another multiplication. This is being done manually in here so it's worth considering as the default merger strategy.

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 Sep 17, 2024

Changes to circuit sizes

Generated at commit: 9032919fe8d94d52b2471a4923f390fa70aa4709, compared to commit: fb1a8ca67c58d87991358078e6c532b49824fdb8

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
conditional_1 -1,202 ✅ -28.94% -1,196 ✅ -10.03%
regression_5252 -3,600 ✅ -10.41% -5,336 ✅ -11.13%
hashmap -17,088 ✅ -26.64% -26,078 ✅ -18.91%
debug_logs -6 ✅ -13.04% -54 ✅ -73.97%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
array_sort 55 (+2) +3.77% 132 (+4) +3.13%
sha256_regression 20,388 (-14,702) -41.90% 200,488 (+3,028) +1.53%
binary_operator_overloading 284 (+2) +0.71% 4,465 (+2) +0.04%
bench_eddsa_poseidon 16,441 (+2) +0.01% 19,588 (+2) +0.01%
eddsa 64,631 (+2) +0.00% 65,817 (+2) +0.00%
conditional_regression_short_circuit 366 (-4) -1.08% 11,150 (-5) -0.04%
array_dynamic 98 (-4) -3.92% 3,710 (-4) -0.11%
conditional_2 19 (-2) -9.52% 2,778 (-3) -0.11%
schnorr 1,510 (-9) -0.59% 23,903 (-29) -0.12%
array_if_cond_simple 102 (-6) -5.56% 3,125 (-7) -0.22%
regression_3607 34 (-5) -12.82% 2,795 (-7) -0.25%
regression_mem_op_predicate 47 (-9) -16.07% 3,576 (-10) -0.28%
conditional_regression_661 20 (-9) -31.03% 2,787 (-9) -0.32%
bigint 1,118 (-48) -4.12% 8,076 (-41) -0.51%
u128 654 (-26) -3.82% 4,664 (-25) -0.53%
nested_array_in_slice 853 (-30) -3.40% 5,473 (-30) -0.55%
regression 108 (-33) -23.40% 3,647 (-36) -0.98%
hash_to_field 540 (-42) -7.22% 3,582 (-36) -1.00%
ram_blowup_regression 162,771 (-10,752) -6.20% 667,648 (-9,216) -1.36%
slice_dynamic_index 916 (-66) -6.72% 6,351 (-102) -1.58%
sha256 1,542 (-336) -17.89% 20,882 (-425) -1.99%
slices 761 (-74) -8.86% 3,859 (-88) -2.23%
sha256_var_witness_const_regression 1,339 (-336) -20.06% 16,791 (-425) -2.47%
sha256_var_size_regression 9,041 (-7,120) -44.06% 66,856 (-4,105) -5.78%
conditional_1 2,952 (-1,202) -28.94% 10,728 (-1,196) -10.03%
regression_5252 30,996 (-3,600) -10.41% 42,627 (-5,336) -11.13%
hashmap 47,046 (-17,088) -26.64% 111,860 (-26,078) -18.91%
debug_logs 40 (-6) -13.04% 19 (-54) -73.97%

@TomAFrench
Copy link
Member Author

TomAFrench commented Sep 19, 2024

@guipublic This could be something interesting to take up. I'm not able to devote proper time to it atm but it looks very promising in terms of gate count reductions. I'm not sure why the sha256_regression test has regressed though.

@TomAFrench TomAFrench closed this Nov 25, 2024
@TomAFrench TomAFrench deleted the tf/experiment-with-alternative-merger branch November 25, 2024 23:00
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