From c09e28a4f1f31ebf15a8a74bb96c94a2b515cf2e Mon Sep 17 00:00:00 2001 From: sirasistant Date: Fri, 14 Jul 2023 12:23:19 +0000 Subject: [PATCH 1/8] feat: get push/pop working --- .../brillig_slices/Nargo.toml | 5 + .../brillig_slices/Prover.toml | 2 + .../brillig_slices/src/main.nr | 11 + .../src/brillig/brillig_gen.rs | 3 +- .../src/brillig/brillig_gen/brillig_block.rs | 407 ++++++++++++------ .../src/brillig/brillig_gen/brillig_fn.rs | 143 +++++- .../brillig/brillig_gen/brillig_slice_ops.rs | 99 +++++ .../noirc_evaluator/src/brillig/brillig_ir.rs | 110 ++++- 8 files changed, 621 insertions(+), 159 deletions(-) create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/Nargo.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/Prover.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr create mode 100644 crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/Nargo.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/Nargo.toml new file mode 100644 index 00000000000..670888e37cd --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/Nargo.toml @@ -0,0 +1,5 @@ +[package] +authors = [""] +compiler_version = "0.6.0" + +[dependencies] \ No newline at end of file diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/Prover.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/Prover.toml new file mode 100644 index 00000000000..f28f2f8cc48 --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/Prover.toml @@ -0,0 +1,2 @@ +x = "5" +y = "10" diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr new file mode 100644 index 00000000000..4774d451b6f --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr @@ -0,0 +1,11 @@ +use dep::std::slice; +use dep::std; + +unconstrained fn main() { + let mut slice: [Field] = []; + assert(slice.len() == 0); + + slice = slice.push_back(1); + assert(slice.len() == 1); +} + diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen.rs b/crates/noirc_evaluator/src/brillig/brillig_gen.rs index 32c98cb86ff..5d7fbee1d4b 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen.rs @@ -2,6 +2,7 @@ pub(crate) mod brillig_black_box; pub(crate) mod brillig_block; pub(crate) mod brillig_directive; pub(crate) mod brillig_fn; +pub(crate) mod brillig_slice_ops; use crate::ssa_refactor::ir::{function::Function, post_order::PostOrder}; @@ -18,7 +19,7 @@ pub(crate) fn convert_ssa_function(func: &Function) -> BrilligArtifact { reverse_post_order.reverse(); let mut function_context = - FunctionContext { function_id: func.id(), ssa_value_to_register: HashMap::new() }; + FunctionContext { function_id: func.id(), ssa_value_to_brillig_variable: HashMap::new() }; let mut brillig_context = BrilligContext::new( FunctionContext::parameters(func), diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 7e8865328aa..5757fb2e600 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1,9 +1,11 @@ +use crate::brillig::brillig_gen::brillig_slice_ops::{ + convert_array_or_vector_to_vector, slice_push_back_operation, +}; use crate::brillig::brillig_ir::{ BrilligBinaryOp, BrilligContext, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, }; use crate::ssa_refactor::ir::function::FunctionId; use crate::ssa_refactor::ir::instruction::Intrinsic; -use crate::ssa_refactor::ir::types::CompositeType; use crate::ssa_refactor::ir::{ basic_block::{BasicBlock, BasicBlockId}, dfg::DataFlowGraph, @@ -17,7 +19,10 @@ use acvm::FieldElement; use iter_extended::vecmap; use super::brillig_black_box::convert_black_box_call; -use super::brillig_fn::FunctionContext; +use super::brillig_fn::{compute_size_of_composite_type, compute_size_of_type, FunctionContext}; +use super::brillig_slice_ops::{ + slice_pop_back_operation, slice_pop_front_operation, slice_push_front_operation, +}; /// Generate the compilation artifacts for compiling a function into brillig bytecode. pub(crate) struct BrilligBlock<'block> { @@ -100,7 +105,7 @@ impl<'block> BrilligBlock<'block> { ) { match terminator_instruction { TerminatorInstruction::JmpIf { condition, then_destination, else_destination } => { - let condition = self.convert_ssa_value(*condition, dfg); + let condition = self.convert_ssa_register_value(*condition, dfg); self.brillig_context.jump_if_instruction( condition, self.create_block_label_for_current_function(*then_destination), @@ -112,9 +117,14 @@ impl<'block> BrilligBlock<'block> { TerminatorInstruction::Jmp { destination, arguments } => { let target = &dfg[*destination]; for (src, dest) in arguments.iter().zip(target.parameters()) { - let destination = self.convert_ssa_value(*dest, dfg); + // Destination variable might have already been created by another block that jumps to this target + let destination = self.function_context.get_or_create_variable( + self.brillig_context, + *dest, + dfg, + ); let source = self.convert_ssa_value(*src, dfg); - self.brillig_context.mov_instruction(destination, source); + self.pass_value(source, destination); } self.brillig_context .jump_instruction(self.create_block_label_for_current_function(*destination)); @@ -122,13 +132,49 @@ impl<'block> BrilligBlock<'block> { TerminatorInstruction::Return { return_values } => { let return_registers: Vec<_> = return_values .iter() - .map(|value_id| self.convert_ssa_value(*value_id, dfg)) + .flat_map(|value_id| { + let return_variable = self.convert_ssa_value(*value_id, dfg); + self.function_context.extract_registers(return_variable) + }) .collect(); self.brillig_context.return_instruction(&return_registers); } } } + fn pass_value(&mut self, source: RegisterOrMemory, destination: RegisterOrMemory) { + match (source, destination) { + ( + RegisterOrMemory::RegisterIndex(source_register), + RegisterOrMemory::RegisterIndex(destination_register), + ) => { + self.brillig_context.mov_instruction(destination_register, source_register); + } + ( + RegisterOrMemory::HeapArray(HeapArray { pointer: source_pointer, .. }), + RegisterOrMemory::HeapArray(HeapArray { pointer: destination_pointer, .. }), + ) => { + self.brillig_context.mov_instruction(destination_pointer, source_pointer); + } + ( + RegisterOrMemory::HeapVector(HeapVector { + pointer: source_pointer, + size: source_size, + }), + RegisterOrMemory::HeapVector(HeapVector { + pointer: destination_pointer, + size: destination_size, + }), + ) => { + self.brillig_context.mov_instruction(destination_pointer, source_pointer); + self.brillig_context.mov_instruction(destination_size, source_size); + } + (_, _) => { + unreachable!("ICE: Cannot pass value from {:?} to {:?}", source, destination); + } + } + } + /// Converts SSA Block parameters into Brillig Registers. fn convert_block_params(&mut self, block: &BasicBlock, dfg: &DataFlowGraph) { for param_id in block.parameters() { @@ -141,8 +187,14 @@ impl<'block> BrilligBlock<'block> { // Simple parameters and arrays are passed as already filled registers // In the case of arrays, the values should already be in memory and the register should // Be a valid pointer to the array. - Type::Numeric(_) | Type::Array(..) | Type::Reference => { - self.function_context.get_or_create_register(self.brillig_context, *param_id); + // For slices, two registers are passed, the pointer to the data and a register holding the size of the slice. + Type::Numeric(_) | Type::Array(..) | Type::Slice(..) | Type::Reference => { + // This parameter variable might have already been created by another block that jumps to this one. + self.function_context.get_or_create_variable( + self.brillig_context, + *param_id, + dfg, + ); } _ => { todo!("ICE: Param type not supported") @@ -157,53 +209,66 @@ impl<'block> BrilligBlock<'block> { match instruction { Instruction::Binary(binary) => { - let result_ids = dfg.instruction_results(instruction_id); - let result_register = self - .function_context - .get_or_create_register(self.brillig_context, result_ids[0]); + let result_register = self.function_context.create_register_variable( + self.brillig_context, + dfg.instruction_results(instruction_id)[0], + dfg, + ); self.convert_ssa_binary(binary, dfg, result_register); } Instruction::Constrain(value) => { - let condition = self.convert_ssa_value(*value, dfg); + let condition = self.convert_ssa_register_value(*value, dfg); self.brillig_context.constrain_instruction(condition); } Instruction::Allocate => { - let value = dfg.instruction_results(instruction_id)[0]; - let address_register = - self.function_context.get_or_create_register(self.brillig_context, value); - self.brillig_context.allocate_instruction(address_register); + let result_value = dfg.instruction_results(instruction_id)[0]; + let address_register = self.function_context.create_register_variable( + self.brillig_context, + result_value, + dfg, + ); + self.brillig_context.allocate_variable_instruction(address_register); } Instruction::Store { address, value } => { - let address_register = self.convert_ssa_value(*address, dfg); - let source_register = self.convert_ssa_value(*value, dfg); + let address_register = self.convert_ssa_register_value(*address, dfg); + let source_variable = self.convert_ssa_value(*value, dfg); - self.brillig_context.store_instruction(address_register, source_register); + self.brillig_context.store_variable_instruction(address_register, source_variable); } Instruction::Load { address } => { - let target_register = self.function_context.get_or_create_register( + let target_variable = self.function_context.create_variable( self.brillig_context, dfg.instruction_results(instruction_id)[0], + dfg, ); - let address_register = self.convert_ssa_value(*address, dfg); - self.brillig_context.load_instruction(target_register, address_register); + let target = dfg.instruction_results(instruction_id)[0]; + let target_type = dfg.type_of_value(target); + + println!( + "Type of target variable {:?} is {:?} for instruction {:?}", + target, target_type, instruction + ); + let address_register = self.convert_ssa_register_value(*address, dfg); + + self.brillig_context.load_variable_instruction(target_variable, address_register); } Instruction::Not(value) => { - let condition = self.convert_ssa_value(*value, dfg); - let result_ids = dfg.instruction_results(instruction_id); - let result_register = self - .function_context - .get_or_create_register(self.brillig_context, result_ids[0]); + let condition_register = self.convert_ssa_register_value(*value, dfg); + let result_register = self.function_context.create_register_variable( + self.brillig_context, + dfg.instruction_results(instruction_id)[0], + dfg, + ); let bit_size = Self::get_bit_size_from_ssa_type(dfg.type_of_value(*value)); - self.brillig_context.not_instruction(condition, bit_size, result_register); + self.brillig_context.not_instruction(condition_register, bit_size, result_register); } Instruction::Call { func, arguments } => match &dfg[*func] { Value::ForeignFunction(func_name) => { let result_ids = dfg.instruction_results(instruction_id); - let input_registers = vecmap(arguments, |value_id| { - self.convert_ssa_value_to_register_value_or_array(*value_id, dfg) - }); + let input_registers = + vecmap(arguments, |value_id| self.convert_ssa_value(*value_id, dfg)); let output_registers = vecmap(result_ids, |value_id| { self.allocate_external_call_result(*value_id, dfg) }); @@ -227,8 +292,13 @@ impl<'block> BrilligBlock<'block> { } } Value::Function(func_id) => { - let function_arguments: Vec = - vecmap(arguments, |arg| self.convert_ssa_value(*arg, dfg)); + let argument_registers: Vec = arguments + .iter() + .flat_map(|arg| { + let arg = self.convert_ssa_value(*arg, dfg); + self.function_context.extract_registers(arg) + }) + .collect(); let result_ids = dfg.instruction_results(instruction_id); // Create label for the function that will be called @@ -236,16 +306,25 @@ impl<'block> BrilligBlock<'block> { FunctionContext::function_id_to_function_label(*func_id); let saved_registers = - self.brillig_context.pre_call_save_registers_prep_args(&function_arguments); + self.brillig_context.pre_call_save_registers_prep_args(&argument_registers); // Call instruction, which will interpret above registers 0..num args self.brillig_context.add_external_call_instruction(label_of_function_to_call); // Important: resolve after pre_call_save_registers_prep_args // This ensures we don't save the results to registers unnecessarily. - let result_registers = vecmap(result_ids, |a| { - self.function_context.get_or_create_register(self.brillig_context, *a) - }); + let result_registers: Vec = result_ids + .iter() + .flat_map(|arg| { + let arg = self.function_context.create_variable( + self.brillig_context, + *arg, + dfg, + ); + self.function_context.extract_registers(arg) + }) + .collect(); + assert!( !saved_registers.iter().any(|x| result_registers.contains(x)), "should not save registers used as function results" @@ -254,9 +333,8 @@ impl<'block> BrilligBlock<'block> { .post_call_prep_returns_load_registers(&result_registers, &saved_registers); } Value::Intrinsic(Intrinsic::BlackBox(bb_func)) => { - let function_arguments = vecmap(arguments, |arg| { - self.convert_ssa_value_to_register_value_or_array(*arg, dfg) - }); + let function_arguments = + vecmap(arguments, |arg| self.convert_ssa_value(*arg, dfg)); let function_results = dfg.instruction_results(instruction_id); let function_results = vecmap(function_results, |result| { self.allocate_external_call_result(*result, dfg) @@ -268,63 +346,153 @@ impl<'block> BrilligBlock<'block> { &function_results, ); } + Value::Intrinsic(Intrinsic::ArrayLen) => { + let result_register = self.function_context.create_register_variable( + self.brillig_context, + dfg.instruction_results(instruction_id)[0], + dfg, + ); + let param = self.convert_ssa_value(arguments[0], dfg); + self.brillig_context.length_of_variable_instruction(param, result_register); + } + Value::Intrinsic( + Intrinsic::SlicePushBack + | Intrinsic::SlicePopBack + | Intrinsic::SlicePushFront + | Intrinsic::SlicePopFront + | Intrinsic::SliceInsert + | Intrinsic::SliceRemove, + ) => { + self.slice_intrinsic_call_operation( + dfg, + &dfg[*func], + instruction_id, + arguments, + ); + } _ => { unreachable!("unsupported function call type {:?}", dfg[*func]) } }, Instruction::Truncate { value, .. } => { let result_ids = dfg.instruction_results(instruction_id); - let destination = self - .function_context - .get_or_create_register(self.brillig_context, result_ids[0]); - let source = self.convert_ssa_value(*value, dfg); - self.brillig_context.truncate_instruction(destination, source); + let destination_register = self.function_context.create_register_variable( + self.brillig_context, + result_ids[0], + dfg, + ); + let source_register = self.convert_ssa_register_value(*value, dfg); + self.brillig_context.truncate_instruction(destination_register, source_register); } Instruction::Cast(value, target_type) => { let result_ids = dfg.instruction_results(instruction_id); - let destination = self - .function_context - .get_or_create_register(self.brillig_context, result_ids[0]); - let source = self.convert_ssa_value(*value, dfg); - self.convert_cast(destination, source, target_type, &dfg.type_of_value(*value)); + let destination_register = self.function_context.create_register_variable( + self.brillig_context, + result_ids[0], + dfg, + ); + let source_register = self.convert_ssa_register_value(*value, dfg); + self.convert_cast( + destination_register, + source_register, + target_type, + &dfg.type_of_value(*value), + ); } Instruction::ArrayGet { array, index } => { let result_ids = dfg.instruction_results(instruction_id); - let destination = self - .function_context - .get_or_create_register(self.brillig_context, result_ids[0]); - let array_register = self.convert_ssa_value(*array, dfg); - let index_register = self.convert_ssa_value(*index, dfg); - self.brillig_context.array_get(array_register, index_register, destination); + let destination_register = self.function_context.create_register_variable( + self.brillig_context, + result_ids[0], + dfg, + ); + + let array_variable = self.convert_ssa_value(*array, dfg); + let array_pointer = match array_variable { + RegisterOrMemory::HeapArray(HeapArray { pointer, .. }) => pointer, + RegisterOrMemory::HeapVector(HeapVector { pointer, .. }) => pointer, + _ => unreachable!("ICE: array get on non-array"), + }; + + let index_register = self.convert_ssa_register_value(*index, dfg); + self.brillig_context.array_get(array_pointer, index_register, destination_register); } // Array set operation in SSA returns a new array that is a copy of the parameter array // With a specific value changed. Instruction::ArraySet { array, index, value } => { let result_ids = dfg.instruction_results(instruction_id); - let destination = self - .function_context - .get_or_create_register(self.brillig_context, result_ids[0]); + let destination_variable = + self.function_context.create_variable(self.brillig_context, result_ids[0], dfg); + let destination = self.function_context.extract_heap_array(destination_variable); // First issue a array copy to the destination let array_size = compute_size_of_type(&dfg.type_of_value(*array)); - self.brillig_context.allocate_fixed_length_array(destination, array_size); - let source_array_register: RegisterIndex = self.convert_ssa_value(*array, dfg); + self.brillig_context.allocate_fixed_length_array(destination.pointer, array_size); + let source_variable = self.convert_ssa_value(*array, dfg); + let source_array = self.function_context.extract_heap_array(source_variable); + let size_register = self.brillig_context.make_constant(array_size.into()); self.brillig_context.copy_array_instruction( - source_array_register, - destination, + source_array.pointer, + destination.pointer, size_register, ); // Then set the value in the newly created array - let index_register = self.convert_ssa_value(*index, dfg); - let value_register = self.convert_ssa_value(*value, dfg); - self.brillig_context.array_set(destination, index_register, value_register); + let index_register = self.convert_ssa_register_value(*index, dfg); + let value_register = self.convert_ssa_register_value(*value, dfg); + self.brillig_context.array_set(destination.pointer, index_register, value_register); } _ => todo!("ICE: Instruction not supported {instruction:?}"), }; } + fn slice_intrinsic_call_operation( + &mut self, + dfg: &DataFlowGraph, + intrinsic: &Value, + instruction_id: InstructionId, + arguments: &[ValueId], + ) { + let target_variable = self.function_context.create_variable( + self.brillig_context, + dfg.instruction_results(instruction_id)[0], + dfg, + ); + let target_vector = self.function_context.extract_heap_vector(target_variable); + let source_variable = self.convert_ssa_value(arguments[0], dfg); + let source_vector = + convert_array_or_vector_to_vector(self.brillig_context, source_variable); + + match intrinsic { + Value::Intrinsic(Intrinsic::SlicePushBack) => { + let item_value = self.convert_ssa_register_value(arguments[1], dfg); + slice_push_back_operation( + self.brillig_context, + target_vector, + source_vector, + item_value, + ); + } + Value::Intrinsic(Intrinsic::SlicePushFront) => { + let item_value = self.convert_ssa_register_value(arguments[1], dfg); + slice_push_front_operation( + self.brillig_context, + target_vector, + source_vector, + item_value, + ); + } + Value::Intrinsic(Intrinsic::SlicePopBack) => { + slice_pop_back_operation(self.brillig_context, target_vector, source_vector); + } + Value::Intrinsic(Intrinsic::SlicePopFront) => { + slice_pop_front_operation(self.brillig_context, target_vector, source_vector); + } + _ => unreachable!("ICE: Slice operation not supported"), + } + } + /// This function allows storing a Value in memory starting at the address specified by the /// address_register. The value can be a single value or an array. The function will recursively /// store the value in memory. @@ -337,7 +505,7 @@ impl<'block> BrilligBlock<'block> { let value = &dfg[value_id]; match value { Value::Param { .. } | Value::Instruction { .. } | Value::NumericConstant { .. } => { - let value_register = self.convert_ssa_value(value_id, dfg); + let value_register = self.convert_ssa_register_value(value_id, dfg); self.brillig_context.store_instruction(address_register, value_register); } Value::Array { array, element_type } => { @@ -420,8 +588,8 @@ impl<'block> BrilligBlock<'block> { let binary_type = type_of_binary_operation(dfg[binary.lhs].get_type(), dfg[binary.rhs].get_type()); - let left = self.convert_ssa_value(binary.lhs, dfg); - let right = self.convert_ssa_value(binary.rhs, dfg); + let left = self.convert_ssa_register_value(binary.lhs, dfg); + let right = self.convert_ssa_register_value(binary.rhs, dfg); let brillig_binary_op = convert_ssa_binary_op_to_brillig_binary_op(binary.operator, binary_type); @@ -429,55 +597,53 @@ impl<'block> BrilligBlock<'block> { self.brillig_context.binary_instruction(left, right, result_register, brillig_binary_op); } - /// Converts an SSA `ValueId` into a `RegisterIndex`. - fn convert_ssa_value(&mut self, value_id: ValueId, dfg: &DataFlowGraph) -> RegisterIndex { + /// Converts an SSA `ValueId` into a `RegisterOrMemory`. Initializes if necessary. + fn convert_ssa_value(&mut self, value_id: ValueId, dfg: &DataFlowGraph) -> RegisterOrMemory { let value = &dfg[value_id]; - let register = match value { + let variable = match value { Value::Param { .. } | Value::Instruction { .. } => { // All block parameters and instruction results should have already been // converted to registers so we fetch from the cache. - self.function_context.get_or_create_register(self.brillig_context, value_id) + self.function_context.get_variable(value_id) } Value::NumericConstant { constant, .. } => { - let register_index = - self.function_context.get_or_create_register(self.brillig_context, value_id); + // Constants might have been converted previously or not, so we get or create and + // (re)initialize the value inside. + let new_variable = self.function_context.get_or_create_variable( + self.brillig_context, + value_id, + dfg, + ); + let register_index = self.function_context.extract_register(new_variable); self.brillig_context.const_instruction(register_index, (*constant).into()); - register_index + new_variable } Value::Array { .. } => { - let address_register = self.brillig_context.allocate_register(); - self.brillig_context.allocate_fixed_length_array( - address_register, - compute_size_of_type(&dfg.type_of_value(value_id)), - ); - self.store_in_memory(address_register, value_id, dfg); - address_register + let new_variable = + self.function_context.create_variable(self.brillig_context, value_id, dfg); + let heap_array = self.function_context.extract_heap_array(new_variable); + + self.brillig_context + .allocate_fixed_length_array(heap_array.pointer, heap_array.size); + self.store_in_memory(heap_array.pointer, value_id, dfg); + new_variable } _ => { - todo!("ICE: Should have been in cache {value:?}") + todo!("ICE: Cannot convert value {value:?}") } }; - register + variable } - fn convert_ssa_value_to_register_value_or_array( + fn convert_ssa_register_value( &mut self, value_id: ValueId, dfg: &DataFlowGraph, - ) -> RegisterOrMemory { - let register_index = self.convert_ssa_value(value_id, dfg); - let typ = dfg[value_id].get_type(); - match typ { - Type::Numeric(_) => RegisterOrMemory::RegisterIndex(register_index), - Type::Array(_, size) => { - RegisterOrMemory::HeapArray(HeapArray { pointer: register_index, size }) - } - _ => { - unreachable!("type not supported for conversion into brillig register") - } - } + ) -> RegisterIndex { + let variable = self.convert_ssa_value(value_id, dfg); + self.function_context.extract_register(variable) } fn allocate_external_call_result( @@ -487,25 +653,27 @@ impl<'block> BrilligBlock<'block> { ) -> RegisterOrMemory { let typ = dfg[result].get_type(); match typ { - Type::Numeric(_) => RegisterOrMemory::RegisterIndex( - self.function_context.get_or_create_register(self.brillig_context, result), - ), + Type::Numeric(_) => { + self.function_context.create_variable(self.brillig_context, result, dfg) + } + Type::Array(..) => { - let pointer = - self.function_context.get_or_create_register(self.brillig_context, result); - let array_size_in_memory = compute_size_of_type(&typ); - self.brillig_context.allocate_fixed_length_array(pointer, array_size_in_memory); - RegisterOrMemory::HeapArray(HeapArray { pointer, size: array_size_in_memory }) + let variable = + self.function_context.create_variable(self.brillig_context, result, dfg); + let array = self.function_context.extract_heap_array(variable); + self.brillig_context.allocate_fixed_length_array(array.pointer, array.size); + variable } Type::Slice(_) => { - let pointer = - self.function_context.get_or_create_register(self.brillig_context, result); - let array_size_register = self.brillig_context.allocate_register(); + let variable = + self.function_context.create_variable(self.brillig_context, result, dfg); + let vector = self.function_context.extract_heap_vector(variable); + // Set the pointer to the current stack frame // The stack pointer will then be update by the caller of this method // once the external call is resolved and the array size is known - self.brillig_context.set_array_pointer(pointer); - RegisterOrMemory::HeapVector(HeapVector { pointer, size: array_size_register }) + self.brillig_context.set_array_pointer(vector.pointer); + variable } _ => { unreachable!("ICE: unsupported return type for black box call {typ:?}") @@ -614,18 +782,3 @@ pub(crate) fn convert_ssa_binary_op_to_brillig_binary_op( None => binary_op_to_field_op(ssa_op), } } - -/// Computes the size of an SSA composite type -fn compute_size_of_composite_type(typ: &CompositeType) -> usize { - typ.iter().map(compute_size_of_type).sum() -} - -/// Finds out the size of a given SSA type -/// This is needed to store values in memory -pub(crate) fn compute_size_of_type(typ: &Type) -> usize { - match typ { - Type::Numeric(_) => 1, - Type::Array(types, item_count) => compute_size_of_composite_type(types) * item_count, - _ => todo!("ICE: Type not supported {typ:?}"), - } -} diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs index 4a5ae6f3f09..a514c31408a 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; -use acvm::acir::brillig::RegisterIndex; +use acvm::brillig_vm::brillig::{HeapArray, HeapVector, RegisterIndex, RegisterOrMemory}; use crate::{ brillig::brillig_ir::{ @@ -8,44 +8,128 @@ use crate::{ BrilligContext, }, ssa_refactor::ir::{ + dfg::DataFlowGraph, function::{Function, FunctionId}, - types::Type, + types::{CompositeType, Type}, value::ValueId, }, }; -use super::brillig_block::compute_size_of_type; - pub(crate) struct FunctionContext { pub(crate) function_id: FunctionId, - /// Map from SSA values to Register Indices. - pub(crate) ssa_value_to_register: HashMap, + /// Map from SSA values to register or memory. + pub(crate) ssa_value_to_brillig_variable: HashMap, } impl FunctionContext { - /// Gets a `RegisterIndex` for a `ValueId`, if one already exists - /// or creates a new `RegisterIndex` using the latest available - /// free register. - pub(crate) fn get_or_create_register( + /// For a given SSA value id, create and cache the a corresponding variable. + /// This will allocate the needed registers for the variable. + pub(crate) fn create_variable( + &mut self, + brillig_context: &mut BrilligContext, + value: ValueId, + dfg: &DataFlowGraph, + ) -> RegisterOrMemory { + let typ = dfg.type_of_value(value); + + let variable = match typ { + Type::Numeric(_) | Type::Reference => { + let register = brillig_context.allocate_register(); + RegisterOrMemory::RegisterIndex(register) + } + Type::Array(_, _) => { + let pointer_register = brillig_context.allocate_register(); + let size = compute_size_of_type(&typ); + RegisterOrMemory::HeapArray(HeapArray { pointer: pointer_register, size }) + } + Type::Slice(_) => { + let pointer_register = brillig_context.allocate_register(); + let size_register = brillig_context.allocate_register(); + RegisterOrMemory::HeapVector(HeapVector { + pointer: pointer_register, + size: size_register, + }) + } + Type::Function => { + unreachable!("ICE: Function values should have been removed from the SSA") + } + }; + + // Cache the `ValueId` so that if we call get_variable, it will + // return the registers that have just been created. + // + // WARNING: This assumes that a registers won't be reused for a different value. + // If you overwrite the registers, then the cache will be invalid. + + if self.ssa_value_to_brillig_variable.insert(value, variable).is_some() { + unreachable!("ICE: ValueId {value:?} was already in cache"); + } + + variable + } + + /// For a given SSA value id, return the corresponding cached variable. + pub(crate) fn get_variable(&mut self, value: ValueId) -> RegisterOrMemory { + *self + .ssa_value_to_brillig_variable + .get(&value) + .unwrap_or_else(|| panic!("ICE: Value not found in cache {value}")) + } + + pub(crate) fn get_or_create_variable( &mut self, brillig_context: &mut BrilligContext, value: ValueId, + dfg: &DataFlowGraph, + ) -> RegisterOrMemory { + if let Some(variable) = self.ssa_value_to_brillig_variable.get(&value) { + return *variable; + } + + self.create_variable(brillig_context, value, dfg) + } + + pub(crate) fn create_register_variable( + &mut self, + brillig_context: &mut BrilligContext, + value: ValueId, + dfg: &DataFlowGraph, ) -> RegisterIndex { - if let Some(register_index) = self.ssa_value_to_register.get(&value) { - return *register_index; + let variable = self.create_variable(brillig_context, value, dfg); + self.extract_register(variable) + } + + pub(crate) fn extract_register(&self, variable: RegisterOrMemory) -> RegisterIndex { + match variable { + RegisterOrMemory::RegisterIndex(register_index) => register_index, + _ => unreachable!("ICE: Expected register, got {variable:?}"), } + } - let register = brillig_context.allocate_register(); + pub(crate) fn extract_heap_array(&self, variable: RegisterOrMemory) -> HeapArray { + match variable { + RegisterOrMemory::HeapArray(array) => array, + _ => unreachable!("ICE: Expected array, got {variable:?}"), + } + } - // Cache the `ValueId` so that if we call it again, it will - // return the register that has just been created. - // - // WARNING: This assumes that a register has not been - // modified. If a MOV instruction has overwritten the value - // at a register, then this cache will be invalid. - self.ssa_value_to_register.insert(value, register); + pub(crate) fn extract_heap_vector(&self, variable: RegisterOrMemory) -> HeapVector { + match variable { + RegisterOrMemory::HeapVector(vector) => vector, + _ => unreachable!("ICE: Expected vector, got {variable:?}"), + } + } - register + pub(crate) fn extract_registers(&self, variable: RegisterOrMemory) -> Vec { + match variable { + RegisterOrMemory::RegisterIndex(register_index) => vec![register_index], + RegisterOrMemory::HeapArray(array) => { + vec![array.pointer] + } + RegisterOrMemory::HeapVector(vector) => { + vec![vector.pointer, vector.size] + } + } } /// Creates a function label from a given SSA function id. @@ -62,6 +146,7 @@ impl FunctionContext { match typ { Type::Numeric(_) | Type::Reference => BrilligParameter::Register, Type::Array(..) => BrilligParameter::HeapArray(compute_size_of_type(&typ)), + Type::Slice(_) => BrilligParameter::HeapVector, _ => unimplemented!("Unsupported function parameter type {typ:?}"), } }) @@ -77,9 +162,25 @@ impl FunctionContext { match typ { Type::Numeric(_) | Type::Reference => BrilligParameter::Register, Type::Array(..) => BrilligParameter::HeapArray(compute_size_of_type(&typ)), + Type::Slice(_) => BrilligParameter::HeapVector, _ => unimplemented!("Unsupported return value type {typ:?}"), } }) .collect() } } + +/// Computes the size of an SSA composite type +pub(crate) fn compute_size_of_composite_type(typ: &CompositeType) -> usize { + typ.iter().map(compute_size_of_type).sum() +} + +/// Finds out the size of a given SSA type +/// This is needed to store values in memory +pub(crate) fn compute_size_of_type(typ: &Type) -> usize { + match typ { + Type::Numeric(_) => 1, + Type::Array(types, item_count) => compute_size_of_composite_type(types) * item_count, + _ => todo!("ICE: Type not supported {typ:?}"), + } +} diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs new file mode 100644 index 00000000000..d3bc7bcf5f9 --- /dev/null +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs @@ -0,0 +1,99 @@ +use acvm::brillig_vm::brillig::{BinaryIntOp, HeapVector, RegisterIndex, RegisterOrMemory}; + +use crate::brillig::brillig_ir::BrilligContext; + +pub(crate) fn slice_push_back_operation( + brillig_context: &mut BrilligContext, + target_vector: HeapVector, + source_vector: HeapVector, + item_to_insert: RegisterIndex, +) { + // First we need to allocate the target vector incrementing the size by 1 + brillig_context.usize_op(source_vector.size, target_vector.size, BinaryIntOp::Add, 1); + brillig_context.allocate_array_instruction(target_vector.pointer, target_vector.size); + + // Now we copy the source vector into the target vector + brillig_context.copy_array_instruction( + source_vector.pointer, + target_vector.pointer, + source_vector.size, + ); + + brillig_context.array_set(target_vector.pointer, source_vector.size, item_to_insert); +} + +pub(crate) fn slice_push_front_operation( + brillig_context: &mut BrilligContext, + target_vector: HeapVector, + source_vector: HeapVector, + item_to_insert: RegisterIndex, +) { + // First we need to allocate the target vector incrementing the size by 1 + brillig_context.usize_op(source_vector.size, target_vector.size, BinaryIntOp::Add, 1); + brillig_context.allocate_array_instruction(target_vector.pointer, target_vector.size); + + // Now we offset the target pointer by one + let destination_copy_pointer = brillig_context.allocate_register(); + brillig_context.usize_op(target_vector.pointer, destination_copy_pointer, BinaryIntOp::Add, 1); + + // Now we copy the source vector into the target vector starting at index 1 + brillig_context.copy_array_instruction( + source_vector.pointer, + destination_copy_pointer, + source_vector.size, + ); + + // Then we write the item to insert at index 0 + let zero = brillig_context.make_constant(0_u128.into()); + brillig_context.array_set(target_vector.pointer, zero, item_to_insert); + brillig_context.deallocate_register(zero); +} + +pub(crate) fn slice_pop_front_operation( + brillig_context: &mut BrilligContext, + target_vector: HeapVector, + source_vector: HeapVector, +) { + // First we need to allocate the target vector decrementing the size by 1 + brillig_context.usize_op(source_vector.size, target_vector.size, BinaryIntOp::Sub, 1); + brillig_context.allocate_array_instruction(target_vector.pointer, target_vector.size); + + // Now we offset the source pointer by one + let source_copy_pointer = brillig_context.allocate_register(); + brillig_context.usize_op(source_vector.pointer, source_copy_pointer, BinaryIntOp::Add, 1); + + // Now we copy the source vector starting at index 1 into the target vector + brillig_context.copy_array_instruction( + source_copy_pointer, + target_vector.pointer, + source_vector.size, + ); +} + +pub(crate) fn slice_pop_back_operation( + brillig_context: &mut BrilligContext, + target_vector: HeapVector, + source_vector: HeapVector, +) { + // First we need to allocate the target vector decrementing the size by 1 + brillig_context.usize_op(source_vector.size, target_vector.size, BinaryIntOp::Sub, 1); + brillig_context.allocate_array_instruction(target_vector.pointer, target_vector.size); + + // Now we copy all elements but the last into the target vector + brillig_context.copy_array_instruction( + source_vector.pointer, + target_vector.pointer, + target_vector.size, + ); +} + +pub(crate) fn convert_array_or_vector_to_vector( + brillig_context: &mut BrilligContext, + source_variable: RegisterOrMemory, +) -> HeapVector { + match source_variable { + RegisterOrMemory::HeapVector(source_vector) => source_vector, + RegisterOrMemory::HeapArray(source_array) => brillig_context.array_to_vector(&source_array), + _ => unreachable!("ICE: unsupported slice push back source {:?}", source_variable), + } +} diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir.rs b/crates/noirc_evaluator/src/brillig/brillig_ir.rs index 1d1a4534ec3..b06741352f7 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir.rs @@ -162,9 +162,10 @@ impl BrilligContext { /// Allocates a single value and stores the /// pointer to the array in `pointer_register` - pub(crate) fn allocate_instruction(&mut self, pointer_register: RegisterIndex) { + pub(crate) fn allocate_variable_instruction(&mut self, pointer_register: RegisterIndex) { debug_show::allocate_instruction(pointer_register); - let size_register = self.make_constant(1_u128.into()); + // A variable can be stored in up to two values, so we reserve two values for that. + let size_register = self.make_constant(2_u128.into()); self.push_opcode(BrilligOpcode::Mov { destination: pointer_register, source: ReservedRegisters::stack_pointer(), @@ -226,11 +227,15 @@ impl BrilligContext { /// Into the array pointed by destination pub(crate) fn copy_array_instruction( &mut self, - source: RegisterIndex, - destination: RegisterIndex, + source_pointer: RegisterIndex, + destination_pointer: RegisterIndex, num_elements_register: RegisterIndex, ) { - debug_show::copy_array_instruction(source, destination, num_elements_register); + debug_show::copy_array_instruction( + source_pointer, + destination_pointer, + num_elements_register, + ); let index_register = self.make_constant(0_u128.into()); let loop_label = self.next_section_label(); @@ -257,8 +262,8 @@ impl BrilligContext { // Copy the element from source to destination let value_register = self.allocate_register(); - self.array_get(source, index_register, value_register); - self.array_set(destination, index_register, value_register); + self.array_get(source_pointer, index_register, value_register); + self.array_set(destination_pointer, index_register, value_register); // Increment the index register let one_register = self.make_constant(1u128.into()); @@ -513,6 +518,31 @@ impl BrilligContext { self.push_opcode(BrilligOpcode::Load { destination, source_pointer }); } + pub(crate) fn load_variable_instruction( + &mut self, + destination: RegisterOrMemory, + variable_pointer: RegisterIndex, + ) { + match destination { + RegisterOrMemory::RegisterIndex(register_index) => { + self.load_instruction(register_index, variable_pointer); + } + RegisterOrMemory::HeapArray(HeapArray { pointer, .. }) => { + self.load_instruction(pointer, variable_pointer); + } + RegisterOrMemory::HeapVector(HeapVector { pointer, size }) => { + self.load_instruction(pointer, variable_pointer); + + let size_pointer = self.allocate_register(); + self.mov_instruction(size_pointer, variable_pointer); + self.usize_op_in_place(size_pointer, BinaryIntOp::Add, 1_usize); + + self.load_instruction(size, size_pointer); + self.deallocate_register(size_pointer); + } + } + } + /// Emits a store instruction pub(crate) fn store_instruction( &mut self, @@ -523,6 +553,55 @@ impl BrilligContext { self.push_opcode(BrilligOpcode::Store { destination_pointer, source }); } + pub(crate) fn length_of_variable_instruction( + &mut self, + variable: RegisterOrMemory, + result: RegisterIndex, + ) { + match variable { + RegisterOrMemory::RegisterIndex(_) => { + self.const_instruction(result, 1_u128.into()); + } + RegisterOrMemory::HeapArray(HeapArray { size, .. }) => { + self.const_instruction(result, size.into()); + } + RegisterOrMemory::HeapVector(HeapVector { size, .. }) => { + self.mov_instruction(result, size); + } + } + } + + pub(crate) fn store_variable_instruction( + &mut self, + variable_pointer: RegisterIndex, + source: RegisterOrMemory, + ) { + let size_pointer = self.allocate_register(); + self.mov_instruction(size_pointer, variable_pointer); + self.usize_op_in_place(size_pointer, BinaryIntOp::Add, 1_usize); + + match source { + RegisterOrMemory::RegisterIndex(register_index) => { + self.store_instruction(variable_pointer, register_index); + let size_constant = self.make_constant(Value::from(1_usize)); + self.store_instruction(size_pointer, size_constant); + self.deallocate_register(size_constant); + } + RegisterOrMemory::HeapArray(HeapArray { pointer, size }) => { + self.store_instruction(variable_pointer, pointer); + let size_constant = self.make_constant(Value::from(size)); + self.store_instruction(size_pointer, size_constant); + self.deallocate_register(size_constant); + } + RegisterOrMemory::HeapVector(HeapVector { pointer, size }) => { + self.store_instruction(variable_pointer, pointer); + self.store_instruction(size_pointer, size); + } + } + + self.deallocate_register(size_pointer); + } + /// Emits a truncate instruction. /// /// Note: Truncation is used as an optimization in the SSA IR @@ -668,7 +747,7 @@ impl BrilligContext { for register in used_registers.iter() { self.store_instruction(ReservedRegisters::stack_pointer(), *register); // Add one to our stack pointer - self.usize_op(ReservedRegisters::stack_pointer(), BinaryIntOp::Add, 1); + self.usize_op_in_place(ReservedRegisters::stack_pointer(), BinaryIntOp::Add, 1); } // Store the location of our registers in the previous stack pointer @@ -689,21 +768,32 @@ impl BrilligContext { for register in used_registers.iter().rev() { // Subtract one from our stack pointer - self.usize_op(iterator_register, BinaryIntOp::Sub, 1); + self.usize_op_in_place(iterator_register, BinaryIntOp::Sub, 1); self.load_instruction(*register, iterator_register); } } + /// Utility method to perform a binary instruction with a constant value in place + pub(crate) fn usize_op_in_place( + &mut self, + destination: RegisterIndex, + op: BinaryIntOp, + constant: usize, + ) { + self.usize_op(destination, destination, op, constant); + } + /// Utility method to perform a binary instruction with a constant value pub(crate) fn usize_op( &mut self, + operand: RegisterIndex, destination: RegisterIndex, op: BinaryIntOp, constant: usize, ) { let const_register = self.make_constant(Value::from(constant)); self.binary_instruction( - destination, + operand, const_register, destination, BrilligBinaryOp::Integer { op, bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE }, From 2b522acef4f022ebc4d92875fdb2c091f789146b Mon Sep 17 00:00:00 2001 From: sirasistant Date: Fri, 14 Jul 2023 16:08:11 +0000 Subject: [PATCH 2/8] feat: get all slice operations working --- .../brillig_slices/src/main.nr | 70 +++++++++++- .../src/brillig/brillig_gen/brillig_block.rs | 108 +++++++++++++++--- .../brillig/brillig_gen/brillig_slice_ops.rs | 105 +++++++++++++++++ .../noirc_evaluator/src/brillig/brillig_ir.rs | 19 ++- .../src/brillig/brillig_ir/artifact.rs | 6 + crates/noirc_evaluator/src/ssa_refactor.rs | 2 +- 6 files changed, 284 insertions(+), 26 deletions(-) diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr index 4774d451b6f..83d42111753 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr @@ -1,11 +1,71 @@ use dep::std::slice; use dep::std; -unconstrained fn main() { - let mut slice: [Field] = []; - assert(slice.len() == 0); +#[oracle(oracle_print_impl)] +unconstrained fn oracle_print(_x : Field) -> Field {} - slice = slice.push_back(1); - assert(slice.len() == 1); +unconstrained fn oracle_print_wrapper(x: Field) { + oracle_print(x); +} + + +unconstrained fn main(x: Field, y: Field) { + // Mark it as mut so the compiler doesn't simplify the following operations + // But don't reuse the mut slice variable until this is fixed https://github.com/noir-lang/noir/issues/1931 + let mut slice: [Field] = [x, y]; + assert(slice.len() == 2); + + let pushed_back_slice = slice.push_back(1); + assert(pushed_back_slice.len() == 3); + assert(pushed_back_slice[0] == x); + assert(pushed_back_slice[1] == y); + assert(pushed_back_slice[2] == 1); + + assert(slice.len() == 2); + + let pushed_front_slice = pushed_back_slice.push_front(2); + assert(pushed_front_slice.len() == 4); + assert(pushed_front_slice[0] == 2); + assert(pushed_front_slice[1] == x); + assert(pushed_front_slice[2] == y); + assert(pushed_front_slice[3] == 1); + + let (item, popped_front_slice) = pushed_front_slice.pop_front(); + assert(item == 2); + + assert(popped_front_slice.len() == 3); + assert(popped_front_slice[0] == x); + assert(popped_front_slice[1] == y); + assert(popped_front_slice[2] == 1); + + let (popped_back_slice, another_item) = popped_front_slice.pop_back(); + assert(another_item == 1); + + assert(popped_back_slice.len() == 2); + assert(popped_back_slice[0] == x); + assert(popped_back_slice[1] == y); + + let inserted_slice = popped_back_slice.insert(1, 2); + assert(inserted_slice.len() == 3); + assert(inserted_slice[0] == x); + assert(inserted_slice[1] == 2); + assert(inserted_slice[2] == y); + + let (removed_slice, should_be_2) = inserted_slice.remove(1); + assert(should_be_2 == 2); + + assert(removed_slice.len() == 2); + assert(removed_slice[0] == x); + assert(removed_slice[1] == y); + + let (slice_with_only_x, should_be_y) = removed_slice.remove(1); + assert(should_be_y == y); + + assert(slice_with_only_x.len() == 1); + assert(removed_slice[0] == x); + + let (empty_slice, should_be_x) = slice_with_only_x.remove(0); + assert(should_be_x == x); + assert(empty_slice.len() == 0); } diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 5757fb2e600..cd6e536c84c 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -21,7 +21,8 @@ use iter_extended::vecmap; use super::brillig_black_box::convert_black_box_call; use super::brillig_fn::{compute_size_of_composite_type, compute_size_of_type, FunctionContext}; use super::brillig_slice_ops::{ - slice_pop_back_operation, slice_pop_front_operation, slice_push_front_operation, + slice_insert_operation, slice_pop_back_operation, slice_pop_front_operation, + slice_push_front_operation, slice_remove_operation, }; /// Generate the compilation artifacts for compiling a function into brillig bytecode. @@ -242,13 +243,6 @@ impl<'block> BrilligBlock<'block> { dfg, ); - let target = dfg.instruction_results(instruction_id)[0]; - let target_type = dfg.type_of_value(target); - - println!( - "Type of target variable {:?} is {:?} for instruction {:?}", - target, target_type, instruction - ); let address_register = self.convert_ssa_register_value(*address, dfg); self.brillig_context.load_variable_instruction(target_variable, address_register); @@ -300,6 +294,11 @@ impl<'block> BrilligBlock<'block> { }) .collect(); let result_ids = dfg.instruction_results(instruction_id); + // TODO reconciliate argument types with the types of the function being called + println!( + "Types of the arguments are: {:?}", + arguments.iter().map(|arg| dfg.type_of_value(*arg)).collect::>() + ); // Create label for the function that will be called let label_of_function_to_call = @@ -454,18 +453,18 @@ impl<'block> BrilligBlock<'block> { instruction_id: InstructionId, arguments: &[ValueId], ) { - let target_variable = self.function_context.create_variable( - self.brillig_context, - dfg.instruction_results(instruction_id)[0], - dfg, - ); - let target_vector = self.function_context.extract_heap_vector(target_variable); let source_variable = self.convert_ssa_value(arguments[0], dfg); let source_vector = convert_array_or_vector_to_vector(self.brillig_context, source_variable); match intrinsic { Value::Intrinsic(Intrinsic::SlicePushBack) => { + let target_variable = self.function_context.create_variable( + self.brillig_context, + dfg.instruction_results(instruction_id)[0], + dfg, + ); + let target_vector = self.function_context.extract_heap_vector(target_variable); let item_value = self.convert_ssa_register_value(arguments[1], dfg); slice_push_back_operation( self.brillig_context, @@ -475,6 +474,12 @@ impl<'block> BrilligBlock<'block> { ); } Value::Intrinsic(Intrinsic::SlicePushFront) => { + let target_variable = self.function_context.create_variable( + self.brillig_context, + dfg.instruction_results(instruction_id)[0], + dfg, + ); + let target_vector = self.function_context.extract_heap_vector(target_variable); let item_value = self.convert_ssa_register_value(arguments[1], dfg); slice_push_front_operation( self.brillig_context, @@ -484,10 +489,81 @@ impl<'block> BrilligBlock<'block> { ); } Value::Intrinsic(Intrinsic::SlicePopBack) => { - slice_pop_back_operation(self.brillig_context, target_vector, source_vector); + let results = dfg.instruction_results(instruction_id); + + let target_variable = + self.function_context.create_variable(self.brillig_context, results[0], dfg); + let target_vector = self.function_context.extract_heap_vector(target_variable); + + let pop_item = self.function_context.create_register_variable( + self.brillig_context, + results[1], + dfg, + ); + + slice_pop_back_operation( + self.brillig_context, + target_vector, + source_vector, + pop_item, + ); } Value::Intrinsic(Intrinsic::SlicePopFront) => { - slice_pop_front_operation(self.brillig_context, target_vector, source_vector); + let results = dfg.instruction_results(instruction_id); + + let pop_item = self.function_context.create_register_variable( + self.brillig_context, + results[0], + dfg, + ); + let target_variable = + self.function_context.create_variable(self.brillig_context, results[1], dfg); + let target_vector = self.function_context.extract_heap_vector(target_variable); + + slice_pop_front_operation( + self.brillig_context, + target_vector, + source_vector, + pop_item, + ); + } + Value::Intrinsic(Intrinsic::SliceInsert) => { + let results = dfg.instruction_results(instruction_id); + let index = self.convert_ssa_register_value(arguments[1], dfg); + let item = self.convert_ssa_register_value(arguments[2], dfg); + let target_variable = + self.function_context.create_variable(self.brillig_context, results[0], dfg); + + let target_vector = self.function_context.extract_heap_vector(target_variable); + slice_insert_operation( + self.brillig_context, + target_vector, + source_vector, + index, + item, + ); + } + Value::Intrinsic(Intrinsic::SliceRemove) => { + let results = dfg.instruction_results(instruction_id); + let index = self.convert_ssa_register_value(arguments[1], dfg); + + let target_variable = + self.function_context.create_variable(self.brillig_context, results[0], dfg); + let target_vector = self.function_context.extract_heap_vector(target_variable); + + let removed_item_register = self.function_context.create_register_variable( + self.brillig_context, + results[1], + dfg, + ); + + slice_remove_operation( + self.brillig_context, + target_vector, + source_vector, + index, + removed_item_register, + ); } _ => unreachable!("ICE: Slice operation not supported"), } diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs index d3bc7bcf5f9..5e77600166a 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs @@ -53,6 +53,7 @@ pub(crate) fn slice_pop_front_operation( brillig_context: &mut BrilligContext, target_vector: HeapVector, source_vector: HeapVector, + removed_item: RegisterIndex, ) { // First we need to allocate the target vector decrementing the size by 1 brillig_context.usize_op(source_vector.size, target_vector.size, BinaryIntOp::Sub, 1); @@ -68,12 +69,17 @@ pub(crate) fn slice_pop_front_operation( target_vector.pointer, source_vector.size, ); + + let zero = brillig_context.make_constant(0_u128.into()); + brillig_context.array_get(source_vector.pointer, zero, removed_item); + brillig_context.deallocate_register(zero); } pub(crate) fn slice_pop_back_operation( brillig_context: &mut BrilligContext, target_vector: HeapVector, source_vector: HeapVector, + removed_item: RegisterIndex, ) { // First we need to allocate the target vector decrementing the size by 1 brillig_context.usize_op(source_vector.size, target_vector.size, BinaryIntOp::Sub, 1); @@ -85,6 +91,105 @@ pub(crate) fn slice_pop_back_operation( target_vector.pointer, target_vector.size, ); + + brillig_context.array_get(source_vector.pointer, target_vector.size, removed_item); +} + +pub(crate) fn slice_insert_operation( + brillig_context: &mut BrilligContext, + target_vector: HeapVector, + source_vector: HeapVector, + index: RegisterIndex, + item: RegisterIndex, +) { + // First we need to allocate the target vector incrementing the size by 1 + brillig_context.usize_op(source_vector.size, target_vector.size, BinaryIntOp::Add, 1); + brillig_context.allocate_array_instruction(target_vector.pointer, target_vector.size); + + // Copy the elements to the left of the index + brillig_context.copy_array_instruction(source_vector.pointer, target_vector.pointer, index); + + // Compute the source pointer just at the index + let source_pointer_at_index = brillig_context.allocate_register(); + brillig_context.memory_op( + source_vector.pointer, + index, + source_pointer_at_index, + BinaryIntOp::Add, + ); + + // Compute the target pointer after the index + let target_pointer_after_index = brillig_context.allocate_register(); + brillig_context.memory_op( + target_vector.pointer, + index, + target_pointer_after_index, + BinaryIntOp::Add, + ); + brillig_context.usize_op_in_place(target_pointer_after_index, BinaryIntOp::Add, 1); + + // Compute the number of elements to the right of the index + let item_count = brillig_context.allocate_register(); + brillig_context.memory_op(source_vector.size, index, item_count, BinaryIntOp::Sub); + + // Copy the elements to the right of the index + brillig_context.copy_array_instruction( + source_pointer_at_index, + target_pointer_after_index, + item_count, + ); + + // Write the item to insert at the index + brillig_context.array_set(target_vector.pointer, index, item); +} + +pub(crate) fn slice_remove_operation( + brillig_context: &mut BrilligContext, + target_vector: HeapVector, + source_vector: HeapVector, + index: RegisterIndex, + removed_item: RegisterIndex, +) { + // First we need to allocate the target vector decrementing the size by 1 + brillig_context.usize_op(source_vector.size, target_vector.size, BinaryIntOp::Sub, 1); + brillig_context.allocate_array_instruction(target_vector.pointer, target_vector.size); + + // Copy the elements to the left of the index + brillig_context.copy_array_instruction(source_vector.pointer, target_vector.pointer, index); + + // Compute the source pointer after the index + let source_pointer_after_index = brillig_context.allocate_register(); + brillig_context.memory_op( + source_vector.pointer, + index, + source_pointer_after_index, + BinaryIntOp::Add, + ); + brillig_context.usize_op_in_place(source_pointer_after_index, BinaryIntOp::Add, 1); + + // Compute the target pointer at the index + let target_pointer_at_index = brillig_context.allocate_register(); + brillig_context.memory_op( + target_vector.pointer, + index, + target_pointer_at_index, + BinaryIntOp::Add, + ); + + // Compute the number of elements to the right of the index + let item_count = brillig_context.allocate_register(); + brillig_context.memory_op(source_vector.size, index, item_count, BinaryIntOp::Sub); + brillig_context.usize_op_in_place(item_count, BinaryIntOp::Sub, 1); + + // Copy the elements to the right of the index + brillig_context.copy_array_instruction( + source_pointer_after_index, + target_pointer_at_index, + item_count, + ); + + // Get the item at the index + brillig_context.array_get(source_vector.pointer, index, removed_item); } pub(crate) fn convert_array_or_vector_to_vector( diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir.rs b/crates/noirc_evaluator/src/brillig/brillig_ir.rs index b06741352f7..2a0b5355cdb 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir.rs @@ -792,14 +792,25 @@ impl BrilligContext { constant: usize, ) { let const_register = self.make_constant(Value::from(constant)); + self.memory_op(operand, const_register, destination, op); + // Mark as no longer used for this purpose, frees for reuse + self.deallocate_register(const_register); + } + + /// Utility method to perform a binary instruction with a memory address + pub(crate) fn memory_op( + &mut self, + lhs: RegisterIndex, + rhs: RegisterIndex, + destination: RegisterIndex, + op: BinaryIntOp, + ) { self.binary_instruction( - operand, - const_register, + lhs, + rhs, destination, BrilligBinaryOp::Integer { op, bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE }, ); - // Mark as no longer used for this purpose, frees for reuse - self.deallocate_register(const_register); } // Used before a call instruction. diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index 53e4702213e..fbc58eeddff 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -9,6 +9,9 @@ pub(crate) enum BrilligParameter { Register, // A heap array is filled in memory and a pointer to the first element is passed in the register. HeapArray(usize), + // A heap vector is filled in memory and two registers are passed in, the pointer to the first element + // and the length of the vector + HeapVector, } #[derive(Default, Debug, Clone)] @@ -114,6 +117,9 @@ impl BrilligArtifact { .map(|arg| match arg { BrilligParameter::Register => 0, BrilligParameter::HeapArray(size) => *size, + BrilligParameter::HeapVector => { + unreachable!("ICE: Heap vectors cannot be passed as entry point arguments") + } }) .sum::(); diff --git a/crates/noirc_evaluator/src/ssa_refactor.rs b/crates/noirc_evaluator/src/ssa_refactor.rs index 4c9dc848b0e..7d5e9ce210e 100644 --- a/crates/noirc_evaluator/src/ssa_refactor.rs +++ b/crates/noirc_evaluator/src/ssa_refactor.rs @@ -35,7 +35,7 @@ pub(crate) fn optimize_into_acir( ) -> GeneratedAcir { let abi_distinctness = program.return_distinctness; let mut ssa = ssa_gen::generate_ssa(program) - .print(print_ssa_passes, "Initial SSA:") + .print(true, "Initial SSA:") .defunctionalize() .print(print_ssa_passes, "After Defunctionalization:"); From f9b7529be80e119295bf761958b2e4f1416f306d Mon Sep 17 00:00:00 2001 From: sirasistant Date: Fri, 14 Jul 2023 16:09:26 +0000 Subject: [PATCH 3/8] chore: remove printing of initial ssa --- crates/noirc_evaluator/src/ssa_refactor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor.rs b/crates/noirc_evaluator/src/ssa_refactor.rs index 7d5e9ce210e..4c9dc848b0e 100644 --- a/crates/noirc_evaluator/src/ssa_refactor.rs +++ b/crates/noirc_evaluator/src/ssa_refactor.rs @@ -35,7 +35,7 @@ pub(crate) fn optimize_into_acir( ) -> GeneratedAcir { let abi_distinctness = program.return_distinctness; let mut ssa = ssa_gen::generate_ssa(program) - .print(true, "Initial SSA:") + .print(print_ssa_passes, "Initial SSA:") .defunctionalize() .print(print_ssa_passes, "After Defunctionalization:"); From e1a49158e6b90f742f6da5e2940e67926ec0734f Mon Sep 17 00:00:00 2001 From: sirasistant Date: Fri, 14 Jul 2023 16:10:56 +0000 Subject: [PATCH 4/8] chore: remove print --- .../noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index cd6e536c84c..d0d43efec0e 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -294,11 +294,6 @@ impl<'block> BrilligBlock<'block> { }) .collect(); let result_ids = dfg.instruction_results(instruction_id); - // TODO reconciliate argument types with the types of the function being called - println!( - "Types of the arguments are: {:?}", - arguments.iter().map(|arg| dfg.type_of_value(*arg)).collect::>() - ); // Create label for the function that will be called let label_of_function_to_call = From 4c1910dc1c455dba0ec8ac951ff8d2e8f5efed0c Mon Sep 17 00:00:00 2001 From: sirasistant Date: Fri, 14 Jul 2023 16:23:26 +0000 Subject: [PATCH 5/8] refactor: use memory op --- .../src/brillig/brillig_gen/brillig_block.rs | 11 +-- .../noirc_evaluator/src/brillig/brillig_ir.rs | 72 +++++-------------- 2 files changed, 19 insertions(+), 64 deletions(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index d0d43efec0e..72fd59e07d3 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1,9 +1,7 @@ use crate::brillig::brillig_gen::brillig_slice_ops::{ convert_array_or_vector_to_vector, slice_push_back_operation, }; -use crate::brillig::brillig_ir::{ - BrilligBinaryOp, BrilligContext, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, -}; +use crate::brillig::brillig_ir::{BrilligBinaryOp, BrilligContext}; use crate::ssa_refactor::ir::function::FunctionId; use crate::ssa_refactor::ir::instruction::Intrinsic; use crate::ssa_refactor::ir::{ @@ -593,14 +591,11 @@ impl<'block> BrilligBlock<'block> { // Store the item in memory self.store_in_memory(iterator_register, *element_id, dfg); // Increment the iterator by the size of the items - self.brillig_context.binary_instruction( + self.brillig_context.memory_op( iterator_register, size_of_item_register, iterator_register, - BrilligBinaryOp::Integer { - op: BinaryIntOp::Add, - bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, - }, + BinaryIntOp::Add, ); } } diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir.rs b/crates/noirc_evaluator/src/brillig/brillig_ir.rs index 2a0b5355cdb..d0e3380e5f6 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir.rs @@ -142,25 +142,15 @@ impl BrilligContext { } pub(crate) fn update_stack_pointer(&mut self, size_register: RegisterIndex) { - debug_show::binary_instruction( + self.memory_op( ReservedRegisters::stack_pointer(), size_register, ReservedRegisters::stack_pointer(), - BrilligBinaryOp::Integer { - op: BinaryIntOp::Add, - bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, - }, + BinaryIntOp::Add, ); - self.push_opcode(BrilligOpcode::BinaryIntOp { - destination: ReservedRegisters::stack_pointer(), - op: BinaryIntOp::Add, - bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, - lhs: ReservedRegisters::stack_pointer(), - rhs: size_register, - }); } - /// Allocates a single value and stores the + /// Allocates a variable in memory and stores the /// pointer to the array in `pointer_register` pub(crate) fn allocate_variable_instruction(&mut self, pointer_register: RegisterIndex) { debug_show::allocate_instruction(pointer_register); @@ -170,13 +160,12 @@ impl BrilligContext { destination: pointer_register, source: ReservedRegisters::stack_pointer(), }); - self.push_opcode(BrilligOpcode::BinaryIntOp { - destination: ReservedRegisters::stack_pointer(), - op: BinaryIntOp::Add, - bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, - lhs: ReservedRegisters::stack_pointer(), - rhs: size_register, - }); + self.memory_op( + ReservedRegisters::stack_pointer(), + size_register, + ReservedRegisters::stack_pointer(), + BinaryIntOp::Add, + ); } /// Gets the value in the array at index `index` and stores it in `result` @@ -245,14 +234,11 @@ impl BrilligContext { // Check if index < num_elements let index_less_than_array_len = self.allocate_register(); - self.binary_instruction( + self.memory_op( index_register, num_elements_register, index_less_than_array_len, - BrilligBinaryOp::Integer { - op: BinaryIntOp::LessThan, - bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, - }, + BinaryIntOp::LessThan, ); let exit_loop_label = self.next_section_label(); @@ -266,16 +252,7 @@ impl BrilligContext { self.array_set(destination_pointer, index_register, value_register); // Increment the index register - let one_register = self.make_constant(1u128.into()); - self.binary_instruction( - index_register, - one_register, - index_register, - BrilligBinaryOp::Integer { - op: BinaryIntOp::Add, - bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, - }, - ); + self.usize_op_in_place(index_register, BinaryIntOp::Add, 1); self.jump_instruction(loop_label); @@ -284,7 +261,6 @@ impl BrilligContext { // Deallocate our temporary registers self.deallocate_register(index_less_than_array_len); self.deallocate_register(value_register); - self.deallocate_register(one_register); self.deallocate_register(index_register); } @@ -889,9 +865,9 @@ mod tests { use acvm::brillig_vm::{Registers, VMStatus, VM}; use acvm::{BlackBoxFunctionSolver, BlackBoxResolutionError, FieldElement}; - use crate::brillig::brillig_ir::{BrilligContext, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE}; + use crate::brillig::brillig_ir::BrilligContext; - use super::{BrilligBinaryOp, BrilligOpcode, ReservedRegisters}; + use super::{BrilligOpcode, ReservedRegisters}; struct DummyBlackBoxSolver; @@ -950,25 +926,9 @@ mod tests { &[RegisterOrMemory::HeapVector(HeapVector { pointer: r_stack, size: r_output_size })], ); // push stack frame by r_returned_size - context.binary_instruction( - r_stack, - r_output_size, - r_stack, - BrilligBinaryOp::Integer { - op: BinaryIntOp::Add, - bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, - }, - ); + context.memory_op(r_stack, r_output_size, r_stack, BinaryIntOp::Add); // check r_input_size == r_output_size - context.binary_instruction( - r_input_size, - r_output_size, - r_equality, - BrilligBinaryOp::Integer { - op: BinaryIntOp::Equals, - bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, - }, - ); + context.memory_op(r_input_size, r_output_size, r_equality, BinaryIntOp::Equals); // We push a JumpIf and Trap opcode directly as the constrain instruction // uses unresolved jumps which requires a block to be constructed in SSA and // we don't need this for Brillig IR tests From c5465ecc0cd131052c889acb1a2347f17883d5a3 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Fri, 14 Jul 2023 17:11:20 +0000 Subject: [PATCH 6/8] feat: implement array_set for slices --- .../brillig_slices/src/main.nr | 13 ++- .../src/brillig/brillig_gen/brillig_block.rs | 92 ++++++++++++++----- .../src/brillig/brillig_gen/brillig_fn.rs | 2 + .../brillig/brillig_gen/brillig_slice_ops.rs | 10 ++ .../noirc_evaluator/src/brillig/brillig_ir.rs | 2 + 5 files changed, 93 insertions(+), 26 deletions(-) diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr index 83d42111753..06697822410 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr @@ -12,11 +12,20 @@ unconstrained fn oracle_print_wrapper(x: Field) { unconstrained fn main(x: Field, y: Field) { // Mark it as mut so the compiler doesn't simplify the following operations // But don't reuse the mut slice variable until this is fixed https://github.com/noir-lang/noir/issues/1931 - let mut slice: [Field] = [x, y]; + let slice: [Field] = [y, x]; assert(slice.len() == 2); - let pushed_back_slice = slice.push_back(1); + let mut pushed_back_slice = slice.push_back(7); assert(pushed_back_slice.len() == 3); + assert(pushed_back_slice[0] == y); + assert(pushed_back_slice[1] == x); + assert(pushed_back_slice[2] == 7); + + // Array set on slice target + pushed_back_slice[0] = x; + pushed_back_slice[1] = y; + pushed_back_slice[2] = 1; + assert(pushed_back_slice[0] == x); assert(pushed_back_slice[1] == y); assert(pushed_back_slice[2] == 1); diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 72fd59e07d3..e9215b81a2f 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -17,7 +17,7 @@ use acvm::FieldElement; use iter_extended::vecmap; use super::brillig_black_box::convert_black_box_call; -use super::brillig_fn::{compute_size_of_composite_type, compute_size_of_type, FunctionContext}; +use super::brillig_fn::{compute_size_of_composite_type, FunctionContext}; use super::brillig_slice_ops::{ slice_insert_operation, slice_pop_back_operation, slice_pop_front_operation, slice_push_front_operation, slice_remove_operation, @@ -123,7 +123,7 @@ impl<'block> BrilligBlock<'block> { dfg, ); let source = self.convert_ssa_value(*src, dfg); - self.pass_value(source, destination); + self.pass_variable(source, destination); } self.brillig_context .jump_instruction(self.create_block_label_for_current_function(*destination)); @@ -141,7 +141,8 @@ impl<'block> BrilligBlock<'block> { } } - fn pass_value(&mut self, source: RegisterOrMemory, destination: RegisterOrMemory) { + /// Passes an arbitrary variable from the registers of the source to the registers of the destination + fn pass_variable(&mut self, source: RegisterOrMemory, destination: RegisterOrMemory) { match (source, destination) { ( RegisterOrMemory::RegisterIndex(source_register), @@ -355,7 +356,7 @@ impl<'block> BrilligBlock<'block> { | Intrinsic::SliceInsert | Intrinsic::SliceRemove, ) => { - self.slice_intrinsic_call_operation( + self.convert_ssa_slice_intrinsic_call( dfg, &dfg[*func], instruction_id, @@ -409,37 +410,79 @@ impl<'block> BrilligBlock<'block> { let index_register = self.convert_ssa_register_value(*index, dfg); self.brillig_context.array_get(array_pointer, index_register, destination_register); } - // Array set operation in SSA returns a new array that is a copy of the parameter array - // With a specific value changed. Instruction::ArraySet { array, index, value } => { + let source_variable = self.convert_ssa_value(*array, dfg); + let index_register = self.convert_ssa_register_value(*index, dfg); + let value_register = self.convert_ssa_register_value(*value, dfg); + let result_ids = dfg.instruction_results(instruction_id); let destination_variable = self.function_context.create_variable(self.brillig_context, result_ids[0], dfg); - let destination = self.function_context.extract_heap_array(destination_variable); - - // First issue a array copy to the destination - let array_size = compute_size_of_type(&dfg.type_of_value(*array)); - self.brillig_context.allocate_fixed_length_array(destination.pointer, array_size); - let source_variable = self.convert_ssa_value(*array, dfg); - let source_array = self.function_context.extract_heap_array(source_variable); - let size_register = self.brillig_context.make_constant(array_size.into()); - self.brillig_context.copy_array_instruction( - source_array.pointer, - destination.pointer, - size_register, + self.convert_ssa_array_set( + source_variable, + destination_variable, + index_register, + value_register, ); - - // Then set the value in the newly created array - let index_register = self.convert_ssa_register_value(*index, dfg); - let value_register = self.convert_ssa_register_value(*value, dfg); - self.brillig_context.array_set(destination.pointer, index_register, value_register); } _ => todo!("ICE: Instruction not supported {instruction:?}"), }; } - fn slice_intrinsic_call_operation( + /// Array set operation in SSA returns a new array or slice that is a copy of the parameter array or slice + /// With a specific value changed. + fn convert_ssa_array_set( + &mut self, + source_variable: RegisterOrMemory, + destination_variable: RegisterOrMemory, + index_register: RegisterIndex, + value_register: RegisterIndex, + ) { + let destination_pointer = match destination_variable { + RegisterOrMemory::HeapArray(HeapArray { pointer, .. }) => pointer, + RegisterOrMemory::HeapVector(HeapVector { pointer, .. }) => pointer, + _ => unreachable!("ICE: array set returns non-array"), + }; + + // First issue a array copy to the destination + let (source_pointer, source_size_as_register) = match source_variable { + RegisterOrMemory::HeapArray(HeapArray { size, pointer }) => { + let source_size_register = self.brillig_context.allocate_register(); + self.brillig_context.const_instruction(source_size_register, size.into()); + (pointer, source_size_register) + } + RegisterOrMemory::HeapVector(HeapVector { size, pointer }) => { + let source_size_register = self.brillig_context.allocate_register(); + self.brillig_context.mov_instruction(source_size_register, size); + (pointer, source_size_register) + } + _ => unreachable!("ICE: array set on non-array"), + }; + + self.brillig_context + .allocate_array_instruction(destination_pointer, source_size_as_register); + + self.brillig_context.copy_array_instruction( + source_pointer, + destination_pointer, + source_size_as_register, + ); + + if let RegisterOrMemory::HeapVector(HeapVector { size: target_size, .. }) = + destination_variable + { + self.brillig_context.mov_instruction(target_size, source_size_as_register); + } + + // Then set the value in the newly created array + self.brillig_context.array_set(destination_pointer, index_register, value_register); + + self.brillig_context.deallocate_register(source_size_as_register); + } + + /// Convert the SSA slice operations to brillig slice operations + fn convert_ssa_slice_intrinsic_call( &mut self, dfg: &DataFlowGraph, intrinsic: &Value, @@ -703,6 +746,7 @@ impl<'block> BrilligBlock<'block> { variable } + /// Converts an SSA `ValueId` into a `RegisterIndex`. Initializes if necessary. fn convert_ssa_register_value( &mut self, value_id: ValueId, diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs index a514c31408a..21a36d79d5a 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs @@ -89,6 +89,7 @@ impl FunctionContext { self.create_variable(brillig_context, value, dfg) } + /// Creates a variable that fits in a single register and returns the register. pub(crate) fn create_register_variable( &mut self, brillig_context: &mut BrilligContext, @@ -120,6 +121,7 @@ impl FunctionContext { } } + /// Collects the registers that a given variable is stored in. pub(crate) fn extract_registers(&self, variable: RegisterOrMemory) -> Vec { match variable { RegisterOrMemory::RegisterIndex(register_index) => vec![register_index], diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs index 5e77600166a..63a7b4b518a 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs @@ -42,6 +42,7 @@ pub(crate) fn slice_push_front_operation( destination_copy_pointer, source_vector.size, ); + brillig_context.deallocate_register(destination_copy_pointer); // Then we write the item to insert at index 0 let zero = brillig_context.make_constant(0_u128.into()); @@ -69,6 +70,7 @@ pub(crate) fn slice_pop_front_operation( target_vector.pointer, source_vector.size, ); + brillig_context.deallocate_register(source_copy_pointer); let zero = brillig_context.make_constant(0_u128.into()); brillig_context.array_get(source_vector.pointer, zero, removed_item); @@ -139,6 +141,10 @@ pub(crate) fn slice_insert_operation( item_count, ); + brillig_context.deallocate_register(source_pointer_at_index); + brillig_context.deallocate_register(target_pointer_after_index); + brillig_context.deallocate_register(item_count); + // Write the item to insert at the index brillig_context.array_set(target_vector.pointer, index, item); } @@ -188,6 +194,10 @@ pub(crate) fn slice_remove_operation( item_count, ); + brillig_context.deallocate_register(source_pointer_after_index); + brillig_context.deallocate_register(target_pointer_at_index); + brillig_context.deallocate_register(item_count); + // Get the item at the index brillig_context.array_get(source_vector.pointer, index, removed_item); } diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir.rs b/crates/noirc_evaluator/src/brillig/brillig_ir.rs index d0e3380e5f6..e662b7d2ea6 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir.rs @@ -494,6 +494,7 @@ impl BrilligContext { self.push_opcode(BrilligOpcode::Load { destination, source_pointer }); } + /// Loads a variable stored previously pub(crate) fn load_variable_instruction( &mut self, destination: RegisterOrMemory, @@ -547,6 +548,7 @@ impl BrilligContext { } } + /// Stores a variable by saving its registers to memory pub(crate) fn store_variable_instruction( &mut self, variable_pointer: RegisterIndex, From 7b9b5a9f4ada2076e9c5c6da02a63e0dd7b0d79b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Rodr=C3=ADguez?= Date: Fri, 14 Jul 2023 19:30:18 +0200 Subject: [PATCH 7/8] Update crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr --- .../test_data_ssa_refactor/brillig_slices/src/main.nr | 8 -------- 1 file changed, 8 deletions(-) diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr index 06697822410..7e4e8729199 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_slices/src/main.nr @@ -1,14 +1,6 @@ use dep::std::slice; use dep::std; -#[oracle(oracle_print_impl)] -unconstrained fn oracle_print(_x : Field) -> Field {} - -unconstrained fn oracle_print_wrapper(x: Field) { - oracle_print(x); -} - - unconstrained fn main(x: Field, y: Field) { // Mark it as mut so the compiler doesn't simplify the following operations // But don't reuse the mut slice variable until this is fixed https://github.com/noir-lang/noir/issues/1931 From 3eddc648e063c9af39aa08474f911edeae3ca9af Mon Sep 17 00:00:00 2001 From: sirasistant Date: Mon, 17 Jul 2023 08:43:12 +0000 Subject: [PATCH 8/8] test: added unit testing for slice ops --- .../brillig/brillig_gen/brillig_slice_ops.rs | 493 +++++++++++++++++- 1 file changed, 492 insertions(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs index 63a7b4b518a..1f9d56a0e4a 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs @@ -68,7 +68,7 @@ pub(crate) fn slice_pop_front_operation( brillig_context.copy_array_instruction( source_copy_pointer, target_vector.pointer, - source_vector.size, + target_vector.size, ); brillig_context.deallocate_register(source_copy_pointer); @@ -212,3 +212,494 @@ pub(crate) fn convert_array_or_vector_to_vector( _ => unreachable!("ICE: unsupported slice push back source {:?}", source_variable), } } + +#[cfg(test)] +mod tests { + use std::vec; + + use acvm::acir::brillig::{HeapVector, Value}; + use acvm::brillig_vm::brillig::{Opcode, RegisterIndex}; + use acvm::brillig_vm::{Registers, VMStatus, VM}; + use acvm::{BlackBoxFunctionSolver, BlackBoxResolutionError, FieldElement}; + + use crate::brillig::brillig_gen::brillig_slice_ops::{ + slice_insert_operation, slice_pop_back_operation, slice_pop_front_operation, + slice_push_back_operation, slice_push_front_operation, slice_remove_operation, + }; + use crate::brillig::brillig_ir::artifact::{BrilligArtifact, BrilligParameter}; + use crate::brillig::brillig_ir::BrilligContext; + + struct DummyBlackBoxSolver; + + impl BlackBoxFunctionSolver for DummyBlackBoxSolver { + fn schnorr_verify( + &self, + _public_key_x: &FieldElement, + _public_key_y: &FieldElement, + _signature: &[u8], + _message: &[u8], + ) -> Result { + Ok(true) + } + fn pedersen( + &self, + _inputs: &[FieldElement], + _domain_separator: u32, + ) -> Result<(FieldElement, FieldElement), BlackBoxResolutionError> { + Ok((2_u128.into(), 3_u128.into())) + } + fn fixed_base_scalar_mul( + &self, + _input: &FieldElement, + ) -> Result<(FieldElement, FieldElement), BlackBoxResolutionError> { + Ok((4_u128.into(), 5_u128.into())) + } + } + + fn create_context( + arguments: Vec, + returns: Vec, + ) -> BrilligContext { + let mut context = BrilligContext::new(arguments, returns); + context.enter_context("test"); + context + } + + fn create_entry_point_bytecode( + context: BrilligContext, + arguments: Vec, + returns: Vec, + ) -> Vec { + let artifact = context.artifact(); + let mut entry_point_artifact = + BrilligArtifact::new_entry_point_artifact(arguments, returns, "test".to_string()); + entry_point_artifact.link_with(&artifact); + entry_point_artifact.finish() + } + + fn create_and_run_vm( + memory: Vec, + param_registers: Vec, + context: BrilligContext, + arguments: Vec, + returns: Vec, + ) -> VM<'static, DummyBlackBoxSolver> { + let mut vm = VM::new( + Registers { inner: param_registers }, + memory, + create_entry_point_bytecode(context, arguments, returns), + vec![], + &DummyBlackBoxSolver, + ); + + let status = vm.process_opcodes(); + assert_eq!(status, VMStatus::Finished); + vm + } + + #[test] + fn test_slice_push_operation() { + fn test_case_push( + push_back: bool, + array: Vec, + expected_mem: Vec, + item_to_push: Value, + ) { + let arguments = + vec![BrilligParameter::HeapArray(array.len()), BrilligParameter::Register]; + let returns = + vec![BrilligParameter::HeapArray(array.len() + 1), BrilligParameter::Register]; + + let mut context = create_context(arguments.clone(), returns.clone()); + + // Allocate the parameters + let array_pointer = context.allocate_register(); + let item_to_insert = context.allocate_register(); + + // Cast the source array to a vector + let array_size = context.make_constant(array.len().into()); + + // Allocate the results + let copied_array_pointer = context.allocate_register(); + let copied_array_size = context.allocate_register(); + + if push_back { + slice_push_back_operation( + &mut context, + HeapVector { pointer: copied_array_pointer, size: copied_array_size }, + HeapVector { pointer: array_pointer, size: array_size }, + item_to_insert, + ); + } else { + slice_push_front_operation( + &mut context, + HeapVector { pointer: copied_array_pointer, size: copied_array_size }, + HeapVector { pointer: array_pointer, size: array_size }, + item_to_insert, + ); + } + + context.return_instruction(&[copied_array_pointer, copied_array_size]); + + let vm = create_and_run_vm( + array.clone(), + vec![Value::from(0_usize), item_to_push], + context, + arguments, + returns, + ); + + assert_eq!(vm.get_memory(), &expected_mem); + + assert_eq!(vm.get_registers().get(RegisterIndex(0)), Value::from(array.len())); + assert_eq!(vm.get_registers().get(RegisterIndex(1)), Value::from(array.len() + 1)); + } + + test_case_push( + true, + vec![Value::from(1_usize), Value::from(2_usize), Value::from(3_usize)], + vec![ + Value::from(1_usize), + Value::from(2_usize), + Value::from(3_usize), + Value::from(1_usize), + Value::from(2_usize), + Value::from(3_usize), + Value::from(27_usize), + ], + Value::from(27_usize), + ); + test_case_push(true, vec![], vec![Value::from(27_usize)], Value::from(27_usize)); + test_case_push( + false, + vec![Value::from(1_usize), Value::from(2_usize), Value::from(3_usize)], + vec![ + Value::from(1_usize), + Value::from(2_usize), + Value::from(3_usize), + Value::from(27_usize), + Value::from(1_usize), + Value::from(2_usize), + Value::from(3_usize), + ], + Value::from(27_usize), + ); + test_case_push(false, vec![], vec![Value::from(27_usize)], Value::from(27_usize)); + } + + #[test] + fn test_slice_pop_back_operation() { + fn test_case_pop( + pop_back: bool, + array: Vec, + expected_mem: Vec, + expected_removed_item: Value, + ) { + let arguments = vec![BrilligParameter::HeapArray(array.len())]; + let returns = vec![ + BrilligParameter::HeapArray(array.len() - 1), + BrilligParameter::Register, + BrilligParameter::Register, + ]; + + let mut context = create_context(arguments.clone(), returns.clone()); + + // Allocate the parameters + let array_pointer = context.allocate_register(); + + // Cast the source array to a vector + let array_size = context.make_constant(array.len().into()); + + // Allocate the results + let copied_array_pointer = context.allocate_register(); + let removed_item = context.allocate_register(); + + let copied_array_size = context.allocate_register(); + + if pop_back { + slice_pop_back_operation( + &mut context, + HeapVector { pointer: copied_array_pointer, size: copied_array_size }, + HeapVector { pointer: array_pointer, size: array_size }, + removed_item, + ); + } else { + slice_pop_front_operation( + &mut context, + HeapVector { pointer: copied_array_pointer, size: copied_array_size }, + HeapVector { pointer: array_pointer, size: array_size }, + removed_item, + ); + } + + context.return_instruction(&[copied_array_pointer, copied_array_size, removed_item]); + + let vm = create_and_run_vm( + array.clone(), + vec![Value::from(0_usize)], + context, + arguments, + returns, + ); + + assert_eq!(vm.get_memory(), &expected_mem); + + assert_eq!(vm.get_registers().get(RegisterIndex(0)), Value::from(array.len())); + assert_eq!(vm.get_registers().get(RegisterIndex(1)), Value::from(array.len() - 1)); + assert_eq!(vm.get_registers().get(RegisterIndex(2)), expected_removed_item); + } + + test_case_pop( + true, + vec![Value::from(1_usize), Value::from(2_usize), Value::from(3_usize)], + vec![ + Value::from(1_usize), + Value::from(2_usize), + Value::from(3_usize), + Value::from(1_usize), + Value::from(2_usize), + ], + Value::from(3_usize), + ); + test_case_pop( + true, + vec![Value::from(1_usize)], + vec![Value::from(1_usize)], + Value::from(1_usize), + ); + test_case_pop( + false, + vec![Value::from(1_usize), Value::from(2_usize), Value::from(3_usize)], + vec![ + Value::from(1_usize), + Value::from(2_usize), + Value::from(3_usize), + Value::from(2_usize), + Value::from(3_usize), + ], + Value::from(1_usize), + ); + } + + #[test] + fn test_slice_insert_operation() { + fn test_case_insert( + array: Vec, + expected_mem: Vec, + item: Value, + index: Value, + ) { + let arguments = vec![ + BrilligParameter::HeapArray(array.len()), + BrilligParameter::Register, + BrilligParameter::Register, + ]; + let returns = + vec![BrilligParameter::HeapArray(array.len() + 1), BrilligParameter::Register]; + + let mut context = create_context(arguments.clone(), returns.clone()); + + // Allocate the parameters + let array_pointer = context.allocate_register(); + let item_to_insert = context.allocate_register(); + let index_to_insert = context.allocate_register(); + + // Cast the source array to a vector + let array_size = context.make_constant(array.len().into()); + + // Allocate the results + let copied_array_pointer = context.allocate_register(); + + let copied_array_size = context.allocate_register(); + + slice_insert_operation( + &mut context, + HeapVector { pointer: copied_array_pointer, size: copied_array_size }, + HeapVector { pointer: array_pointer, size: array_size }, + index_to_insert, + item_to_insert, + ); + + context.return_instruction(&[copied_array_pointer, copied_array_size]); + + let vm = create_and_run_vm( + array.clone(), + vec![Value::from(0_usize), item, index], + context, + arguments, + returns, + ); + + assert_eq!(vm.get_memory(), &expected_mem); + + assert_eq!(vm.get_registers().get(RegisterIndex(0)), Value::from(array.len())); + assert_eq!(vm.get_registers().get(RegisterIndex(1)), Value::from(array.len() + 1)); + } + + test_case_insert( + vec![Value::from(1_usize), Value::from(2_usize), Value::from(3_usize)], + vec![ + Value::from(1_usize), + Value::from(2_usize), + Value::from(3_usize), + Value::from(1_usize), + Value::from(27_usize), + Value::from(2_usize), + Value::from(3_usize), + ], + Value::from(27_usize), + Value::from(1_usize), + ); + + test_case_insert( + vec![Value::from(1_usize), Value::from(2_usize), Value::from(3_usize)], + vec![ + Value::from(1_usize), + Value::from(2_usize), + Value::from(3_usize), + Value::from(27_usize), + Value::from(1_usize), + Value::from(2_usize), + Value::from(3_usize), + ], + Value::from(27_usize), + Value::from(0_usize), + ); + test_case_insert( + vec![Value::from(1_usize), Value::from(2_usize), Value::from(3_usize)], + vec![ + Value::from(1_usize), + Value::from(2_usize), + Value::from(3_usize), + Value::from(1_usize), + Value::from(2_usize), + Value::from(27_usize), + Value::from(3_usize), + ], + Value::from(27_usize), + Value::from(2_usize), + ); + test_case_insert( + vec![Value::from(1_usize), Value::from(2_usize), Value::from(3_usize)], + vec![ + Value::from(1_usize), + Value::from(2_usize), + Value::from(3_usize), + Value::from(1_usize), + Value::from(2_usize), + Value::from(3_usize), + Value::from(27_usize), + ], + Value::from(27_usize), + Value::from(3_usize), + ); + test_case_insert( + vec![], + vec![Value::from(27_usize)], + Value::from(27_usize), + Value::from(0_usize), + ); + } + + #[test] + fn test_slice_remove_operation() { + fn test_case_remove( + array: Vec, + expected_mem: Vec, + index: Value, + expected_removed_item: Value, + ) { + let arguments = + vec![BrilligParameter::HeapArray(array.len()), BrilligParameter::Register]; + let returns = vec![ + BrilligParameter::HeapArray(array.len() - 1), + BrilligParameter::Register, + BrilligParameter::Register, + ]; + + let mut context = create_context(arguments.clone(), returns.clone()); + + // Allocate the parameters + let array_pointer = context.allocate_register(); + let index_to_insert = context.allocate_register(); + + // Cast the source array to a vector + let array_size = context.make_constant(array.len().into()); + + // Allocate the results + let copied_array_pointer = context.allocate_register(); + let removed_item = context.allocate_register(); + + let copied_array_size = context.allocate_register(); + + slice_remove_operation( + &mut context, + HeapVector { pointer: copied_array_pointer, size: copied_array_size }, + HeapVector { pointer: array_pointer, size: array_size }, + index_to_insert, + removed_item, + ); + + context.return_instruction(&[copied_array_pointer, copied_array_size, removed_item]); + + let vm = create_and_run_vm( + array.clone(), + vec![Value::from(0_usize), index], + context, + arguments, + returns, + ); + + assert_eq!(vm.get_memory(), &expected_mem); + + assert_eq!(vm.get_registers().get(RegisterIndex(0)), Value::from(array.len())); + assert_eq!(vm.get_registers().get(RegisterIndex(1)), Value::from(array.len() - 1)); + assert_eq!(vm.get_registers().get(RegisterIndex(2)), expected_removed_item); + } + + test_case_remove( + vec![Value::from(1_usize), Value::from(2_usize), Value::from(3_usize)], + vec![ + Value::from(1_usize), + Value::from(2_usize), + Value::from(3_usize), + Value::from(2_usize), + Value::from(3_usize), + ], + Value::from(0_usize), + Value::from(1_usize), + ); + + test_case_remove( + vec![Value::from(1_usize), Value::from(2_usize), Value::from(3_usize)], + vec![ + Value::from(1_usize), + Value::from(2_usize), + Value::from(3_usize), + Value::from(1_usize), + Value::from(3_usize), + ], + Value::from(1_usize), + Value::from(2_usize), + ); + + test_case_remove( + vec![Value::from(1_usize), Value::from(2_usize), Value::from(3_usize)], + vec![ + Value::from(1_usize), + Value::from(2_usize), + Value::from(3_usize), + Value::from(1_usize), + Value::from(2_usize), + ], + Value::from(2_usize), + Value::from(3_usize), + ); + test_case_remove( + vec![Value::from(1_usize)], + vec![Value::from(1_usize)], + Value::from(0_usize), + Value::from(1_usize), + ); + } +}