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.

Sign up for GitHub

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

Jump to bottom

chore: SliceRemove minor optimization #3652

Merged
merged 127 commits into from
Dec 1, 2023
Merged

chore: SliceRemove minor optimization #3652

merged 127 commits into from
Dec 1, 2023

Conversation

vezenovm
Copy link
Contributor

Description

Problem*

Resolves this discussion #3617 (comment)

Summary*

Reference the discussion and the PR it is under for more context.

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*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@vezenovm
enabled dynamic indices on nested arrays
c2f2f62
@vezenovm
remove debug
45b3787
@vezenovm
working non-homogenous arr accesses for block params
7ccf8e9
@vezenovm
add array_set, broken, need to maintain structure from SSA for accura…
d5d16d7 
…te index
@vezenovm
merge conflcits w/ master
ec54e01
@vezenovm
working non-homogenous arrays using internal type element size array,…
e2eff9b 
… heavy cleanup still needed
@vezenovm
cleanup lots of debug and move const index access into its own method
219c8f2
@vezenovm
updating array_get and array_set to reduce repeated code
3195325
@vezenovm
restrict dynamic indices for non-homogenous slices
8fb3bb1
@vezenovm
cleanup array_get_value
469c333
@vezenovm
add internal_memory_blocks map
0648142
@vezenovm
do not make separate bool for handle_constant_index
b99f0e2
@vezenovm
fetch dummy value with predicate index
5784170
@vezenovm
flatten the index once for both array_set and array_get, do not modif…
05f35c6 
…y modify the predicate index for the non-dummy val
@vezenovm
remove first_elem param
f2dd702
@vezenovm
initial slice changes with fetch flat size from SSA values, need to g…
490f5af 
…et element type size array correct
@vezenovm
merge master
0037867
@vezenovm
initial working nested dynamic slices
3636bc6
@vezenovm
cleanup and fmt
31e7bc2
@vezenovm
cargo clippy and fmt
4354b76
@vezenovm
merge conflicts w/ master
87e8314
@vezenovm
remove boolean AcirType
658d234
@vezenovm
remove deaad code not in master
408ac2c
@vezenovm
better solution for fetching nested slice with multiple slice mergesr
22ce20c
@vezenovm
bring back assert in nested_array_dynamic
686640e
@vezenovm
add assert to nested_slice_dynamic test
626616b
@vezenovm
cleanup w/ copy_dynamic_array method
f60183c
@vezenovm
starting test for slices in struct fields
be5dc79
@vezenovm
more initial debugging for adding slices to struct fields
a7fde3e
@vezenovm
basic slice struct fields working, but broken for struct fields of di…
d1c9192 
…fferent lengths
vezenovm and others added 17 commits November 30, 2023 17:43
@vezenovm
Merge branch 'master' into mv/dyn-nested-slice-builtin
02fbaa0
@vezenovm
remove unnecessary reads from insert
23253e9
@vezenovm @TomAFrench
Update compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
c3d7c8c 
Co-authored-by: Tom French <[email protected]>
@vezenovm @TomAFrench
Update compiler/noirc_evaluator/src/ssa/opt/fill_internal_slices.rs
9947f6c 
Co-authored-by: Tom French <[email protected]>
@vezenovm @TomAFrench
Update compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
2a69c91 
Co-authored-by: Tom French <[email protected]>
@vezenovm @TomAFrench
Update compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
ac02e52 
Co-authored-by: Tom French <[email protected]>
@vezenovm
add comments to intrinsics arguments
7a4fc8d
@vezenovm
Merge branch 'master' into mv/dyn-nested-slice-builtin
f41e354
@vezenovm
feat: Dynamic index for slice insert and remove (#3624)
3ab0504 
# 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
cleanup SliceRemove
d8991b6
@vezenovm
Merge branch 'mv/dyn-nested-slice-builtin' into mv/dyn-slice-opt
cd62fa9
@vezenovm
merge conflicts w/ master
32fe5bb
@vezenovm
Merge branch 'mv/dyn-nested-slice-builtin' into mv/dyn-slice-opt
d5f7dc3
@vezenovm
minor optimization in SliceRemove and some better comments
4f9c2fd
@vezenovm
master merge conflicts
d05e40a
@vezenovm
merge conflcits
a76ce18
@vezenovm
cargo fmt
950adb0
@vezenovm vezenovm requested a review from TomAFrench November 30, 2023 21:20
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 30, 2023 •
edited
Loading

Changes to circuit sizes

Generated at commit: 41647fcb21b2991247c02a3208ba55543774655f, compared to commit: 5a4a73d24ddd7d2bb46c85714397ee6e031c97a6

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
slice_dynamic_index -16 ✅ -0.60% -24 ✅ -0.21%
slice_struct_field -310 ✅ -0.90% -450 ✅ -0.42%
Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
slice_dynamic_index 2,650 (-16) -0.60% 11,273 (-24) -0.21%
slice_struct_field 34,270 (-310) -0.90% 106,284 (-450) -0.42%

@TomAFrench
Copy link
Member

Can we remove this write operation entirely? We've already initialised the array so if we don't care about the value written here we can just leave it as is, right?

@vezenovm
Copy link
Contributor Author

vezenovm commented Dec 1, 2023

Can we remove this write operation entirely? We've already initialised the array so if we don't care about the value written here we can just leave it as is, right?

I switched to initializing without the original slice value to avoid having to clone the AcirValue. I will see how that reduces constraint counts because I lean towards optimizing the ACIR over the compiler.

@vezenovm
Copy link
Contributor Author

vezenovm commented Dec 1, 2023

@TomAFrench It looks to have reduced the ACIR opcodes for slice_struct_field by ~30 ACIR opcodes and ~100 backend constraints.

TomAFrench
TomAFrench approved these changes Dec 1, 2023
View reviewed changes
Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. LGTM

@TomAFrench TomAFrench enabled auto-merge December 1, 2023 15:13
@TomAFrench TomAFrench added this pull request to the merge queue Dec 1, 2023
Merged via the queue into master with commit 36844fa Dec 1, 2023
33 checks passed
@TomAFrench TomAFrench deleted the mv/dyn-slice-opt branch December 1, 2023 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomAFrench TomAFrench TomAFrench approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vezenovm @TomAFrench