From efa5cc4bf173b0ce49f47b1954165a2bdb276792 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 21 Nov 2024 12:03:43 -0300 Subject: [PATCH 01/11] fix: correct type when simplifying `derive_pedersen_generators` (#6579) --- .../src/ssa/ir/instruction/call.rs | 34 ++++++++++++++++++- .../noirc_evaluator/src/ssa/parser/mod.rs | 5 +++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 95447f941b0..7e41512fd8f 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -806,7 +806,8 @@ fn simplify_derive_generators( results.push(is_infinite); } let len = results.len(); - let typ = Type::Array(vec![Type::field()].into(), len); + let typ = + Type::Array(vec![Type::field(), Type::field(), Type::unsigned(1)].into(), len / 3); let result = make_array(dfg, results.into(), typ, block, call_stack); SimplifyResult::SimplifiedTo(result) } else { @@ -816,3 +817,34 @@ fn simplify_derive_generators( unreachable!("Unexpected number of arguments to derive_generators"); } } + +#[cfg(test)] +mod tests { + use crate::ssa::{opt::assert_normalized_ssa_equals, Ssa}; + + #[test] + fn simplify_derive_generators_has_correct_type() { + let src = " + brillig(inline) fn main f0 { + b0(): + v0 = make_array [u8 68, u8 69, u8 70, u8 65, u8 85, u8 76, u8 84, u8 95, u8 68, u8 79, u8 77, u8 65, u8 73, u8 78, u8 95, u8 83, u8 69, u8 80, u8 65, u8 82, u8 65, u8 84, u8 79, u8 82] : [u8; 24] + + // This call was previously incorrectly simplified to something that returned `[Field; 3]` + v2 = call derive_pedersen_generators(v0, u32 0) -> [(Field, Field, u1); 1] + + return v2 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + + let expected = " + brillig(inline) fn main f0 { + b0(): + v15 = make_array [u8 68, u8 69, u8 70, u8 65, u8 85, u8 76, u8 84, u8 95, u8 68, u8 79, u8 77, u8 65, u8 73, u8 78, u8 95, u8 83, u8 69, u8 80, u8 65, u8 82, u8 65, u8 84, u8 79, u8 82] : [u8; 24] + v19 = make_array [Field 3728882899078719075161482178784387565366481897740339799480980287259621149274, Field -9903063709032878667290627648209915537972247634463802596148419711785767431332, u1 0] : [(Field, Field, u1); 1] + return v19 + } + "; + assert_normalized_ssa_equals(ssa, expected); + } +} diff --git a/compiler/noirc_evaluator/src/ssa/parser/mod.rs b/compiler/noirc_evaluator/src/ssa/parser/mod.rs index 2db2c636a8f..7753908b2bd 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/mod.rs @@ -28,6 +28,11 @@ mod tests; mod token; impl Ssa { + /// Creates an Ssa object from the given string. + /// + /// Note that the resulting Ssa might not be exactly the same as the given string. + /// This is because, internally, the Ssa is built using a `FunctionBuilder`, so + /// some instructions might be simplified while they are inserted. pub(crate) fn from_str(src: &str) -> Result { let mut parser = Parser::new(src).map_err(|err| SsaErrorWithSource::parse_error(err, src))?; From e4c66b91d42b20d17837fe5e7c32c9a83b6ab354 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 21 Nov 2024 12:33:13 -0300 Subject: [PATCH 02/11] feat: try to inline brillig calls with all constant arguments (#6548) --- compiler/noirc_evaluator/src/acir/mod.rs | 52 +- .../src/brillig/brillig_gen.rs | 54 +- compiler/noirc_evaluator/src/ssa.rs | 17 + .../src/ssa/opt/constant_folding.rs | 529 +++++++++++++++++- 4 files changed, 579 insertions(+), 73 deletions(-) diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index 5c7899b5035..46d0924b322 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -24,12 +24,10 @@ mod big_int; mod brillig_directive; mod generated_acir; +use crate::brillig::brillig_gen::gen_brillig_for; use crate::brillig::{ brillig_gen::brillig_fn::FunctionContext as BrilligFunctionContext, - brillig_ir::{ - artifact::{BrilligParameter, GeneratedBrillig}, - BrilligContext, - }, + brillig_ir::artifact::{BrilligParameter, GeneratedBrillig}, Brillig, }; use crate::errors::{InternalError, InternalWarning, RuntimeError, SsaReport}; @@ -518,7 +516,7 @@ impl<'a> Context<'a> { let outputs: Vec = vecmap(main_func.returns(), |result_id| dfg.type_of_value(*result_id).into()); - let code = self.gen_brillig_for(main_func, arguments.clone(), brillig)?; + let code = gen_brillig_for(main_func, arguments.clone(), brillig)?; // We specifically do not attempt execution of the brillig code being generated as this can result in it being // replaced with constraints on witnesses to the program outputs. @@ -878,8 +876,7 @@ impl<'a> Context<'a> { None, )? } else { - let code = - self.gen_brillig_for(func, arguments.clone(), brillig)?; + let code = gen_brillig_for(func, arguments.clone(), brillig)?; let generated_pointer = self.shared_context.new_generated_pointer(); let output_values = self.acir_context.brillig_call( @@ -999,47 +996,6 @@ impl<'a> Context<'a> { .collect() } - fn gen_brillig_for( - &self, - func: &Function, - arguments: Vec, - brillig: &Brillig, - ) -> Result, InternalError> { - // Create the entry point artifact - let mut entry_point = BrilligContext::new_entry_point_artifact( - arguments, - BrilligFunctionContext::return_values(func), - func.id(), - ); - entry_point.name = func.name().to_string(); - - // Link the entry point with all dependencies - while let Some(unresolved_fn_label) = entry_point.first_unresolved_function_call() { - let artifact = &brillig.find_by_label(unresolved_fn_label.clone()); - let artifact = match artifact { - Some(artifact) => artifact, - None => { - return Err(InternalError::General { - message: format!("Cannot find linked fn {unresolved_fn_label}"), - call_stack: CallStack::new(), - }) - } - }; - entry_point.link_with(artifact); - // Insert the range of opcode locations occupied by a procedure - if let Some(procedure_id) = &artifact.procedure { - let num_opcodes = entry_point.byte_code.len(); - let previous_num_opcodes = entry_point.byte_code.len() - artifact.byte_code.len(); - // We subtract one as to keep the range inclusive on both ends - entry_point - .procedure_locations - .insert(procedure_id.clone(), (previous_num_opcodes, num_opcodes - 1)); - } - } - // Generate the final bytecode - Ok(entry_point.finish()) - } - /// Handles an ArrayGet or ArraySet instruction. /// To set an index of the array (and create a new array in doing so), pass Some(value) for /// store_value. To just retrieve an index of the array, pass None for store_value. diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen.rs index 786a03031d6..ca4e783aa93 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen.rs @@ -9,11 +9,17 @@ mod variable_liveness; use acvm::FieldElement; use self::{brillig_block::BrilligBlock, brillig_fn::FunctionContext}; -use super::brillig_ir::{ - artifact::{BrilligArtifact, Label}, - BrilligContext, +use super::{ + brillig_ir::{ + artifact::{BrilligArtifact, BrilligParameter, GeneratedBrillig, Label}, + BrilligContext, + }, + Brillig, +}; +use crate::{ + errors::InternalError, + ssa::ir::{dfg::CallStack, function::Function}, }; -use crate::ssa::ir::function::Function; /// Converting an SSA function into Brillig bytecode. pub(crate) fn convert_ssa_function( @@ -36,3 +42,43 @@ pub(crate) fn convert_ssa_function( artifact.name = func.name().to_string(); artifact } + +pub(crate) fn gen_brillig_for( + func: &Function, + arguments: Vec, + brillig: &Brillig, +) -> Result, InternalError> { + // Create the entry point artifact + let mut entry_point = BrilligContext::new_entry_point_artifact( + arguments, + FunctionContext::return_values(func), + func.id(), + ); + entry_point.name = func.name().to_string(); + + // Link the entry point with all dependencies + while let Some(unresolved_fn_label) = entry_point.first_unresolved_function_call() { + let artifact = &brillig.find_by_label(unresolved_fn_label.clone()); + let artifact = match artifact { + Some(artifact) => artifact, + None => { + return Err(InternalError::General { + message: format!("Cannot find linked fn {unresolved_fn_label}"), + call_stack: CallStack::new(), + }) + } + }; + entry_point.link_with(artifact); + // Insert the range of opcode locations occupied by a procedure + if let Some(procedure_id) = &artifact.procedure { + let num_opcodes = entry_point.byte_code.len(); + let previous_num_opcodes = entry_point.byte_code.len() - artifact.byte_code.len(); + // We subtract one as to keep the range inclusive on both ends + entry_point + .procedure_locations + .insert(procedure_id.clone(), (previous_num_opcodes, num_opcodes - 1)); + } + } + // Generate the final bytecode + Ok(entry_point.finish()) +} diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 9e11441caf4..f0a6fcd986f 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -140,6 +140,23 @@ pub(crate) fn optimize_into_acir( ssa.to_brillig(options.enable_brillig_logging) }); + let ssa_gen_span = span!(Level::TRACE, "ssa_generation"); + let ssa_gen_span_guard = ssa_gen_span.enter(); + + let ssa = SsaBuilder { + ssa, + print_ssa_passes: options.enable_ssa_logging, + print_codegen_timings: options.print_codegen_timings, + } + .run_pass( + |ssa| ssa.fold_constants_with_brillig(&brillig), + "After Constant Folding with Brillig:", + ) + .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") + .finish(); + + drop(ssa_gen_span_guard); + let artifacts = time("SSA to ACIR", options.print_codegen_timings, || { ssa.into_acir(&brillig, options.expression_width) })?; diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 32f66e5a0f0..019bace33a3 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -19,22 +19,35 @@ //! //! This is the only pass which removes duplicated pure [`Instruction`]s however and so is needed when //! different blocks are merged, i.e. after the [`flatten_cfg`][super::flatten_cfg] pass. -use std::collections::{HashSet, VecDeque}; +use std::collections::{BTreeMap, HashSet, VecDeque}; -use acvm::{acir::AcirField, FieldElement}; +use acvm::{ + acir::AcirField, + brillig_vm::{MemoryValue, VMStatus, VM}, + FieldElement, +}; +use bn254_blackbox_solver::Bn254BlackBoxSolver; +use im::Vector; use iter_extended::vecmap; -use crate::ssa::{ - ir::{ - basic_block::BasicBlockId, - dfg::{DataFlowGraph, InsertInstructionResult}, - dom::DominatorTree, - function::Function, - instruction::{Instruction, InstructionId}, - types::Type, - value::{Value, ValueId}, +use crate::{ + brillig::{ + brillig_gen::gen_brillig_for, + brillig_ir::{artifact::BrilligParameter, brillig_variable::get_bit_size_from_ssa_type}, + Brillig, + }, + ssa::{ + ir::{ + basic_block::BasicBlockId, + dfg::{DataFlowGraph, InsertInstructionResult}, + dom::DominatorTree, + function::{Function, FunctionId, RuntimeType}, + instruction::{Instruction, InstructionId}, + types::Type, + value::{Value, ValueId}, + }, + ssa_gen::Ssa, }, - ssa_gen::Ssa, }; use fxhash::FxHashMap as HashMap; @@ -45,7 +58,7 @@ impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn fold_constants(mut self) -> Ssa { for function in self.functions.values_mut() { - function.constant_fold(false); + function.constant_fold(false, None); } self } @@ -58,17 +71,82 @@ impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn fold_constants_using_constraints(mut self) -> Ssa { for function in self.functions.values_mut() { - function.constant_fold(true); + function.constant_fold(true, None); } self } + + /// Performs constant folding on each instruction while also replacing calls to brillig functions + /// with all constant arguments by trying to evaluate those calls. + #[tracing::instrument(level = "trace", skip(self, brillig))] + pub(crate) fn fold_constants_with_brillig(mut self, brillig: &Brillig) -> Ssa { + // Collect all brillig functions so that later we can find them when processing a call instruction + let mut brillig_functions: BTreeMap = BTreeMap::new(); + for (func_id, func) in &self.functions { + if let RuntimeType::Brillig(..) = func.runtime() { + let cloned_function = Function::clone_with_id(*func_id, func); + brillig_functions.insert(*func_id, cloned_function); + }; + } + + let brillig_info = Some(BrilligInfo { brillig, brillig_functions: &brillig_functions }); + + for function in self.functions.values_mut() { + function.constant_fold(false, brillig_info); + } + + // It could happen that we inlined all calls to a given brillig function. + // In that case it's unused so we can remove it. This is what we check next. + self.remove_unused_brillig_functions(brillig_functions) + } + + fn remove_unused_brillig_functions( + mut self, + mut brillig_functions: BTreeMap, + ) -> Ssa { + // Remove from the above map functions that are called + for function in self.functions.values() { + for block_id in function.reachable_blocks() { + for instruction_id in function.dfg[block_id].instructions() { + let instruction = &function.dfg[*instruction_id]; + let Instruction::Call { func: func_id, arguments: _ } = instruction else { + continue; + }; + + let func_value = &function.dfg[*func_id]; + let Value::Function(func_id) = func_value else { continue }; + + brillig_functions.remove(func_id); + } + } + } + + // The ones that remain are never called: let's remove them. + for func_id in brillig_functions.keys() { + // We never want to remove the main function (it could be `unconstrained` or it + // could have been turned into brillig if `--force-brillig` was given). + // We also don't want to remove entry points. + if self.main_id == *func_id || self.entry_point_to_generated_index.contains_key(func_id) + { + continue; + } + + self.functions.remove(func_id); + } + + self + } } impl Function { /// The structure of this pass is simple: /// Go through each block and re-insert all instructions. - pub(crate) fn constant_fold(&mut self, use_constraint_info: bool) { - let mut context = Context::new(self, use_constraint_info); + pub(crate) fn constant_fold( + &mut self, + use_constraint_info: bool, + brillig_info: Option, + ) { + let mut context = Context::new(self, use_constraint_info, brillig_info); context.block_queue.push_back(self.entry_block()); while let Some(block) = context.block_queue.pop_front() { @@ -82,8 +160,9 @@ impl Function { } } -struct Context { +struct Context<'a> { use_constraint_info: bool, + brillig_info: Option>, /// Maps pre-folded ValueIds to the new ValueIds obtained by re-inserting the instruction. visited_blocks: HashSet, block_queue: VecDeque, @@ -103,6 +182,12 @@ struct Context { dom: DominatorTree, } +#[derive(Copy, Clone)] +pub(crate) struct BrilligInfo<'a> { + brillig: &'a Brillig, + brillig_functions: &'a BTreeMap, +} + /// HashMap from (Instruction, side_effects_enabled_var) to the results of the instruction. /// Stored as a two-level map to avoid cloning Instructions during the `.get` call. /// @@ -118,10 +203,15 @@ struct ResultCache { result: Option<(BasicBlockId, Vec)>, } -impl Context { - fn new(function: &Function, use_constraint_info: bool) -> Self { +impl<'brillig> Context<'brillig> { + fn new( + function: &Function, + use_constraint_info: bool, + brillig_info: Option>, + ) -> Self { Self { use_constraint_info, + brillig_info, visited_blocks: Default::default(), block_queue: Default::default(), constraint_simplification_mappings: Default::default(), @@ -177,8 +267,25 @@ impl Context { } } - // Otherwise, try inserting the instruction again to apply any optimizations using the newly resolved inputs. - let new_results = Self::push_instruction(id, instruction.clone(), &old_results, block, dfg); + let new_results = + // First try to inline a call to a brillig function with all constant arguments. + Self::try_inline_brillig_call_with_all_constants( + &instruction, + &old_results, + block, + dfg, + self.brillig_info, + ) + .unwrap_or_else(|| { + // Otherwise, try inserting the instruction again to apply any optimizations using the newly resolved inputs. + Self::push_instruction( + id, + instruction.clone(), + &old_results, + block, + dfg, + ) + }); Self::replace_result_ids(dfg, &old_results, &new_results); @@ -342,6 +449,166 @@ impl Context { results_for_instruction.get(&predicate)?.get(block, &mut self.dom) } + + /// Checks if the given instruction is a call to a brillig function with all constant arguments. + /// If so, we can try to evaluate that function and replace the results with the evaluation results. + fn try_inline_brillig_call_with_all_constants( + instruction: &Instruction, + old_results: &[ValueId], + block: BasicBlockId, + dfg: &mut DataFlowGraph, + brillig_info: Option, + ) -> Option> { + let evaluation_result = Self::evaluate_const_brillig_call( + instruction, + brillig_info?.brillig, + brillig_info?.brillig_functions, + dfg, + ); + + match evaluation_result { + EvaluationResult::NotABrilligCall | EvaluationResult::CannotEvaluate(_) => None, + EvaluationResult::Evaluated(memory_values) => { + let mut memory_index = 0; + let new_results = vecmap(old_results, |old_result| { + let typ = dfg.type_of_value(*old_result); + Self::new_value_for_type_and_memory_values( + typ, + block, + &memory_values, + &mut memory_index, + dfg, + ) + }); + Some(new_results) + } + } + } + + /// Tries to evaluate an instruction if it's a call that points to a brillig function, + /// and all its arguments are constant. + /// We do this by directly executing the function with a brillig VM. + fn evaluate_const_brillig_call( + instruction: &Instruction, + brillig: &Brillig, + brillig_functions: &BTreeMap, + dfg: &mut DataFlowGraph, + ) -> EvaluationResult { + let Instruction::Call { func: func_id, arguments } = instruction else { + return EvaluationResult::NotABrilligCall; + }; + + let func_value = &dfg[*func_id]; + let Value::Function(func_id) = func_value else { + return EvaluationResult::NotABrilligCall; + }; + + let Some(func) = brillig_functions.get(func_id) else { + return EvaluationResult::NotABrilligCall; + }; + + if !arguments.iter().all(|argument| dfg.is_constant(*argument)) { + return EvaluationResult::CannotEvaluate(*func_id); + } + + let mut brillig_arguments = Vec::new(); + for argument in arguments { + let typ = dfg.type_of_value(*argument); + let Some(parameter) = type_to_brillig_parameter(&typ) else { + return EvaluationResult::CannotEvaluate(*func_id); + }; + brillig_arguments.push(parameter); + } + + // Check that return value types are supported by brillig + for return_id in func.returns().iter() { + let typ = func.dfg.type_of_value(*return_id); + if type_to_brillig_parameter(&typ).is_none() { + return EvaluationResult::CannotEvaluate(*func_id); + } + } + + let Ok(generated_brillig) = gen_brillig_for(func, brillig_arguments, brillig) else { + return EvaluationResult::CannotEvaluate(*func_id); + }; + + let mut calldata = Vec::new(); + for argument in arguments { + value_id_to_calldata(*argument, dfg, &mut calldata); + } + + let bytecode = &generated_brillig.byte_code; + let foreign_call_results = Vec::new(); + let black_box_solver = Bn254BlackBoxSolver; + let profiling_active = false; + let mut vm = + VM::new(calldata, bytecode, foreign_call_results, &black_box_solver, profiling_active); + let vm_status: VMStatus<_> = vm.process_opcodes(); + let VMStatus::Finished { return_data_offset, return_data_size } = vm_status else { + return EvaluationResult::CannotEvaluate(*func_id); + }; + + let memory = + vm.get_memory()[return_data_offset..(return_data_offset + return_data_size)].to_vec(); + + EvaluationResult::Evaluated(memory) + } + + /// Creates a new value inside this function by reading it from `memory_values` starting at + /// `memory_index` depending on the given Type: if it's an array multiple values will be read + /// and a new `make_array` instruction will be created. + fn new_value_for_type_and_memory_values( + typ: Type, + block_id: BasicBlockId, + memory_values: &[MemoryValue], + memory_index: &mut usize, + dfg: &mut DataFlowGraph, + ) -> ValueId { + match typ { + Type::Numeric(_) => { + let memory = memory_values[*memory_index]; + *memory_index += 1; + + let field_value = match memory { + MemoryValue::Field(field_value) => field_value, + MemoryValue::Integer(u128_value, _) => u128_value.into(), + }; + dfg.make_constant(field_value, typ) + } + Type::Array(types, length) => { + let mut new_array_values = Vector::new(); + for _ in 0..length { + for typ in types.iter() { + let new_value = Self::new_value_for_type_and_memory_values( + typ.clone(), + block_id, + memory_values, + memory_index, + dfg, + ); + new_array_values.push_back(new_value); + } + } + + let instruction = Instruction::MakeArray { + elements: new_array_values, + typ: Type::Array(types, length), + }; + let instruction_id = dfg.make_instruction(instruction, None); + dfg[block_id].instructions_mut().push(instruction_id); + *dfg.instruction_results(instruction_id).first().unwrap() + } + Type::Reference(_) => { + panic!("Unexpected reference type in brillig function result") + } + Type::Slice(_) => { + panic!("Unexpected slice type in brillig function result") + } + Type::Function => { + panic!("Unexpected function type in brillig function result") + } + } + } } impl ResultCache { @@ -376,6 +643,50 @@ enum CacheResult<'a> { NeedToHoistToCommonBlock(BasicBlockId, &'a [ValueId]), } +/// Result of trying to evaluate an instruction (any instruction) in this pass. +enum EvaluationResult { + /// Nothing was done because the instruction wasn't a call to a brillig function, + /// or some arguments to it were not constants. + NotABrilligCall, + /// The instruction was a call to a brillig function, but we couldn't evaluate it. + /// This can occur in the situation where the brillig function reaches a "trap" or a foreign call opcode. + CannotEvaluate(FunctionId), + /// The instruction was a call to a brillig function and we were able to evaluate it, + /// returning evaluation memory values. + Evaluated(Vec>), +} + +/// Similar to FunctionContext::ssa_type_to_parameter but never panics and disallows reference types. +pub(crate) fn type_to_brillig_parameter(typ: &Type) -> Option { + match typ { + Type::Numeric(_) => Some(BrilligParameter::SingleAddr(get_bit_size_from_ssa_type(typ))), + Type::Array(item_type, size) => { + let mut parameters = Vec::with_capacity(item_type.len()); + for item_typ in item_type.iter() { + parameters.push(type_to_brillig_parameter(item_typ)?); + } + Some(BrilligParameter::Array(parameters, *size)) + } + _ => None, + } +} + +fn value_id_to_calldata(value_id: ValueId, dfg: &DataFlowGraph, calldata: &mut Vec) { + if let Some(value) = dfg.get_numeric_constant(value_id) { + calldata.push(value); + return; + } + + if let Some((values, _type)) = dfg.get_array_constant(value_id) { + for value in values { + value_id_to_calldata(value, dfg, calldata); + } + return; + } + + panic!("Expected ValueId to be numeric constant or array constant"); +} + #[cfg(test)] mod test { use std::sync::Arc; @@ -854,4 +1165,180 @@ mod test { let ssa = ssa.fold_constants_using_constraints(); assert_normalized_ssa_equals(ssa, expected); } + + #[test] + fn inlines_brillig_call_without_arguments() { + let src = " + acir(inline) fn main f0 { + b0(): + v0 = call f1() -> Field + return v0 + } + + brillig(inline) fn one f1 { + b0(): + v0 = add Field 2, Field 3 + return v0 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let brillig = ssa.to_brillig(false); + + let expected = " + acir(inline) fn main f0 { + b0(): + return Field 5 + } + "; + let ssa = ssa.fold_constants_with_brillig(&brillig); + assert_normalized_ssa_equals(ssa, expected); + } + + #[test] + fn inlines_brillig_call_with_two_field_arguments() { + let src = " + acir(inline) fn main f0 { + b0(): + v0 = call f1(Field 2, Field 3) -> Field + return v0 + } + + brillig(inline) fn one f1 { + b0(v0: Field, v1: Field): + v2 = add v0, v1 + return v2 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let brillig = ssa.to_brillig(false); + + let expected = " + acir(inline) fn main f0 { + b0(): + return Field 5 + } + "; + let ssa = ssa.fold_constants_with_brillig(&brillig); + assert_normalized_ssa_equals(ssa, expected); + } + + #[test] + fn inlines_brillig_call_with_two_i32_arguments() { + let src = " + acir(inline) fn main f0 { + b0(): + v0 = call f1(i32 2, i32 3) -> i32 + return v0 + } + + brillig(inline) fn one f1 { + b0(v0: i32, v1: i32): + v2 = add v0, v1 + return v2 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let brillig = ssa.to_brillig(false); + + let expected = " + acir(inline) fn main f0 { + b0(): + return i32 5 + } + "; + let ssa = ssa.fold_constants_with_brillig(&brillig); + assert_normalized_ssa_equals(ssa, expected); + } + + #[test] + fn inlines_brillig_call_with_array_return() { + let src = " + acir(inline) fn main f0 { + b0(): + v0 = call f1(Field 2, Field 3, Field 4) -> [Field; 3] + return v0 + } + + brillig(inline) fn one f1 { + b0(v0: Field, v1: Field, v2: Field): + v3 = make_array [v0, v1, v2] : [Field; 3] + return v3 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let brillig = ssa.to_brillig(false); + + let expected = " + acir(inline) fn main f0 { + b0(): + v3 = make_array [Field 2, Field 3, Field 4] : [Field; 3] + return v3 + } + "; + let ssa = ssa.fold_constants_with_brillig(&brillig); + assert_normalized_ssa_equals(ssa, expected); + } + + #[test] + fn inlines_brillig_call_with_composite_array_return() { + let src = " + acir(inline) fn main f0 { + b0(): + v0 = call f1(Field 2, i32 3, Field 4, i32 5) -> [(Field, i32); 2] + return v0 + } + + brillig(inline) fn one f1 { + b0(v0: Field, v1: i32, v2: i32, v3: Field): + v4 = make_array [v0, v1, v2, v3] : [(Field, i32); 2] + return v4 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let brillig = ssa.to_brillig(false); + + let expected = " + acir(inline) fn main f0 { + b0(): + v4 = make_array [Field 2, i32 3, Field 4, i32 5] : [(Field, i32); 2] + return v4 + } + "; + let ssa = ssa.fold_constants_with_brillig(&brillig); + assert_normalized_ssa_equals(ssa, expected); + } + + #[test] + fn inlines_brillig_call_with_array_arguments() { + let src = " + acir(inline) fn main f0 { + b0(): + v0 = make_array [Field 2, Field 3] : [Field; 2] + v1 = call f1(v0) -> Field + return v1 + } + + brillig(inline) fn one f1 { + b0(v0: [Field; 2]): + inc_rc v0 + v2 = array_get v0, index u32 0 -> Field + v4 = array_get v0, index u32 1 -> Field + v5 = add v2, v4 + dec_rc v0 + return v5 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let brillig = ssa.to_brillig(false); + + let expected = " + acir(inline) fn main f0 { + b0(): + v2 = make_array [Field 2, Field 3] : [Field; 2] + return Field 5 + } + "; + let ssa = ssa.fold_constants_with_brillig(&brillig); + assert_normalized_ssa_equals(ssa, expected); + } } From 013e2000f1d7e7346b5cac0427732d545f501444 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Thu, 21 Nov 2024 18:39:43 +0000 Subject: [PATCH 03/11] fix: consider prereleases to be compatible with pre-1.0.0 releases (#6580) --- tooling/nargo_toml/src/errors.rs | 2 ++ tooling/nargo_toml/src/semver.rs | 49 ++++++++++++++++++++++++++++---- 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/tooling/nargo_toml/src/errors.rs b/tooling/nargo_toml/src/errors.rs index 1ee8e90c8e5..7e1003d04f7 100644 --- a/tooling/nargo_toml/src/errors.rs +++ b/tooling/nargo_toml/src/errors.rs @@ -80,6 +80,8 @@ pub enum ManifestError { #[allow(clippy::enum_variant_names)] #[derive(Error, Debug, PartialEq, Eq, Clone)] pub enum SemverError { + #[error("Invalid value for `compiler_version` in package {package_name}. Requirements may only refer to full releases")] + InvalidCompilerVersionRequirement { package_name: CrateName, required_compiler_version: String }, #[error("Incompatible compiler version in package {package_name}. Required compiler version is {required_compiler_version} but the compiler version is {compiler_version_found}.\n Update the compiler_version field in Nargo.toml to >={required_compiler_version} or compile this project with version {required_compiler_version}")] IncompatibleVersion { package_name: CrateName, diff --git a/tooling/nargo_toml/src/semver.rs b/tooling/nargo_toml/src/semver.rs index 253ac82aa34..ececa1b30dd 100644 --- a/tooling/nargo_toml/src/semver.rs +++ b/tooling/nargo_toml/src/semver.rs @@ -3,11 +3,14 @@ use nargo::{ package::{Dependency, Package}, workspace::Workspace, }; -use semver::{Error, Version, VersionReq}; +use noirc_driver::CrateName; +use semver::{Error, Prerelease, Version, VersionReq}; // Parse a semver compatible version string pub(crate) fn parse_semver_compatible_version(version: &str) -> Result { - Version::parse(version) + let mut version = Version::parse(version)?; + version.pre = Prerelease::EMPTY; + Ok(version) } // Check that all of the packages in the workspace are compatible with the current compiler version @@ -25,10 +28,7 @@ pub(crate) fn semver_check_workspace( } // Check that a package and all of its dependencies are compatible with the current compiler version -pub(crate) fn semver_check_package( - package: &Package, - compiler_version: &Version, -) -> Result<(), SemverError> { +fn semver_check_package(package: &Package, compiler_version: &Version) -> Result<(), SemverError> { // Check that this package's compiler version requirements are satisfied if let Some(version) = &package.compiler_required_version { let version_req = match VersionReq::parse(version) { @@ -40,6 +40,9 @@ pub(crate) fn semver_check_package( }) } }; + + validate_compiler_version_requirement(&package.name, &version_req)?; + if !version_req.matches(compiler_version) { return Err(SemverError::IncompatibleVersion { package_name: package.name.clone(), @@ -61,6 +64,20 @@ pub(crate) fn semver_check_package( Ok(()) } +fn validate_compiler_version_requirement( + package_name: &CrateName, + required_compiler_version: &VersionReq, +) -> Result<(), SemverError> { + if required_compiler_version.comparators.iter().any(|comparator| !comparator.pre.is_empty()) { + return Err(SemverError::InvalidCompilerVersionRequirement { + package_name: package_name.clone(), + required_compiler_version: required_compiler_version.to_string(), + }); + } + + Ok(()) +} + // Strip the build meta data from the version string since it is ignored by semver. fn strip_build_meta_data(version: &Version) -> String { let version_string = version.to_string(); @@ -191,6 +208,26 @@ mod tests { }; } + #[test] + fn test_semver_prerelease() { + let compiler_version = parse_semver_compatible_version("1.0.0-beta.0").unwrap(); + + let package = Package { + compiler_required_version: Some(">=0.1.0".to_string()), + root_dir: PathBuf::new(), + package_type: PackageType::Library, + entry_path: PathBuf::new(), + name: CrateName::from_str("test").unwrap(), + dependencies: BTreeMap::new(), + version: Some("1.0".to_string()), + expression_width: None, + }; + + if let Err(err) = semver_check_package(&package, &compiler_version) { + panic!("{err}"); + }; + } + #[test] fn test_semver_build_data() { let compiler_version = Version::parse("0.1.0+this-is-ignored-by-semver").unwrap(); From 7a077de8599951559c2b0b5bb12a89106805f387 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Thu, 21 Nov 2024 20:23:25 +0000 Subject: [PATCH 04/11] chore: fix typo in test name (#6589) --- .../Nargo.toml | 2 +- .../Prover.toml | 0 .../src/main.nr | 0 3 files changed, 1 insertion(+), 1 deletion(-) rename test_programs/execution_success/{brillig_unitialised_arrays => brillig_uninitialized_arrays}/Nargo.toml (58%) rename test_programs/execution_success/{brillig_unitialised_arrays => brillig_uninitialized_arrays}/Prover.toml (100%) rename test_programs/execution_success/{brillig_unitialised_arrays => brillig_uninitialized_arrays}/src/main.nr (100%) diff --git a/test_programs/execution_success/brillig_unitialised_arrays/Nargo.toml b/test_programs/execution_success/brillig_uninitialized_arrays/Nargo.toml similarity index 58% rename from test_programs/execution_success/brillig_unitialised_arrays/Nargo.toml rename to test_programs/execution_success/brillig_uninitialized_arrays/Nargo.toml index f23ecc787d0..68bcf9929cc 100644 --- a/test_programs/execution_success/brillig_unitialised_arrays/Nargo.toml +++ b/test_programs/execution_success/brillig_uninitialized_arrays/Nargo.toml @@ -1,5 +1,5 @@ [package] -name = "brillig_unitialised_arrays" +name = "brillig_uninitialized_arrays" type = "bin" authors = [""] diff --git a/test_programs/execution_success/brillig_unitialised_arrays/Prover.toml b/test_programs/execution_success/brillig_uninitialized_arrays/Prover.toml similarity index 100% rename from test_programs/execution_success/brillig_unitialised_arrays/Prover.toml rename to test_programs/execution_success/brillig_uninitialized_arrays/Prover.toml diff --git a/test_programs/execution_success/brillig_unitialised_arrays/src/main.nr b/test_programs/execution_success/brillig_uninitialized_arrays/src/main.nr similarity index 100% rename from test_programs/execution_success/brillig_unitialised_arrays/src/main.nr rename to test_programs/execution_success/brillig_uninitialized_arrays/src/main.nr From 01c4a9fb62ffe2190c73f0d5b12933d2eb8f6b5d Mon Sep 17 00:00:00 2001 From: jfecher Date: Thu, 21 Nov 2024 14:28:12 -0600 Subject: [PATCH 05/11] feat: Avoid incrementing reference counts in some cases (#6568) --- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 11 ++++++++--- compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 5 ++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 0c6041029da..ddc3365b551 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -172,6 +172,7 @@ impl<'a> FunctionContext<'a> { /// Always returns a Value::Mutable wrapping the allocate instruction. pub(super) fn new_mutable_variable(&mut self, value_to_store: ValueId) -> Value { let element_type = self.builder.current_function.dfg.type_of_value(value_to_store); + self.builder.increment_array_reference_count(value_to_store); let alloc = self.builder.insert_allocate(element_type); self.builder.insert_store(alloc, value_to_store); let typ = self.builder.type_of_value(value_to_store); @@ -735,7 +736,6 @@ impl<'a> FunctionContext<'a> { // Reference counting in brillig relies on us incrementing reference // counts when arrays/slices are constructed or indexed. // Thus, if we dereference an lvalue which happens to be array/slice we should increment its reference counter. - self.builder.increment_array_reference_count(reference); self.builder.insert_load(reference, element_type).into() }) } @@ -916,7 +916,10 @@ impl<'a> FunctionContext<'a> { let parameters = self.builder.current_function.dfg.block_parameters(entry).to_vec(); for parameter in parameters { - self.builder.increment_array_reference_count(parameter); + // Avoid reference counts for immutable arrays that aren't behind references. + if self.builder.current_function.dfg.value_is_reference(parameter) { + self.builder.increment_array_reference_count(parameter); + } } entry @@ -933,7 +936,9 @@ impl<'a> FunctionContext<'a> { dropped_parameters.retain(|parameter| !terminator_args.contains(parameter)); for parameter in dropped_parameters { - self.builder.decrement_array_reference_count(parameter); + if self.builder.current_function.dfg.value_is_reference(parameter) { + self.builder.decrement_array_reference_count(parameter); + } } } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index c50f0a7f45c..d28236bd360 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -665,12 +665,11 @@ impl<'a> FunctionContext<'a> { values = values.map(|value| { let value = value.eval(self); - // Make sure to increment array reference counts on each let binding - self.builder.increment_array_reference_count(value); - Tree::Leaf(if let_expr.mutable { self.new_mutable_variable(value) } else { + // `new_mutable_variable` already increments rcs internally + self.builder.increment_array_reference_count(value); value::Value::Normal(value) }) }); From df8f2eee5c27d3cd4b6128056afdd9bd4a0322fe Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Thu, 21 Nov 2024 20:35:27 +0000 Subject: [PATCH 06/11] fix: remove `compiler_version` from new `Nargo.toml` (#6590) --- tooling/nargo_cli/src/cli/init_cmd.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/tooling/nargo_cli/src/cli/init_cmd.rs b/tooling/nargo_cli/src/cli/init_cmd.rs index c69775d3323..ffeb5d9ba74 100644 --- a/tooling/nargo_cli/src/cli/init_cmd.rs +++ b/tooling/nargo_cli/src/cli/init_cmd.rs @@ -5,7 +5,6 @@ use super::NargoConfig; use clap::Args; use nargo::constants::{PKG_FILE, SRC_DIR}; use nargo::package::{CrateName, PackageType}; -use noirc_driver::NOIRC_VERSION; use std::path::PathBuf; /// Create a Noir project in the current directory. @@ -66,7 +65,6 @@ pub(crate) fn initialize_project( name = "{package_name}" type = "{package_type}" authors = [""] -compiler_version = ">={NOIRC_VERSION}" [dependencies]"# ); From 7216f0829dcece948d3243471e6d57380522e997 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 21 Nov 2024 22:24:34 -0500 Subject: [PATCH 07/11] feat(ssa): Loop invariant code motion (#6563) --- compiler/noirc_evaluator/src/ssa.rs | 1 + .../src/ssa/ir/function_inserter.rs | 2 +- .../src/ssa/opt/loop_invariant.rs | 378 ++++++++++++++++++ compiler/noirc_evaluator/src/ssa/opt/mod.rs | 1 + .../noirc_evaluator/src/ssa/opt/unrolling.rs | 24 +- cspell.json | 1 + .../loop_invariant_regression/Nargo.toml | 7 + .../loop_invariant_regression/Prover.toml | 2 + .../loop_invariant_regression/src/main.nr | 13 + 9 files changed, 415 insertions(+), 14 deletions(-) create mode 100644 compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs create mode 100644 test_programs/execution_success/loop_invariant_regression/Nargo.toml create mode 100644 test_programs/execution_success/loop_invariant_regression/Prover.toml create mode 100644 test_programs/execution_success/loop_invariant_regression/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index f0a6fcd986f..97c1760d87c 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -103,6 +103,7 @@ pub(crate) fn optimize_into_acir( Ssa::evaluate_static_assert_and_assert_constant, "After `static_assert` and `assert_constant`:", )? + .run_pass(Ssa::loop_invariant_code_motion, "After Loop Invariant Code Motion:") .try_run_pass(Ssa::unroll_loops_iteratively, "After Unrolling:")? .run_pass(Ssa::simplify_cfg, "After Simplifying (2nd):") .run_pass(Ssa::flatten_cfg, "After Flattening:") diff --git a/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs b/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs index 5e133072067..a0c23ad70aa 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs @@ -25,7 +25,7 @@ pub(crate) struct FunctionInserter<'f> { /// /// This is optional since caching arrays relies on the inserter inserting strictly /// in control-flow order. Otherwise, if arrays later in the program are cached first, - /// they may be refered to by instructions earlier in the program. + /// they may be referred to by instructions earlier in the program. array_cache: Option, /// If this pass is loop unrolling, store the block before the loop to optionally diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs new file mode 100644 index 00000000000..14233ca73e5 --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -0,0 +1,378 @@ +//! The loop invariant code motion pass moves code from inside a loop to before the loop +//! if that code will always have the same result on every iteration of the loop. +//! +//! To identify a loop invariant, check whether all of an instruction's values are: +//! - Outside of the loop +//! - Constant +//! - Already marked as loop invariants +//! +//! We also check that we are not hoisting instructions with side effects. +use fxhash::FxHashSet as HashSet; + +use crate::ssa::{ + ir::{ + basic_block::BasicBlockId, + function::{Function, RuntimeType}, + function_inserter::FunctionInserter, + instruction::InstructionId, + value::ValueId, + }, + Ssa, +}; + +use super::unrolling::{Loop, Loops}; + +impl Ssa { + #[tracing::instrument(level = "trace", skip(self))] + pub(crate) fn loop_invariant_code_motion(mut self) -> Ssa { + let brillig_functions = self + .functions + .iter_mut() + .filter(|(_, func)| matches!(func.runtime(), RuntimeType::Brillig(_))); + + for (_, function) in brillig_functions { + function.loop_invariant_code_motion(); + } + + self + } +} + +impl Function { + fn loop_invariant_code_motion(&mut self) { + Loops::find_all(self).hoist_loop_invariants(self); + } +} + +impl Loops { + fn hoist_loop_invariants(self, function: &mut Function) { + let mut context = LoopInvariantContext::new(function); + + for loop_ in self.yet_to_unroll.iter() { + let Ok(pre_header) = loop_.get_pre_header(context.inserter.function, &self.cfg) else { + // If the loop does not have a preheader we skip hoisting loop invariants for this loop + continue; + }; + context.hoist_loop_invariants(loop_, pre_header); + } + + context.map_dependent_instructions(); + } +} + +struct LoopInvariantContext<'f> { + inserter: FunctionInserter<'f>, + defined_in_loop: HashSet, + loop_invariants: HashSet, +} + +impl<'f> LoopInvariantContext<'f> { + fn new(function: &'f mut Function) -> Self { + Self { + inserter: FunctionInserter::new(function), + defined_in_loop: HashSet::default(), + loop_invariants: HashSet::default(), + } + } + + fn hoist_loop_invariants(&mut self, loop_: &Loop, pre_header: BasicBlockId) { + self.set_values_defined_in_loop(loop_); + + for block in loop_.blocks.iter() { + for instruction_id in self.inserter.function.dfg[*block].take_instructions() { + let hoist_invariant = self.can_hoist_invariant(instruction_id); + + if hoist_invariant { + self.inserter.push_instruction(instruction_id, pre_header); + } else { + self.inserter.push_instruction(instruction_id, *block); + } + + self.update_values_defined_in_loop_and_invariants(instruction_id, hoist_invariant); + } + } + } + + /// Gather the variables declared within the loop + fn set_values_defined_in_loop(&mut self, loop_: &Loop) { + for block in loop_.blocks.iter() { + let params = self.inserter.function.dfg.block_parameters(*block); + self.defined_in_loop.extend(params); + for instruction_id in self.inserter.function.dfg[*block].instructions() { + let results = self.inserter.function.dfg.instruction_results(*instruction_id); + self.defined_in_loop.extend(results); + } + } + } + + /// Update any values defined in the loop and loop invariants after a + /// analyzing and re-inserting a loop's instruction. + fn update_values_defined_in_loop_and_invariants( + &mut self, + instruction_id: InstructionId, + hoist_invariant: bool, + ) { + let results = self.inserter.function.dfg.instruction_results(instruction_id).to_vec(); + // We will have new IDs after pushing instructions. + // We should mark the resolved result IDs as also being defined within the loop. + let results = + results.into_iter().map(|value| self.inserter.resolve(value)).collect::>(); + self.defined_in_loop.extend(results.iter()); + + // We also want the update result IDs when we are marking loop invariants as we may not + // be going through the blocks of the loop in execution order + if hoist_invariant { + // Track already found loop invariants + self.loop_invariants.extend(results.iter()); + } + } + + fn can_hoist_invariant(&mut self, instruction_id: InstructionId) -> bool { + let mut is_loop_invariant = true; + // The list of blocks for a nested loop contain any inner loops as well. + // We may have already re-inserted new instructions if two loops share blocks + // so we need to map all the values in the instruction which we want to check. + let (instruction, _) = self.inserter.map_instruction(instruction_id); + instruction.for_each_value(|value| { + // If an instruction value is defined in the loop and not already a loop invariant + // the instruction results are not loop invariants. + // + // We are implicitly checking whether the values are constant as well. + // The set of values defined in the loop only contains instruction results and block parameters + // which cannot be constants. + is_loop_invariant &= + !self.defined_in_loop.contains(&value) || self.loop_invariants.contains(&value); + }); + is_loop_invariant && instruction.can_be_deduplicated(&self.inserter.function.dfg, false) + } + + fn map_dependent_instructions(&mut self) { + let blocks = self.inserter.function.reachable_blocks(); + for block in blocks { + for instruction_id in self.inserter.function.dfg[block].take_instructions() { + self.inserter.push_instruction(instruction_id, block); + } + self.inserter.map_terminator_in_place(block); + } + } +} + +#[cfg(test)] +mod test { + use crate::ssa::opt::assert_normalized_ssa_equals; + use crate::ssa::Ssa; + + #[test] + fn simple_loop_invariant_code_motion() { + let src = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + jmp b1(u32 0) + b1(v2: u32): + v5 = lt v2, u32 4 + jmpif v5 then: b3, else: b2 + b3(): + v6 = mul v0, v1 + constrain v6 == u32 6 + v8 = add v2, u32 1 + jmp b1(v8) + b2(): + return + } + "; + + let mut ssa = Ssa::from_str(src).unwrap(); + let main = ssa.main_mut(); + + let instructions = main.dfg[main.entry_block()].instructions(); + assert_eq!(instructions.len(), 0); // The final return is not counted + + // `v6 = mul v0, v1` in b3 should now be `v3 = mul v0, v1` in b0 + let expected = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v3 = mul v0, v1 + jmp b1(u32 0) + b1(v2: u32): + v6 = lt v2, u32 4 + jmpif v6 then: b3, else: b2 + b3(): + constrain v3 == u32 6 + v9 = add v2, u32 1 + jmp b1(v9) + b2(): + return + } + "; + + let ssa = ssa.loop_invariant_code_motion(); + assert_normalized_ssa_equals(ssa, expected); + } + + #[test] + fn nested_loop_invariant_code_motion() { + // Check that a loop invariant in the inner loop of a nested loop + // is hoisted to the parent loop's pre-header block. + let src = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + jmp b1(u32 0) + b1(v2: u32): + v6 = lt v2, u32 4 + jmpif v6 then: b3, else: b2 + b3(): + jmp b4(u32 0) + b4(v3: u32): + v7 = lt v3, u32 4 + jmpif v7 then: b6, else: b5 + b6(): + v10 = mul v0, v1 + constrain v10 == u32 6 + v12 = add v3, u32 1 + jmp b4(v12) + b5(): + v9 = add v2, u32 1 + jmp b1(v9) + b2(): + return + } + "; + + let mut ssa = Ssa::from_str(src).unwrap(); + let main = ssa.main_mut(); + + let instructions = main.dfg[main.entry_block()].instructions(); + assert_eq!(instructions.len(), 0); // The final return is not counted + + // `v10 = mul v0, v1` in b6 should now be `v4 = mul v0, v1` in b0 + let expected = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v4 = mul v0, v1 + jmp b1(u32 0) + b1(v2: u32): + v7 = lt v2, u32 4 + jmpif v7 then: b3, else: b2 + b3(): + jmp b4(u32 0) + b4(v3: u32): + v8 = lt v3, u32 4 + jmpif v8 then: b6, else: b5 + b6(): + constrain v4 == u32 6 + v12 = add v3, u32 1 + jmp b4(v12) + b5(): + v10 = add v2, u32 1 + jmp b1(v10) + b2(): + return + } + "; + + let ssa = ssa.loop_invariant_code_motion(); + assert_normalized_ssa_equals(ssa, expected); + } + + #[test] + fn hoist_invariant_with_invariant_as_argument() { + // Check that an instruction which has arguments defined in the loop + // but which are already marked loop invariants is still hoisted to the preheader. + // + // For example, in b3 we have the following instructions: + // ```text + // v6 = mul v0, v1 + // v7 = mul v6, v0 + // ``` + // `v6` should be marked a loop invariants as `v0` and `v1` are both declared outside of the loop. + // As we will be hoisting `v6 = mul v0, v1` to the loop preheader we know that we can also + // hoist `v7 = mul v6, v0`. + let src = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + jmp b1(u32 0) + b1(v2: u32): + v5 = lt v2, u32 4 + jmpif v5 then: b3, else: b2 + b3(): + v6 = mul v0, v1 + v7 = mul v6, v0 + v8 = eq v7, u32 12 + constrain v7 == u32 12 + v9 = add v2, u32 1 + jmp b1(v9) + b2(): + return + } + "; + + let mut ssa = Ssa::from_str(src).unwrap(); + let main = ssa.main_mut(); + + let instructions = main.dfg[main.entry_block()].instructions(); + assert_eq!(instructions.len(), 0); // The final return is not counted + + let expected = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v3 = mul v0, v1 + v4 = mul v3, v0 + v6 = eq v4, u32 12 + jmp b1(u32 0) + b1(v2: u32): + v9 = lt v2, u32 4 + jmpif v9 then: b3, else: b2 + b3(): + constrain v4 == u32 12 + v11 = add v2, u32 1 + jmp b1(v11) + b2(): + return + } + "; + + let ssa = ssa.loop_invariant_code_motion(); + assert_normalized_ssa_equals(ssa, expected); + } + + #[test] + fn do_not_hoist_instructions_with_side_effects() { + // In `v12 = load v5` in `b3`, `v5` is defined outside the loop. + // However, as the instruction has side effects, we want to make sure + // we do not hoist the instruction to the loop preheader. + let src = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v4 = make_array [u32 0, u32 0, u32 0, u32 0, u32 0] : [u32; 5] + inc_rc v4 + v5 = allocate -> &mut [u32; 5] + store v4 at v5 + jmp b1(u32 0) + b1(v2: u32): + v7 = lt v2, u32 4 + jmpif v7 then: b3, else: b2 + b3(): + v12 = load v5 -> [u32; 5] + v13 = array_set v12, index v0, value v1 + store v13 at v5 + v15 = add v2, u32 1 + jmp b1(v15) + b2(): + v8 = load v5 -> [u32; 5] + v10 = array_get v8, index u32 2 -> u32 + constrain v10 == u32 3 + return + } + "; + + let mut ssa = Ssa::from_str(src).unwrap(); + let main = ssa.main_mut(); + + let instructions = main.dfg[main.entry_block()].instructions(); + assert_eq!(instructions.len(), 4); // The final return is not counted + + let ssa = ssa.loop_invariant_code_motion(); + // The code should be unchanged + assert_normalized_ssa_equals(ssa, src); + } +} diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 10e86c6601a..06481a12f60 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -12,6 +12,7 @@ mod defunctionalize; mod die; pub(crate) mod flatten_cfg; mod inlining; +mod loop_invariant; mod mem2reg; mod normalize_value_ids; mod rc; diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 89f1b2b2d7d..777c16dacd1 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -18,8 +18,6 @@ //! //! When unrolling ACIR code, we remove reference count instructions because they are //! only used by Brillig bytecode. -use std::collections::HashSet; - use acvm::{acir::AcirField, FieldElement}; use crate::{ @@ -39,7 +37,7 @@ use crate::{ ssa_gen::Ssa, }, }; -use fxhash::FxHashMap as HashMap; +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; impl Ssa { /// Loop unrolling can return errors, since ACIR functions need to be fully unrolled. @@ -84,7 +82,7 @@ impl Function { } } -struct Loop { +pub(super) struct Loop { /// The header block of a loop is the block which dominates all the /// other blocks in the loop. header: BasicBlockId, @@ -94,17 +92,17 @@ struct Loop { back_edge_start: BasicBlockId, /// All the blocks contained within the loop, including `header` and `back_edge_start`. - blocks: HashSet, + pub(super) blocks: HashSet, } -struct Loops { +pub(super) struct Loops { /// The loops that failed to be unrolled so that we do not try to unroll them again. /// Each loop is identified by its header block id. failed_to_unroll: HashSet, - yet_to_unroll: Vec, + pub(super) yet_to_unroll: Vec, modified_blocks: HashSet, - cfg: ControlFlowGraph, + pub(super) cfg: ControlFlowGraph, } impl Loops { @@ -136,7 +134,7 @@ impl Loops { /// loop_end loop_body /// ``` /// `loop_entry` has two predecessors: `main` and `loop_body`, and it dominates `loop_body`. - fn find_all(function: &Function) -> Self { + pub(super) fn find_all(function: &Function) -> Self { let cfg = ControlFlowGraph::with_function(function); let post_order = PostOrder::with_function(function); let mut dom_tree = DominatorTree::with_cfg_and_post_order(&cfg, &post_order); @@ -163,9 +161,9 @@ impl Loops { loops.sort_by_key(|loop_| loop_.blocks.len()); Self { - failed_to_unroll: HashSet::new(), + failed_to_unroll: HashSet::default(), yet_to_unroll: loops, - modified_blocks: HashSet::new(), + modified_blocks: HashSet::default(), cfg, } } @@ -209,7 +207,7 @@ impl Loop { back_edge_start: BasicBlockId, cfg: &ControlFlowGraph, ) -> Self { - let mut blocks = HashSet::new(); + let mut blocks = HashSet::default(); blocks.insert(header); let mut insert = |block, stack: &mut Vec| { @@ -393,7 +391,7 @@ impl Loop { /// The loop pre-header is the block that comes before the loop begins. Generally a header block /// is expected to have 2 predecessors: the pre-header and the final block of the loop which jumps /// back to the beginning. Other predecessors can come from `break` or `continue`. - fn get_pre_header( + pub(super) fn get_pre_header( &self, function: &Function, cfg: &ControlFlowGraph, diff --git a/cspell.json b/cspell.json index a386ed80ee9..b0cd635e46f 100644 --- a/cspell.json +++ b/cspell.json @@ -171,6 +171,7 @@ "PLONKish", "pprof", "precomputes", + "preheader", "preimage", "preprocess", "prettytable", diff --git a/test_programs/execution_success/loop_invariant_regression/Nargo.toml b/test_programs/execution_success/loop_invariant_regression/Nargo.toml new file mode 100644 index 00000000000..9590789f52e --- /dev/null +++ b/test_programs/execution_success/loop_invariant_regression/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "loop_invariant_regression" +type = "bin" +authors = [""] +compiler_version = ">=0.38.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_success/loop_invariant_regression/Prover.toml b/test_programs/execution_success/loop_invariant_regression/Prover.toml new file mode 100644 index 00000000000..18680c805a7 --- /dev/null +++ b/test_programs/execution_success/loop_invariant_regression/Prover.toml @@ -0,0 +1,2 @@ +x = "2" +y = "3" diff --git a/test_programs/execution_success/loop_invariant_regression/src/main.nr b/test_programs/execution_success/loop_invariant_regression/src/main.nr new file mode 100644 index 00000000000..25f6e92f868 --- /dev/null +++ b/test_programs/execution_success/loop_invariant_regression/src/main.nr @@ -0,0 +1,13 @@ +// Tests a simple loop where we expect loop invariant instructions +// to be hoisted to the loop's pre-header block. +fn main(x: u32, y: u32) { + loop(4, x, y); +} + +fn loop(upper_bound: u32, x: u32, y: u32) { + for _ in 0..upper_bound { + let mut z = x * y; + z = z * x; + assert_eq(z, 12); + } +} From b934669c2a22bdc7c47f0b4f9935372816b67b8d Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 22 Nov 2024 05:17:58 -0500 Subject: [PATCH 08/11] chore: Typo in oracles how to (#6598) --- docs/docs/how_to/how-to-oracles.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/docs/how_to/how-to-oracles.md b/docs/docs/how_to/how-to-oracles.md index 4763b7788d6..0bb8743e361 100644 --- a/docs/docs/how_to/how-to-oracles.md +++ b/docs/docs/how_to/how-to-oracles.md @@ -30,7 +30,7 @@ This guide has 3 major steps: An oracle is defined in a Noir program by defining two methods: -- An unconstrained method - This tells the compiler that it is executing an [unconstrained functions](../noir/concepts//unconstrained.md). +- An unconstrained method - This tells the compiler that it is executing an [unconstrained function](../noir/concepts//unconstrained.md). - A decorated oracle method - This tells the compiler that this method is an RPC call. An example of an oracle that returns a `Field` would be: From 8e046afbbe3fba06c1e177f74aacefdd1bf871b6 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Fri, 22 Nov 2024 11:10:26 +0000 Subject: [PATCH 09/11] chore!: remove eddsa from stdlib (#6591) --- .../cryptographic_primitives/ec_primitives.md | 5 +- .../cryptographic_primitives/eddsa.mdx | 37 --------- noir_stdlib/src/eddsa.nr | 76 ------------------- noir_stdlib/src/lib.nr | 1 - .../bench_eddsa_poseidon/src/main.nr | 55 +++++++++++++- .../execution_success/eddsa/src/main.nr | 74 +++++++++++++++++- 6 files changed, 127 insertions(+), 121 deletions(-) delete mode 100644 docs/docs/noir/standard_library/cryptographic_primitives/eddsa.mdx delete mode 100644 noir_stdlib/src/eddsa.nr diff --git a/docs/docs/noir/standard_library/cryptographic_primitives/ec_primitives.md b/docs/docs/noir/standard_library/cryptographic_primitives/ec_primitives.md index f262d8160d6..00b8071487e 100644 --- a/docs/docs/noir/standard_library/cryptographic_primitives/ec_primitives.md +++ b/docs/docs/noir/standard_library/cryptographic_primitives/ec_primitives.md @@ -97,6 +97,5 @@ fn bjj_pub_key(priv_key: Field) -> Point This would come in handy in a Merkle proof. - EdDSA signature verification: This is a matter of combining these primitives with a suitable hash - function. See - [feat(stdlib): EdDSA sig verification noir#1136](https://github.com/noir-lang/noir/pull/1136) for - the case of Baby Jubjub and the Poseidon hash function. + function. See the [eddsa](https://github.com/noir-lang/eddsa) library an example of eddsa signature verification + over the Baby Jubjub curve. diff --git a/docs/docs/noir/standard_library/cryptographic_primitives/eddsa.mdx b/docs/docs/noir/standard_library/cryptographic_primitives/eddsa.mdx deleted file mode 100644 index b283de693c8..00000000000 --- a/docs/docs/noir/standard_library/cryptographic_primitives/eddsa.mdx +++ /dev/null @@ -1,37 +0,0 @@ ---- -title: EdDSA Verification -description: Learn about the cryptographic primitives regarding EdDSA -keywords: [cryptographic primitives, Noir project, eddsa, signatures] -sidebar_position: 5 ---- - -import BlackBoxInfo from '@site/src/components/Notes/_blackbox'; - -## eddsa::eddsa_poseidon_verify - -Verifier for EdDSA signatures - -```rust -fn eddsa_poseidon_verify(public_key_x : Field, public_key_y : Field, signature_s: Field, signature_r8_x: Field, signature_r8_y: Field, message: Field) -> bool -``` - -It is also possible to specify the hash algorithm used for the signature by using the `eddsa_verify` function by passing a type implementing the Hasher trait with the turbofish operator. -For instance, if you want to use Poseidon2 instead, you can do the following: -```rust -use std::hash::poseidon2::Poseidon2Hasher; - -eddsa_verify::(pub_key_a.x, pub_key_a.y, s_a, r8_a.x, r8_a.y, msg); -``` - - - -## eddsa::eddsa_to_pub - -Private to public key conversion. - -Returns `(pub_key_x, pub_key_y)` - -```rust -fn eddsa_to_pub(secret : Field) -> (Field, Field) -``` - diff --git a/noir_stdlib/src/eddsa.nr b/noir_stdlib/src/eddsa.nr deleted file mode 100644 index c049b7abbb5..00000000000 --- a/noir_stdlib/src/eddsa.nr +++ /dev/null @@ -1,76 +0,0 @@ -use crate::default::Default; -use crate::ec::consts::te::baby_jubjub; -use crate::ec::tecurve::affine::Point as TEPoint; -use crate::hash::Hasher; -use crate::hash::poseidon::PoseidonHasher; - -// Returns true if signature is valid -pub fn eddsa_poseidon_verify( - pub_key_x: Field, - pub_key_y: Field, - signature_s: Field, - signature_r8_x: Field, - signature_r8_y: Field, - message: Field, -) -> bool { - eddsa_verify::( - pub_key_x, - pub_key_y, - signature_s, - signature_r8_x, - signature_r8_y, - message, - ) -} - -pub fn eddsa_verify( - pub_key_x: Field, - pub_key_y: Field, - signature_s: Field, - signature_r8_x: Field, - signature_r8_y: Field, - message: Field, -) -> bool -where - H: Hasher + Default, -{ - // Verifies by testing: - // S * B8 = R8 + H(R8, A, m) * A8 - let bjj = baby_jubjub(); - - let pub_key = TEPoint::new(pub_key_x, pub_key_y); - assert(bjj.curve.contains(pub_key)); - - let signature_r8 = TEPoint::new(signature_r8_x, signature_r8_y); - assert(bjj.curve.contains(signature_r8)); - // Ensure S < Subgroup Order - assert(signature_s.lt(bjj.suborder)); - // Calculate the h = H(R, A, msg) - let mut hasher = H::default(); - hasher.write(signature_r8_x); - hasher.write(signature_r8_y); - hasher.write(pub_key_x); - hasher.write(pub_key_y); - hasher.write(message); - let hash: Field = hasher.finish(); - // Calculate second part of the right side: right2 = h*8*A - // Multiply by 8 by doubling 3 times. This also ensures that the result is in the subgroup. - let pub_key_mul_2 = bjj.curve.add(pub_key, pub_key); - let pub_key_mul_4 = bjj.curve.add(pub_key_mul_2, pub_key_mul_2); - let pub_key_mul_8 = bjj.curve.add(pub_key_mul_4, pub_key_mul_4); - // We check that A8 is not zero. - assert(!pub_key_mul_8.is_zero()); - // Compute the right side: R8 + h * A8 - let right = bjj.curve.add(signature_r8, bjj.curve.mul(hash, pub_key_mul_8)); - // Calculate left side of equation left = S * B8 - let left = bjj.curve.mul(signature_s, bjj.base8); - - left.eq(right) -} - -// Returns the public key of the given secret key as (pub_key_x, pub_key_y) -pub fn eddsa_to_pub(secret: Field) -> (Field, Field) { - let bjj = baby_jubjub(); - let pub_key = bjj.curve.mul(secret, bjj.curve.gen); - (pub_key.x, pub_key.y) -} diff --git a/noir_stdlib/src/lib.nr b/noir_stdlib/src/lib.nr index 91a1980fe70..b1aabc48d3b 100644 --- a/noir_stdlib/src/lib.nr +++ b/noir_stdlib/src/lib.nr @@ -6,7 +6,6 @@ pub mod merkle; pub mod schnorr; pub mod ecdsa_secp256k1; pub mod ecdsa_secp256r1; -pub mod eddsa; pub mod embedded_curve_ops; pub mod sha256; pub mod sha512; diff --git a/test_programs/benchmarks/bench_eddsa_poseidon/src/main.nr b/test_programs/benchmarks/bench_eddsa_poseidon/src/main.nr index cb853e48c30..bcb0930cf24 100644 --- a/test_programs/benchmarks/bench_eddsa_poseidon/src/main.nr +++ b/test_programs/benchmarks/bench_eddsa_poseidon/src/main.nr @@ -1,4 +1,8 @@ -use std::eddsa::eddsa_poseidon_verify; +use std::default::Default; +use std::ec::consts::te::baby_jubjub; +use std::ec::tecurve::affine::Point as TEPoint; +use std::hash::Hasher; +use std::hash::poseidon::PoseidonHasher; fn main( msg: pub Field, @@ -6,7 +10,52 @@ fn main( pub_key_y: Field, r8_x: Field, r8_y: Field, - s: Field + s: Field, ) -> pub bool { - eddsa_poseidon_verify(pub_key_x, pub_key_y, s, r8_x, r8_y, msg) + eddsa_verify::(pub_key_x, pub_key_y, s, r8_x, r8_y, msg) +} + +pub fn eddsa_verify( + pub_key_x: Field, + pub_key_y: Field, + signature_s: Field, + signature_r8_x: Field, + signature_r8_y: Field, + message: Field, +) -> bool +where + H: Hasher + Default, +{ + // Verifies by testing: + // S * B8 = R8 + H(R8, A, m) * A8 + let bjj = baby_jubjub(); + + let pub_key = TEPoint::new(pub_key_x, pub_key_y); + assert(bjj.curve.contains(pub_key)); + + let signature_r8 = TEPoint::new(signature_r8_x, signature_r8_y); + assert(bjj.curve.contains(signature_r8)); + // Ensure S < Subgroup Order + assert(signature_s.lt(bjj.suborder)); + // Calculate the h = H(R, A, msg) + let mut hasher = H::default(); + hasher.write(signature_r8_x); + hasher.write(signature_r8_y); + hasher.write(pub_key_x); + hasher.write(pub_key_y); + hasher.write(message); + let hash: Field = hasher.finish(); + // Calculate second part of the right side: right2 = h*8*A + // Multiply by 8 by doubling 3 times. This also ensures that the result is in the subgroup. + let pub_key_mul_2 = bjj.curve.add(pub_key, pub_key); + let pub_key_mul_4 = bjj.curve.add(pub_key_mul_2, pub_key_mul_2); + let pub_key_mul_8 = bjj.curve.add(pub_key_mul_4, pub_key_mul_4); + // We check that A8 is not zero. + assert(!pub_key_mul_8.is_zero()); + // Compute the right side: R8 + h * A8 + let right = bjj.curve.add(signature_r8, bjj.curve.mul(hash, pub_key_mul_8)); + // Calculate left side of equation left = S * B8 + let left = bjj.curve.mul(signature_s, bjj.base8); + + left.eq(right) } diff --git a/test_programs/execution_success/eddsa/src/main.nr b/test_programs/execution_success/eddsa/src/main.nr index d4c3664f0c9..83e6c6565a9 100644 --- a/test_programs/execution_success/eddsa/src/main.nr +++ b/test_programs/execution_success/eddsa/src/main.nr @@ -1,7 +1,8 @@ use std::compat; use std::ec::consts::te::baby_jubjub; use std::ec::tecurve::affine::Point as TEPoint; -use std::eddsa::{eddsa_poseidon_verify, eddsa_to_pub, eddsa_verify}; +use std::hash::Hasher; +use std::hash::poseidon::PoseidonHasher; use std::hash::poseidon2::Poseidon2Hasher; fn main(msg: pub Field, _priv_key_a: Field, _priv_key_b: Field) { @@ -54,3 +55,74 @@ fn main(msg: pub Field, _priv_key_a: Field, _priv_key_b: Field) { ); } } + +// Returns true if signature is valid +pub fn eddsa_poseidon_verify( + pub_key_x: Field, + pub_key_y: Field, + signature_s: Field, + signature_r8_x: Field, + signature_r8_y: Field, + message: Field, +) -> bool { + eddsa_verify::( + pub_key_x, + pub_key_y, + signature_s, + signature_r8_x, + signature_r8_y, + message, + ) +} + +pub fn eddsa_verify( + pub_key_x: Field, + pub_key_y: Field, + signature_s: Field, + signature_r8_x: Field, + signature_r8_y: Field, + message: Field, +) -> bool +where + H: Hasher + Default, +{ + // Verifies by testing: + // S * B8 = R8 + H(R8, A, m) * A8 + let bjj = baby_jubjub(); + + let pub_key = TEPoint::new(pub_key_x, pub_key_y); + assert(bjj.curve.contains(pub_key)); + + let signature_r8 = TEPoint::new(signature_r8_x, signature_r8_y); + assert(bjj.curve.contains(signature_r8)); + // Ensure S < Subgroup Order + assert(signature_s.lt(bjj.suborder)); + // Calculate the h = H(R, A, msg) + let mut hasher = H::default(); + hasher.write(signature_r8_x); + hasher.write(signature_r8_y); + hasher.write(pub_key_x); + hasher.write(pub_key_y); + hasher.write(message); + let hash: Field = hasher.finish(); + // Calculate second part of the right side: right2 = h*8*A + // Multiply by 8 by doubling 3 times. This also ensures that the result is in the subgroup. + let pub_key_mul_2 = bjj.curve.add(pub_key, pub_key); + let pub_key_mul_4 = bjj.curve.add(pub_key_mul_2, pub_key_mul_2); + let pub_key_mul_8 = bjj.curve.add(pub_key_mul_4, pub_key_mul_4); + // We check that A8 is not zero. + assert(!pub_key_mul_8.is_zero()); + // Compute the right side: R8 + h * A8 + let right = bjj.curve.add(signature_r8, bjj.curve.mul(hash, pub_key_mul_8)); + // Calculate left side of equation left = S * B8 + let left = bjj.curve.mul(signature_s, bjj.base8); + + left.eq(right) +} + +// Returns the public key of the given secret key as (pub_key_x, pub_key_y) +pub fn eddsa_to_pub(secret: Field) -> (Field, Field) { + let bjj = baby_jubjub(); + let pub_key = bjj.curve.mul(secret, bjj.curve.gen); + (pub_key.x, pub_key.y) +} From 0a6207dde6c744e2853905014e70d33b29b3e53b Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 22 Nov 2024 09:07:59 -0300 Subject: [PATCH 10/11] fix: parse a bit more SSA stuff (#6599) --- .../noirc_evaluator/src/ssa/ir/printer.rs | 4 +- .../noirc_evaluator/src/ssa/parser/ast.rs | 7 +++ .../src/ssa/parser/into_ssa.rs | 41 ++++++++++++++--- .../noirc_evaluator/src/ssa/parser/lexer.rs | 46 ++++++++++++++++++- .../noirc_evaluator/src/ssa/parser/mod.rs | 45 ++++++++++++++++-- .../noirc_evaluator/src/ssa/parser/tests.rs | 37 +++++++++++++++ .../noirc_evaluator/src/ssa/parser/token.rs | 8 ++++ 7 files changed, 175 insertions(+), 13 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index b981b81f365..6bebd21fe61 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -272,13 +272,13 @@ fn display_constrain_error( ) -> Result { match error { ConstrainError::StaticString(assert_message_string) => { - writeln!(f, " '{assert_message_string:?}'") + writeln!(f, ", {assert_message_string:?}") } ConstrainError::Dynamic(_, is_string, values) => { if let Some(constant_string) = try_to_extract_string_from_error_payload(*is_string, values, &function.dfg) { - writeln!(f, " '{}'", constant_string) + writeln!(f, ", {constant_string:?}") } else { writeln!(f, ", data {}", value_list(function, values)) } diff --git a/compiler/noirc_evaluator/src/ssa/parser/ast.rs b/compiler/noirc_evaluator/src/ssa/parser/ast.rs index a34b7fd70d3..6c7608a2f16 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/ast.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/ast.rs @@ -89,6 +89,7 @@ pub(crate) enum ParsedInstruction { Constrain { lhs: ParsedValue, rhs: ParsedValue, + assert_message: Option, }, DecrementRc { value: ParsedValue, @@ -129,6 +130,12 @@ pub(crate) enum ParsedInstruction { }, } +#[derive(Debug)] +pub(crate) enum AssertMessage { + Static(String), + Dynamic(Vec), +} + #[derive(Debug)] pub(crate) enum ParsedTerminator { Jmp { destination: Identifier, arguments: Vec }, diff --git a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs index 552ac0781c7..e78cbbd75a1 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs @@ -1,13 +1,18 @@ use std::collections::HashMap; +use acvm::acir::circuit::ErrorSelector; + use crate::ssa::{ function_builder::FunctionBuilder, - ir::{basic_block::BasicBlockId, function::FunctionId, value::ValueId}, + ir::{ + basic_block::BasicBlockId, function::FunctionId, instruction::ConstrainError, + value::ValueId, + }, }; use super::{ - Identifier, ParsedBlock, ParsedFunction, ParsedInstruction, ParsedSsa, ParsedTerminator, - ParsedValue, RuntimeType, Ssa, SsaError, + ast::AssertMessage, Identifier, ParsedBlock, ParsedFunction, ParsedInstruction, ParsedSsa, + ParsedTerminator, ParsedValue, RuntimeType, Ssa, SsaError, }; impl ParsedSsa { @@ -31,6 +36,8 @@ struct Translator { /// passes already which replaced some of the original IDs. The translator /// will recreate the SSA step by step, which can result in a new ID layout. variables: HashMap>, + + error_selector_counter: u64, } impl Translator { @@ -64,8 +71,13 @@ impl Translator { functions.insert(function.internal_name.clone(), function_id); } - let mut translator = - Self { builder, functions, variables: HashMap::new(), blocks: HashMap::new() }; + let mut translator = Self { + builder, + functions, + variables: HashMap::new(), + blocks: HashMap::new(), + error_selector_counter: 0, + }; translator.translate_function_body(main_function)?; Ok(translator) @@ -198,10 +210,25 @@ impl Translator { let value_id = self.builder.insert_cast(lhs, typ); self.define_variable(target, value_id)?; } - ParsedInstruction::Constrain { lhs, rhs } => { + ParsedInstruction::Constrain { lhs, rhs, assert_message } => { let lhs = self.translate_value(lhs)?; let rhs = self.translate_value(rhs)?; - self.builder.insert_constrain(lhs, rhs, None); + let assert_message = match assert_message { + Some(AssertMessage::Static(string)) => { + Some(ConstrainError::StaticString(string)) + } + Some(AssertMessage::Dynamic(values)) => { + let error_selector = ErrorSelector::new(self.error_selector_counter); + self.error_selector_counter += 1; + + let is_string_type = false; + let values = self.translate_values(values)?; + + Some(ConstrainError::Dynamic(error_selector, is_string_type, values)) + } + None => None, + }; + self.builder.insert_constrain(lhs, rhs, assert_message); } ParsedInstruction::DecrementRc { value } => { let value = self.translate_value(value)?; diff --git a/compiler/noirc_evaluator/src/ssa/parser/lexer.rs b/compiler/noirc_evaluator/src/ssa/parser/lexer.rs index 4c90475be74..d89bc1e9e28 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/lexer.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/lexer.rs @@ -61,6 +61,7 @@ impl<'a> Lexer<'a> { Some('&') => self.single_char_token(Token::Ampersand), Some('-') if self.peek_char() == Some('>') => self.double_char_token(Token::Arrow), Some('-') => self.single_char_token(Token::Dash), + Some('"') => self.eat_string_literal(), Some(ch) if ch.is_ascii_alphanumeric() || ch == '_' => self.eat_alpha_numeric(ch), Some(char) => Err(LexerError::UnexpectedCharacter { char, @@ -177,6 +178,41 @@ impl<'a> Lexer<'a> { Ok(integer_token.into_span(start, end)) } + fn eat_string_literal(&mut self) -> SpannedTokenResult { + let start = self.position; + let mut string = String::new(); + + while let Some(next) = self.next_char() { + let char = match next { + '"' => break, + '\\' => match self.next_char() { + Some('r') => '\r', + Some('n') => '\n', + Some('t') => '\t', + Some('0') => '\0', + Some('"') => '"', + Some('\\') => '\\', + Some(escaped) => { + let span = Span::inclusive(start, self.position); + return Err(LexerError::InvalidEscape { escaped, span }); + } + None => { + let span = Span::inclusive(start, self.position); + return Err(LexerError::UnterminatedStringLiteral { span }); + } + }, + other => other, + }; + + string.push(char); + } + + let str_literal_token = Token::Str(string); + + let end = self.position; + Ok(str_literal_token.into_span(start, end)) + } + fn eat_while bool>( &mut self, initial_char: Option, @@ -247,6 +283,12 @@ pub(crate) enum LexerError { InvalidIntegerLiteral { span: Span, found: String }, #[error("Integer literal too large")] IntegerLiteralTooLarge { span: Span, limit: String }, + #[error("Unterminated string literal")] + UnterminatedStringLiteral { span: Span }, + #[error( + "'\\{escaped}' is not a valid escape sequence. Use '\\' for a literal backslash character." + )] + InvalidEscape { escaped: char, span: Span }, } impl LexerError { @@ -254,7 +296,9 @@ impl LexerError { match self { LexerError::UnexpectedCharacter { span, .. } | LexerError::InvalidIntegerLiteral { span, .. } - | LexerError::IntegerLiteralTooLarge { span, .. } => *span, + | LexerError::IntegerLiteralTooLarge { span, .. } + | LexerError::UnterminatedStringLiteral { span } + | LexerError::InvalidEscape { span, .. } => *span, } } } diff --git a/compiler/noirc_evaluator/src/ssa/parser/mod.rs b/compiler/noirc_evaluator/src/ssa/parser/mod.rs index 7753908b2bd..3d8bd37dead 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/mod.rs @@ -10,8 +10,8 @@ use super::{ use acvm::{AcirField, FieldElement}; use ast::{ - Identifier, ParsedBlock, ParsedFunction, ParsedInstruction, ParsedParameter, ParsedSsa, - ParsedValue, + AssertMessage, Identifier, ParsedBlock, ParsedFunction, ParsedInstruction, ParsedParameter, + ParsedSsa, ParsedValue, }; use lexer::{Lexer, LexerError}; use noirc_errors::Span; @@ -313,7 +313,20 @@ impl<'a> Parser<'a> { let lhs = self.parse_value_or_error()?; self.eat_or_error(Token::Equal)?; let rhs = self.parse_value_or_error()?; - Ok(Some(ParsedInstruction::Constrain { lhs, rhs })) + + let assert_message = if self.eat(Token::Comma)? { + if let Some(str) = self.eat_str()? { + Some(AssertMessage::Static(str)) + } else if self.eat_keyword(Keyword::Data)? { + Some(AssertMessage::Dynamic(self.parse_comma_separated_values()?)) + } else { + return self.expected_string_or_data(); + } + } else { + None + }; + + Ok(Some(ParsedInstruction::Constrain { lhs, rhs, assert_message })) } fn parse_decrement_rc(&mut self) -> ParseResult> { @@ -654,6 +667,10 @@ impl<'a> Parser<'a> { return Ok(Type::Reference(Arc::new(typ))); } + if self.eat_keyword(Keyword::Function)? { + return Ok(Type::Function); + } + self.expected_type() } @@ -767,6 +784,18 @@ impl<'a> Parser<'a> { } } + fn eat_str(&mut self) -> ParseResult> { + if matches!(self.token.token(), Token::Str(..)) { + let token = self.bump()?; + match token.into_token() { + Token::Str(string) => Ok(Some(string)), + _ => unreachable!(), + } + } else { + Ok(None) + } + } + fn eat(&mut self, token: Token) -> ParseResult { if self.token.token() == &token { self.bump()?; @@ -812,6 +841,13 @@ impl<'a> Parser<'a> { }) } + fn expected_string_or_data(&mut self) -> ParseResult { + Err(ParserError::ExpectedStringOrData { + found: self.token.token().clone(), + span: self.token.to_span(), + }) + } + fn expected_identifier(&mut self) -> ParseResult { Err(ParserError::ExpectedIdentifier { found: self.token.token().clone(), @@ -873,6 +909,8 @@ pub(crate) enum ParserError { ExpectedType { found: Token, span: Span }, #[error("Expected an instruction or terminator, found '{found}'")] ExpectedInstructionOrTerminator { found: Token, span: Span }, + #[error("Expected a string literal or 'data', found '{found}'")] + ExpectedStringOrData { found: Token, span: Span }, #[error("Expected a value, found '{found}'")] ExpectedValue { found: Token, span: Span }, #[error("Multiple return values only allowed for call")] @@ -889,6 +927,7 @@ impl ParserError { | ParserError::ExpectedInt { span, .. } | ParserError::ExpectedType { span, .. } | ParserError::ExpectedInstructionOrTerminator { span, .. } + | ParserError::ExpectedStringOrData { span, .. } | ParserError::ExpectedValue { span, .. } => *span, ParserError::MultipleReturnValuesOnlyAllowedForCall { second_target, .. } => { second_target.span diff --git a/compiler/noirc_evaluator/src/ssa/parser/tests.rs b/compiler/noirc_evaluator/src/ssa/parser/tests.rs index 60d398bf9d5..593b66d0c98 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/tests.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/tests.rs @@ -214,6 +214,31 @@ fn test_constrain() { assert_ssa_roundtrip(src); } +#[test] +fn test_constrain_with_static_message() { + let src = r#" + acir(inline) fn main f0 { + b0(v0: Field): + constrain v0 == Field 1, "Oh no!" + return + } + "#; + assert_ssa_roundtrip(src); +} + +#[test] +fn test_constrain_with_dynamic_message() { + let src = " + acir(inline) fn main f0 { + b0(v0: Field, v1: Field): + v7 = make_array [u8 123, u8 120, u8 125, u8 32, u8 123, u8 121, u8 125] : [u8; 7] + constrain v0 == Field 1, data v7, u32 2, v0, v1 + return + } + "; + assert_ssa_roundtrip(src); +} + #[test] fn test_enable_side_effects() { let src = " @@ -441,3 +466,15 @@ fn test_negative() { "; assert_ssa_roundtrip(src); } + +#[test] +fn test_function_type() { + let src = " + acir(inline) fn main f0 { + b0(): + v0 = allocate -> &mut function + return + } + "; + assert_ssa_roundtrip(src); +} diff --git a/compiler/noirc_evaluator/src/ssa/parser/token.rs b/compiler/noirc_evaluator/src/ssa/parser/token.rs index f663879e899..d8dd4ec011e 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/token.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/token.rs @@ -29,6 +29,7 @@ impl SpannedToken { pub(crate) enum Token { Ident(String), Int(FieldElement), + Str(String), Keyword(Keyword), IntType(IntType), /// = @@ -77,6 +78,7 @@ impl Display for Token { match self { Token::Ident(ident) => write!(f, "{}", ident), Token::Int(int) => write!(f, "{}", int), + Token::Str(string) => write!(f, "{string:?}"), Token::Keyword(keyword) => write!(f, "{}", keyword), Token::IntType(int_type) => write!(f, "{}", int_type), Token::Assign => write!(f, "="), @@ -120,6 +122,7 @@ pub(crate) enum Keyword { Call, Cast, Constrain, + Data, DecRc, Div, Inline, @@ -130,6 +133,7 @@ pub(crate) enum Keyword { Field, Fold, Fn, + Function, IncRc, Index, Jmp, @@ -175,6 +179,7 @@ impl Keyword { "call" => Keyword::Call, "cast" => Keyword::Cast, "constrain" => Keyword::Constrain, + "data" => Keyword::Data, "dec_rc" => Keyword::DecRc, "div" => Keyword::Div, "else" => Keyword::Else, @@ -185,6 +190,7 @@ impl Keyword { "Field" => Keyword::Field, "fold" => Keyword::Fold, "fn" => Keyword::Fn, + "function" => Keyword::Function, "inc_rc" => Keyword::IncRc, "index" => Keyword::Index, "jmp" => Keyword::Jmp, @@ -234,6 +240,7 @@ impl Display for Keyword { Keyword::Call => write!(f, "call"), Keyword::Cast => write!(f, "cast"), Keyword::Constrain => write!(f, "constrain"), + Keyword::Data => write!(f, "data"), Keyword::DecRc => write!(f, "dec_rc"), Keyword::Div => write!(f, "div"), Keyword::Else => write!(f, "else"), @@ -242,6 +249,7 @@ impl Display for Keyword { Keyword::Field => write!(f, "Field"), Keyword::Fold => write!(f, "fold"), Keyword::Fn => write!(f, "fn"), + Keyword::Function => write!(f, "function"), Keyword::IncRc => write!(f, "inc_rc"), Keyword::Index => write!(f, "index"), Keyword::Inline => write!(f, "inline"), From d94eb085adf2cdd8f0e80d9cfd712c19c8810974 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 22 Nov 2024 11:55:19 -0300 Subject: [PATCH 11/11] fix: preserve newlines between comments when formatting statements (#6601) --- tooling/nargo_fmt/src/formatter/expression.rs | 2 +- tooling/nargo_fmt/src/formatter/function.rs | 30 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/tooling/nargo_fmt/src/formatter/expression.rs b/tooling/nargo_fmt/src/formatter/expression.rs index 0ac4c98bb95..0730d06ad72 100644 --- a/tooling/nargo_fmt/src/formatter/expression.rs +++ b/tooling/nargo_fmt/src/formatter/expression.rs @@ -1165,7 +1165,7 @@ impl<'a, 'b> ChunkFormatter<'a, 'b> { // Finally format the comment, if any group.text(self.chunk(|formatter| { - formatter.skip_comments_and_whitespace(); + formatter.skip_comments_and_whitespace_writing_multiple_lines_if_found(); })); group.decrease_indentation(); diff --git a/tooling/nargo_fmt/src/formatter/function.rs b/tooling/nargo_fmt/src/formatter/function.rs index fd6977df613..8207db5e486 100644 --- a/tooling/nargo_fmt/src/formatter/function.rs +++ b/tooling/nargo_fmt/src/formatter/function.rs @@ -571,6 +571,36 @@ fn baz() { let z = 3 ; let y = 2; } +"; + let expected = src; + assert_format(src, expected); + } + + #[test] + fn keeps_newlines_between_comments_no_statements() { + let src = "fn foo() { + // foo + + // bar + + // baz +} +"; + let expected = src; + assert_format(src, expected); + } + + #[test] + fn keeps_newlines_between_comments_one_statement() { + let src = "fn foo() { + let x = 1; + + // foo + + // bar + + // baz +} "; let expected = src; assert_format(src, expected);