From 93060e054aa5dadd44808cc35f255acb5dc85776 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Thu, 28 Nov 2024 15:24:52 +0000 Subject: [PATCH 1/3] chore: pull out cfg simplification changes --- .../src/ssa/opt/constant_folding.rs | 895 ++++++++++++++++-- .../src/ssa/opt/flatten_cfg.rs | 29 + .../src/ssa/opt/simplify_cfg.rs | 112 ++- 3 files changed, 946 insertions(+), 90 deletions(-) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 9f55e69868c8..96683804042b 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -6,7 +6,7 @@ //! by the [`DataFlowGraph`] automatically as new instructions are pushed. //! - Check whether any input values have been constrained to be equal to a value of a simpler form //! by a [constrain instruction][Instruction::Constrain]. If so, replace the input value with the simpler form. -//! - Check whether the instruction [can_be_replaced][Instruction::can_be_replaced()] +//! - Check whether the instruction [can_be_deduplicated][Instruction::can_be_deduplicated()] //! by duplicate instruction earlier in the same block. //! //! These operations are done in parallel so that they can each benefit from each other @@ -19,32 +19,49 @@ //! //! 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; +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}, - 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; impl Ssa { /// Performs constant folding on each instruction. /// + /// It will not look at constraints to inform simplifications + /// based on the stated equivalence of two instructions. + /// /// See [`constant_folding`][self] module for more information. #[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 } @@ -57,8 +74,69 @@ 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 } } @@ -66,11 +144,15 @@ impl Ssa { 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 { use_constraint_info, ..Default::default() }; - context.block_queue.push(self.entry_block()); + 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() { + while let Some(block) = context.block_queue.pop_front() { if context.visited_blocks.contains(&block) { continue; } @@ -81,34 +163,73 @@ impl Function { } } -#[derive(Default)] -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: Vec, + block_queue: VecDeque, + + /// Contains sets of values which are constrained to be equivalent to each other. + /// + /// The mapping's structure is `side_effects_enabled_var => (constrained_value => simplified_value)`. + /// + /// We partition the maps of constrained values according to the side-effects flag at the point + /// at which the values are constrained. This prevents constraints which are only sometimes enforced + /// being used to modify the rest of the program. + constraint_simplification_mappings: HashMap>, + + // Cache of instructions without any side-effects along with their outputs. + cached_instruction_results: InstructionResultCache, + + dom: DominatorTree, } -/// HashMap from (Instruction, side_effects_enabled_var) to the results of the instruction. +#[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. -type InstructionResultCache = HashMap, Vec>>; +/// +/// The `side_effects_enabled_var` is optional because we only use them when `Instruction::requires_acir_gen_predicate` +/// is true _and_ the constraint information is also taken into account. +/// +/// In addition to each result, the original BasicBlockId is stored as well. This allows us +/// to deduplicate instructions across blocks as long as the new block dominates the original. +type InstructionResultCache = HashMap, ResultCache>>; + +/// Records the results of all duplicate [`Instruction`]s along with the blocks in which they sit. +/// +/// For more information see [`InstructionResultCache`]. +#[derive(Default)] +struct ResultCache { + result: Option<(BasicBlockId, Vec)>, +} + +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(), + cached_instruction_results: Default::default(), + dom: DominatorTree::with_function(function), + } + } -impl Context { fn fold_constants_in_block(&mut self, function: &mut Function, block: BasicBlockId) { let instructions = function.dfg[block].take_instructions(); - // Cache of instructions without any side-effects along with their outputs. - let mut cached_instruction_results = HashMap::default(); - - // Contains sets of values which are constrained to be equivalent to each other. - // - // The mapping's structure is `side_effects_enabled_var => (constrained_value => simplified_value)`. - // - // We partition the maps of constrained values according to the side-effects flag at the point - // at which the values are constrained. This prevents constraints which are only sometimes enforced - // being used to modify the rest of the program. - let mut constraint_simplification_mappings: HashMap> = - HashMap::default(); + // Default side effect condition variable with an enabled state. let mut side_effects_enabled_var = function.dfg.make_constant(FieldElement::one(), Type::bool()); @@ -117,8 +238,6 @@ impl Context { &mut function.dfg, block, instruction_id, - &mut cached_instruction_results, - &mut constraint_simplification_mappings, &mut side_effects_enabled_var, ); } @@ -126,29 +245,54 @@ impl Context { } fn fold_constants_into_instruction( - &self, + &mut self, dfg: &mut DataFlowGraph, - block: BasicBlockId, + mut block: BasicBlockId, id: InstructionId, - instruction_result_cache: &mut InstructionResultCache, - constraint_simplification_mappings: &mut HashMap>, side_effects_enabled_var: &mut ValueId, ) { - let constraint_simplification_mapping = - constraint_simplification_mappings.entry(*side_effects_enabled_var).or_default(); + let constraint_simplification_mapping = self.get_constraint_map(*side_effects_enabled_var); let instruction = Self::resolve_instruction(id, dfg, constraint_simplification_mapping); let old_results = dfg.instruction_results(id).to_vec(); // If a copy of this instruction exists earlier in the block, then reuse the previous results. - if let Some(cached_results) = - Self::get_cached(dfg, instruction_result_cache, &instruction, *side_effects_enabled_var) + if let Some(cache_result) = + self.get_cached(dfg, &instruction, *side_effects_enabled_var, block) { - Self::replace_result_ids(dfg, &old_results, cached_results); - return; + match cache_result { + CacheResult::Cached(cached) => { + Self::replace_result_ids(dfg, &old_results, cached); + return; + } + CacheResult::NeedToHoistToCommonBlock(dominator, _cached) => { + // Just change the block to insert in the common dominator instead. + // This will only move the current instance of the instruction right now. + // When constant folding is run a second time later on, it'll catch + // that the previous instance can be deduplicated to this instance. + block = dominator; + } + } } - // 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); @@ -156,9 +300,8 @@ impl Context { instruction.clone(), new_results, dfg, - instruction_result_cache, - constraint_simplification_mapping, *side_effects_enabled_var, + block, ); // If we just inserted an `Instruction::EnableSideEffectsIf`, we need to update `side_effects_enabled_var` @@ -229,13 +372,12 @@ impl Context { } fn cache_instruction( - &self, + &mut self, instruction: Instruction, instruction_results: Vec, dfg: &DataFlowGraph, - instruction_result_cache: &mut InstructionResultCache, - constraint_simplification_mapping: &mut HashMap, side_effects_enabled_var: ValueId, + block: BasicBlockId, ) { if self.use_constraint_info { // If the instruction was a constraint, then create a link between the two `ValueId`s @@ -248,18 +390,18 @@ impl Context { // Prefer replacing with constants where possible. (Value::NumericConstant { .. }, _) => { - constraint_simplification_mapping.insert(rhs, lhs); + self.get_constraint_map(side_effects_enabled_var).insert(rhs, lhs); } (_, Value::NumericConstant { .. }) => { - constraint_simplification_mapping.insert(lhs, rhs); + self.get_constraint_map(side_effects_enabled_var).insert(lhs, rhs); } // Otherwise prefer block parameters over instruction results. // This is as block parameters are more likely to be a single witness rather than a full expression. (Value::Param { .. }, Value::Instruction { .. }) => { - constraint_simplification_mapping.insert(rhs, lhs); + self.get_constraint_map(side_effects_enabled_var).insert(rhs, lhs); } (Value::Instruction { .. }, Value::Param { .. }) => { - constraint_simplification_mapping.insert(lhs, rhs); + self.get_constraint_map(side_effects_enabled_var).insert(lhs, rhs); } (_, _) => (), } @@ -268,18 +410,30 @@ impl Context { // If the instruction doesn't have side-effects and if it won't interact with enable_side_effects during acir_gen, // we cache the results so we can reuse them if the same instruction appears again later in the block. + // Others have side effects representing failure, which are implicit in the ACIR code and can also be deduplicated. if instruction.can_be_deduplicated(dfg, self.use_constraint_info) { let use_predicate = self.use_constraint_info && instruction.requires_acir_gen_predicate(dfg); let predicate = use_predicate.then_some(side_effects_enabled_var); - instruction_result_cache + self.cached_instruction_results .entry(instruction) .or_default() - .insert(predicate, instruction_results); + .entry(predicate) + .or_default() + .cache(block, instruction_results); } } + /// Get the simplification mapping from complex to simpler instructions, + /// which all depend on the same side effect condition variable. + fn get_constraint_map( + &mut self, + side_effects_enabled_var: ValueId, + ) -> &mut HashMap { + self.constraint_simplification_mappings.entry(side_effects_enabled_var).or_default() + } + /// Replaces a set of [`ValueId`]s inside the [`DataFlowGraph`] with another. fn replace_result_ids( dfg: &mut DataFlowGraph, @@ -291,26 +445,258 @@ impl Context { } } - fn get_cached<'a>( + fn get_cached( + &mut self, dfg: &DataFlowGraph, - instruction_result_cache: &'a mut InstructionResultCache, instruction: &Instruction, side_effects_enabled_var: ValueId, - ) -> Option<&'a Vec> { - let results_for_instruction = instruction_result_cache.get(instruction); + block: BasicBlockId, + ) -> Option { + let results_for_instruction = self.cached_instruction_results.get(instruction)?; + + let predicate = self.use_constraint_info && instruction.requires_acir_gen_predicate(dfg); + let predicate = predicate.then_some(side_effects_enabled_var); - // See if there's a cached version with no predicate first - if let Some(results) = results_for_instruction.and_then(|map| map.get(&None)) { - return Some(results); + 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) + } } + } - let predicate = - instruction.requires_acir_gen_predicate(dfg).then_some(side_effects_enabled_var); + /// 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; + }; - results_for_instruction.and_then(|map| map.get(&predicate)) + 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 { + /// Records that an `Instruction` in block `block` produced the result values `results`. + fn cache(&mut self, block: BasicBlockId, results: Vec) { + if self.result.is_none() { + self.result = Some((block, results)); + } + } + + /// Returns a set of [`ValueId`]s produced from a copy of this [`Instruction`] which sits + /// within a block which dominates `block`. + /// + /// We require that the cached instruction's block dominates `block` in order to avoid + /// cycles causing issues (e.g. two instructions being replaced with the results of each other + /// such that neither instruction exists anymore.) + fn get(&self, block: BasicBlockId, dom: &mut DominatorTree) -> Option { + self.result.as_ref().map(|(origin_block, results)| { + if dom.dominates(*origin_block, block) { + CacheResult::Cached(results) + } else { + // Insert a copy of this instruction in the common dominator + let dominator = dom.common_dominator(*origin_block, block); + CacheResult::NeedToHoistToCommonBlock(dominator, results) + } + }) + } +} + +enum CacheResult<'a> { + Cached(&'a [ValueId]), + 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; @@ -547,22 +933,32 @@ mod test { // Regression for #4600 #[test] fn array_get_regression() { + // fn main f0 { + // b0(v0: u1, v1: u64): + // enable_side_effects_if v0 + // v2 = make_array [Field 0, Field 1] + // v3 = array_get v2, index v1 + // v4 = not v0 + // enable_side_effects_if v4 + // v5 = array_get v2, index v1 + // } + // // We want to make sure after constant folding both array_gets remain since they are // under different enable_side_effects_if contexts and thus one may be disabled while // the other is not. If one is removed, it is possible e.g. v4 is replaced with v2 which // is disabled (only gets from index 0) and thus returns the wrong result. let src = " - acir(inline) fn main f0 { - b0(v0: u1, v1: u64): - enable_side_effects v0 - v4 = make_array [Field 0, Field 1] : [Field; 2] - v5 = array_get v4, index v1 -> Field - v6 = not v0 - enable_side_effects v6 - v7 = array_get v4, index v1 -> Field - return - } - "; + acir(inline) fn main f0 { + b0(v0: u1, v1: u64): + enable_side_effects v0 + v4 = make_array [Field 0, Field 1] : [Field; 2] + v5 = array_get v4, index v1 -> Field + v6 = not v0 + enable_side_effects v6 + v7 = array_get v4, index v1 -> Field + return + } + "; let ssa = Ssa::from_str(src).unwrap(); // Expected output is unchanged @@ -620,14 +1016,14 @@ mod test { assert_normalized_ssa_equals(ssa, expected); } - // This test currently fails. It being fixed will address the issue https://github.com/noir-lang/noir/issues/5756 #[test] - #[should_panic] fn constant_array_deduplication() { // fn main f0 { // b0(v0: u64): - // v5 = call keccakf1600([v0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0]) - // v6 = call keccakf1600([v0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0]) + // v1 = make_array [v0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0] + // v2 = make_array [v0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0] + // v5 = call keccakf1600(v1) + // v6 = call keccakf1600(v2) // } // // Here we're checking a situation where two identical arrays are being initialized twice and being assigned separate `ValueId`s. @@ -647,12 +1043,13 @@ mod test { let array1 = builder.insert_make_array(array_contents.clone(), typ.clone()); let array2 = builder.insert_make_array(array_contents, typ.clone()); - assert_eq!(array1, array2, "arrays were assigned different value ids"); + assert_ne!(array1, array2, "arrays were not assigned different value ids"); let keccakf1600 = builder.import_intrinsic("keccakf1600").expect("keccakf1600 intrinsic should exist"); let _v10 = builder.insert_call(keccakf1600, vec![array1], vec![typ.clone()]); let _v11 = builder.insert_call(keccakf1600, vec![array2], vec![typ.clone()]); + builder.terminate_with_return(Vec::new()); let mut ssa = builder.finish(); ssa.normalize_ids(); @@ -662,8 +1059,13 @@ mod test { let main = ssa.main(); let instructions = main.dfg[main.entry_block()].instructions(); let starting_instruction_count = instructions.len(); - assert_eq!(starting_instruction_count, 2); + assert_eq!(starting_instruction_count, 4); + // fn main f0 { + // b0(v0: u64): + // v1 = make_array [v0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0] + // v5 = call keccakf1600(v1) + // } let ssa = ssa.fold_constants(); println!("{ssa}"); @@ -671,6 +1073,321 @@ mod test { let main = ssa.main(); let instructions = main.dfg[main.entry_block()].instructions(); let ending_instruction_count = instructions.len(); - assert_eq!(ending_instruction_count, 1); + assert_eq!(ending_instruction_count, 2); + } + + #[test] + fn deduplicate_across_blocks() { + // fn main f0 { + // b0(v0: u1): + // v1 = not v0 + // jmp b1() + // b1(): + // v2 = not v0 + // return v2 + // } + let main_id = Id::test_new(0); + + // Compiling main + let mut builder = FunctionBuilder::new("main".into(), main_id); + let b1 = builder.insert_block(); + + let v0 = builder.add_parameter(Type::bool()); + let _v1 = builder.insert_not(v0); + builder.terminate_with_jmp(b1, Vec::new()); + + builder.switch_to_block(b1); + let v2 = builder.insert_not(v0); + builder.terminate_with_return(vec![v2]); + + let ssa = builder.finish(); + let main = ssa.main(); + assert_eq!(main.dfg[main.entry_block()].instructions().len(), 1); + assert_eq!(main.dfg[b1].instructions().len(), 1); + + // Expected output: + // + // fn main f0 { + // b0(v0: u1): + // v1 = not v0 + // jmp b1() + // b1(): + // return v1 + // } + let ssa = ssa.fold_constants_using_constraints(); + let main = ssa.main(); + assert_eq!(main.dfg[main.entry_block()].instructions().len(), 1); + assert_eq!(main.dfg[b1].instructions().len(), 0); + } + + #[test] + fn deduplicate_across_non_dominated_blocks() { + let src = " + brillig(inline) fn main f0 { + b0(v0: u32): + v2 = lt u32 1000, v0 + jmpif v2 then: b1, else: b2 + b1(): + v4 = add v0, u32 1 + v5 = lt v0, v4 + constrain v5 == u1 1 + jmp b2() + b2(): + v7 = lt u32 1000, v0 + jmpif v7 then: b3, else: b4 + b3(): + v8 = add v0, u32 1 + v9 = lt v0, v8 + constrain v9 == u1 1 + jmp b4() + b4(): + return + } + "; + let ssa = Ssa::from_str(src).unwrap(); + + // v4 has been hoisted, although: + // - v5 has not yet been removed since it was encountered earlier in the program + // - v8 hasn't been recognized as a duplicate of v6 yet since they still reference v4 and + // v5 respectively + let expected = " + brillig(inline) fn main f0 { + b0(v0: u32): + v2 = lt u32 1000, v0 + v4 = add v0, u32 1 + jmpif v2 then: b1, else: b2 + b1(): + v5 = add v0, u32 1 + v6 = lt v0, v5 + constrain v6 == u1 1 + jmp b2() + b2(): + jmpif v2 then: b3, else: b4 + b3(): + v8 = lt v0, v4 + constrain v8 == u1 1 + jmp b4() + b4(): + return + } + "; + + 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); + } + + #[test] + fn deduplicates_side_effecting_intrinsics() { + let src = " + // After EnableSideEffectsIf removal: + acir(inline) fn main f0 { + b0(v0: Field, v1: Field, v2: u1): + v4 = call is_unconstrained() -> u1 + v7 = call to_be_radix(v0, u32 256) -> [u8; 1] // `a.to_be_radix(256)`; + inc_rc v7 + v8 = call to_be_radix(v0, u32 256) -> [u8; 1] // duplicate load of `a` + inc_rc v8 + v9 = cast v2 as Field // `if c { a.to_be_radix(256) }` + v10 = mul v0, v9 // attaching `c` to `a` + v11 = call to_be_radix(v10, u32 256) -> [u8; 1] // calling `to_radix(c * a)` + inc_rc v11 + enable_side_effects v2 // side effect var for `c` shifted down by removal + return + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let expected = " + acir(inline) fn main f0 { + b0(v0: Field, v1: Field, v2: u1): + v4 = call is_unconstrained() -> u1 + v7 = call to_be_radix(v0, u32 256) -> [u8; 1] + inc_rc v7 + inc_rc v7 + v8 = cast v2 as Field + v9 = mul v0, v8 + v10 = call to_be_radix(v9, u32 256) -> [u8; 1] + inc_rc v10 + enable_side_effects v2 + return + } + "; + let ssa = ssa.fold_constants_using_constraints(); + assert_normalized_ssa_equals(ssa, expected); } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index b6899a71fee7..5d114672a556 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -447,6 +447,16 @@ impl<'f> Context<'f> { }; self.condition_stack.push(cond_context); self.insert_current_side_effects_enabled(); + + // We disallow this case as it results in the `else_destination` block + // being inlined before the `then_destination` block due to block deduplication in the work queue. + // + // The `else_destination` block then gets treated as if it were the `then_destination` block + // and has the incorrect condition applied to it. + assert_ne!( + self.branch_ends[if_entry], *then_destination, + "ICE: branches merge inside of `then` branch" + ); vec![self.branch_ends[if_entry], *else_destination, *then_destination] } @@ -1526,4 +1536,23 @@ mod test { _ => unreachable!("Should have terminator instruction"), } } + + #[test] + #[should_panic = "ICE: branches merge inside of `then` branch"] + fn panics_if_branches_merge_within_then_branch() { + //! This is a regression test for https://github.com/noir-lang/noir/issues/6620 + + let src = " + acir(inline) fn main f0 { + b0(v0: u1): + jmpif v0 then: b2, else: b1 + b2(): + return + b1(): + jmp b2() + } + "; + let merged_ssa = Ssa::from_str(src).unwrap(); + let _ = merged_ssa.flatten_cfg(); + } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index 46941775c5e8..c282e2df4510 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -18,7 +18,8 @@ use crate::ssa::{ basic_block::BasicBlockId, cfg::ControlFlowGraph, function::{Function, RuntimeType}, - instruction::TerminatorInstruction, + instruction::{Instruction, TerminatorInstruction}, + value::Value, }, ssa_gen::Ssa, }; @@ -31,6 +32,7 @@ impl Ssa { /// 4. Removing any blocks which have no instructions other than a single terminating jmp. /// 5. Replacing any jmpifs with constant conditions with jmps. If this causes the block to have /// only 1 successor then (2) also will be applied. + /// 6. Replacing any jmpifs with a negated condition with a jmpif with a un-negated condition and reversed branches. /// /// Currently, 1 is unimplemented. #[tracing::instrument(level = "trace", skip(self))] @@ -55,6 +57,8 @@ impl Function { stack.extend(self.dfg[block].successors().filter(|block| !visited.contains(block))); } + check_for_negated_jmpif_condition(self, block, &mut cfg); + // This call is before try_inline_into_predecessor so that if it succeeds in changing a // jmpif into a jmp, the block may then be inlined entirely into its predecessor in try_inline_into_predecessor. check_for_constant_jmpif(self, block, &mut cfg); @@ -184,6 +188,55 @@ fn check_for_double_jmp(function: &mut Function, block: BasicBlockId, cfg: &mut cfg.recompute_block(function, block); } +/// Optimize a jmpif on a negated condition by swapping the branches. +fn check_for_negated_jmpif_condition( + function: &mut Function, + block: BasicBlockId, + cfg: &mut ControlFlowGraph, +) { + if matches!(function.runtime(), RuntimeType::Acir(_)) { + // Swapping the `then` and `else` branches of a `JmpIf` within an ACIR function + // can result in the situation where the branches merge together again in the `then` block, e.g. + // + // acir(inline) fn main f0 { + // b0(v0: u1): + // jmpif v0 then: b2, else: b1 + // b2(): + // return + // b1(): + // jmp b2() + // } + // + // This breaks the `flatten_cfg` pass as it assumes that merges only happen in + // the `else` block or a 3rd block. + // + // See: https://github.com/noir-lang/noir/pull/5891#issuecomment-2500219428 + return; + } + + if let Some(TerminatorInstruction::JmpIf { + condition, + then_destination, + else_destination, + call_stack, + }) = function.dfg[block].terminator() + { + if let Value::Instruction { instruction, .. } = function.dfg[*condition] { + if let Instruction::Not(negated_condition) = function.dfg[instruction] { + let call_stack = call_stack.clone(); + let jmpif = TerminatorInstruction::JmpIf { + condition: negated_condition, + then_destination: *else_destination, + else_destination: *then_destination, + call_stack, + }; + function.dfg[block].set_terminator(jmpif); + cfg.recompute_block(function, block); + } + } + } +} + /// If the given block has block parameters, replace them with the jump arguments from the predecessor. /// /// Currently, if this function is needed, `try_inline_into_predecessor` will also always apply, @@ -246,6 +299,8 @@ mod test { map::Id, types::Type, }, + opt::assert_normalized_ssa_equals, + Ssa, }; use acvm::acir::AcirField; @@ -359,4 +414,59 @@ mod test { other => panic!("Unexpected terminator {other:?}"), } } + + #[test] + fn swap_negated_jmpif_branches_in_brillig() { + let src = " + brillig(inline) fn main f0 { + b0(v0: u1): + v1 = allocate -> &mut Field + store Field 0 at v1 + v3 = not v0 + jmpif v3 then: b1, else: b2 + b1(): + store Field 2 at v1 + jmp b2() + b2(): + v5 = load v1 -> Field + v6 = eq v5, Field 2 + constrain v5 == Field 2 + return + }"; + let ssa = Ssa::from_str(src).unwrap(); + + let expected = " + brillig(inline) fn main f0 { + b0(v0: u1): + v1 = allocate -> &mut Field + store Field 0 at v1 + v3 = not v0 + jmpif v0 then: b2, else: b1 + b2(): + v5 = load v1 -> Field + v6 = eq v5, Field 2 + constrain v5 == Field 2 + return + b1(): + store Field 2 at v1 + jmp b2() + }"; + assert_normalized_ssa_equals(ssa.simplify_cfg(), expected); + } + + #[test] + fn does_not_swap_negated_jmpif_branches_in_acir() { + let src = " + acir(inline) fn main f0 { + b0(v0: u1): + v1 = not v0 + jmpif v1 then: b1, else: b2 + b1(): + jmp b2() + b2(): + return + }"; + let ssa = Ssa::from_str(src).unwrap(); + assert_normalized_ssa_equals(ssa.simplify_cfg(), src); + } } From 0f4d57ec858e69967ec7534145ee751ba46b0339 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Thu, 28 Nov 2024 16:55:40 +0000 Subject: [PATCH 2/3] . --- .../src/ssa/opt/constant_folding.rs | 620 +----------------- 1 file changed, 29 insertions(+), 591 deletions(-) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index b3dc62bcb668..a219e038c64c 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -6,7 +6,7 @@ //! by the [`DataFlowGraph`] automatically as new instructions are pushed. //! - Check whether any input values have been constrained to be equal to a value of a simpler form //! by a [constrain instruction][Instruction::Constrain]. If so, replace the input value with the simpler form. -//! - Check whether the instruction [can_be_deduplicated][Instruction::can_be_deduplicated()] +//! - Check whether the instruction [can_be_replaced][Instruction::can_be_replaced()] //! by duplicate instruction earlier in the same block. //! //! These operations are done in parallel so that they can each benefit from each other @@ -21,13 +21,7 @@ //! different blocks are merged, i.e. after the [`flatten_cfg`][super::flatten_cfg] pass. use std::collections::{HashSet, VecDeque}; -use acvm::{ - acir::AcirField, - brillig_vm::{MemoryValue, VMStatus, VM}, - FieldElement, -}; -use bn254_blackbox_solver::Bn254BlackBoxSolver; -use im::Vector; +use acvm::{acir::AcirField, FieldElement}; use iter_extended::vecmap; use crate::ssa::{ @@ -40,20 +34,18 @@ use crate::ssa::{ types::Type, value::{Value, ValueId}, }, + ssa_gen::Ssa, }; use fxhash::FxHashMap as HashMap; impl Ssa { /// Performs constant folding on each instruction. /// - /// It will not look at constraints to inform simplifications - /// based on the stated equivalence of two instructions. - /// /// See [`constant_folding`][self] module for more information. #[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, None); + function.constant_fold(false); } self } @@ -66,71 +58,10 @@ 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, None); + function.constant_fold(true); } 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 { @@ -153,7 +84,6 @@ impl Function { struct Context { 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, @@ -225,7 +155,7 @@ impl Context { fn fold_constants_into_instruction( &mut self, dfg: &mut DataFlowGraph, - mut block: BasicBlockId, + block: BasicBlockId, id: InstructionId, side_effects_enabled_var: &mut ValueId, ) { @@ -244,40 +174,12 @@ impl Context { if let Some(cached_results) = self.get_cached(dfg, &instruction, *side_effects_enabled_var, block) { - match cache_result { - CacheResult::Cached(cached) => { - Self::replace_result_ids(dfg, &old_results, cached); - return; - } - CacheResult::NeedToHoistToCommonBlock(dominator, _cached) => { - // Just change the block to insert in the common dominator instead. - // This will only move the current instance of the instruction right now. - // When constant folding is run a second time later on, it'll catch - // that the previous instance can be deduplicated to this instance. - block = dominator; - } - } + Self::replace_result_ids(dfg, &old_results, cached_results); + return; } - 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, - ) - }); + // 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); Self::replace_result_ids(dfg, &old_results, &new_results); @@ -393,7 +295,6 @@ impl Context { // If the instruction doesn't have side-effects and if it won't interact with enable_side_effects during acir_gen, // we cache the results so we can reuse them if the same instruction appears again later in the block. - // Others have side effects representing failure, which are implicit in the ACIR code and can also be deduplicated. if instruction.can_be_deduplicated(dfg, self.use_constraint_info) { let use_predicate = self.use_constraint_info && instruction.requires_acir_gen_predicate(dfg); @@ -481,138 +382,6 @@ fn simplify(dfg: &DataFlowGraph, lhs: ValueId, rhs: ValueId) -> Option<(ValueId, (Value::Instruction { .. }, Value::Param { .. }) => Some((lhs, rhs)), (_, _) => None, } - - /// 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 { - /// Records that an `Instruction` in block `block` produced the result values `results`. - fn cache(&mut self, block: BasicBlockId, results: Vec) { - if self.result.is_none() { - self.result = Some((block, results)); - } - } - - /// Returns a set of [`ValueId`]s produced from a copy of this [`Instruction`] which sits - /// within a block which dominates `block`. - /// - /// We require that the cached instruction's block dominates `block` in order to avoid - /// cycles causing issues (e.g. two instructions being replaced with the results of each other - /// such that neither instruction exists anymore.) - fn get(&self, block: BasicBlockId, dom: &mut DominatorTree) -> Option { - self.result.as_ref().map(|(origin_block, results)| { - if dom.dominates(*origin_block, block) { - CacheResult::Cached(results) - } else { - // Insert a copy of this instruction in the common dominator - let dominator = dom.common_dominator(*origin_block, block); - CacheResult::NeedToHoistToCommonBlock(dominator, results) - } - }) - } -} - -enum CacheResult<'a> { - Cached(&'a [ValueId]), - 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)] @@ -851,32 +620,22 @@ mod test { // Regression for #4600 #[test] fn array_get_regression() { - // fn main f0 { - // b0(v0: u1, v1: u64): - // enable_side_effects_if v0 - // v2 = make_array [Field 0, Field 1] - // v3 = array_get v2, index v1 - // v4 = not v0 - // enable_side_effects_if v4 - // v5 = array_get v2, index v1 - // } - // // We want to make sure after constant folding both array_gets remain since they are // under different enable_side_effects_if contexts and thus one may be disabled while // the other is not. If one is removed, it is possible e.g. v4 is replaced with v2 which // is disabled (only gets from index 0) and thus returns the wrong result. let src = " - acir(inline) fn main f0 { - b0(v0: u1, v1: u64): - enable_side_effects v0 - v4 = make_array [Field 0, Field 1] : [Field; 2] - v5 = array_get v4, index v1 -> Field - v6 = not v0 - enable_side_effects v6 - v7 = array_get v4, index v1 -> Field - return - } - "; + acir(inline) fn main f0 { + b0(v0: u1, v1: u64): + enable_side_effects v0 + v4 = make_array [Field 0, Field 1] : [Field; 2] + v5 = array_get v4, index v1 -> Field + v6 = not v0 + enable_side_effects v6 + v7 = array_get v4, index v1 -> Field + return + } + "; let ssa = Ssa::from_str(src).unwrap(); // Expected output is unchanged @@ -934,14 +693,14 @@ mod test { assert_normalized_ssa_equals(ssa, expected); } + // This test currently fails. It being fixed will address the issue https://github.com/noir-lang/noir/issues/5756 #[test] + #[should_panic] fn constant_array_deduplication() { // fn main f0 { // b0(v0: u64): - // v1 = make_array [v0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0] - // v2 = make_array [v0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0] - // v5 = call keccakf1600(v1) - // v6 = call keccakf1600(v2) + // v5 = call keccakf1600([v0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0]) + // v6 = call keccakf1600([v0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0]) // } // // Here we're checking a situation where two identical arrays are being initialized twice and being assigned separate `ValueId`s. @@ -961,13 +720,12 @@ mod test { let array1 = builder.insert_make_array(array_contents.clone(), typ.clone()); let array2 = builder.insert_make_array(array_contents, typ.clone()); - assert_ne!(array1, array2, "arrays were not assigned different value ids"); + assert_eq!(array1, array2, "arrays were assigned different value ids"); let keccakf1600 = builder.import_intrinsic("keccakf1600").expect("keccakf1600 intrinsic should exist"); let _v10 = builder.insert_call(keccakf1600, vec![array1], vec![typ.clone()]); let _v11 = builder.insert_call(keccakf1600, vec![array2], vec![typ.clone()]); - builder.terminate_with_return(Vec::new()); let mut ssa = builder.finish(); ssa.normalize_ids(); @@ -977,13 +735,8 @@ mod test { let main = ssa.main(); let instructions = main.dfg[main.entry_block()].instructions(); let starting_instruction_count = instructions.len(); - assert_eq!(starting_instruction_count, 4); + assert_eq!(starting_instruction_count, 2); - // fn main f0 { - // b0(v0: u64): - // v1 = make_array [v0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0, u64 0] - // v5 = call keccakf1600(v1) - // } let ssa = ssa.fold_constants(); println!("{ssa}"); @@ -991,322 +744,7 @@ mod test { let main = ssa.main(); let instructions = main.dfg[main.entry_block()].instructions(); let ending_instruction_count = instructions.len(); - assert_eq!(ending_instruction_count, 2); - } - - #[test] - fn deduplicate_across_blocks() { - // fn main f0 { - // b0(v0: u1): - // v1 = not v0 - // jmp b1() - // b1(): - // v2 = not v0 - // return v2 - // } - let main_id = Id::test_new(0); - - // Compiling main - let mut builder = FunctionBuilder::new("main".into(), main_id); - let b1 = builder.insert_block(); - - let v0 = builder.add_parameter(Type::bool()); - let _v1 = builder.insert_not(v0); - builder.terminate_with_jmp(b1, Vec::new()); - - builder.switch_to_block(b1); - let v2 = builder.insert_not(v0); - builder.terminate_with_return(vec![v2]); - - let ssa = builder.finish(); - let main = ssa.main(); - assert_eq!(main.dfg[main.entry_block()].instructions().len(), 1); - assert_eq!(main.dfg[b1].instructions().len(), 1); - - // Expected output: - // - // fn main f0 { - // b0(v0: u1): - // v1 = not v0 - // jmp b1() - // b1(): - // return v1 - // } - let ssa = ssa.fold_constants_using_constraints(); - let main = ssa.main(); - assert_eq!(main.dfg[main.entry_block()].instructions().len(), 1); - assert_eq!(main.dfg[b1].instructions().len(), 0); - } - - #[test] - fn deduplicate_across_non_dominated_blocks() { - let src = " - brillig(inline) fn main f0 { - b0(v0: u32): - v2 = lt u32 1000, v0 - jmpif v2 then: b1, else: b2 - b1(): - v4 = add v0, u32 1 - v5 = lt v0, v4 - constrain v5 == u1 1 - jmp b2() - b2(): - v7 = lt u32 1000, v0 - jmpif v7 then: b3, else: b4 - b3(): - v8 = add v0, u32 1 - v9 = lt v0, v8 - constrain v9 == u1 1 - jmp b4() - b4(): - return - } - "; - let ssa = Ssa::from_str(src).unwrap(); - - // v4 has been hoisted, although: - // - v5 has not yet been removed since it was encountered earlier in the program - // - v8 hasn't been recognized as a duplicate of v6 yet since they still reference v4 and - // v5 respectively - let expected = " - brillig(inline) fn main f0 { - b0(v0: u32): - v2 = lt u32 1000, v0 - v4 = add v0, u32 1 - jmpif v2 then: b1, else: b2 - b1(): - v5 = add v0, u32 1 - v6 = lt v0, v5 - constrain v6 == u1 1 - jmp b2() - b2(): - jmpif v2 then: b3, else: b4 - b3(): - v8 = lt v0, v4 - constrain v8 == u1 1 - jmp b4() - b4(): - return - } - "; - - 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); - } - - #[test] - fn deduplicates_side_effecting_intrinsics() { - let src = " - // After EnableSideEffectsIf removal: - acir(inline) fn main f0 { - b0(v0: Field, v1: Field, v2: u1): - v4 = call is_unconstrained() -> u1 - v7 = call to_be_radix(v0, u32 256) -> [u8; 1] // `a.to_be_radix(256)`; - inc_rc v7 - v8 = call to_be_radix(v0, u32 256) -> [u8; 1] // duplicate load of `a` - inc_rc v8 - v9 = cast v2 as Field // `if c { a.to_be_radix(256) }` - v10 = mul v0, v9 // attaching `c` to `a` - v11 = call to_be_radix(v10, u32 256) -> [u8; 1] // calling `to_radix(c * a)` - inc_rc v11 - enable_side_effects v2 // side effect var for `c` shifted down by removal - return - } - "; - let ssa = Ssa::from_str(src).unwrap(); - let expected = " - acir(inline) fn main f0 { - b0(v0: Field, v1: Field, v2: u1): - v4 = call is_unconstrained() -> u1 - v7 = call to_be_radix(v0, u32 256) -> [u8; 1] - inc_rc v7 - inc_rc v7 - v8 = cast v2 as Field - v9 = mul v0, v8 - v10 = call to_be_radix(v9, u32 256) -> [u8; 1] - inc_rc v10 - enable_side_effects v2 - return - } - "; - let ssa = ssa.fold_constants_using_constraints(); - assert_normalized_ssa_equals(ssa, expected); + assert_eq!(ending_instruction_count, 1); } #[test] @@ -1352,4 +790,4 @@ mod test { assert_eq!(main.dfg[main.entry_block()].instructions().len(), 1); assert_eq!(main.dfg[b1].instructions().len(), 0); } -} +} \ No newline at end of file From a8afa832fb9719d110cba76f5aa837b859bfe6de Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Thu, 28 Nov 2024 17:23:37 +0000 Subject: [PATCH 3/3] . --- .../compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index a219e038c64c..9ee9a52b5adb 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -790,4 +790,4 @@ mod test { assert_eq!(main.dfg[main.entry_block()].instructions().len(), 1); assert_eq!(main.dfg[b1].instructions().len(), 0); } -} \ No newline at end of file +}