feat(ssa): Merge slices in if statements with witness conditions

crates/nargo_cli/tests/compile_failure/slice_access_failure/src/main.nr

@@ -6,6 +6,8 @@ fn main(x : Field, y : pub Field) {
     } else {
         slice = slice.push_back(x);
     }
-    // This constraint should fail as the slice length is 2 and the index is 2
-    assert(slice[2] == 0);
+    // This constraint should fail as the slice length is 3 and the index is 3
+    // The right hand side AND case ensures that the circuit inputs have not changed
+    // and we always hit the else case in the if statement above.
+    assert((slice[3] == 0) & (slice[2] != y));
 }

crates/nargo_cli/tests/execution_success/brillig_to_le_bytes/src/main.nr

@@ -4,7 +4,6 @@ unconstrained fn main(x : Field) -> pub [u8; 31] {
     // The result of this byte array will be little-endian
     let byte_array = x.to_le_bytes(31);
     assert(byte_array.len() == 31);
-
     let mut bytes = [0; 31];
     for i in 0..31 {
         bytes[i] = byte_array[i];

crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs

@@ -318,8 +318,14 @@
                         dfg,
                     );
                     let param_id = arguments[0];
+                    // Slices are represented as a tuple in the form: (length, slice contents).
+                    // Thus, we can expect the first argument to a field in the case of a slice
+                    // or an array in the case of an array.
                     if let Type::Numeric(_) = dfg.type_of_value(param_id) {
-                        self.convert_ssa_value(arguments[0], dfg);
+                        let len_variable = self.convert_ssa_value(arguments[0], dfg);
+                        let len_register_index =
+                            self.function_context.extract_register(len_variable);
+                        self.brillig_context.mov_instruction(result_register, len_register_index);
                     } else {
                         self.convert_ssa_array_len(arguments[0], result_register, dfg);
                     }
@@ -346,14 +352,12 @@
 
                     let results = dfg.instruction_results(instruction_id);
 
-                    let target_len = match self.function_context.get_or_create_variable(
+                    let target_len_variable = self.function_context.get_or_create_variable(
                         self.brillig_context,
                         results[0],
                         dfg,
-                    ) {
-                        RegisterOrMemory::RegisterIndex(register_index) => register_index,
-                        _ => unreachable!("ICE: first value of a slice must be a register index"),
-                    };
+                    );
+                    let target_len = self.function_context.extract_register(target_len_variable);
 
                     let target_slice = self.function_context.create_variable(
                         self.brillig_context,
@@ -362,9 +366,12 @@
                     );
 
                     let heap_vec = self.brillig_context.extract_heap_vector(target_slice);
+
+                    // Update the user-facing slice length
+                    self.brillig_context.mov_instruction(target_len, limb_count);
+
                     self.brillig_context.radix_instruction(
                         source,
-                        target_len,
                         heap_vec,
                         radix,
                         limb_count,
@@ -377,14 +384,12 @@
 
                     let results = dfg.instruction_results(instruction_id);
 
-                    let target_len = match self.function_context.get_or_create_variable(
+                    let target_len_variable = self.function_context.get_or_create_variable(
                         self.brillig_context,
                         results[0],
                         dfg,
-                    ) {
-                        RegisterOrMemory::RegisterIndex(register_index) => register_index,
-                        _ => unreachable!("ICE: first value of a slice must be a register index"),
-                    };
+                    );
+                    let target_len = self.function_context.extract_register(target_len_variable);
 
                     let target_slice = self.function_context.create_variable(
                         self.brillig_context,
@@ -394,9 +399,12 @@
 
                     let radix = self.brillig_context.make_constant(2_usize.into());
                     let heap_vec = self.brillig_context.extract_heap_vector(target_slice);
+
+                    // Update the user-facing slice length
+                    self.brillig_context.mov_instruction(target_len, limb_count);
+
                     self.brillig_context.radix_instruction(
                         source,
-                        target_len,
                         heap_vec,
                         radix,
                         limb_count,
@@ -815,21 +823,25 @@
         }
     }
 
+    /// Slices have a tuple structure (slice length, slice contents) to enable logic
+    /// that uses dynamic slice lengths (such as with merging slices in the flattening pass).
+    /// This method codegens an update to the slice length.
+    ///
+    /// The binary operation performed on the slice length is always an addition or subtraction of `1`.
+    /// This is because the slice length holds the user length (length as displayed by a `.len()` call),
+    /// and not a flattened length used internally to represent arrays of tuples.
+    /// The length inside of `RegisterOrMemory::HeapVector` represents the entire flattened number
+    /// of fields in the vector.
     fn update_slice_length(
         &mut self,
         target_len: RegisterIndex,
         source_value: ValueId,
         dfg: &DataFlowGraph,
         binary_op: BinaryIntOp,
     ) {
-        let source_len = match self.function_context.get_or_create_variable(
-            self.brillig_context,
-            source_value,
-            dfg,
-        ) {
-            RegisterOrMemory::RegisterIndex(register_index) => register_index,
-            _ => unreachable!("ICE: first value of a slice must be a register index"),
-        };
+        let source_len_variable =
+            self.function_context.get_or_create_variable(self.brillig_context, source_value, dfg);
+        let source_len = self.function_context.extract_register(source_len_variable);
 
         self.brillig_context.usize_op(source_len, target_len, binary_op, 1);
     }

crates/noirc_evaluator/src/brillig/brillig_ir.rs

@@ -853,13 +853,11 @@ impl BrilligContext {
     pub(crate) fn radix_instruction(
         &mut self,
         source: RegisterIndex,
-        target_vector_len: RegisterIndex,
         target_vector: HeapVector,
         radix: RegisterIndex,
         limb_count: RegisterIndex,
         big_endian: bool,
     ) {
-        self.mov_instruction(target_vector_len, limb_count);
         self.mov_instruction(target_vector.size, limb_count);
         self.allocate_array_instruction(target_vector.pointer, target_vector.size);
 

crates/noirc_evaluator/src/ssa.rs

@@ -58,8 +58,9 @@ pub(crate) fn optimize_into_acir(
             .simplify_cfg()
             .print(print_ssa_passes, "After Simplifying:")
             // Run mem2reg before flattening to handle any promotion
-            // of values that can be accessed after loop unrolling
-            // If this pass is missed slice merging will fail inside of flattening
+            // of values that can be accessed after loop unrolling.
+            // If there are slice mergers uncovered by loop unrolling
+            // and this pass is missed, slice merging will fail inside of flattening.
             .mem2reg()
             .print(print_ssa_passes, "After Mem2Reg:")
             .flatten_cfg()

crates/noirc_evaluator/src/ssa/ir/instruction/call.rs

@@ -75,8 +75,6 @@
             if let Some(length) = dfg.try_get_array_length(arguments[0]) {
                 let length = FieldElement::from(length as u128);
                 SimplifyResult::SimplifiedTo(dfg.make_constant(length, Type::field()))
-            } else if let Some(length) = dfg.get_numeric_constant(arguments[0]) {
-                SimplifyResult::SimplifiedTo(dfg.make_constant(length, Type::field()))
             } else if matches!(dfg.type_of_value(arguments[1]), Type::Slice(_)) {
                 SimplifyResult::SimplifiedTo(arguments[0])
             } else {
@@ -222,19 +220,19 @@
     }
 }
 
-// Slices have a tuple structure (slice length, slice contents) to enable logic
-// that uses dynamic slice lengths (such as with merging slices in the flattening pass).
-// This method codegens an update to the slice length.
+/// Slices have a tuple structure (slice length, slice contents) to enable logic
+/// that uses dynamic slice lengths (such as with merging slices in the flattening pass).
+/// This method codegens an update to the slice length.
+///
+/// The binary operation performed on the slice length is always an addition or subtraction of `1`.
+/// This is because the slice length holds the user length (length as displayed by a `.len()` call),
+/// and not a flattened length used internally to represent arrays of tuples.
 fn update_slice_length(slice_len: ValueId, dfg: &mut DataFlowGraph, operator: BinaryOp) -> ValueId {
     let one = dfg.make_constant(FieldElement::one(), Type::field());
     let block = dfg.make_block();
-    dfg.insert_instruction_and_results(
-        Instruction::Binary(Binary { lhs: slice_len, operator, rhs: one }),
-        block,
-        None,
-        dfg.get_value_call_stack(slice_len),
-    )
-    .first()
+    let instruction = Instruction::Binary(Binary { lhs: slice_len, operator, rhs: one });
+    let call_stack = dfg.get_value_call_stack(slice_len);
+    dfg.insert_instruction_and_results(instruction, block, None, call_stack).first()
 }
 
 /// Try to simplify this black box call. If the call can be simplified to a known value,

crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs

@@ -427,7 +427,7 @@ impl<'f> Context<'f> {
             _ => panic!("Expected array value"),
         };
 
-        let len = if len > else_len { len } else { else_len };
+        let len = len.max(else_len);
 
         for i in 0..len {
             for (element_index, element_type) in element_types.iter().enumerate() {
@@ -440,10 +440,8 @@ impl<'f> Context<'f> {
                     // The smaller slice is filled with placeholder data. Codegen for slice accesses must
                     // include checks against the dynamic slice length so that this placeholder data is not incorrectly accessed.
                     if (len - 1) < index_value.to_u128() as usize {
-                        self.inserter
-                            .function
-                            .dfg
-                            .make_constant(FieldElement::zero(), Type::field())
+                        let zero = FieldElement::zero();
+                        self.inserter.function.dfg.make_constant(zero, Type::field())
                     } else {
                         let get = Instruction::ArrayGet { array, index };
                         self.insert_instruction_with_typevars(get, typevars).first()

crates/noirc_evaluator/src/ssa/ssa_gen/context.rs

@@ -226,10 +226,7 @@ impl<'a> FunctionContext<'a> {
             ast::Type::Unit => panic!("convert_non_tuple_type called on a unit type"),
             ast::Type::Tuple(_) => panic!("convert_non_tuple_type called on a tuple: {typ}"),
             ast::Type::Function(_, _, _) => Type::Function,
-            ast::Type::Slice(element) => {
-                let element_types = Self::convert_type(element).flatten();
-                Type::Slice(Rc::new(element_types))
-            }
+            ast::Type::Slice(_) => panic!("convert_non_tuple_type called on a slice: {typ}"),
             ast::Type::MutableReference(element) => {
                 // Recursive call to panic if element is a tuple
                 Self::convert_non_tuple_type(element);
@@ -585,10 +582,10 @@ impl<'a> FunctionContext<'a> {
     }
 
     /// Compile the given `array[index]` expression as a reference.
-    /// This will return a triple of (array, index, lvalue_ref, Option<max_length>) where the lvalue_ref records the
+    /// This will return a triple of (array, index, lvalue_ref, Option<length>) where the lvalue_ref records the
     /// structure of the lvalue expression for use by `assign_new_value`.
-    /// The optional max length is for the case where we are indexing a slice rather than an array as slices
-    /// are representing as the following tuple: (length, slice contents).
+    /// The optional length is for indexing slices rather than arrays since slices
+    /// are represented as a tuple in the form: (length, slice contents).
     fn index_lvalue(
         &mut self,
         array: &ast::LValue,
@@ -599,14 +596,19 @@ impl<'a> FunctionContext<'a> {
         let array_lvalue = Box::new(array_lvalue);
         let array_values = old_array.clone().into_value_list(self);
 
-        // A slice is represented as a tuple (length, slice contents)
-        // We need to fetch the second
+        // A slice is represented as a tuple (length, slice contents).
+        // We need to fetch the second value.
         if array_values.len() > 1 {
             let slice_lvalue =
                 LValue::SliceIndex { old_slice: old_array, index, slice_lvalue: array_lvalue };
             (array_values[1], index, slice_lvalue, Some(array_values[0]))
         } else {
-            (array_values[0], index, LValue::Index { old_array: array_values[0], index, array_lvalue }, None)
+            (
+                array_values[0],
+                index,
+                LValue::Index { old_array: array_values[0], index, array_lvalue },
+                None,
+            )
         }
     }
 
@@ -650,34 +652,15 @@ impl<'a> FunctionContext<'a> {
         match lvalue {
             LValue::Ident => unreachable!("Cannot assign to a variable without a reference"),
             LValue::Index { old_array: mut array, index, array_lvalue } => {
-                let element_size = self.builder.field_constant(self.element_size(array));
-
-                // The actual base index is the user's index * the array element type's size
-                let mut index = self.builder.insert_binary(index, BinaryOp::Mul, element_size);
-                let one = self.builder.field_constant(FieldElement::one());
-
-                new_value.for_each(|value| {
-                    let value = value.eval(self);
-                    array = self.builder.insert_array_set(array, index, value);
-                    index = self.builder.insert_binary(index, BinaryOp::Add, one);
-                });
+                array = self.assign_lvalue_index(new_value, array, index);
                 self.assign_new_value(*array_lvalue, array.into());
             }
             LValue::SliceIndex { old_slice: slice, index, slice_lvalue } => {
                 let mut slice_values = slice.into_value_list(self);
 
-                let element_size = self.builder.field_constant(self.element_size(slice_values[1]));
+                slice_values[1] = self.assign_lvalue_index(new_value, slice_values[1], index);
 
-                // The actual base index is the user's index * the array element type's size
-                let mut index = self.builder.insert_binary(index, BinaryOp::Mul, element_size);
-                let one = self.builder.field_constant(FieldElement::one());
-
-                new_value.for_each(|value| {
-                    let value = value.eval(self);
-                    slice_values[1] = self.builder.insert_array_set(slice_values[1], index, value);
-                    index = self.builder.insert_binary(index, BinaryOp::Add, one);
-                });
-                // The size of the slice does not change in an assign so we can reuse the same length value
+                // The size of the slice does not change in a slice index assignment so we can reuse the same length value
                 let new_slice = Tree::Branch(vec![slice_values[0].into(), slice_values[1].into()]);
                 self.assign_new_value(*slice_lvalue, new_slice);
             }
@@ -691,6 +674,26 @@ impl<'a> FunctionContext<'a> {
         }
     }
 
+    fn assign_lvalue_index(
+        &mut self,
+        new_value: Values,
+        mut array: ValueId,
+        index: ValueId,
+    ) -> ValueId {
+        let element_size = self.builder.field_constant(self.element_size(array));
+
+        // The actual base index is the user's index * the array element type's size
+        let mut index = self.builder.insert_binary(index, BinaryOp::Mul, element_size);
+        let one = self.builder.field_constant(FieldElement::one());
+
+        new_value.for_each(|value| {
+            let value = value.eval(self);
+            array = self.builder.insert_array_set(array, index, value);
+            index = self.builder.insert_binary(index, BinaryOp::Add, one);
+        });
+        array
+    }
+
     fn element_size(&self, array: ValueId) -> FieldElement {
         let size = self.builder.type_of_value(array).element_size();
         FieldElement::from(size as u128)