Flattening pass merges entire arrays even if only one index was changed #4692
Assignees
Labels
bug
Something isn't working
Comments
|
This is a possible cause for the code blowup in: |
|
Initial difficulties in working on fixing this: it is hard for the flattening pass to track these changes since the arrays are being loaded from and stored to with references. This is normally the domain of mem2reg which itself is already a fairly complex pass. For flattening to reason about which allocations are loaded to, modified at certain indices, then stored back to would likely require alias tracking for example. |
This was referenced Apr 2, 2024
github-merge-queue bot
pushed a commit
that referenced
this issue
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 <tom@tomfren.ch> Co-authored-by: vezenovm <mvezenov@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Aim
Mutating one index in an array inside a conditional:
Expected Behavior
Only the one index that was changed needs to be merged from the implicit "else" case where it was unchanged.
Bug
The flattening pass instead merges every value inside
arraywhich can cause exponential blowup if the array is mutated inside a loop like this.To Reproduce
Project Impact
None
Impact Context
No response
Workaround
None
Workaround Description
No response
Additional Context
No response
Installation Method
None
Nargo Version
No response
NoirJS Version
No response
Would you like to submit a PR for this Issue?
None
Support Needs
No response
The text was updated successfully, but these errors were encountered: