chore: Add Instruction::MakeArray to SSA (#6071)

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

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

@@ -160,13 +160,9 @@ impl<'block> BrilligBlock<'block> {
                 );
             }
             TerminatorInstruction::Return { return_values, .. } => {
-                let return_registers: Vec<_> = return_values
-                    .iter()
-                    .map(|value_id| {
-                        let return_variable = self.convert_ssa_value(*value_id, dfg);
-                        return_variable.extract_register()
-                    })
-                    .collect();
+                let return_registers = vecmap(return_values, |value_id| {
+                    self.convert_ssa_value(*value_id, dfg).extract_register()
+                });
                 self.brillig_context.codegen_return(&return_registers);
             }
         }
@@ -763,6 +759,43 @@ impl<'block> BrilligBlock<'block> {
             Instruction::IfElse { .. } => {
                 unreachable!("IfElse instructions should not be possible in brillig")
             }
+            Instruction::MakeArray { elements: array, typ } => {
+                let value_id = dfg.instruction_results(instruction_id)[0];
+                if !self.variables.is_allocated(&value_id) {
+                    let new_variable = self.variables.define_variable(
+                        self.function_context,
+                        self.brillig_context,
+                        value_id,
+                        dfg,
+                    );
+
+                    // Initialize the variable
+                    match new_variable {
+                        BrilligVariable::BrilligArray(brillig_array) => {
+                            self.brillig_context.codegen_initialize_array(brillig_array);
+                        }
+                        BrilligVariable::BrilligVector(vector) => {
+                            let size = self
+                                .brillig_context
+                                .make_usize_constant_instruction(array.len().into());
+                            self.brillig_context.codegen_initialize_vector(vector, size, None);
+                            self.brillig_context.deallocate_single_addr(size);
+                        }
+                        _ => unreachable!(
+                            "ICE: Cannot initialize array value created as {new_variable:?}"
+                        ),
+                    };
+
+                    // Write the items
+                    let items_pointer = self
+                        .brillig_context
+                        .codegen_make_array_or_vector_items_pointer(new_variable);
+
+                    self.initialize_constant_array(array, typ, dfg, items_pointer);
+
+                    self.brillig_context.deallocate_register(items_pointer);
+                }
+            }
         };
 
         let dead_variables = self
@@ -1500,46 +1533,6 @@ impl<'block> BrilligBlock<'block> {
                     new_variable
                 }
             }
