chore: Optimize mutating arrays in if statements #4695
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jfecher
changed the title
Changes to circuit sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
5 tasks
|
Closing this in favor of #4716 |
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 3, 2024
# Description ## Problem\* Alternate version of #4695 Resolves #4692 ## Summary\* - Flattening produces a new `IfElse` instruction to merge values instead of immediately merging them - `IfElse` instructions are removed by actually merging the values once flattening and mem2reg is done. This is done in a new remove_if_else pass. - Since slice capacity tracking was done in flattening before and required to merge values, I've had to move this to the remove_if_else pass instead. It is somewhat simpler since we don't have to track loads/stores. ## Additional Context Currently only failing the nested_array_in_slice and nested_array_dynamic tests. The array being asserted somehow has the value `[1, 2, 3]` despite none of these values being set in the program. There are currently no actual optimizations here, this is just structural changes so it should otherwise be equivalent. ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Tom French <[email protected]> Co-authored-by: vezenovm <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Problem*
Resolves #4692
Summary*
Tries to make merging of array values during flattening smarter by keeping track of only which indices may change.
Note that pinning which indices actually change is much more difficult since these arrays are in references which are loaded from and stored to. This'd require mem2reg and alias analysis to do. So I instead opted for an approximation: we track which indices are changed for each array type within an if branch instead.
If the indices that may be stored to are known, and this list is less than the array's length, then we store to only these indices instead.
Additional Context
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.