chore(acir): Add some unit tests for arrays and light refactors

compiler/noirc_evaluator/src/acir/arrays.rs

@@ -377,15 +377,15 @@ impl Context<'_> {
 
     /// We need to properly setup the inputs for array operations in ACIR.
     /// From the original SSA values we compute the following AcirVars:
-    /// - new_index is the index of the array. ACIR memory operations work with a flat memory, so we fully flattened the specified index
+    /// - `index_var` is the index of the array. ACIR memory operations work with a flat memory, so we fully flattened the specified index
     ///   in case we have a nested array. The index for SSA array operations only represents the flattened index of the current array.
     ///   Thus internal array element type sizes need to be computed to accurately transform the index.
     ///
-    /// - predicate_index is offset, or the index if the predicate is true
+    /// - If the predicate is known to be true or the array access is guaranteed to be safe, we can directly return `index_var`
+    ///   Otherwise, `predicate_index` is a fallback offset set by [Self::predicated_index].
     ///
-    /// - new_value is the optional value when the operation is an array_set
-    ///   When there is a predicate, it is predicate*value + (1-predicate)*dummy, where dummy is the value of the array at the requested index.
-    ///   It is a dummy value because in the case of a false predicate, the value stored at the requested index will be itself.
+    /// - `new_value` is the optional value when the operation is an array_set.
+    ///   The value used in an array_set is also dependent upon the predicate and is set in [Self::predicated_store_value]
     fn convert_array_operation_inputs(
         &mut self,
         array_id: ValueId,
@@ -400,42 +400,57 @@ impl Context<'_> {
         let index_var = self.convert_numeric_value(index, dfg)?;
         let index_var = self.get_flattened_index(&array_typ, array_id, index_var, dfg)?;
 
-        let predicate_index = if dfg.is_safe_index(index, array_id) {
-            index_var
-        } else {
-            // index*predicate + (1-predicate)*offset
-            let offset = self.acir_context.add_constant(offset);
-            let sub = self.acir_context.sub_var(index_var, offset)?;
-            let pred = self.acir_context.mul_var(sub, self.current_side_effects_enabled_var)?;
-            self.acir_context.add_var(pred, offset)?
-        };
+        // Side-effects are always enabled so we do not need to do any predication
+        if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) {
+            let store_value = store_value.map(|store| self.convert_value(store, dfg));
+            return Ok((index_var, store_value));
+        }
 
-        let new_value = if let Some(store) = store_value {
-            let store_value = self.convert_value(store, dfg);
-            if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) {
-                Some(store_value)
-            } else {
-                let store_type = dfg.type_of_value(store);
+        let predicate_index = self.predicated_index(index_var, index, array_id, dfg, offset)?;
 
-                let mut dummy_predicate_index = predicate_index;
-                // We must setup the dummy value to match the type of the value we wish to store
-                let dummy =
-                    self.array_get_value(&store_type, block_id, &mut dummy_predicate_index)?;
+        // Handle the predicated store value
+        let new_value = store_value
+            .map(|store| self.predicated_store_value(store, dfg, block_id, predicate_index))
+            .transpose()?;
 
-                Some(self.convert_array_set_store_value(&store_value, &dummy)?)
-            }
-        } else {
-            None
-        };
+        Ok((predicate_index, new_value))
+    }
 
-        let new_index = if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var)
-        {
-            index_var
+    /// Computes the predicated index for an array access.
+    /// If the index is always safe, it is returned directly.
+    /// Otherwise, we compute `predicate * index + (1 - predicate) * offset`.
+    fn predicated_index(
+        &mut self,
+        index_var: AcirVar,
+        index: ValueId,
+        array_id: ValueId,
+        dfg: &DataFlowGraph,
+        offset: usize,
+    ) -> Result<AcirVar, RuntimeError> {
+        if dfg.is_safe_index(index, array_id) {
+            Ok(index_var)
         } else {
-            predicate_index
-        };
+            let offset = self.acir_context.add_constant(offset);
+            let sub = self.acir_context.sub_var(index_var, offset)?;
+            let pred = self.acir_context.mul_var(sub, self.current_side_effects_enabled_var)?;
+            self.acir_context.add_var(pred, offset)
+        }
+    }
 