-            Value::Array { array, typ } => {
-                if self.variables.is_allocated(&value_id) {
-                    self.variables.get_allocation(self.function_context, value_id, dfg)
-                } else {
-                    let new_variable = self.variables.define_variable(
-                        self.function_context,
-                        self.brillig_context,
-                        value_id,
-                        dfg,
-                    );
-
-                    // Initialize the variable
-                    match new_variable {
-                        BrilligVariable::BrilligArray(brillig_array) => {
-                            self.brillig_context.codegen_initialize_array(brillig_array);
-                        }
-                        BrilligVariable::BrilligVector(vector) => {
-                            let size = self
-                                .brillig_context
-                                .make_usize_constant_instruction(array.len().into());
-                            self.brillig_context.codegen_initialize_vector(vector, size, None);
-                            self.brillig_context.deallocate_single_addr(size);
-                        }
-                        _ => unreachable!(
-                            "ICE: Cannot initialize array value created as {new_variable:?}"
-                        ),
-                    };
-
-                    // Write the items
-                    let items_pointer = self
-                        .brillig_context
-                        .codegen_make_array_or_vector_items_pointer(new_variable);
-
-                    self.initialize_constant_array(array, typ, dfg, items_pointer);
-
-                    self.brillig_context.deallocate_register(items_pointer);
-
-                    new_variable
-                }
-            }
             Value::Function(_) => {
                 // For the debugger instrumentation we want to allow passing
                 // around values representing function pointers, even though

compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs

@@ -89,8 +89,7 @@ impl ConstantAllocation {
             }
             if let Some(terminator_instruction) = block.terminator() {
                 terminator_instruction.for_each_value(|value_id| {
-                    let variables = collect_variables_of_value(value_id, &func.dfg);
-                    for variable in variables {
+                    if let Some(variable) = collect_variables_of_value(value_id, &func.dfg) {
                         record_if_constant(block_id, variable, InstructionLocation::Terminator);
                     }
                 });
@@ -166,7 +165,7 @@ impl ConstantAllocation {
 }
 
 pub(crate) fn is_constant_value(id: ValueId, dfg: &DataFlowGraph) -> bool {
-    matches!(&dfg[dfg.resolve(id)], Value::NumericConstant { .. } | Value::Array { .. })
+    matches!(&dfg[dfg.resolve(id)], Value::NumericConstant { .. })
 }
 
 /// For a given function, finds all the blocks that are within loops

compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs

@@ -45,32 +45,19 @@ fn find_back_edges(
 }
 
 /// Collects the underlying variables inside a value id. It might be more than one, for example in constant arrays that are constructed with multiple vars.
-pub(crate) fn collect_variables_of_value(value_id: ValueId, dfg: &DataFlowGraph) -> Vec<ValueId> {
+pub(crate) fn collect_variables_of_value(
+    value_id: ValueId,
+    dfg: &DataFlowGraph,
+) -> Option<ValueId> {
     let value_id = dfg.resolve(value_id);
     let value = &dfg[value_id];
 
     match value {
-        Value::Instruction { .. } | Value::Param { .. } => {
-            vec![value_id]
-        }
-        // Literal arrays are constants, but might use variable values to initialize.
-        Value::Array { array, .. } => {
-            let mut value_ids = vec![value_id];
-
-            array.iter().for_each(|item_id| {
-                let underlying_ids = collect_variables_of_value(*item_id, dfg);
-                value_ids.extend(underlying_ids);
-            });
-
-            value_ids
-        }
-        Value::NumericConstant { .. } => {
-            vec![value_id]
+        Value::Instruction { .. } | Value::Param { .. } | Value::NumericConstant { .. } => {
+            Some(value_id)
         }
         // Functions are not variables in a defunctionalized SSA. Only constant function values should appear.
-        Value::ForeignFunction(_) | Value::Function(_) | Value::Intrinsic(..) => {
-            vec![]
-        }
+        Value::ForeignFunction(_) | Value::Function(_) | Value::Intrinsic(..) => None,
     }
 }
 

compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs

@@ -47,13 +47,15 @@ impl<F: AcirField + DebugToString, Registers: RegisterAllocator> BrilligContext<
             let destinations_of_temp = movements_map.remove(first_source).unwrap();
             movements_map.insert(temp_register, destinations_of_temp);
         }
+
         // After removing loops we should have an DAG with each node having only one ancestor (but could have multiple successors)
         // Now we should be able to move the registers just by performing a DFS on the movements map
         let heads: Vec<_> = movements_map
             .keys()
             .filter(|source| !destinations_set.contains(source))
             .copied()
             .collect();
+
         for head in heads {
             self.perform_movements(&movements_map, head);
         }

compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs

@@ -767,6 +767,12 @@ impl<'a> Context<'a> {
             Instruction::IfElse { .. } => {
                 unreachable!("IfElse instruction remaining in acir-gen")
             }
+            Instruction::MakeArray { elements, typ: _ } => {
+                let elements = elements.iter().map(|element| self.convert_value(*element, dfg));
+                let value = AcirValue::Array(elements.collect());
+                let result = dfg.instruction_results(instruction_id)[0];
+                self.ssa_values.insert(result, value);
+            }
         }
 
         self.acir_context.set_call_stack(CallStack::new());
@@ -1557,7 +1563,7 @@ impl<'a> Context<'a> {
         if !already_initialized {
             let value = &dfg[array];
             match value {
-                Value::Array { .. } | Value::Instruction { .. } => {
+                Value::Instruction { .. } => {
                     let value = self.convert_value(array, dfg);
                     let array_typ = dfg.type_of_value(array);
                     let len = if !array_typ.contains_slice_element() {
@@ -1600,13 +1606,13 @@ impl<'a> Context<'a> {
         match array_typ {
             Type::Array(_, _) | Type::Slice(_) => {
                 match &dfg[array_id] {
-                    Value::Array { array, .. } => {
-                        for (i, value) in array.iter().enumerate() {
-                            flat_elem_type_sizes.push(
-                                self.flattened_slice_size(*value, dfg) + flat_elem_type_sizes[i],
-                            );
-                        }
-                    }
+                    // Value::Array { array, .. } => {
+                    //     for (i, value) in array.iter().enumerate() {
+                    //         flat_elem_type_sizes.push(
+                    //             self.flattened_slice_size(*value, dfg) + flat_elem_type_sizes[i],
+                    //         );
+                    //     }
+                    // }
                     Value::Instruction { .. } | Value::Param { .. } => {
                         // An instruction representing the slice means it has been processed previously during ACIR gen.
                         // Use the previously defined result of an array operation to fetch the internal type information.
@@ -1739,13 +1745,13 @@ impl<'a> Context<'a> {
     fn flattened_slice_size(&mut self, array_id: ValueId, dfg: &DataFlowGraph) -> usize {
         let mut size = 0;
         match &dfg[array_id] {
-            Value::Array { array, .. } => {
-                // The array is going to be the flattened outer array
-                // Flattened slice size from SSA value does not need to be multiplied by the len
-                for value in array {
-                    size += self.flattened_slice_size(*value, dfg);
-                }
-            }
+            // Value::Array { array, .. } => {
+            //     // The array is going to be the flattened outer array
+            //     // Flattened slice size from SSA value does not need to be multiplied by the len
+            //     for value in array {
+            //         size += self.flattened_slice_size(*value, dfg);
+            //     }
+            // }
             Value::NumericConstant { .. } => {
                 size += 1;
             }
@@ -1909,10 +1915,6 @@ impl<'a> Context<'a> {
             Value::NumericConstant { constant, typ } => {
                 AcirValue::Var(self.acir_context.add_constant(*constant), typ.into())
             }
-            Value::Array { array, .. } => {
-                let elements = array.iter().map(|element| self.convert_value(*element, dfg));
-                AcirValue::Array(elements.collect())
-            }
             Value::Intrinsic(..) => todo!(),
             Value::Function(function_id) => {
                 // This conversion is for debugging support only, to allow the

compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs

@@ -191,7 +191,8 @@ impl Context {
                 | Instruction::Load { .. }
                 | Instruction::Not(..)
                 | Instruction::Store { .. }
-                | Instruction::Truncate { .. } => {
+                | Instruction::Truncate { .. }
+                | Instruction::MakeArray { .. } => {
                     self.value_sets.push(instruction_arguments_and_results);
                 }
 
@@ -247,8 +248,7 @@ impl Context {
                         Value::ForeignFunction(..) => {
                             panic!("Should not be able to reach foreign function from non-brillig functions, {func_id} in function {}", function.name());
                         }
-                        Value::Array { .. }
-                        | Value::Instruction { .. }
+                        Value::Instruction { .. }
                         | Value::NumericConstant { .. }
                         | Value::Param { .. } => {
                             panic!("At the point we are running disconnect there shouldn't be any other values as arguments")

compiler/noirc_evaluator/src/ssa/function_builder/data_bus.rs

@@ -1,6 +1,6 @@
 use std::{collections::BTreeMap, sync::Arc};
 
-use crate::ssa::ir::{types::Type, value::ValueId};
+use crate::ssa::ir::{function::RuntimeType, types::Type, value::ValueId};
 use acvm::FieldElement;
 use fxhash::FxHashMap as HashMap;
 use noirc_frontend::ast;
@@ -100,7 +100,8 @@ impl DataBus {
     ) -> DataBus {
         let mut call_data_args = Vec::new();
         for call_data_item in call_data {
-            let array_id = call_data_item.databus.expect("Call data should have an array id");
+            // databus can be None if `main` is a brillig function
+            let Some(array_id) = call_data_item.databus else { continue };
             let call_data_id =
                 call_data_item.call_data_id.expect("Call data should have a user id");
             call_data_args.push(CallData { array_id, call_data_id, index_map: call_data_item.map });
@@ -161,13 +162,11 @@ impl FunctionBuilder {
         }
         let len = databus.values.len();
 
-        let array = if len > 0 {
-            let array = self
-                .array_constant(databus.values, Type::Array(Arc::new(vec![Type::field()]), len));
-            Some(array)
-        } else {
-            None
-        };
+        let array = (len > 0 && matches!(self.current_function.runtime(), RuntimeType::Acir(_)))
+            .then(|| {
+                let array_type = Type::Array(Arc::new(vec![Type::field()]), len);
+                self.insert_make_array(databus.values, array_type)
+            });
 
         DataBusBuilder {
             index: 0,

compiler/noirc_evaluator/src/ssa/function_builder/mod.rs

@@ -136,11 +136,6 @@ impl FunctionBuilder {
         self.numeric_constant(value.into(), Type::length_type())
     }
 
-    /// Insert an array constant into the current function with the given element values.
-    pub(crate) fn array_constant(&mut self, elements: im::Vector<ValueId>, typ: Type) -> ValueId {
-        self.current_function.dfg.make_array(elements, typ)
-    }
-
     /// Returns the type of the given value.
     pub(crate) fn type_of_value(&self, value: ValueId) -> Type {
         self.current_function.dfg.type_of_value(value)
@@ -355,6 +350,17 @@ impl FunctionBuilder {
         self.insert_instruction(Instruction::EnableSideEffectsIf { condition }, None);
     }
 
+    /// Insert a `make_array` instruction to create a new array or slice.
+    /// Returns the new array value. Expects `typ` to be an array or slice type.
+    pub(crate) fn insert_make_array(
+        &mut self,
+        elements: im::Vector<ValueId>,
+        typ: Type,
+    ) -> ValueId {
+        assert!(matches!(typ, Type::Array(..) | Type::Slice(_)));
+        self.insert_instruction(Instruction::MakeArray { elements, typ }, None).first()
+    }
+
     /// Terminates the current block with the given terminator instruction
     /// if the current block does not already have a terminator instruction.
     fn terminate_block_with(&mut self, terminator: TerminatorInstruction) {
@@ -504,7 +510,6 @@ mod tests {
         instruction::{Endian, Intrinsic},
         map::Id,
         types::Type,
-        value::Value,
     };
 
     use super::FunctionBuilder;
@@ -526,10 +531,7 @@ mod tests {
         let call_results =
             builder.insert_call(to_bits_id, vec![input, length], result_types).into_owned();
 
-        let slice = match &builder.current_function.dfg[call_results[0]] {
-            Value::Array { array, .. } => array,
-            _ => panic!(),
-        };
+        let slice = builder.current_function.dfg.get_array_constant(call_results[0]).unwrap().0;
         assert_eq!(slice[0], one);
         assert_eq!(slice[1], one);
         assert_eq!(slice[2], one);