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

SSA: Update merging slice values to handle nested slices #3188

Closed
Tracked by #3363
vezenovm opened this issue Oct 17, 2023 · 1 comment
Closed
Tracked by #3363

SSA: Update merging slice values to handle nested slices #3188

vezenovm opened this issue Oct 17, 2023 · 1 comment
Assignees
Labels
bug Something isn't working enhancement New feature or request ssa
Milestone

Comments

@vezenovm
Copy link
Contributor

Problem

We added support for merging slices in this PR (#2347). Since the original work there has been a few follow-up PRs as slices have shown to require more compiler logic than originally anticipated (#2599).

Merging of slice values during flattening needs to be updated once more to support nested slices. The current implementation fetches the length of a slice using a method in the value merger called get_slice_length. This method currently bases the array length off the entire length of the Value::Array in SSA. This is not indicative of the user facing length as nested arrays/slices are flattening in SSA and the length should be array.len() / element_size rather than simply array.len().

This has been fine for the time being as we only supported dynamically indexing slices with internal types whose size is known at compile time (such as numbers and arrays). As we move to being able to dynamically index nested slices flattening needs to be updated.

Happy Case

Flattening nested slices should maintain the internal structure of a nested slice similar to how it is done in this PR (#3187).

The ArrayGet instruction needs to be appropriately handled which means we need to be able to determine the internal nested slice size of the array a given slice is being fetched from.

Alternatives Considered

We could possibly handle this entirely in ACIR gen, but this would add more opcodes and reduce the ability to handle constant indices which will be generated during flattening. It would be best to match up the flattening structure of slices with that of the type structure that arrays follow.

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@vezenovm
Copy link
Contributor Author

vezenovm commented Feb 5, 2024

Closing this issue as per #4017

@vezenovm vezenovm closed this as completed Feb 5, 2024
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request ssa
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants