Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Mar 13, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6b8ed27
wip: refactor to ensure matching bit sizes in binary ops
sirasistant Mar 1, 2024
3361a26
Merge branch 'master' into arv/consistent_binary_ops
sirasistant Mar 11, 2024
fb55e2a
feat: more compile time checks for bit sizes
sirasistant Mar 12, 2024
0cadaa4
Merge branch 'master' into arv/consistent_binary_ops
sirasistant Mar 12, 2024
89dbc09
refactor
sirasistant Mar 12, 2024
28c5bdc
refactor to custom usize var constructor
sirasistant Mar 12, 2024
096ead8
chore: remove some todos
sirasistant Mar 13, 2024
13fbea3
refactor deallocation
sirasistant Mar 13, 2024
29606ea
Merge branch 'master' into arv/consistent_binary_ops
sirasistant Mar 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
209 changes: 100 additions & 109 deletions compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
View file
Open in desktop

Large diffs are not rendered by default.

19 changes: 4 additions & 15 deletions compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 {
Expand Down Expand Up @@ -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) => {
Expand Down
14 changes: 7 additions & 7 deletions compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use acvm::FieldElement;
use iter_extended::vecmap;

use crate::{
Expand All @@ -11,7 +10,7 @@ use crate::{
basic_block::BasicBlockId,
function::{Function, FunctionId},
post_order::PostOrder,
types::{NumericType, Type},
types::Type,
value::ValueId,
},
};
Expand Down Expand Up @@ -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"),
}
}
88 changes: 58 additions & 30 deletions compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;

Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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()
Expand All @@ -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,
);
Expand All @@ -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,
);
Expand All @@ -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);
Expand All @@ -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()
Expand All @@ -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,
);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
Loading
Loading