feat: Enable dynamic indices on slices#2446
Merged
Conversation
…nnot merge a load
…up and add other dnyamic slice intrinsics
git status
Contributor
Author
I mainly checked this box because once this PR is merged, support for slices will fully match arrays. I wasn't sure if the documentation had anything noting that slices have certain limitations vs. arrays. |
This was referenced Sep 5, 2023
kevaundray
reviewed
Sep 6, 2023
kevaundray
reviewed
Sep 6, 2023
kevaundray
previously approved these changes
Sep 6, 2023
jfecher
requested changes
Sep 6, 2023
Contributor
jfecher
left a comment
jfecher
left a comment
There was a problem hiding this comment.
Blocking this temporarily to give me time to review
jfecher
reviewed
Sep 6, 2023
Contributor
jfecher
left a comment
jfecher
left a comment
There was a problem hiding this comment.
Mostly questions on the various parts of this PR, why certain parts are needed, etc
crates/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr
Show resolved
Hide resolved
crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
Outdated
Show resolved
Hide resolved
jfecher
approved these changes
Sep 7, 2023
Contributor
jfecher
left a comment
jfecher
left a comment
There was a problem hiding this comment.
Now that loop unrolling is disabled for brillig, can you merge master and verify the checks that were removed are still not needed? Or I suppose that will be done by the CI automatically.
Contributor
|
We can look into removing |
5 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 8, 2024
# Description ## Problem\* Resolves #4202 and #2446 (comment) ## Summary\* This PR brings back the `capacity_tracker` that was used by the fill internal slices pass and for merging nested slices (#3979). We now track the capacity of slices as we insert instructions. The capacity tracker has also been simplified as it no longer needs to handle nested slice. The current strategy checks for previously saved store instructions and backtracks the instructions to see what is the capacity of a slice when it comes time to merge two slice values. This required an additional `outer_block_stores` map as we `store_values` only track stores within the current branch that is being inlined, and a `get_slice_length` method that ultimately did the recursive backtracking to determine the slice length. With the capacity tracker we already have the slice length once it comes time to perform a slice merger allowing us to remove `outer_block_stores` and `get_slice_length`. Overall, the capacity tracker enables us to have more accurate tracking of slice capacities and gives us a more general strategy for tracking a slice capacity per instruction. ## Additional Context ## Documentation\* Check one: - [X] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** 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 <15848336+TomAFrench@users.noreply.github.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 #374
Resolves #2536
Resolves #2538
Summary*
This PR implements handling of dynamic indices for slices. After #2347 the slice structure was changed to be a tuple (length, contents) so that we can dynamic carry a slice's true user-facing length throughout SSA. This new length value can now be used to implement dynamic indexing and merging of slices.
The slice length eventually becomes known by the end of SSA through multiple optimization passes such as mem2reg and constant folding. Additional handling of this length needed to be included in flattening. This is the final location in SSA where the length of a slice must be resolved, and where the majority of SSA changes can be found.
All slice intrinsics now need to also be handled inside of ACIR gen.
SliceInsert and SliceRemove in ACIR gen still need to be implemented #2462 (specifically the case where we want to perform an insertion or removal at a dynamic index), and dynamic indexing of arrays/slices needs to be implemented in general (#2461)
Documentation
This PR requires documentation updates when merged.
Additional Context
PR Checklist*
cargo fmton default settings.