feat(slices): Fill slice internal dummy data initial pass

[vezenovm]

Description

Built off of #3187

Problem*

Works towards resolving #3188 by including work needed by the nested slices PR. This PR introduces the initial pass for filling the slices represented by SSA array values with dummy data. Filling a nested array with dummy data is necessary as when we want to dynamically index nested slices, we cannot discern which size to use for reading from memory. For example, if we have a slice of slices one of the internal slices could potentially be of size 4 while another internal slice has size 6. As we do not know which internal slice size we are using, we must read the max size. However, if we do not fill in the SSA array to account for this max size we will run into either OOB errors or reading/writing incorrect data.

Summary*

A good overview of the pass is included in the comments at the top of the pass file. The unit test at the bottom also provides a good simple overview of what the pass aims to do.

I add a pass that goes through every ArrayGet and ArraySet instruction and follows from an initial SSA value. This helps determine the max nested depth for filling dummy data inside of the nested slice.

This PR is meant to be an initial pass to get nested slice fields working with ArraySet and ArrayGet. The pass determines the max nested depth at each layer of a nested type structure. It also tracks intrinsics and the PR includes a test using slice intrinsics on a nested slice containing primitive types. Performing dynamic slice intrinsic operations using nested slice inputs is left for another PR.

Documentation

• This PR requires documentation updates when merged.

  • I will submit a noir-lang/docs PR.
  • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

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

[jfecher]

It's honestly a tad hard to assess if the specifics of your approach are right but I don't see an issue with the overall approach at least. I think it could benefit from separating out the steps of the algorithm more clearly though. Once the PR is finalized, some documentation on the pass would be helpful as well.

[jfecher]

            let slice_sizes = &slice_sizes[1..];

Although since you're converting to a Vec later, you may as well remove the first element like was done before then you don't have to allocate another Vec

            slice_sizes.remove(0);

[vezenovm]

Either way I have to clone the result slices though no? Ah nvm I see your suggestion, done.

[jfecher]

Forgot to mention - since it looks like you're trying to find each Value::Array by going through get and set instructions, perhaps something like the Instruction::MakeArray PR would be helpful for this use case? #2494. It is something that I never quite got around to finishing but it could help here since right now to find all arrays you'd need to look in all arrayget, arrayset, call, store, and load instructions to find each array while in the PR you'd only need to look for MakeArray instructions. The mem2reg pass has a similar problem.

[vezenovm]

Hmm good call. I will explore this option. Right now dynamic nested slices is already leading to a lot of updates so if does not seriously reduce load I may continue with the pass as is for now and then in a later PR consider adding Instruction::MakeArray to improve this pass as well as mem2reg.

[github-actions]

Changes to circuit sizes

Generated at commit: 833926e8ce7863bf10d8e47ebcc0372cf701243c, compared to commit: f30062ef56c59551b3b883f28a50dff664f93f31

