diff --git a/compiler/noirc_evaluator/src/errors.rs b/compiler/noirc_evaluator/src/errors.rs index 3a2f7dfc1c8..259d5908995 100644 --- a/compiler/noirc_evaluator/src/errors.rs +++ b/compiler/noirc_evaluator/src/errors.rs @@ -16,6 +16,8 @@ use thiserror::Error; use crate::ssa::ir::types::NumericType; use serde::{Deserialize, Serialize}; +pub type RtResult = Result; + #[derive(Debug, PartialEq, Eq, Clone, Error)] pub enum RuntimeError { #[error(transparent)] @@ -64,6 +66,14 @@ pub enum RuntimeError { "Could not resolve some references to the array. All references must be resolved at compile time" )] UnknownReference { call_stack: CallStack }, + #[error( + "Cannot return references from an if or match expression, or assignment within these expressions" + )] + ReturnedReferenceFromDynamicIf { call_stack: CallStack }, + #[error( + "Cannot return a function from an if or match expression, or assignment within these expressions" + )] + ReturnedFunctionFromDynamicIf { call_stack: CallStack }, } #[derive(Debug, Clone, Serialize, Deserialize, Hash)] @@ -174,6 +184,8 @@ impl RuntimeError { | RuntimeError::UnconstrainedSliceReturnToConstrained { call_stack } | RuntimeError::UnconstrainedOracleReturnToConstrained { call_stack } | RuntimeError::UnknownReference { call_stack } => call_stack, + RuntimeError::ReturnedReferenceFromDynamicIf { call_stack } => call_stack, + RuntimeError::ReturnedFunctionFromDynamicIf { call_stack } => call_stack, } } } diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index a182503eeac..4e1dc9b15a0 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -178,7 +178,7 @@ pub fn primary_passes(options: &SsaEvaluatorOptions) -> Vec { move |ssa| ssa.inline_functions_with_no_predicates(options.inliner_aggressiveness), "Inlining", ), - SsaPass::new(Ssa::remove_if_else, "Remove IfElse"), + SsaPass::new_try(Ssa::remove_if_else, "Remove IfElse"), SsaPass::new(Ssa::purity_analysis, "Purity Analysis"), SsaPass::new(Ssa::fold_constants, "Constant Folding"), SsaPass::new(Ssa::flatten_basic_conditionals, "Simplify conditionals for unconstrained"), diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/call.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/call.rs index 48fe315d379..36c27dd75e3 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/call.rs @@ -519,12 +519,17 @@ fn simplify_slice_push_back( let mut value_merger = ValueMerger::new(dfg, block, &mut slice_sizes, call_stack); - let new_slice = value_merger.merge_values( + let Ok(new_slice) = value_merger.merge_values( len_not_equals_capacity, len_equals_capacity, set_last_slice_value, new_slice, - ); + ) else { + // If we were to percolate up the error here, it'd get to insert_instruction and eventually + // all of ssa. Instead we just choose not to simplify the slice call since this should + // be a rare case. + return SimplifyResult::None; + }; SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) } diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index cf679d0e2e7..7cddafa824e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -1697,6 +1697,7 @@ mod test { .flatten_cfg() .mem2reg() .remove_if_else() + .unwrap() .fold_constants() .dead_instruction_elimination(); diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs index ce875388efe..49bdf152a65 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -2,12 +2,15 @@ use acvm::{FieldElement, acir::AcirField}; use fxhash::FxHashMap as HashMap; use noirc_errors::call_stack::CallStackId; -use crate::ssa::ir::{ - basic_block::BasicBlockId, - dfg::DataFlowGraph, - instruction::{ArrayOffset, BinaryOp, Instruction}, - types::{NumericType, Type}, - value::ValueId, +use crate::{ + errors::{RtResult, RuntimeError}, + ssa::ir::{ + basic_block::BasicBlockId, + dfg::DataFlowGraph, + instruction::{ArrayOffset, BinaryOp, Instruction}, + types::{NumericType, Type}, + value::ValueId, + }, }; pub(crate) struct ValueMerger<'a> { @@ -45,28 +48,36 @@ impl<'a> ValueMerger<'a> { else_condition: ValueId, then_value: ValueId, else_value: ValueId, - ) -> ValueId { + ) -> RtResult { if then_value == else_value { - return then_value; + return Ok(then_value); } match self.dfg.type_of_value(then_value) { - Type::Numeric(_) => Self::merge_numeric_values( + Type::Numeric(_) => Ok(Self::merge_numeric_values( self.dfg, self.block, then_condition, else_condition, then_value, else_value, - ), + )), typ @ Type::Array(_, _) => { self.merge_array_values(typ, then_condition, else_condition, then_value, else_value) } typ @ Type::Slice(_) => { self.merge_slice_values(typ, then_condition, else_condition, then_value, else_value) } - Type::Reference(_) => panic!("Cannot return references from an if expression"), - Type::Function => panic!("Cannot return functions from an if expression"), + Type::Reference(_) => { + // FIXME: none of then_value, else_value, then_condition, or else_condition have + // non-empty call stacks + let call_stack = self.dfg.get_value_call_stack(then_value); + Err(RuntimeError::ReturnedReferenceFromDynamicIf { call_stack }) + } + Type::Function => { + let call_stack = self.dfg.get_value_call_stack(then_value); + Err(RuntimeError::ReturnedFunctionFromDynamicIf { call_stack }) + } } } @@ -130,7 +141,7 @@ impl<'a> ValueMerger<'a> { else_condition: ValueId, then_value: ValueId, else_value: ValueId, - ) -> ValueId { + ) -> Result { let mut merged = im::Vector::new(); let (element_types, len) = match &typ { @@ -162,14 +173,14 @@ impl<'a> ValueMerger<'a> { else_condition, then_element, else_element, - )); + )?); } } let instruction = Instruction::MakeArray { elements: merged, typ }; - self.dfg - .insert_instruction_and_results(instruction, self.block, None, self.call_stack) - .first() + let result = + self.dfg.insert_instruction_and_results(instruction, self.block, None, self.call_stack); + Ok(result.first()) } fn merge_slice_values( @@ -179,7 +190,7 @@ impl<'a> ValueMerger<'a> { else_condition: ValueId, then_value_id: ValueId, else_value_id: ValueId, - ) -> ValueId { + ) -> Result { let mut merged = im::Vector::new(); let element_types = match &typ { @@ -219,37 +230,36 @@ impl<'a> ValueMerger<'a> { } else { let offset = ArrayOffset::None; let get = Instruction::ArrayGet { array, index, offset }; - self.dfg - .insert_instruction_and_results( - get, - self.block, - typevars, - self.call_stack, - ) - .first() + let results = self.dfg.insert_instruction_and_results( + get, + self.block, + typevars, + self.call_stack, + ); + results.first() } }; - let then_element = get_element( - then_value_id, - typevars.clone(), - then_len * element_types.len() as u32, - ); - let else_element = - get_element(else_value_id, typevars, else_len * element_types.len() as u32); + let len = then_len * element_types.len() as u32; + let then_element = get_element(then_value_id, typevars.clone(), len); + + let len = else_len * element_types.len() as u32; + let else_element = get_element(else_value_id, typevars, len); merged.push_back(self.merge_values( then_condition, else_condition, then_element, else_element, - )); + )?); } } let instruction = Instruction::MakeArray { elements: merged, typ }; let call_stack = self.call_stack; - self.dfg.insert_instruction_and_results(instruction, self.block, None, call_stack).first() + let result = + self.dfg.insert_instruction_and_results(instruction, self.block, None, call_stack); + Ok(result.first()) } /// Construct a dummy value to be attached to the smaller of two slices being merged. diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index 54f3a219944..272431d3a36 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -2,6 +2,7 @@ use std::collections::hash_map::Entry; use fxhash::FxHashMap as HashMap; +use crate::errors::RtResult; use crate::ssa::ir::function::RuntimeType; use crate::ssa::ir::instruction::Hint; use crate::ssa::ir::value::ValueId; @@ -27,25 +28,26 @@ impl Ssa { /// the given array may alias another array (e.g. function parameters or /// a `load`ed array from a reference). #[tracing::instrument(level = "trace", skip(self))] - pub(crate) fn remove_if_else(mut self) -> Ssa { + pub(crate) fn remove_if_else(mut self) -> RtResult { for function in self.functions.values_mut() { - function.remove_if_else(); + function.remove_if_else()?; } - self + Ok(self) } } impl Function { - pub(crate) fn remove_if_else(&mut self) { + pub(crate) fn remove_if_else(&mut self) -> RtResult<()> { // This should match the check in flatten_cfg if matches!(self.runtime(), RuntimeType::Brillig(_)) { // skip } else { - Context::default().remove_if_else(self); + Context::default().remove_if_else(self)?; } #[cfg(debug_assertions)] remove_if_else_post_check(self); + Ok(()) } } @@ -55,13 +57,13 @@ struct Context { } impl Context { - fn remove_if_else(&mut self, function: &mut Function) { + fn remove_if_else(&mut self, function: &mut Function) -> RtResult<()> { let block = function.entry_block(); // Make sure this optimization runs when there's only one block assert_eq!(function.dfg[block].successors().count(), 0); - function.simple_reachable_blocks_optimization(|context| { + function.simple_reachable_blocks_optimization_result(|context| { let instruction_id = context.instruction_id; let instruction = context.instruction(); @@ -84,16 +86,11 @@ impl Context { else_condition, then_value, else_value, - ); + )?; let _typ = context.dfg.type_of_value(value); let results = context.dfg.instruction_results(instruction_id); let result = results[0]; - // let result = match typ { - // Type::Array(..) => results[0], - // Type::Slice(..) => results[1], - // other => unreachable!("IfElse instructions should only have arrays or slices at this point. Found {other:?}"), - // }; context.remove_current_instruction(); context.replace_value(result, value); @@ -129,7 +126,8 @@ impl Context { } _ => (), } - }); + Ok(()) + }) } fn get_or_find_capacity(&mut self, dfg: &DataFlowGraph, value: ValueId) -> u32 { @@ -290,7 +288,7 @@ mod tests { "; let mut ssa = Ssa::from_str(src).unwrap(); - ssa = ssa.remove_if_else(); + ssa = ssa.remove_if_else().unwrap(); // In case our if block is never activated, we need to fetch each value from the original array. // We then should create a new array where each value can be mapped to `(then_condition * then_value) + (!then_condition * else_value)`. @@ -359,7 +357,7 @@ mod tests { "; let mut ssa = Ssa::from_str(src).unwrap(); - ssa = ssa.remove_if_else(); + ssa = ssa.remove_if_else().unwrap(); // We attempt to optimize array mergers to only handle where an array was modified, // rather than merging the entire array. As we only modify the `y` array at a single index, diff --git a/compiler/noirc_evaluator/src/ssa/opt/simple_optimization.rs b/compiler/noirc_evaluator/src/ssa/opt/simple_optimization.rs index 15877deab53..63a9e43099b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/simple_optimization.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/simple_optimization.rs @@ -1,9 +1,12 @@ -use crate::ssa::ir::{ - basic_block::BasicBlockId, - dfg::DataFlowGraph, - function::Function, - instruction::{Instruction, InstructionId}, - value::{ValueId, ValueMapping}, +use crate::{ + errors::RtResult, + ssa::ir::{ + basic_block::BasicBlockId, + dfg::DataFlowGraph, + function::Function, + instruction::{Instruction, InstructionId}, + value::{ValueId, ValueMapping}, + }, }; impl Function { @@ -23,6 +26,34 @@ impl Function { pub(crate) fn simple_reachable_blocks_optimization(&mut self, mut f: F) where F: FnMut(&mut SimpleOptimizationContext<'_, '_>), + { + self.simple_reachable_blocks_optimization_result(move |context| { + f(context); + Ok(()) + }) + .expect("`f` cannot error internally so this should be unreachable"); + } + + /// Performs a simple optimization according to the given callback, returning early if + /// an error occurred. + /// + /// The function's [`Function::reachable_blocks`] are traversed in turn, and instructions in those blocks + /// are then traversed in turn. For each one, `f` will be called with a context. + /// + /// The current instruction will be inserted at the end of the callback given to `mutate` unless + /// `remove_current_instruction` or `insert_current_instruction` are called. + /// + /// `insert_current_instruction` is useful if you need to insert new instructions after the current + /// one, so this can be done before the callback ends. + /// + /// `replace_value` can be used to replace a value with another one. This substitution will be + /// performed in all subsequent instructions. + pub(crate) fn simple_reachable_blocks_optimization_result( + &mut self, + mut f: F, + ) -> RtResult<()> + where + F: FnMut(&mut SimpleOptimizationContext<'_, '_>) -> RtResult<()>, { let mut values_to_replace = ValueMapping::default(); @@ -44,7 +75,7 @@ impl Function { values_to_replace: &mut values_to_replace, insert_current_instruction_at_callback_end: true, }; - f(&mut context); + f(&mut context)?; if context.insert_current_instruction_at_callback_end { self.dfg[block_id].insert_instruction(instruction_id); @@ -55,6 +86,7 @@ impl Function { } self.dfg.data_bus.replace_values(&values_to_replace); + Ok(()) } }