feat: Merging nested slices in if statements based upon witness conditions #3979

vezenovm · 2024-01-08T15:31:55Z

Description

Problem*

Resolves #3188

Summary*

This PR enables nested slices to be used with if statements based upon witness conditions. It does this by moving out the slice capacity tracker from the fill_internal_slices pass into its own instance that is updated by calling the collect_slice_information function. We are able to tracked specific instructions from flattening that we then need to appropriately fetch a slice's len during merging of slice values. This enabled us to get rid of using store_values, outer_store_values, and the get_slice_length functions in the ValueMerger during flattening. Instead we track the slice length as we go through flattening and then fetch it directly from the slice capacity map.

I had some trouble getting the merging of nested slices to work with an optimized strategy that tracked the specific max nested slice size that is used. I think this could be done in a followup if we decide to enable nested slices for the public. I have some ideas of how to optimize and will put this into an issue.

Although inefficient, this PR does show a PoC that nested slices can be fully supported in ACIR.

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.

…te index

… heavy cleanup still needed

…y modify the predicate index for the non-dummy val

…et element type size array correct

…fferent lengths

…anup still

…nd flattening pass

github-actions · 2024-01-08T15:35:53Z

Changes to circuit sizes

Generated at commit: 717a4f8cc8838d5c4aae28ac40682854d09eb781, compared to commit: ee823954fd63cac03bd5eaf649be21eae149ad37

