feat: Add checks for bit size consistency on brillig gen (#4542)

# Description ## Problem\* Adds checks for brillig codegen for #4369 (Previously we only had consistency checks in SSA). ## Summary\* This checks have caught some inconsistencies in the process, including those that were caught manually in AztecProtocol/aztec-packages#5091 ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings.
noir-lang · Mar 13, 2024 · f3243b7 · f3243b7
1 parent 00d6494
commit f3243b7
Show file tree

Hide file tree

Showing 9 changed files with 368 additions and 305 deletions.
diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs
@@ -3,7 +3,7 @@ use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet};
 use crate::{
     brillig::brillig_ir::{
         brillig_variable::{BrilligArray, BrilligVariable, BrilligVector, SingleAddrVariable},
-        BrilligContext, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE,
+        BrilligContext,
     },
     ssa::ir::{
         basic_block::BasicBlockId,
@@ -13,7 +13,7 @@ use crate::{
     },
 };
 
-use super::brillig_fn::FunctionContext;
+use super::brillig_fn::{get_bit_size_from_ssa_type, FunctionContext};
 
 #[derive(Debug, Default)]
 pub(crate) struct BlockVariables {
@@ -189,21 +189,10 @@ pub(crate) fn allocate_value(
     let typ = dfg.type_of_value(value_id);
 
     match typ {
-        Type::Numeric(numeric_type) => BrilligVariable::SingleAddr(SingleAddrVariable {
-            address: brillig_context.allocate_register(),
-            bit_size: numeric_type.bit_size(),
-        }),
-        Type::Reference(_) => BrilligVariable::SingleAddr(SingleAddrVariable {
-            address: brillig_context.allocate_register(),
-            bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE,
-        }),
-        Type::Function => {
-            // NB. function references are converted to a constant when
-            // translating from SSA to Brillig (to allow for debugger
-            // instrumentation to work properly)
+        Type::Numeric(_) | Type::Reference(_) | Type::Function => {
             BrilligVariable::SingleAddr(SingleAddrVariable {
                 address: brillig_context.allocate_register(),
-                bit_size: 32,
+                bit_size: get_bit_size_from_ssa_type(&typ),
             })
         }
         Type::Array(item_typ, elem_count) => {

diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs
@@ -1,4 +1,3 @@
-use acvm::FieldElement;
 use iter_extended::vecmap;
 
 use crate::{
@@ -11,7 +10,7 @@ use crate::{
         basic_block::BasicBlockId,
         function::{Function, FunctionId},
         post_order::PostOrder,
-        types::{NumericType, Type},
+        types::Type,
         value::ValueId,
     },
 };
@@ -116,11 +115,12 @@ impl FunctionContext {
 
 pub(crate) fn get_bit_size_from_ssa_type(typ: &Type) -> u32 {
     match typ {
-        Type::Numeric(num_type) => match num_type {
-            NumericType::Signed { bit_size } | NumericType::Unsigned { bit_size } => *bit_size,
-            NumericType::NativeField => FieldElement::max_num_bits(),
-        },
+        Type::Numeric(num_type) => num_type.bit_size(),
         Type::Reference(_) => BRILLIG_MEMORY_ADDRESSING_BIT_SIZE,
-        _ => unreachable!("ICE bitwise not on a non numeric type"),
+        // NB. function references are converted to a constant when
+        // translating from SSA to Brillig (to allow for debugger
+        // instrumentation to work properly)
+        Type::Function => 32,
+        _ => unreachable!("ICE bit size not on a non numeric type"),
     }
 }
diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs
@@ -1,6 +1,8 @@
-use acvm::brillig_vm::brillig::{BinaryIntOp, MemoryAddress};
+use acvm::brillig_vm::brillig::BinaryIntOp;
 
-use crate::brillig::brillig_ir::brillig_variable::{BrilligVariable, BrilligVector};
+use crate::brillig::brillig_ir::brillig_variable::{
+    BrilligVariable, BrilligVector, SingleAddrVariable,
+};
 
 use super::brillig_block::BrilligBlock;
 
@@ -26,19 +28,19 @@ impl<'block> BrilligBlock<'block> {
         self.brillig_context.copy_array_instruction(
             source_vector.pointer,
             target_vector.pointer,
-            source_vector.size,
+            SingleAddrVariable::new_usize(source_vector.size),
         );
 
         for (index, variable) in variables_to_insert.iter().enumerate() {
             let target_index = self.brillig_context.make_usize_constant(index.into());
             self.brillig_context.memory_op(
-                target_index,
+                target_index.address,
                 source_vector.size,
-                target_index,
+                target_index.address,
                 BinaryIntOp::Add,
             );
             self.store_variable_in_array(target_vector.pointer, target_index, *variable);
-            self.brillig_context.deallocate_register(target_index);
+            self.brillig_context.deallocate_single_addr(target_index);
         }
     }
 
@@ -72,14 +74,14 @@ impl<'block> BrilligBlock<'block> {
         self.brillig_context.copy_array_instruction(
             source_vector.pointer,
             destination_copy_pointer,
-            source_vector.size,
+            SingleAddrVariable::new_usize(source_vector.size),
         );
 
         // Then we write the items to insert at the start
         for (index, variable) in variables_to_insert.iter().enumerate() {
             let target_index = self.brillig_context.make_usize_constant(index.into());
             self.store_variable_in_array(target_vector.pointer, target_index, *variable);
-            self.brillig_context.deallocate_register(target_index);
+            self.brillig_context.deallocate_single_addr(target_index);
         }
 
         self.brillig_context.deallocate_register(destination_copy_pointer);
@@ -115,13 +117,13 @@ impl<'block> BrilligBlock<'block> {
         self.brillig_context.copy_array_instruction(
             source_copy_pointer,
             target_vector.pointer,
-            target_vector.size,
+            SingleAddrVariable::new_usize(target_vector.size),
         );
 
         for (index, variable) in removed_items.iter().enumerate() {
             let target_index = self.brillig_context.make_usize_constant(index.into());
             self.retrieve_variable_from_array(source_vector.pointer, target_index, *variable);
-            self.brillig_context.deallocate_register(target_index);
+            self.brillig_context.deallocate_single_addr(target_index);
         }
 
         self.brillig_context.deallocate_register(source_copy_pointer);
@@ -148,27 +150,27 @@ impl<'block> BrilligBlock<'block> {
         self.brillig_context.copy_array_instruction(
             source_vector.pointer,
             target_vector.pointer,
-            target_vector.size,
+            SingleAddrVariable::new_usize(target_vector.size),
         );
 
         for (index, variable) in removed_items.iter().enumerate() {
             let target_index = self.brillig_context.make_usize_constant(index.into());
             self.brillig_context.memory_op(
-                target_index,
+                target_index.address,
                 target_vector.size,
-                target_index,
+                target_index.address,
                 BinaryIntOp::Add,
             );
             self.retrieve_variable_from_array(source_vector.pointer, target_index, *variable);
-            self.brillig_context.deallocate_register(target_index);
+            self.brillig_context.deallocate_single_addr(target_index);
         }
     }
 
     pub(crate) fn slice_insert_operation(
         &mut self,
         target_vector: BrilligVector,
         source_vector: BrilligVector,
-        index: MemoryAddress,
+        index: SingleAddrVariable,
         items: &[BrilligVariable],
     ) {
         // First we need to allocate the target vector incrementing the size by items.len()
@@ -193,7 +195,7 @@ impl<'block> BrilligBlock<'block> {
         let source_pointer_at_index = self.brillig_context.allocate_register();
         self.brillig_context.memory_op(
             source_vector.pointer,
-            index,
+            index.address,
             source_pointer_at_index,
             BinaryIntOp::Add,
         );
@@ -202,7 +204,7 @@ impl<'block> BrilligBlock<'block> {
         let target_pointer_after_index = self.brillig_context.allocate_register();
         self.brillig_context.memory_op(
             target_vector.pointer,
-            index,
+            index.address,
             target_pointer_after_index,
             BinaryIntOp::Add,
         );
@@ -214,21 +216,31 @@ impl<'block> BrilligBlock<'block> {
 
         // Compute the number of elements to the right of the index
         let item_count = self.brillig_context.allocate_register();
-        self.brillig_context.memory_op(source_vector.size, index, item_count, BinaryIntOp::Sub);
+        self.brillig_context.memory_op(
+            source_vector.size,
+            index.address,
+            item_count,
+            BinaryIntOp::Sub,
+        );
 
         // Copy the elements to the right of the index
         self.brillig_context.copy_array_instruction(
             source_pointer_at_index,
             target_pointer_after_index,
-            item_count,
+            SingleAddrVariable::new_usize(item_count),
         );
 
         // Write the items to insert starting at the index
         for (subitem_index, variable) in items.iter().enumerate() {
             let target_index = self.brillig_context.make_usize_constant(subitem_index.into());
-            self.brillig_context.memory_op(target_index, index, target_index, BinaryIntOp::Add);
+            self.brillig_context.memory_op(
+                target_index.address,
+                index.address,
+                target_index.address,
+                BinaryIntOp::Add,
+            );
             self.store_variable_in_array(target_vector.pointer, target_index, *variable);
-            self.brillig_context.deallocate_register(target_index);
+            self.brillig_context.deallocate_single_addr(target_index);
         }
 
         self.brillig_context.deallocate_register(source_pointer_at_index);
@@ -240,7 +252,7 @@ impl<'block> BrilligBlock<'block> {
         &mut self,
         target_vector: BrilligVector,
         source_vector: BrilligVector,
-        index: MemoryAddress,
+        index: SingleAddrVariable,
         removed_items: &[BrilligVariable],
     ) {
         // First we need to allocate the target vector decrementing the size by removed_items.len()
@@ -265,7 +277,7 @@ impl<'block> BrilligBlock<'block> {
         let source_pointer_after_index = self.brillig_context.allocate_register();
         self.brillig_context.memory_op(
             source_vector.pointer,
-            index,
+            index.address,
             source_pointer_after_index,
             BinaryIntOp::Add,
         );
@@ -279,29 +291,39 @@ impl<'block> BrilligBlock<'block> {
         let target_pointer_at_index = self.brillig_context.allocate_register();
         self.brillig_context.memory_op(
             target_vector.pointer,
-            index,
+            index.address,
             target_pointer_at_index,
             BinaryIntOp::Add,
         );
 
         // Compute the number of elements to the right of the index
         let item_count = self.brillig_context.allocate_register();
-        self.brillig_context.memory_op(source_vector.size, index, item_count, BinaryIntOp::Sub);
+        self.brillig_context.memory_op(
+            source_vector.size,
+            index.address,
+            item_count,
+            BinaryIntOp::Sub,
+        );
         self.brillig_context.usize_op_in_place(item_count, BinaryIntOp::Sub, removed_items.len());
 
         // Copy the elements to the right of the index
         self.brillig_context.copy_array_instruction(
             source_pointer_after_index,
             target_pointer_at_index,
-            item_count,
+            SingleAddrVariable::new_usize(item_count),
         );
 
         // Get the removed items
         for (subitem_index, variable) in removed_items.iter().enumerate() {
             let target_index = self.brillig_context.make_usize_constant(subitem_index.into());
-            self.brillig_context.memory_op(target_index, index, target_index, BinaryIntOp::Add);
+            self.brillig_context.memory_op(
+                target_index.address,
+                index.address,
+                target_index.address,
+                BinaryIntOp::Add,
+            );
             self.retrieve_variable_from_array(source_vector.pointer, target_index, *variable);
-            self.brillig_context.deallocate_register(target_index);
+            self.brillig_context.deallocate_single_addr(target_index);
         }
 
         self.brillig_context.deallocate_register(source_pointer_after_index);
@@ -592,7 +614,10 @@ mod tests {
                 address: context.allocate_register(),
                 bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE,
             };
-            let index_to_insert = context.allocate_register();
+            let index_to_insert = SingleAddrVariable::new(
+                context.allocate_register(),
+                BRILLIG_MEMORY_ADDRESSING_BIT_SIZE,
+            );
 
             // Cast the source array to a vector
             let source_vector = context.array_to_vector(&array_variable);
@@ -710,7 +735,10 @@ mod tests {
                 size: array.len(),
                 rc: context.allocate_register(),
             };
-            let index_to_insert = context.allocate_register();
+            let index_to_insert = SingleAddrVariable::new(
+                context.allocate_register(),
+                BRILLIG_MEMORY_ADDRESSING_BIT_SIZE,
+            );
 
             // Cast the source array to a vector
             let source_vector = context.array_to_vector(&array_variable);