Skip to content

chore: Add optimization to #4716#4718

Merged
jfecher merged 5 commits intojf/opt-array-merging2from
jf/opt-array-merging3
Apr 5, 2024
Merged

chore: Add optimization to #4716#4718
jfecher merged 5 commits intojf/opt-array-merging2from
jf/opt-array-merging3

Conversation

@jfecher
Copy link
Contributor

@jfecher jfecher commented Apr 4, 2024

Description

Problem*

Summary*

Adds the actual array set optimization to #4716. This is separate since the other PR still has errors and I want to fix those before adding more with the actual optimization. At the same time, I've put up this PR to see the circuit size changes in CI.

Additional Context

This PR is currently failing a few additional tests. I think these are mostly array out of bounds accesses from storing which indices were changed. In some if statements, these indices may be out of bounds but not error since that if case was not run. Trying to get and store to this same index after the if then gives an OOB error. This can be fixed by only applying the optimization when constant indices are used and filtering out the OOB ones but then dynamic indices won't see this important optimization at all.

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.

@jfecher jfecher changed the title chore: Add optimization to https://github.com/noir-lang/noir/pull/4716 chore: Add optimization to #4716 Apr 4, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2024

Changes to circuit sizes

Generated at commit: fa939dbe5630f8ef73c4b0e3d5aaa8b4e73d39d1, compared to commit: 222bc0e34953a8e95032e47013134f5080dfe017

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
hashmap -41,737 ✅ -19.25% -67,473 ✅ -18.49%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
hashmap 175,073 (-41,737) -19.25% 297,490 (-67,473) -18.49%

@jfecher
Copy link
Contributor Author

jfecher commented Apr 5, 2024

Update: adding the enable_side_effect_if instructions has fixed the additional errors from this optimization. Now all that remains are the original errors from the new pass. The tests: nested_array_dynamic, and nested_array_in_slice

@jfecher
Copy link
Contributor Author

jfecher commented Apr 5, 2024

Ah, ok. I was trying to figure out why the opt percentages on this were smaller than #4716, but they're just relative to that PR. So the 20% hashmap improvement is a further improvement on top of the 25% on the other PR. Merging this now since the additional bugs are fixed.

@jfecher jfecher marked this pull request as ready for review April 5, 2024 14:09
@jfecher jfecher merged commit 0a39c7b into jf/opt-array-merging2 Apr 5, 2024
@jfecher jfecher deleted the jf/opt-array-merging3 branch April 5, 2024 14:10
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