chore(debug): Add ownership analysis pass

EXTERNAL_NOIR_LIBRARIES.yml

@@ -10,7 +10,7 @@ libraries:
     critical: true
   eddsa:
     repo: noir-lang/eddsa
-    timeout: 2
+    timeout: 3
     critical: true
   mimc:
     repo: noir-lang/mimc

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

@@ -881,6 +881,12 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> {
                 // RC is always directly pointed by the array/vector pointer
                 self.brillig_context
                     .load_instruction(rc_register, array_or_vector.extract_register());
+
+                // Ensure we're not incrementing from 0 back to 1
+                if self.brillig_context.enable_debug_assertions() {
+                    self.assert_rc_neq_zero(rc_register);
+                }
+
                 self.brillig_context.codegen_usize_op_in_place(
                     rc_register,
                     BrilligBinaryOp::Add,
@@ -901,20 +907,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> {
                 // and become usize::MAX, and then return to 1, then it will indicate
                 // an array as mutable when it probably shouldn't be.
                 if self.brillig_context.enable_debug_assertions() {
-                    let min_addr = ReservedRegisters::usize_one();
-                    let condition =
-                        SingleAddrVariable::new(self.brillig_context.allocate_register(), 1);
-                    self.brillig_context.memory_op_instruction(
-                        min_addr,
-                        rc_register,
-                        condition.address,
-                        BrilligBinaryOp::LessThan,
-                    );
-                    self.brillig_context.codegen_constrain(
-                        condition,
-                        Some("array ref-count underflow detected".to_owned()),
-                    );
-                    self.brillig_context.deallocate_single_addr(condition);
+                    self.assert_rc_neq_zero(rc_register);
                 }
 
                 self.brillig_context.codegen_usize_op_in_place(
@@ -1049,6 +1042,25 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> {
         self.brillig_context.set_call_stack(CallStack::new());
     }
 
+    fn assert_rc_neq_zero(&mut self, rc_register: MemoryAddress) {
+        let zero = SingleAddrVariable::new(self.brillig_context.allocate_register(), 32);
+
+        self.brillig_context.const_instruction(zero, FieldElement::zero());
+
+        let condition = SingleAddrVariable::new(self.brillig_context.allocate_register(), 1);
+
+        self.brillig_context.memory_op_instruction(
+            zero.address,
+            rc_register,
+            condition.address,
+            BrilligBinaryOp::Equals,
+        );
+        self.brillig_context.not_instruction(condition, condition);
+        self.brillig_context
+            .codegen_constrain(condition, Some("array ref-count underflow detected".to_owned()));
+        self.brillig_context.deallocate_single_addr(condition);
+    }
+
     fn convert_ssa_function_call(
         &mut self,
         func_id: FunctionId,

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

@@ -523,17 +523,7 @@ impl FunctionBuilder {
             return None;
         }
         match self.type_of_value(value) {
-            Type::Numeric(_) | Type::Function => None,
-            Type::Reference(element) => {
-                if element.contains_an_array() {
-                    let reference = value;
-                    let value = self.insert_load(reference, element.as_ref().clone());
-                    self.update_array_reference_count(value, increment);
-                    Some(value)
-                } else {
-                    None
-                }
-            }
+            Type::Numeric(_) | Type::Function | Type::Reference(_) => None,
             Type::Array(..) | Type::Slice(..) => {
                 // If there are nested arrays or slices, we wait until ArrayGet
                 // is issued to increment the count of that array.

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

@@ -24,7 +24,7 @@ use crate::ssa::ir::value::ValueId;
 
 use super::GlobalsGraph;
 use super::value::{Tree, Value, Values};
-use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet};
+use fxhash::FxHashMap as HashMap;
 
 /// The FunctionContext is the main context object for translating a
 /// function into SSA form during the SSA-gen pass.
@@ -172,8 +172,8 @@ impl<'a> FunctionContext<'a> {
         let parameter_value = Self::map_type(parameter_type, |typ| {
             let value = self.builder.add_parameter(typ);
             if mutable {
-                // This will wrap any `mut var: T` in a reference and increase the rc of an array if needed
-                self.new_mutable_variable(value, true)
+                // This will wrap any `mut var: T` in a reference
+                self.new_mutable_variable(value)
             } else {
                 value.into()
             }
@@ -184,17 +184,9 @@ impl<'a> FunctionContext<'a> {
 
     /// Allocate a single slot of memory and store into it the given initial value of the variable.
     /// Always returns a Value::Mutable wrapping the allocate instruction.
-    pub(super) fn new_mutable_variable(
-        &mut self,
-        value_to_store: ValueId,
-        increment_array_rc: bool,
-    ) -> Value {
+    pub(super) fn new_mutable_variable(&mut self, value_to_store: ValueId) -> Value {
         let element_type = self.builder.current_function.dfg.type_of_value(value_to_store);
 
-        if increment_array_rc {
-            self.builder.increment_array_reference_count(value_to_store);
-        }
-
         let alloc = self.builder.insert_allocate(element_type);
         self.builder.insert_store(alloc, value_to_store);
         let typ = self.builder.type_of_value(value_to_store);
@@ -219,7 +211,7 @@ impl<'a> FunctionContext<'a> {
             ast::Type::Unit => Tree::empty(),
             // A mutable reference wraps each element into a reference.
             // This can be multiple values if the element type is a tuple.
-            ast::Type::MutableReference(element) => {
+            ast::Type::Reference(element, _) => {
                 Self::map_type_helper(element, &mut |typ| f(Type::Reference(Arc::new(typ))))
             }
             ast::Type::FmtString(len, fields) => {
@@ -272,7 +264,7 @@ impl<'a> FunctionContext<'a> {
             ast::Type::Tuple(_) => panic!("convert_non_tuple_type called on a tuple: {typ}"),
             ast::Type::Function(_, _, _, _) => Type::Function,
             ast::Type::Slice(_) => panic!("convert_non_tuple_type called on a slice: {typ}"),
-            ast::Type::MutableReference(element) => {
+            ast::Type::Reference(element, _) => {
                 // Recursive call to panic if element is a tuple
                 let element = Self::convert_non_tuple_type(element);
                 Type::Reference(Arc::new(element))
@@ -957,65 +949,6 @@ impl<'a> FunctionContext<'a> {
         }
     }
 
-    /// Increments the reference count of mutable reference array parameters.
-    /// Any mutable-value (`mut a: [T; N]` versus `a: &mut [T; N]`) are already incremented
-    /// by `FunctionBuilder::add_parameter_to_scope`.
-    /// Returns each array id that was incremented.
-    ///
-    /// This is done on parameters rather than call arguments so that we can optimize out
-    /// paired inc/dec instructions within brillig functions more easily.
-    ///
-    /// Returns the list of parameters incremented, together with the value ID of the arrays they refer to.
-    pub(crate) fn increment_parameter_rcs(&mut self) -> Vec<(ValueId, ValueId)> {
-        let entry = self.builder.current_function.entry_block();
-        let parameters = self.builder.current_function.dfg.block_parameters(entry).to_vec();
-
-        let mut incremented = Vec::default();
-        let mut seen_array_types = HashSet::default();
-
-        for parameter in parameters {
-            // Avoid reference counts for immutable arrays that aren't behind references.
-            let typ = self.builder.current_function.dfg.type_of_value(parameter);
-
-            if let Type::Reference(element) = typ {
-                if element.contains_an_array() {
-                    // If we haven't already seen this array type, the value may be possibly
-                    // aliased, so issue an inc_rc for it.
-                    if seen_array_types.insert(element.get_contained_array().clone()) {
-                        continue;
-                    }
-                    if let Some(id) = self.builder.increment_array_reference_count(parameter) {
-                        incremented.push((parameter, id));
-                    }
-                }
-            }
-        }
-
-        incremented
-    }
-
-    /// Ends a local scope of a function.
-    /// This will issue DecrementRc instructions for any arrays in the given starting scope
-    /// block's parameters. Arrays that are also used in terminator instructions for the scope are
-    /// ignored.
-    pub(crate) fn end_scope(
-        &mut self,
-        mut incremented_params: Vec<(ValueId, ValueId)>,
-        terminator_args: &[ValueId],
-    ) {
-        // TODO: This check likely leads to unsoundness.
-        // It is here to avoid decrementing the RC of a parameter we're returning but we
-        // only check the exact ValueId which can be easily circumvented by storing to and
-        // loading from a temporary reference.
-        incremented_params.retain(|(parameter, _)| !terminator_args.contains(parameter));
-
-        for (parameter, original) in incremented_params {
-            if self.builder.current_function.dfg.value_is_reference(parameter) {
-                self.builder.decrement_array_reference_count(original);
-            }
-        }
-    }
-
     pub(crate) fn enter_loop(&mut self, loop_: Loop) {
         self.loops.push(loop_);
     }

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

@@ -136,10 +136,8 @@
     /// Codegen a function's body and set its return value to that of its last parameter.
     /// For functions returning nothing, this will be an empty list.
     fn codegen_function_body(&mut self, body: &Expression) -> Result<(), RuntimeError> {
-        let incremented_params = self.increment_parameter_rcs();
         let return_value = self.codegen_expression(body)?;
         let results = return_value.into_value_list(self);
-        self.end_scope(incremented_params, &results);
 
         self.builder.terminate_with_return(results);
         Ok(())
@@ -172,6 +170,8 @@
             Expression::Semi(semi) => self.codegen_semi(semi),
             Expression::Break => Ok(self.codegen_break()),
             Expression::Continue => Ok(self.codegen_continue()),
+            Expression::Clone(expr) => self.codegen_clone(expr),
+            Expression::Drop(expr) => self.codegen_drop(expr),
         }
     }
 
@@ -277,17 +277,13 @@
     fn codegen_array_elements(
         &mut self,
         elements: &[Expression],
-    ) -> Result<Vec<(Values, bool)>, RuntimeError> {
-        try_vecmap(elements, |element| {
-            let value = self.codegen_expression(element)?;
-            Ok((value, element.is_array_or_slice_literal()))
-        })
+    ) -> Result<Vec<Values>, RuntimeError> {
+        try_vecmap(elements, |element| self.codegen_expression(element))
     }
 
     fn codegen_string(&mut self, string: &str) -> Values {
         let elements = vecmap(string.as_bytes(), |byte| {
-            let char = self.builder.numeric_constant(*byte as u128, NumericType::char());
-            (char.into(), false)
+            self.builder.numeric_constant(*byte as u128, NumericType::char()).into()
         });
         let typ = Self::convert_non_tuple_type(&ast::Type::String(elements.len() as u32));
         self.codegen_array(elements, typ)
@@ -300,7 +296,7 @@
     /// constant to be moved into this larger array constant.
     fn codegen_array_checked(
         &mut self,
-        elements: Vec<(Values, bool)>,
+        elements: Vec<Values>,
         typ: Type,
     ) -> Result<Values, RuntimeError> {
         if typ.is_nested_slice() {
@@ -322,22 +318,12 @@
     /// constant to be moved into this larger array constant.
     ///
     /// The value returned from this function is always that of the allocate instruction.
-    fn codegen_array(&mut self, elements: Vec<(Values, bool)>, typ: Type) -> Values {
+    fn codegen_array(&mut self, elements: Vec<Values>, typ: Type) -> Values {
         let mut array = im::Vector::new();
 
-        for (element, is_array_constant) in elements {
+        for element in elements {
             element.for_each(|element| {
                 let element = element.eval(self);
-
-                // If we're referencing a sub-array in a larger nested array we need to
-                // increase the reference count of the sub array. This maintains a
-                // pessimistic reference count (since some are likely moved rather than shared)
-                // which is important for Brillig's copy on write optimization. This has no
-                // effect in ACIR code.
-                if !is_array_constant {
-                    self.builder.increment_array_reference_count(element);
-                }
-
                 array.push_back(element);
             });
         }
@@ -486,7 +472,6 @@
             // counts when nested arrays/slices are constructed or indexed. This
             // has no effect in ACIR code.
             let result = self.builder.insert_array_get(array, offset, typ);
-            self.builder.increment_array_reference_count(result);
             result.into()
         }))
     }
@@ -528,7 +513,7 @@
     ///   br loop_entry(v0)
     /// loop_entry(i: Field):
     ///   v2 = lt i v1
     ///   brif v2, then: loop_body, else: loop_end
     /// loop_body():
     ///   v3 = ... codegen body ...
     ///   v4 = add 1, i
@@ -672,7 +657,7 @@
     ///
     /// ```text
     ///   v0 = ... codegen cond ...
     ///   brif v0, then: then_block, else: else_block
     /// then_block():
     ///   v1 = ... codegen a ...
     ///   br end_if(v1)
@@ -687,7 +672,7 @@
     ///
     /// ```text
     ///   v0 = ... codegen cond ...
     ///   brif v0, then: then_block, else: end_if
     /// then_block:
     ///   v1 = ... codegen a ...
     ///   br end_if()
@@ -1029,22 +1014,12 @@
     fn codegen_let(&mut self, let_expr: &ast::Let) -> Result<Values, RuntimeError> {
         let mut values = self.codegen_expression(&let_expr.expression)?;
 
-        // Don't mutate the reference count if we're assigning an array literal to a Let:
-        // `let mut foo = [1, 2, 3];`
-        // we consider the array to be moved, so we should have an initial rc of just 1.
-        let should_inc_rc = !let_expr.expression.is_array_or_slice_literal();
-
         values = values.map(|value| {
             let value = value.eval(self);
 
             Tree::Leaf(if let_expr.mutable {
-                self.new_mutable_variable(value, should_inc_rc)
+                self.new_mutable_variable(value)
             } else {
-                // `new_mutable_variable` increments rcs internally so we have to
-                // handle it separately for the immutable case
-                if should_inc_rc {
-                    self.builder.increment_array_reference_count(value);
-                }
                 value::Value::Normal(value)
             })
         });
@@ -1103,15 +1078,6 @@
     fn codegen_assign(&mut self, assign: &ast::Assign) -> Result<Values, RuntimeError> {
         let lhs = self.extract_current_value(&assign.lvalue)?;
         let rhs = self.codegen_expression(&assign.expression)?;
-        let should_inc_rc = !assign.expression.is_array_or_slice_literal();
-
-        rhs.clone().for_each(|value| {
-            let value = value.eval(self);
-
-            if should_inc_rc {
-                self.builder.increment_array_reference_count(value);
-            }
-        });
 
         self.assign_new_value(lhs, rhs);
         Ok(Self::unit_value())
@@ -1140,4 +1106,26 @@
         }
         Self::unit_value()
     }
+
+    /// Evaluate the given expression, increment the reference count of each array within,
+    /// and return the evaluated expression.
+    fn codegen_clone(&mut self, expr: &Expression) -> Result<Values, RuntimeError> {
+        let values = self.codegen_expression(expr)?;
+        Ok(values.map(|value| {
+            let value = value.eval(self);
+            self.builder.increment_array_reference_count(value);
+            Values::from(value)
+        }))
+    }
+
+    /// Evaluate the given expression, decrement the reference count of each array within,
+    /// and return unit.
+    fn codegen_drop(&mut self, expr: &Expression) -> Result<Values, RuntimeError> {
+        let values = self.codegen_expression(expr)?;
+        values.for_each(|value| {
+            let value = value.eval(self);
+            self.builder.decrement_array_reference_count(value);
+        });
+        Ok(Self::unit_value())
+    }
 }

compiler/noirc_frontend/src/lib.rs

@@ -22,6 +22,7 @@ pub mod lexer;
 pub mod locations;
 pub mod monomorphization;
 pub mod node_interner;
+pub(crate) mod ownership;
 pub mod parser;
 pub mod resolve_locations;
 pub mod shared;