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

Enable dynamic index argument for slice insert and slice remove #2462

Closed
Tracked by #3363
vezenovm opened this issue Aug 28, 2023 · 2 comments
Closed
Tracked by #3363

Enable dynamic index argument for slice insert and slice remove #2462

vezenovm opened this issue Aug 28, 2023 · 2 comments
Assignees
Labels
acir-gen enhancement New feature or request

Comments

@vezenovm
Copy link
Contributor

Problem

Support for dynamic indices is included in this PR (#2446). However, it still has a missing feature with handling dynamic indices for the SliceInsert and SliceRemove intrinsics. Implementing SliceInsert and SliceRemove with non-const inputs is not as straightforward as the other slice intrinsics in ACIR gen.

A more involved codegen will be required. In both SSA gen and ACIR gen, slice intrinsicssimplifications could be performed in most cases with the im::Vector builtins. Without a constant index, the im::Vector builtin cannot be used, and an equivalent codegen of the insert and remove methods will most likely be required.

Happy Case

We can use insert and remove on slices with witness values.

For example, this should be enabled:

fn main(x: Field) {
    let mut slice = [];
    for i in 0..5 {
        slice = slice.push_back(i);
    }
    
    slice = slice.insert(x - 2, 20);
    assert( ... );
}

Alternatives Considered

If this is not implemented the feature will just be missing.

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@jfecher
Copy link
Contributor

jfecher commented Aug 28, 2023

I think an approach I'd like for this bug and one direction I strongly want to move towards in the future is to have a compiler error for unimplemented features like this so that users do not randomly encounter these crashes.

For example, for here I think we can add a std::assert_constant(index) within our slice insert and remove methods in the standard library. This should give users a nicer error while we're trying to implement it in the meantime.

vezenovm added a commit that referenced this issue Nov 30, 2023
# Description

## Problem\*

Resolves #2462 

## Summary\*

This PR simply builds upon #3617
by including some tests that use a dynamic index on SliceInsert and
SliceRemove.

## Additional Context

## Documentation\*

Check one:
- [ ] 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.
@vezenovm
Copy link
Contributor Author

This was closed as per #3624

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acir-gen enhancement New feature or request
Projects
Archived in project
Development

No branches or pull requests

2 participants