feat: Complex slice inputs for dynamic slice builtins (#3617)

# Description

## Problem\*

Resolves #3364 

## Summary\*

This PR heavily changes the way slice intrinsics are implemented in ACIR
gen. Following the `fill_internal_slices` pass nested slices now have
dummy data automatically attached to them to enable known sizes to be
used when fetching from an array with nested slices. Most of the slice
intrinsics were written for non-complex inputs and also did not account
for the addition of already included dummy data. With dummy data already
included we cannot simply push back or push front elements for example,
we need to make sure we do not alter the size of the internal slice we
are using.

The slice intrinsic implementations essentially treat the input already
as a dynamic array and perform the appropriate memory operations upon
them. I considered alternatives and went with what I think is the most
efficient way to implement these intrinsics, however, there is
definitely room for cleanup and in the name of getting a first iteration
working, we can consider how to optimize these intrinsics in another PR.

I have added comments in the ACIR gen itself to help with some of the
logic for the implementation, but do let me know if something is not
clear and I can document it further.

The `fill_internal_slices` pass now also accounts for slice constants
passed as inputs to slice intrinsics as these must also be filled with
dummy data when they are being attached to a nested slice.

## Additional Context

A bug was found in the `fill_internal_slices` pass that was necessary to
resolve this issue. Essentially the children fetched from a slice were
not being accurately tracked when an `array_get` occurs. This fix is
included in this PR as it did not come up until more complex operations
with nested slices were attempted.

SliceInsert and SliceRemove now use a dynamic index. I only tested using
a constant index in this PR and will have a followup PR that shows this
tested out as this PR is already large enough.

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

---------

Co-authored-by: Tom French <[email protected]>
Co-authored-by: Tom French <[email protected]>
Co-authored-by: jfecher <[email protected]>

compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs

@@ -937,7 +937,7 @@ impl AcirContext {
 
     /// Returns an `AcirVar` which will be `1` if lhs >= rhs
     /// and `0` otherwise.
-    fn more_than_eq_var(
+    pub(crate) fn more_than_eq_var(
         &mut self,
         lhs: AcirVar,
         rhs: AcirVar,

compiler/noirc_evaluator/src/ssa/ir/types.rs

@@ -104,6 +104,14 @@ impl Type {
         }
     }
 
+    pub(crate) fn is_nested_slice(&self) -> bool {
+        if let Type::Slice(element_types) = self {
+            element_types.as_ref().iter().any(|typ| typ.contains_slice_element())
+        } else {
+            false
+        }
+    }
+
     /// True if this type is an array (or slice) or internally contains an array (or slice)
     pub(crate) fn contains_an_array(&self) -> bool {
         match self {

compiler/noirc_evaluator/src/ssa/opt/fill_internal_slices.rs

@@ -174,6 +174,9 @@ impl<'f> Context<'f> {
                                 panic!("ICE: should have inner slice set for {slice_value}")
                             });
                             slice_sizes.insert(results[0], inner_slice.clone());
+                            if slice_value != results[0] {
+                                self.mapped_slice_values.insert(slice_value, results[0]);
+                            }
                         }
                     }
                 }
@@ -198,17 +201,11 @@ impl<'f> Context<'f> {
 
                     let inner_sizes = slice_sizes.get_mut(array).expect("ICE expected slice sizes");
                     inner_sizes.1.push(*value);
-
-                    let value_parent = self.resolve_slice_parent(*value);
-                    if slice_values.contains(&value_parent) {
-                        // Map the value parent to the current array in case nested slices
-                        // from the current array are set to larger values later in the program
-                        self.mapped_slice_values.insert(value_parent, *array);
-                    }
                 }
 
                 if let Some(inner_sizes) = slice_sizes.get_mut(array) {
                     let inner_sizes = inner_sizes.clone();
+
                     slice_sizes.insert(results[0], inner_sizes);
 
                     self.mapped_slice_values.insert(*array, results[0]);
@@ -224,14 +221,27 @@ impl<'f> Context<'f> {
                         | Intrinsic::SlicePopBack
                         | Intrinsic::SliceInsert
                         | Intrinsic::SliceRemove => (1, 1),
-                        Intrinsic::SlicePopFront => (1, 2),
+                        // `pop_front` returns the popped element, and then the respective slice.
+                        // This means in the case of a slice with structs, the result index of the popped slice
+                        // will change depending on the number of elements in the struct.
+                        // For example, a slice with four elements will look as such in SSA:
+                        // v3, v4, v5, v6, v7, v8 = call slice_pop_front(v1, v2)
+                        // where v7 is the slice length and v8 is the popped slice itself.
+                        Intrinsic::SlicePopFront => (1, results.len() - 1),
                         _ => return,
                     };
+                    let slice_contents = arguments[argument_index];
                     match intrinsic {
                         Intrinsic::SlicePushBack
                         | Intrinsic::SlicePushFront
                         | Intrinsic::SliceInsert => {
-                            let slice_contents = arguments[argument_index];
+                            for arg in &arguments[(argument_index + 1)..] {
+                                let element_typ = self.inserter.function.dfg.type_of_value(*arg);
+                                if element_typ.contains_slice_element() {
+                                    slice_values.push(*arg);
+                                    self.compute_slice_sizes(*arg, slice_sizes);
+                                }
+                            }
                             if let Some(inner_sizes) = slice_sizes.get_mut(&slice_contents) {
                                 inner_sizes.0 += 1;
 
@@ -240,12 +250,12 @@ impl<'f> Context<'f> {
 
                                 self.mapped_slice_values
                                     .insert(slice_contents, results[result_index]);
+                                self.slice_parents.insert(results[result_index], slice_contents);
                             }
                         }
                         Intrinsic::SlicePopBack
-                        | Intrinsic::SlicePopFront
-                        | Intrinsic::SliceRemove => {
-                            let slice_contents = arguments[argument_index];
+                        | Intrinsic::SliceRemove
+                        | Intrinsic::SlicePopFront => {
                             // We do not decrement the size on intrinsics that could remove values from a slice.
                             // This is because we could potentially go back to the smaller slice and not fill in dummies.
                             // This pass should be tracking the potential max that a slice ***could be***
@@ -255,6 +265,7 @@ impl<'f> Context<'f> {
 
                                 self.mapped_slice_values
                                     .insert(slice_contents, results[result_index]);
+                                self.slice_parents.insert(results[result_index], slice_contents);
                             }
                         }
                         _ => {}
@@ -277,7 +288,6 @@ impl<'f> Context<'f> {
                 if slice_values.contains(array) {
                     let (new_array_op_instr, call_stack) =
                         self.get_updated_array_op_instr(*array, slice_sizes, instruction);
-
                     self.inserter.push_instruction_value(
                         new_array_op_instr,
                         instruction,
@@ -288,6 +298,55 @@ impl<'f> Context<'f> {
                     self.inserter.push_instruction(instruction, block);
                 }
             }
+            Instruction::Call { func: _, arguments } => {
+                let mut args_to_replace = Vec::new();
+                for (i, arg) in arguments.iter().enumerate() {
+                    let element_typ = self.inserter.function.dfg.type_of_value(*arg);
+                    if slice_values.contains(arg) && element_typ.contains_slice_element() {
+                        args_to_replace.push((i, *arg));
+                    }
+                }
+                if args_to_replace.is_empty() {
+                    self.inserter.push_instruction(instruction, block);
+                } else {
+                    // Using the original slice is ok to do as during collection of slice information
+                    // we guarantee that only the arguments to slice intrinsic calls can be replaced.
+                    let slice_contents = arguments[1];
+
+                    let element_typ = self.inserter.function.dfg.type_of_value(arguments[1]);
+                    let elem_depth = Self::compute_nested_slice_depth(&element_typ);
+
+                    let mut max_sizes = Vec::new();
+                    max_sizes.resize(elem_depth, 0);
+                    // We want the max for the parent of the argument
+                    let parent = self.resolve_slice_parent(slice_contents);
+                    self.compute_slice_max_sizes(parent, slice_sizes, &mut max_sizes, 0);
+
+                    for (index, arg) in args_to_replace {
+                        let element_typ = self.inserter.function.dfg.type_of_value(arg);
+                        max_sizes.remove(0);
+                        let new_array =
+                            self.attach_slice_dummies(&element_typ, Some(arg), false, &max_sizes);
+
+                        let instruction_id = instruction;
+                        let (instruction, call_stack) =
+                            self.inserter.map_instruction(instruction_id);
+                        let new_call_instr = match instruction {
+                            Instruction::Call { func, mut arguments } => {
+                                arguments[index] = new_array;
+                                Instruction::Call { func, arguments }
+                            }
+                            _ => panic!("Expected call instruction"),
+                        };
+                        self.inserter.push_instruction_value(
+                            new_call_instr,
+                            instruction_id,
+                            block,
+                            call_stack,
+                        );
+                    }
+                }
+            }
             _ => {
                 self.inserter.push_instruction(instruction, block);
             }
@@ -314,6 +373,7 @@ impl<'f> Context<'f> {
         let typ = self.inserter.function.dfg.type_of_value(array_id);
         let depth = Self::compute_nested_slice_depth(&typ);
         max_sizes.resize(depth, 0);
+
         max_sizes[0] = *current_size;
         self.compute_slice_max_sizes(array_id, slice_sizes, &mut max_sizes, 1);
 
@@ -370,9 +430,12 @@ impl<'f> Context<'f> {
                 if let Some(value) = value {
                     let mut slice = im::Vector::new();
 
-                    let array = match self.inserter.function.dfg[value].clone() {
+                    let value = self.inserter.function.dfg[value].clone();
+                    let array = match value {
                         Value::Array { array, .. } => array,
-                        _ => panic!("Expected an array value"),
+                        _ => {
+                            panic!("Expected an array value");
+                        }
                     };
 
                     if is_parent_slice {
@@ -487,7 +550,6 @@ impl<'f> Context<'f> {
             self.compute_slice_max_sizes(*inner_slice, slice_sizes, max_sizes, depth + 1);
         }
 
-        max_sizes[depth] = max;
         if max > max_sizes[depth] {
             max_sizes[depth] = max;
         }

noir_stdlib/src/slice.nr

@@ -21,28 +21,16 @@ impl<T> [T] {
     #[builtin(slice_pop_front)]
     pub fn pop_front(_self: Self) -> (T, Self) { }
 
-    pub fn insert(self, _index: Field, _elem: T) -> Self {
-        // TODO(#2462): Slice insert with a dynamic index
-        crate::assert_constant(_index);
-        self.__slice_insert(_index, _elem)
-    }
-
     /// Insert an element at a specified index, shifting all elements 
     /// after it to the right
     #[builtin(slice_insert)]
-    fn __slice_insert(_self: Self, _index: Field, _elem: T) -> Self { }
-
-    pub fn remove(self, _index: Field) -> (Self, T) {
-        // TODO(#2462): Slice remove with a dynamic index
-        crate::assert_constant(_index);
-        self.__slice_remove(_index)
-    }
+    pub fn insert(_self: Self, _index: Field, _elem: T) -> Self { }
 
     /// Remove an element at a specified index, shifting all elements
     /// after it to the left, returning the altered slice and 
     /// the removed element
     #[builtin(slice_remove)]
-    fn __slice_remove(_self: Self, _index: Field) -> (Self, T) { }
+    pub fn remove(_self: Self, _index: Field) -> (Self, T) { }
 
     // Append each element of the `other` slice to the end of `self`.
     // This returns a new slice and leaves both input slices unchanged.

test_programs/execution_success/slice_dynamic_index/src/main.nr

@@ -123,13 +123,13 @@ fn dynamic_slice_merge_if(mut slice: [Field], x: Field) {
         let (first_elem, rest_of_slice) = popped_slice.pop_front();
         assert(first_elem == 12);
         assert(rest_of_slice.len() == 6);
-        // TODO(#2462): SliceInsert and SliceRemove with a dynamic index are not yet implemented in ACIR gen
-        slice = rest_of_slice.insert(2, 20);
+
+        slice = rest_of_slice.insert(x - 2, 20);
         assert(slice[2] == 20);
         assert(slice[6] == 30);
         assert(slice.len() == 7);
-        // TODO(#2462): SliceInsert and SliceRemove with a dynamic index are not yet implemented in ACIR gen
-        let (removed_slice, removed_elem) = slice.remove(3);
+
+        let (removed_slice, removed_elem) = slice.remove(x - 1);
         // The deconstructed tuple assigns to the slice but is not seen outside of the if statement
         // without a direct assignment
         slice = removed_slice;