fix: Experiment with DecrementRc

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

@@ -20,7 +20,6 @@ use crate::ssa::ir::{
     value::{Value, ValueId},
 };
 use acvm::acir::brillig::{MemoryAddress, ValueOrArray};
-use acvm::brillig_vm::MEMORY_ADDRESSING_BIT_SIZE;
 use acvm::{FieldElement, acir::AcirField};
 use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet};
 use iter_extended::vecmap;
@@ -872,65 +871,40 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> {
                     .store_instruction(array_or_vector.extract_register(), rc_register);
                 self.brillig_context.deallocate_register(rc_register);
             }
-            Instruction::DecrementRc { value, original } => {
+            Instruction::DecrementRc { value } => {
                 let array_or_vector = self.convert_ssa_value(*value, dfg);
-                let orig_array_or_vector = self.convert_ssa_value(*original, dfg);
-
                 let array_register = array_or_vector.extract_register();
-                let orig_array_register = orig_array_or_vector.extract_register();
-
-                let codegen_dec_rc = |ctx: &mut BrilligContext<FieldElement, Registers>| {
-                    let rc_register = ctx.allocate_register();
-
-                    ctx.load_instruction(rc_register, array_register);
-
-                    ctx.codegen_usize_op_in_place(rc_register, BrilligBinaryOp::Sub, 1);
-
-                    // Check that the refcount didn't go below 1. If we allow it to underflow
-                    // and become 0 or usize::MAX, and then return to 1, then it will indicate
-                    // an array as mutable when it probably shouldn't be.
-                    if ctx.enable_debug_assertions() {
-                        let min_addr = ReservedRegisters::usize_one();
-                        let condition = SingleAddrVariable::new(ctx.allocate_register(), 1);
-                        ctx.memory_op_instruction(
-                            min_addr,
-                            rc_register,
-                            condition.address,
-                            BrilligBinaryOp::LessThanEquals,
-                        );
-                        ctx.codegen_constrain(
-                            condition,
-                            Some("array ref-count went to zero".to_owned()),
-                        );
-                        ctx.deallocate_single_addr(condition);
-                    }
 
-                    ctx.store_instruction(array_register, rc_register);
-                    ctx.deallocate_register(rc_register);
-                };
+                let rc_register = self.brillig_context.allocate_register();
+                self.brillig_context.load_instruction(rc_register, array_register);
 
-                if array_register == orig_array_register {
-                    codegen_dec_rc(self.brillig_context);
-                } else {
-                    // Check if the current and original register point at the same memory.
-                    // If they don't, it means that an array-copy procedure has created a
-                    // new array, which includes decrementing the RC of the original,
-                    // which means we don't have to do the decrement here.
+                // Check that the refcount isn't already 0 before we decrement. If we allow it to underflow
+                // 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);
-                    // We will use a binary op to interpret the pointer as a number.
-                    let bit_size: u32 = MEMORY_ADDRESSING_BIT_SIZE.into();
-                    let array_var = SingleAddrVariable::new(array_register, bit_size);
-                    let orig_array_var = SingleAddrVariable::new(orig_array_register, bit_size);
-                    self.brillig_context.binary_instruction(
-                        array_var,
-                        orig_array_var,
+                    self.brillig_context.memory_op_instruction(
+                        min_addr,
+                        rc_register,
+                        condition.address,
+                        BrilligBinaryOp::LessThan,
+                    );
+                    self.brillig_context.codegen_constrain(
                         condition,
-                        BrilligBinaryOp::Equals,
+                        Some("array ref-count underflow detected".to_owned()),
                     );
-                    self.brillig_context.codegen_if(condition.address, codegen_dec_rc);
                     self.brillig_context.deallocate_single_addr(condition);
                 }
+
+                self.brillig_context.codegen_usize_op_in_place(
+                    rc_register,
+                    BrilligBinaryOp::Sub,
+                    1,
+                );
+                self.brillig_context.store_instruction(array_register, rc_register);
+                self.brillig_context.deallocate_register(rc_register);
             }
             Instruction::EnableSideEffectsIf { .. } => {
                 unreachable!("enable_side_effects not supported by brillig")

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

@@ -1056,8 +1056,8 @@ mod test {
             v19 = call f1(v5) -> u32
             v20 = add v8, v19
             constrain v6 == v20
-            dec_rc v4 v4
-            dec_rc v5 v5
+            dec_rc v4
+            dec_rc v5
             return
         }
 

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

@@ -394,8 +394,8 @@ impl FunctionBuilder {
 
     /// Insert an instruction to decrement an array's reference count. This only has an effect
     /// in unconstrained code where arrays are reference counted and copy on write.
-    pub(crate) fn insert_dec_rc(&mut self, value: ValueId, original: ValueId) {
-        self.insert_instruction(Instruction::DecrementRc { value, original }, None);
+    pub(crate) fn insert_dec_rc(&mut self, value: ValueId) {
+        self.insert_instruction(Instruction::DecrementRc { value }, None);
     }
 
     /// Insert an enable_side_effects_if instruction. These are normally only automatically
@@ -497,20 +497,16 @@ impl FunctionBuilder {
     ///
     /// Returns the ID of the array that was affected, if any.
     pub(crate) fn increment_array_reference_count(&mut self, value: ValueId) -> Option<ValueId> {
-        self.update_array_reference_count(value, None)
+        self.update_array_reference_count(value, true)
     }
 
     /// Insert instructions to decrement the reference count of any array(s) stored
     /// within the given value. If the given value is not an array and does not contain
     /// any arrays, this does nothing.
     ///
     /// Returns the ID of the array that was affected, if any.
-    pub(crate) fn decrement_array_reference_count(
-        &mut self,
-        value: ValueId,
-        original: ValueId,
-    ) -> Option<ValueId> {
-        self.update_array_reference_count(value, Some(original))
+    pub(crate) fn decrement_array_reference_count(&mut self, value: ValueId) -> Option<ValueId> {
+        self.update_array_reference_count(value, false)
     }
 
     /// Increment or decrement the given value's reference count if it is an array.
@@ -522,11 +518,7 @@ impl FunctionBuilder {
     /// the meantime, which in itself would make sure its RC is decremented.
     ///
     /// Returns the ID of the array that was affected, if any.
-    fn update_array_reference_count(
-        &mut self,
-        value: ValueId,
-        original: Option<ValueId>,
-    ) -> Option<ValueId> {
+    fn update_array_reference_count(&mut self, value: ValueId, increment: bool) -> Option<ValueId> {
         if self.current_function.runtime().is_acir() {
             return None;
         }
@@ -536,7 +528,7 @@ impl FunctionBuilder {
                 if element.contains_an_array() {
                     let reference = value;
                     let value = self.insert_load(reference, element.as_ref().clone());
-                    self.update_array_reference_count(value, original);
+                    self.update_array_reference_count(value, increment);
                     Some(value)
                 } else {
                     None
@@ -545,10 +537,10 @@ impl FunctionBuilder {
             Type::Array(..) | Type::Slice(..) => {
                 // If there are nested arrays or slices, we wait until ArrayGet
                 // is issued to increment the count of that array.
-                if let Some(original) = original {
-                    self.insert_dec_rc(value, original);
-                } else {
+                if increment {
                     self.insert_inc_rc(value);
+                } else {
+                    self.insert_dec_rc(value);
                 }
                 Some(value)
             }

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

@@ -325,9 +325,7 @@
     /// This currently only has an effect in Brillig code where array sharing and copy on write is
     /// implemented via reference counting. In ACIR code this is done with im::Vector and these
     /// DecrementRc instructions are ignored.
-    ///
-    /// The `original` contains the value of the array which was incremented by the pair of this decrement.
-    DecrementRc { value: ValueId, original: ValueId },
+    DecrementRc { value: ValueId },
 
     /// Merge two values returned from opposite branches of a conditional into one.
     ///
@@ -426,7 +424,7 @@
             // These can fail.
             Constrain(..) | ConstrainNotEqual(..) | RangeCheck { .. } => true,
 
             // This should never be side-effectful
             MakeArray { .. } | Noop => false,
 
             // Some binary math can overflow or underflow
@@ -724,9 +722,7 @@
                 mutable: *mutable,
             },
             Instruction::IncrementRc { value } => Instruction::IncrementRc { value: f(*value) },
-            Instruction::DecrementRc { value, original } => {
-                Instruction::DecrementRc { value: f(*value), original: f(*original) }
-            }
+            Instruction::DecrementRc { value } => Instruction::DecrementRc { value: f(*value) },
             Instruction::RangeCheck { value, max_bit_size, assert_message } => {
                 Instruction::RangeCheck {
                     value: f(*value),
@@ -797,9 +793,8 @@
                 *value = f(*value);
             }
             Instruction::IncrementRc { value } => *value = f(*value),
-            Instruction::DecrementRc { value, original } => {
+            Instruction::DecrementRc { value } => {
                 *value = f(*value);
-                *original = f(*original);
             }
             Instruction::RangeCheck { value, max_bit_size: _, assert_message: _ } => {
                 *value = f(*value);
@@ -866,12 +861,10 @@
             Instruction::EnableSideEffectsIf { condition } => {
                 f(*condition);
             }
-            Instruction::IncrementRc { value } | Instruction::RangeCheck { value, .. } => {
-                f(*value);
-            }
-            Instruction::DecrementRc { value, original } => {
+            Instruction::IncrementRc { value }
+            | Instruction::DecrementRc { value }
+            | Instruction::RangeCheck { value, .. } => {
                 f(*value);
-                f(*original);
             }
             Instruction::IfElse { then_condition, then_value, else_condition, else_value } => {
                 f(*then_condition);

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

@@ -250,8 +250,8 @@ fn display_instruction_inner(
         Instruction::IncrementRc { value } => {
             writeln!(f, "inc_rc {}", show(*value))
         }
-        Instruction::DecrementRc { value, original } => {
-            writeln!(f, "dec_rc {} {}", show(*value), show(*original))
+        Instruction::DecrementRc { value } => {
+            writeln!(f, "dec_rc {}", show(*value))
         }
         Instruction::RangeCheck { value, max_bit_size, .. } => {
             writeln!(f, "range_check {} to {} bits", show(*value), *max_bit_size,)

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

@@ -1445,7 +1445,7 @@ mod test {
                 v2 = array_get v0, index u32 0 -> Field
                 v4 = array_get v0, index u32 1 -> Field
                 v5 = add v2, v4
-                dec_rc v0 v0
+                dec_rc v0
                 return v5
             }
             ";

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

@@ -838,7 +838,7 @@ mod test {
               b0(v0: [Field; 2]):
                 inc_rc v0
                 v2 = array_get v0, index u32 0 -> Field
-                dec_rc v0 v0
+                dec_rc v0
                 return v2
             }
             ";
@@ -862,7 +862,7 @@ mod test {
               b0(v0: [Field; 2]):
                 inc_rc v0
                 v2 = array_set v0, index u32 0, value u32 0
-                dec_rc v0 v0
+                dec_rc v0
                 return v2
             }
             ";
@@ -970,7 +970,7 @@ mod test {
                 v3 = load v0 -> [Field; 3]
                 v6 = array_set v3, index u32 0, value Field 5
                 store v6 at v0
-                dec_rc v6 v1
+                dec_rc v6
                 return
             }
             ";

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

@@ -190,7 +190,7 @@ mod test {
           b0(v0: [Field; 2]):
             inc_rc v0
             inc_rc v0
-            dec_rc v0 v0
+            dec_rc v0
             v1 = make_array [v0] : [[Field; 2]; 1]
             return v1
         }
@@ -218,7 +218,7 @@ mod test {
             v2 = load v1 -> [Field; 2]
             v5 = array_set v2, index u64 0, value Field 5
             store v5 at v1
-            dec_rc v0 v0
+            dec_rc v0
             return
         }
         ";
@@ -250,7 +250,7 @@ mod test {
             v5 = array_set v2, index u64 0, value Field 5
             store v5 at v0
             v6 = load v0 -> [Field; 2]
-            dec_rc v6 v1
+            dec_rc v1
             store v6 at v0
             return
         }

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

@@ -1287,7 +1287,7 @@ mod tests {
             jmp b1()
           b1():
             v20 = load v3 -> [u64; 6]
-            dec_rc v0 v0
+            dec_rc v0
             return v20
         }
         ";
@@ -1463,7 +1463,7 @@ mod tests {
             jmp b1(v16)
           b2():
             v8 = load v4 -> [u64; 6]
-            dec_rc v0 v0
+            dec_rc v0
             return v8
         }}
         "

compiler/noirc_evaluator/src/ssa/parser/ast.rs

@@ -116,7 +116,6 @@ pub(crate) enum ParsedInstruction {
     },
     DecrementRc {
         value: ParsedValue,
-        original: ParsedValue,
     },
     EnableSideEffectsIf {
         condition: ParsedValue,

compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs

@@ -284,10 +284,9 @@ impl Translator {
                 };
                 self.builder.insert_constrain(lhs, rhs, assert_message);
             }
-            ParsedInstruction::DecrementRc { value, original } => {
+            ParsedInstruction::DecrementRc { value } => {
                 let value = self.translate_value(value)?;
-                let original = self.translate_value(original)?;
-                self.builder.decrement_array_reference_count(value, original);
+                self.builder.decrement_array_reference_count(value);
             }
             ParsedInstruction::EnableSideEffectsIf { condition } => {
                 let condition = self.translate_value(condition)?;

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

@@ -407,8 +407,7 @@ impl<'a> Parser<'a> {
         }
 
         let value = self.parse_value_or_error()?;
-        let original = self.parse_value_or_error()?;
-        Ok(Some(ParsedInstruction::DecrementRc { value, original }))
+        Ok(Some(ParsedInstruction::DecrementRc { value }))
     }
 
     fn parse_enable_side_effects(&mut self) -> ParseResult<Option<ParsedInstruction>> {

compiler/noirc_evaluator/src/ssa/parser/tests.rs

@@ -474,7 +474,7 @@ fn test_dec_rc() {
     let src = "
         brillig(inline) fn main f0 {
           b0(v0: [Field; 3]):
-            dec_rc v0 v0
+            dec_rc v0
             return
         }
         ";

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

@@ -1001,11 +1001,15 @@ impl<'a> FunctionContext<'a> {
         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(parameter, original);
+                self.builder.decrement_array_reference_count(original);
             }
         }
     }

test_programs/execution_success/reference_counts/Nargo.toml → test_programs/execution_success/reference_counts_inliner_0/Nargo.toml

@@ -1,5 +1,5 @@
 [package]
-name = "reference_counts"
+name = "reference_counts_inliner_0"
 type = "bin"
 authors = [""]
 compiler_version = ">=0.35.0"