🧾 Summary (10% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
slice_struct_field	+87,546 ❌	+258.83%	+233,646 ❌	+222.50%
bit_shifts_runtime	+1,136 ❌	+143.25%	+1,136 ❌	+29.49%
nested_slice_dynamic	-639 ✅	-35.25%	-5,373 ✅	-42.54%

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
slice_struct_field	121,370 (+87,546)	+258.83%	338,654 (+233,646)	+222.50%
bit_shifts_runtime	1,929 (+1,136)	+143.25%	4,988 (+1,136)	+29.49%
slice_dynamic_index	2,552 (+136)	+5.63%	11,219 (+343)	+3.15%
u128	942 (+42)	+4.67%	4,772 (+52)	+1.10%
6_array	579 (+22)	+3.95%	4,288 (+31)	+0.73%
nested_array_dynamic	4,684 (+32)	+0.69%	14,059 (+63)	+0.45%
array_dynamic	120 (+4)	+3.45%	3,738 (+7)	+0.19%
conditional_regression_661	33 (+1)	+3.13%	2,783 (+1)	+0.04%
conditional_1	6,787 (-129)	-1.87%	38,799 (0)	0.00%
merkle_insert	1,979 (+6)	+0.30%	29,032 (0)	0.00%
operator_overloading	532 (+187)	+54.20%	8,214 (0)	0.00%
strings	211 (+93)	+78.81%	14,373 (0)	0.00%
nested_slice_dynamic	1,174 (-639)	-35.25%	7,258 (-5,373)	-42.54%

jfecher · 2024-01-10T17:50:49Z

After an internal meeting we agreed to shelve nested slices for the moment and eventually write a check to disable them either in the frontend or some time after monomorphizaiton. Since we'll be removing nested slices code eventually (although potentially only temporarily), I'm closing this PR.

subrepo: subdir: "noir" merged: "945587396" upstream: origin: "https://github.com/noir-lang/noir" branch: "aztec-packages" commit: "d7c4c669c" git-subrepo: version: "0.4.6" origin: "???" commit: "???"

# Description ## Problem\* Resolves #4017 ## Summary\* After this PR #3979 and an internal discussion we have decided to temporarily block nested slices. This PR adds a check in the frontend and in the SSA against nested slices. The check in the frontend makes sure any struct fields with a nested slice (but without generics) are not blocked as well as any type declarations with nested slices. In order to account for generics in structs we also have a checked array codegen that makes sure we do not have a nested slice. ## Additional Context The actual nested slice code in the ACIR gen is somewhat intertwined so I felt it would be best for a separate PR which will be a followup to this one. ## Documentation\* Check one: - [] No documentation needed. - [X] 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.

# 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>

vezenovm added 30 commits September 14, 2023 18:30

enabled dynamic indices on nested arrays

c2f2f62

remove debug

45b3787

working non-homogenous arr accesses for block params

7ccf8e9

add array_set, broken, need to maintain structure from SSA for accura…

d5d16d7

…te index

merge conflcits w/ master

ec54e01

working non-homogenous arrays using internal type element size array,…

e2eff9b

… heavy cleanup still needed

cleanup lots of debug and move const index access into its own method

219c8f2

updating array_get and array_set to reduce repeated code

3195325

restrict dynamic indices for non-homogenous slices

8fb3bb1

cleanup array_get_value

469c333

add internal_memory_blocks map

0648142

do not make separate bool for handle_constant_index

b99f0e2

fetch dummy value with predicate index

5784170

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

remove first_elem param

f2dd702

initial slice changes with fetch flat size from SSA values, need to g…

490f5af

…et element type size array correct

merge master

0037867

initial working nested dynamic slices

3636bc6

cleanup and fmt

31e7bc2

cargo clippy and fmt

4354b76

merge conflicts w/ master

87e8314

remove boolean AcirType

658d234

remove deaad code not in master

408ac2c

better solution for fetching nested slice with multiple slice mergesr

22ce20c

bring back assert in nested_array_dynamic

686640e

add assert to nested_slice_dynamic test

626616b

cleanup w/ copy_dynamic_array method

f60183c

starting test for slices in struct fields

be5dc79

more initial debugging for adding slices to struct fields

a7fde3e

basic slice struct fields working, but broken for struct fields of di…

d1c9192

…fferent lengths

vezenovm added 16 commits December 6, 2023 18:02

working tracking of slices inside of flattening, code needs heavy cle…

47390f0

…anup still

merge w/ master and some dbg cleanup

560c69a

working slice_struct_field with SliceCapacityTracker

9b10414

remove old collect_slice_information from fill internal slices pass a…

de166c7

…nd flattening pass

cargo fmt

1c55905

start separate test purely for merging nested slices

88add40

merge w/ master

02a6aa6

working merging of nested slices, still some cleanup needed

3c8d203

cleanup tests and comments

7eefa04

remove dbg

49fa625

merge w/ master

a55b86f

cargo fmt

898f7c7

cargo clippy

5b3b6b0

remove comments

f630341

remove dbgs and add comment

75e77f4

more cleanup and comments

5d91ce9

vezenovm marked this pull request as ready for review January 8, 2024 15:34

Merge branch 'master' into mv/merge-nested-slices-2

1e7366a

merge conflicts w/ master

7149d28

jfecher closed this Jan 10, 2024

This was referenced Jan 11, 2024

Ban nested slices #4017

Closed

chore!: Ban nested slices #4018

Merged

Epic: Deprecate nested slices #3363

Closed

This was referenced Feb 1, 2024

fix(ssa): Tracking slices stores correctly for parent blocks and handling in ACIR #4221

Closed

fix: Accurate tracking of slice capacities across blocks #4240

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Merging nested slices in if statements based upon witness conditions #3979

feat: Merging nested slices in if statements based upon witness conditions #3979

vezenovm commented Jan 8, 2024

github-actions bot commented Jan 8, 2024 •

edited

Loading

jfecher commented Jan 10, 2024

feat: Merging nested slices in if statements based upon witness conditions #3979

feat: Merging nested slices in if statements based upon witness conditions #3979

Conversation

vezenovm commented Jan 8, 2024

Description

Problem*

Summary*

Additional Context

Documentation*

PR Checklist*

github-actions bot commented Jan 8, 2024 • edited Loading

Changes to circuit sizes

🧾 Summary (10% most significant diffs)

jfecher commented Jan 10, 2024

github-actions bot commented Jan 8, 2024 •

edited

Loading