-        Ok((new_index, new_value))
+    /// When there is a predicate, the store value is predicate*value + (1-predicate)*dummy, where dummy is the value of the array at the requested index.
+    /// It is a dummy value because in the case of a false predicate, the value stored at the requested index will be itself.
+    fn predicated_store_value(
+        &mut self,
+        store: ValueId,
+        dfg: &DataFlowGraph,
+        block_id: BlockId,
+        mut dummy_predicate_index: AcirVar,
+    ) -> Result<AcirValue, RuntimeError> {
+        let store_value = self.convert_value(store, dfg);
+        let store_type = dfg.type_of_value(store);
+        // We must setup the dummy value to match the type of the value we wish to store
+        let dummy = self.array_get_value(&store_type, block_id, &mut dummy_predicate_index)?;
+        self.convert_array_set_store_value(&store_value, &dummy)
     }
 
     fn convert_array_set_store_value(

compiler/noirc_evaluator/src/acir/call.rs

@@ -3,7 +3,9 @@ use acvm::{AcirField, FieldElement};
 use iter_extended::vecmap;
 
 use crate::acir::AcirVar;
+use crate::brillig::brillig_gen::brillig_fn::FunctionContext as BrilligFunctionContext;
 use crate::brillig::brillig_gen::gen_brillig_for;
+use crate::brillig::brillig_ir::artifact::BrilligParameter;
 use crate::errors::{RuntimeError, SsaReport};
 use crate::ssa::ir::instruction::Hint;
 use crate::ssa::ir::value::Value;
@@ -170,6 +172,40 @@ impl Context<'_> {
         self.handle_ssa_call_outputs(result_ids, output_values, dfg)
     }
 
+    pub(super) fn gen_brillig_parameters(
+        &self,
+        values: &[ValueId],
+        dfg: &DataFlowGraph,
+    ) -> Vec<BrilligParameter> {
+        values
+            .iter()
+            .map(|&value_id| {
+                let typ = dfg.type_of_value(value_id);
+                if let Type::Slice(item_types) = typ {
+                    let len = match self
+                        .ssa_values
+                        .get(&value_id)
+                        .expect("ICE: Unknown slice input to brillig")
+                    {
+                        AcirValue::DynamicArray(AcirDynamicArray { len, .. }) => *len,
+                        AcirValue::Array(array) => array.len(),
+                        _ => unreachable!("ICE: Slice value is not an array"),
+                    };
+
+                    BrilligParameter::Slice(
+                        item_types
+                            .iter()
+                            .map(BrilligFunctionContext::ssa_type_to_parameter)
+                            .collect(),
+                        len / item_types.len(),
+                    )
+                } else {
+                    BrilligFunctionContext::ssa_type_to_parameter(&typ)
+                }
+            })
+            .collect()
+    }
+
     /// Returns a vector of `AcirVar`s constrained to be result of the function call.
     ///
     /// The function being called is required to be intrinsic.

compiler/noirc_evaluator/src/acir/mod.rs

@@ -27,11 +27,8 @@ pub(crate) mod ssa;
 mod tests;
 mod types;
 
+use crate::brillig::Brillig;
 use crate::brillig::brillig_gen::gen_brillig_for;
-use crate::brillig::{
-    Brillig, brillig_gen::brillig_fn::FunctionContext as BrilligFunctionContext,
-    brillig_ir::artifact::BrilligParameter,
-};
 use crate::errors::{InternalError, InternalWarning, RuntimeError, SsaReport};
 use crate::ssa::{
     function_builder::data_bus::DataBus,
@@ -587,40 +584,6 @@ impl<'a> Context<'a> {
         Ok(warnings)
     }
 
-    fn gen_brillig_parameters(
-        &self,
-        values: &[ValueId],
-        dfg: &DataFlowGraph,
-    ) -> Vec<BrilligParameter> {
-        values
-            .iter()
-            .map(|&value_id| {
-                let typ = dfg.type_of_value(value_id);
-                if let Type::Slice(item_types) = typ {
-                    let len = match self
-                        .ssa_values
-                        .get(&value_id)
-                        .expect("ICE: Unknown slice input to brillig")
-                    {
-                        AcirValue::DynamicArray(AcirDynamicArray { len, .. }) => *len,
-                        AcirValue::Array(array) => array.len(),
-                        _ => unreachable!("ICE: Slice value is not an array"),
-                    };
-
-                    BrilligParameter::Slice(
-                        item_types
-                            .iter()
-                            .map(BrilligFunctionContext::ssa_type_to_parameter)
-                            .collect(),
-                        len / item_types.len(),
-                    )
-                } else {
-                    BrilligFunctionContext::ssa_type_to_parameter(&typ)
-                }
-            })
-            .collect()
-    }
-
     /// Remember the result of an instruction returning a single value
     fn define_result(
         &mut self,