🧾 Summary (10% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
slice_struct_field	+4,620 ❌	+300.98%	+15,969 ❌	+165.05%
signed_arithmetic	-179 ✅	-68.32%	-877 ✅	-23.65%
sha2_byte	-66,903 ✅	-66.09%	-275,006 ✅	-67.19%

---

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
slice_struct_field	6,155 (+4,620)	+300.98%	25,644 (+15,969)	+165.05%
slice_dynamic_index	2,481 (+84)	+3.50%	10,931 (+432)	+4.11%
bit_shifts_runtime	565 (+10)	+1.80%	3,479 (+23)	+0.67%
7_function	405 (+16)	+4.11%	3,387 (+10)	+0.30%
1_mul	24 (+9)	+60.00%	2,772 (+8)	+0.29%
conditional_regression_661	40 (+10)	+33.33%	2,785 (+5)	+0.18%
3_add	18 (+5)	+38.46%	2,764 (+4)	+0.14%
2_div	33 (+5)	+17.86%	2,779 (+3)	+0.11%
brillig_acir_as_brillig	10 (+2)	+25.00%	2,751 (+1)	+0.04%
brillig_calls	11 (+2)	+22.22%	2,751 (+1)	+0.04%
array_dynamic	171 (+3)	+1.79%	3,936 (0)	0.00%
conditional_1	8,370 (+890)	+11.90%	38,799 (0)	0.00%
regression	269 (+4)	+1.51%	4,130 (0)	0.00%
simple_bitwise	38 (+11)	+40.74%	8,220 (0)	0.00%
simple_shift_left_right	14 (-2)	-12.50%	2,981 (-1)	-0.03%
bit_shifts_comptime	11 (-2)	-15.38%	2,816 (-4)	-0.14%
signed_division	160 (-42)	-20.79%	3,625 (-37)	-1.01%
6_array	635 (-9)	-1.40%	4,272 (-95)	-2.18%
brillig_fns_as_values	37 (+10)	+37.04%	2,774 (-693)	-19.99%
signed_arithmetic	83 (-179)	-68.32%	2,831 (-877)	-23.65%
sha2_byte	34,328 (-66,903)	-66.09%	134,299 (-275,006)	-67.19%

[vezenovm]

@jfecher @guipublic @TomAFrench @kevaundray This is ready for review again. I have also made this epic to track any leftover issues with nested slices and in case any new issues come up. I left a PR description, but the comments on the pass will probably be more helpful to you.

[kevaundray]

can we reduce some of this nesting?

[vezenovm]

I can't do something like this:

let array = match &self.inserter.function.dfg[value].clone() {
                        Value::Array { array, .. } => {
                            array
                        }
                        _ => {
                            panic!("Expected an array value");
                        }
                    };

as array is a temporary value that is dropped at the end of the match statement. Unless I want to sacrifice cloning each array it looks like the nesting needs to stay.

[TomAFrench]

We can do something like

for (element_index, element_type) in
    element_types.iter().enumerate()
{
    let index_usize = i * element_types.len() + element_index;
    let valid_index = index_usize < array.len();
    let maybe_value = valid_index.then_some(array[index_usize]);
    slice.push_back(self.attach_slice_dummies(
        element_type,
        maybe_value,
        false,
        max_sizes,
    ));
}

which removes one level from the inner loop.

[TomAFrench]

                    let mut slice = im::Vector::new();

                    let array = match self.inserter.function.dfg[value].clone() {
                        Value::Array { array, .. } => array,
                        _ => panic!("Expected an array value"),
                    };

                    if is_parent_slice {
                        max_size = array.len() / element_types.len();
                    }
                    for i in 0..max_size {
                        for (element_index, element_type) in element_types.iter().enumerate() {
                            let index_usize = i * element_types.len() + element_index;
                            let valid_index = index_usize < array.len();
                            let maybe_value = valid_index.then_some(array[index_usize]);
                            slice.push_back(self.attach_slice_dummies(
                                element_type,
                                maybe_value,
                                false,
                                max_sizes,
                            ));
                        }
                    }

                    self.inserter.function.dfg.make_array(slice, typ.clone())

This compiles for me.

[vezenovm]

Ah I just needed to not borrow self.inserter.function.dfg[value]. And creating the maybe value is much cleaner. I will switch, thanks @TomAFrench

[vezenovm]

Hmm we will still error out on array[index_usize] though with this snippet:

let maybe_value = valid_index.then_some(array[index_usize]);

I will look at how we can remove the if statement before pushing though

[vezenovm]

Ok pushed a new version, but I had to keep an if statement around the maybe_value

[jfecher]

as array is a temporary value that is dropped at the end of the match statement. Unless I want to sacrifice cloning each array it looks like the nesting needs to stay.

You can do that actually, since im::Vector's Clone is O(1)

[vezenovm]

You can do that actually, since im::Vector's Clone is O(1)

I went with Tom's suggestion as simply not borrowing also worked but noted for the future.

[jfecher]

Looks good, just some small things I noticed. I appreciate that the code was cleaned up a lot compared to the draft version

[TomAFrench]

some nits

[vezenovm]

This is ready for another look, then we can move onto #3187

[TomAFrench]

This looks sensible but will let others give final approval.

[jfecher]

👍