fix(brillig): Skip decrementing ref-count in array/vector copy and other refactors

compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_call/brillig_slice_ops.rs

@@ -10,6 +10,8 @@ use crate::brillig::brillig_ir::{
 use super::super::brillig_block::BrilligBlock;
 
 impl<Registers: RegisterAllocator> BrilligBlock<'_, Registers> {
+    /// Take a list of [BrilligVariable] and copy the memory they point at to the `write_pointer`,
+    /// increasing the address by 1 between each variable.
     fn write_variables(&mut self, write_pointer: MemoryAddress, variables: &[BrilligVariable]) {
         for (index, variable) in variables.iter().enumerate() {
             self.brillig_context.store_instruction(write_pointer, variable.extract_register());
@@ -23,6 +25,8 @@ impl<Registers: RegisterAllocator> BrilligBlock<'_, Registers> {
         }
     }
 
+    /// Prepare a vector for pushing a number of new (flattened) items to the back,
+    /// then write those variables to the returned write pointer.
     pub(crate) fn slice_push_back_operation(
         &mut self,
         target_vector: BrilligVector,
@@ -43,6 +47,8 @@ impl<Registers: RegisterAllocator> BrilligBlock<'_, Registers> {
         self.write_variables(*write_pointer, variables_to_insert);
     }
 
+    /// Prepare a vector for pushing a number of new (flattened) items to the front,
+    /// then write those variables to the returned write pointer.
     pub(crate) fn slice_push_front_operation(
         &mut self,
         target_vector: BrilligVector,
@@ -97,6 +103,8 @@ impl<Registers: RegisterAllocator> BrilligBlock<'_, Registers> {
         );
     }
 
+    /// Create new vector with a number of (flattened) items popped from the back,
+    /// then read the popped items into the variables representing the removed items.
     pub(crate) fn slice_pop_back_operation(
         &mut self,
         target_vector: BrilligVector,
@@ -116,6 +124,8 @@ impl<Registers: RegisterAllocator> BrilligBlock<'_, Registers> {
         self.read_variables(*read_pointer, removed_items);
     }
 
+    /// Prepare a vector for inserting a number of (flattened) items at a specific index
+    /// by making a hole for them, then write the variables to the returned write pointer.
     pub(crate) fn slice_insert_operation(
         &mut self,
         target_vector: BrilligVector,
@@ -136,6 +146,9 @@ impl<Registers: RegisterAllocator> BrilligBlock<'_, Registers> {
         self.write_variables(*write_pointer, items);
     }
 
+    /// Read a number of (flattened) items at a specific index of a vector into the variables
+    /// representing the removed items, then create a new vector with the same number of
+    /// items removed and subsequent items shifted to the left.
     pub(crate) fn slice_remove_operation(
         &mut self,
         target_vector: BrilligVector,

compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_instructions/brillig_memory.rs

@@ -288,7 +288,7 @@ impl<Registers: RegisterAllocator> BrilligBlock<'_, Registers> {
     /// have been written incorrectly.
     ///
     /// Should only be called if [BrilligContext::enable_debug_assertions] returns true.
-    fn assert_rc_neq_zero(&mut self, rc_register: MemoryAddress) {
+    fn codegen_assert_rc_neq_zero(&mut self, rc_register: MemoryAddress) {
         let zero = self.brillig_context.allocate_single_addr(32);
 
         self.brillig_context.const_instruction(*zero, FieldElement::zero());
@@ -462,34 +462,30 @@ impl<Registers: RegisterAllocator> BrilligBlock<'_, Registers> {
 
     pub(crate) fn codegen_increment_rc(&mut self, value: ValueId, dfg: &DataFlowGraph) {
         let array_or_vector = self.convert_ssa_value(value, dfg);
-        let rc_register = self.brillig_context.allocate_register();
+        let array_register = array_or_vector.extract_register();
 
-        // RC is always directly pointed by the array/vector pointer
-        self.brillig_context.load_instruction(*rc_register, array_or_vector.extract_register());
+        let rc_register = self.brillig_context.codegen_read_rc(array_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.codegen_assert_rc_neq_zero(rc_register.address);
         }
 
-        self.brillig_context.codegen_usize_op_in_place(*rc_register, BrilligBinaryOp::Add, 1);
-        self.brillig_context.store_instruction(array_or_vector.extract_register(), *rc_register);
+        self.brillig_context.codegen_increment_rc(array_register, rc_register.address);
     }
     pub(crate) fn codegen_decrement_rc(&mut self, value: ValueId, dfg: &DataFlowGraph) {
         let array_or_vector = self.convert_ssa_value(value, dfg);
         let array_register = array_or_vector.extract_register();
 
-        let rc_register = self.brillig_context.allocate_register();
-        self.brillig_context.load_instruction(*rc_register, array_register);
+        let rc_register = self.brillig_context.codegen_read_rc(array_register);
 
         // 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() {
-            self.assert_rc_neq_zero(*rc_register);
+            self.codegen_assert_rc_neq_zero(rc_register.address);
         }
 
-        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.codegen_decrement_rc(array_register, rc_register.address);
     }
 }

compiler/noirc_evaluator/src/brillig/brillig_gen/tests/call.rs

@@ -81,12 +81,11 @@ fn brillig_to_bits() {
      7: sp[5] = u32 add sp[2], @2
      8: sp[6] = const u32 8
      9: to_radix(input: sp[1], radix: sp[3], num_limbs: sp[6], output_pointer: sp[5], output_bits: sp[4])
-    10: sp[7] = const u32 8
-    11: @3 = sp[5]
-    12: @4 = sp[7]
-    13: call 0
-    14: sp[1] = sp[2]
-    15: return
+    10: @3 = sp[5]
+    11: @4 = sp[6]
+    12: call 0
+    13: sp[1] = sp[2]
+    14: return
     ");
 }
 
@@ -114,12 +113,11 @@ fn brillig_to_radix() {
      6: sp[5] = u32 add sp[3], @2
      7: sp[6] = const u32 8
      8: to_radix(input: sp[1], radix: sp[2], num_limbs: sp[6], output_pointer: sp[5], output_bits: sp[4])
-     9: sp[7] = const u32 8
-    10: @3 = sp[5]
-    11: @4 = sp[7]
-    12: call 0
-    13: sp[1] = sp[3]
-    14: return
+     9: @3 = sp[5]
+    10: @4 = sp[6]
+    11: call 0
+    12: sp[1] = sp[3]
+    13: return
     ");
 }
 

compiler/noirc_evaluator/src/brillig/brillig_gen/tests/memory.rs

@@ -136,11 +136,9 @@ fn brillig_array_with_rc_ops() {
     24: sp[5] = u32 add sp[3], sp[1]
     25: store sp[2] at sp[5]
     26: sp[2] = load sp[4]
-    27: sp[2] = u32 sub sp[2], @2
-    28: store sp[2] at sp[4]
-    29: sp[4] = u32 add sp[3], sp[1]
-    30: sp[2] = load sp[4]
-    31: sp[1] = sp[2]
-    32: return
+    27: sp[4] = u32 add sp[3], sp[1]
+    28: sp[2] = load sp[4]
+    29: sp[1] = sp[2]
+    30: return
     ");
 }

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

@@ -1,5 +1,7 @@
 use acvm::{AcirField, acir::brillig::MemoryAddress};
 
+use crate::brillig::brillig_ir::{brillig_variable::SingleAddrVariable, registers::Allocated};
+
 use super::{
     BrilligContext, ReservedRegisters, debug_show::DebugToString, instructions::BrilligBinaryOp,
     registers::RegisterAllocator,
@@ -36,4 +38,14 @@ impl<F: AcirField + DebugToString, Registers: RegisterAllocator> BrilligContext<
         let array_copy_counter = self.array_copy_counter_address();
         self.codegen_usize_op(array_copy_counter, array_copy_counter, BrilligBinaryOp::Add, 1);
     }
+
+    /// Utility method to check if the value at a memory address equals one.
+    pub(crate) fn codegen_usize_equals_one(
+        &mut self,
+        operand: SingleAddrVariable,
+    ) -> Allocated<SingleAddrVariable, Registers> {
+        let is_one = self.allocate_single_addr_bool();
+        self.codegen_usize_op(operand.address, is_one.address, BrilligBinaryOp::Equals, 1);
+        is_one
+    }
 }

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

@@ -59,15 +59,18 @@ impl<F: AcirField + DebugToString, Registers: RegisterAllocator> BrilligContext<
         );
     }
 
-    /// Issues a to_radix instruction. This instruction will write the modulus of the source register
-    /// And the radix register limb_count times to the target vector.
+    /// Issues a `to_radix` instruction. This instruction will write the modulus of the `source_field` register
+    /// and the `radix` register `target_array`, with the number of limbs given by the size of `target_array`.
+    ///
+    /// If `output_bits` is true, it generates bit limbs, otherwise it generates byte limbs.
+    /// If `little_endian` is true, then the `target_array` will contain the results in Little Endian order.
     pub(crate) fn codegen_to_radix(
         &mut self,
         source_field: SingleAddrVariable,
         target_array: BrilligArray,
         radix: SingleAddrVariable,
         little_endian: bool,
-        output_bits: bool, // If true will generate bit limbs, if false will generate byte limbs
+        output_bits: bool,
     ) {
         assert!(source_field.bit_size == F::max_num_bits());
         assert!(radix.bit_size == 32);
@@ -87,8 +90,7 @@ impl<F: AcirField + DebugToString, Registers: RegisterAllocator> BrilligContext<
         });
 
         if little_endian {
-            let items_len = self.make_usize_constant_instruction(target_array.size.into());
-            self.codegen_array_reverse(*pointer, items_len.address);
+            self.codegen_array_reverse(*pointer, num_limbs.address);
         }
     }
 }