diff --git a/.noir-sync-commit b/.noir-sync-commit index c7d7dc99a114..caa81d0f7be8 100644 --- a/.noir-sync-commit +++ b/.noir-sync-commit @@ -1 +1 @@ -d4c68066ab35ce1c52510cf0c038fb627a0677c3 +a87c655c6c8c077c71e3372cc9181b7870348a3d diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 2c9d43dc9196..873ebe51e6f0 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -619,7 +619,7 @@ impl<'block> BrilligBlock<'block> { destination_variable, ); } - Instruction::ArraySet { array, index, value, .. } => { + Instruction::ArraySet { array, index, value, mutable: _ } => { let source_variable = self.convert_ssa_value(*array, dfg); let index_register = self.convert_ssa_single_addr_value(*index, dfg); let value_variable = self.convert_ssa_value(*value, dfg); @@ -700,6 +700,9 @@ impl<'block> BrilligBlock<'block> { Instruction::EnableSideEffects { .. } => { todo!("enable_side_effects not supported by brillig") } + Instruction::IfElse { .. } => { + unreachable!("IfElse instructions should not be possible in brillig") + } }; let dead_variables = self diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs index 7d571a2c3bcf..e844bc30354f 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs @@ -69,6 +69,7 @@ pub(crate) fn optimize_into_acir( // Run the inlining pass again to handle functions with `InlineType::NoPredicates`. // Before flattening is run, we treat functions marked with the `InlineType::NoPredicates` as an entry point. .run_pass(Ssa::inline_functions_with_no_predicates, "After Inlining:") + .run_pass(Ssa::remove_if_else, "After Remove IfElse:") .run_pass(Ssa::fold_constants, "After Constant Folding:") .run_pass(Ssa::remove_enable_side_effects, "After EnableSideEffects removal:") .run_pass(Ssa::fold_constants_using_constraints, "After Constraint Folding:") diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 9ea5c5e4a963..9a4d4be1145d 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -705,6 +705,9 @@ impl<'a> Context<'a> { assert_message.clone(), )?; } + Instruction::IfElse { .. } => { + unreachable!("IfElse instruction remaining in acir-gen") + } } self.acir_context.set_call_stack(CallStack::new()); @@ -732,11 +735,10 @@ impl<'a> Context<'a> { assert!(!matches!(inline_type, InlineType::Inline), "ICE: Got an ACIR function named {} that should have already been inlined", func.name()); let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg)); - // TODO(https://github.com/noir-lang/noir/issues/4608): handle complex return types from ACIR functions - let output_count = - result_ids.iter().fold(0usize, |sum, result_id| { - sum + dfg.try_get_array_length(*result_id).unwrap_or(1) - }); + let output_count = result_ids + .iter() + .map(|result_id| dfg.type_of_value(*result_id).flattened_size()) + .sum(); let acir_function_id = ssa .entry_point_to_generated_index @@ -748,6 +750,7 @@ impl<'a> Context<'a> { output_count, self.current_side_effects_enabled_var, )?; + let output_values = self.convert_vars_to_values(output_vars, dfg, result_ids); @@ -1028,6 +1031,7 @@ impl<'a> Context<'a> { }); } }; + if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) { // Report the error if side effects are enabled. if index >= array_size { diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 6b950c327cf2..85630b756149 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -554,7 +554,10 @@ impl<'dfg> InsertInstructionResult<'dfg> { match self { InsertInstructionResult::SimplifiedTo(value) => *value, InsertInstructionResult::SimplifiedToMultiple(values) => values[0], - InsertInstructionResult::Results(_, results) => results[0], + InsertInstructionResult::Results(_, results) => { + assert_eq!(results.len(), 1); + results[0] + } InsertInstructionResult::InstructionRemoved => { panic!("Instruction was removed, no results") } @@ -583,6 +586,24 @@ impl<'dfg> InsertInstructionResult<'dfg> { } } +impl<'dfg> std::ops::Index for InsertInstructionResult<'dfg> { + type Output = ValueId; + + fn index(&self, index: usize) -> &Self::Output { + match self { + InsertInstructionResult::Results(_, results) => &results[index], + InsertInstructionResult::SimplifiedTo(result) => { + assert_eq!(index, 0); + result + } + InsertInstructionResult::SimplifiedToMultiple(results) => &results[index], + InsertInstructionResult::InstructionRemoved => { + panic!("Cannot index into InsertInstructionResult::InstructionRemoved") + } + } + } +} + #[cfg(test)] mod tests { use super::DataFlowGraph; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 3084899f4552..7cc19e9f2b8d 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -11,6 +11,8 @@ use fxhash::FxHasher; use iter_extended::vecmap; use noirc_frontend::hir_def::types::Type as HirType; +use crate::ssa::opt::flatten_cfg::value_merger::ValueMerger; + use super::{ basic_block::BasicBlockId, dfg::{CallStack, DataFlowGraph}, @@ -216,6 +218,14 @@ pub(crate) enum Instruction { /// implemented via reference counting. In ACIR code this is done with im::Vector and these /// DecrementRc instructions are ignored. DecrementRc { value: ValueId }, + + /// Merge two values returned from opposite branches of a conditional into one. + IfElse { + then_condition: ValueId, + then_value: ValueId, + else_condition: ValueId, + else_value: ValueId, + }, } impl Instruction { @@ -229,10 +239,12 @@ impl Instruction { match self { Instruction::Binary(binary) => binary.result_type(), Instruction::Cast(_, typ) => InstructionResultType::Known(typ.clone()), - Instruction::Not(value) | Instruction::Truncate { value, .. } => { + Instruction::Not(value) + | Instruction::Truncate { value, .. } + | Instruction::ArraySet { array: value, .. } + | Instruction::IfElse { then_value: value, .. } => { InstructionResultType::Operand(*value) } - Instruction::ArraySet { array, .. } => InstructionResultType::Operand(*array), Instruction::Constrain(..) | Instruction::Store { .. } | Instruction::IncrementRc { .. } @@ -252,20 +264,11 @@ impl Instruction { matches!(self.result_type(), InstructionResultType::Unknown) } - /// Pure `Instructions` are instructions which have no side-effects and results are a function of the inputs only, - /// i.e. there are no interactions with memory. - /// - /// Pure instructions can be replaced with the results of another pure instruction with the same inputs. - pub(crate) fn is_pure(&self, dfg: &DataFlowGraph) -> bool { + /// Indicates if the instruction can be safely replaced with the results of another instruction with the same inputs. + pub(crate) fn can_be_deduplicated(&self, dfg: &DataFlowGraph) -> bool { use Instruction::*; match self { - Binary(bin) => { - // In ACIR, a division with a false predicate outputs (0,0), so it cannot replace another instruction unless they have the same predicate - bin.operator != BinaryOp::Div - } - Cast(_, _) | Truncate { .. } | Not(_) => true, - // These either have side-effects or interact with memory Constrain(..) | EnableSideEffects { .. } @@ -276,31 +279,37 @@ impl Instruction { | DecrementRc { .. } | RangeCheck { .. } => false, - // These can have different behavior depending on the EnableSideEffectsIf context. - // Enabling constant folding for these potentially enables replacing an enabled - // array get with one that was disabled. See - // https://github.com/noir-lang/noir/pull/4716#issuecomment-2047846328. - ArrayGet { .. } | ArraySet { .. } => false, - Call { func, .. } => match dfg[*func] { Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(), _ => false, }, + + // These can have different behavior depending on the EnableSideEffectsIf context. + // Replacing them with a similar instruction potentially enables replacing an instruction + // with one that was disabled. See + // https://github.com/noir-lang/noir/pull/4716#issuecomment-2047846328. + Binary(_) + | Cast(_, _) + | Not(_) + | Truncate { .. } + | IfElse { .. } + | ArrayGet { .. } + | ArraySet { .. } => !self.requires_acir_gen_predicate(dfg), } } - pub(crate) fn has_side_effects(&self, dfg: &DataFlowGraph) -> bool { + pub(crate) fn can_eliminate_if_unused(&self, dfg: &DataFlowGraph) -> bool { use Instruction::*; match self { Binary(binary) => { if matches!(binary.operator, BinaryOp::Div | BinaryOp::Mod) { if let Some(rhs) = dfg.get_numeric_constant(binary.rhs) { - rhs == FieldElement::zero() + rhs != FieldElement::zero() } else { - true + false } } else { - false + true } } Cast(_, _) @@ -309,32 +318,67 @@ impl Instruction { | Allocate | Load { .. } | ArrayGet { .. } - | ArraySet { .. } => false, + | IfElse { .. } + | ArraySet { .. } => true, Constrain(..) | Store { .. } | EnableSideEffects { .. } | IncrementRc { .. } | DecrementRc { .. } - | RangeCheck { .. } => true, + | RangeCheck { .. } => false, // Some `Intrinsic`s have side effects so we must check what kind of `Call` this is. Call { func, .. } => match dfg[*func] { - Value::Intrinsic(intrinsic) => intrinsic.has_side_effects(), + Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(), // All foreign functions are treated as having side effects. // This is because they can be used to pass information // from the ACVM to the external world during execution. - Value::ForeignFunction(_) => true, + Value::ForeignFunction(_) => false, // We must assume that functions contain a side effect as we cannot inspect more deeply. - Value::Function(_) => true, + Value::Function(_) => false, _ => false, }, } } + /// If true the instruction will depends on enable_side_effects context during acir-gen + fn requires_acir_gen_predicate(&self, dfg: &DataFlowGraph) -> bool { + match self { + Instruction::Binary(binary) + if matches!(binary.operator, BinaryOp::Div | BinaryOp::Mod) => + { + true + } + Instruction::EnableSideEffects { .. } + | Instruction::ArrayGet { .. } + | Instruction::ArraySet { .. } => true, + + Instruction::Call { func, .. } => match dfg[*func] { + Value::Function(_) => true, + Value::Intrinsic(intrinsic) => { + matches!(intrinsic, Intrinsic::SliceInsert | Intrinsic::SliceRemove) + } + _ => false, + }, + Instruction::Cast(_, _) + | Instruction::Binary(_) + | Instruction::Not(_) + | Instruction::Truncate { .. } + | Instruction::Constrain(_, _, _) + | Instruction::RangeCheck { .. } + | Instruction::Allocate + | Instruction::Load { .. } + | Instruction::Store { .. } + | Instruction::IfElse { .. } + | Instruction::IncrementRc { .. } + | Instruction::DecrementRc { .. } => false, + } + } + /// Maps each ValueId inside this instruction to a new ValueId, returning the new instruction. /// Note that the returned instruction is fresh and will not have an assigned InstructionId /// until it is manually inserted in a DataFlowGraph later. @@ -397,6 +441,14 @@ impl Instruction { assert_message: assert_message.clone(), } } + Instruction::IfElse { then_condition, then_value, else_condition, else_value } => { + Instruction::IfElse { + then_condition: f(*then_condition), + then_value: f(*then_value), + else_condition: f(*else_condition), + else_value: f(*else_value), + } + } } } @@ -451,6 +503,12 @@ impl Instruction { | Instruction::RangeCheck { value, .. } => { f(*value); } + Instruction::IfElse { then_condition, then_value, else_condition, else_value } => { + f(*then_condition); + f(*then_value); + f(*else_condition); + f(*else_value); + } } } @@ -602,6 +660,36 @@ impl Instruction { None } } + Instruction::IfElse { then_condition, then_value, else_condition, else_value } => { + let typ = dfg.type_of_value(*then_value); + + if let Some(constant) = dfg.get_numeric_constant(*then_condition) { + if constant.is_one() { + return SimplifiedTo(*then_value); + } else if constant.is_zero() { + return SimplifiedTo(*else_value); + } + } + + if matches!(&typ, Type::Numeric(_)) { + let then_condition = *then_condition; + let then_value = *then_value; + let else_condition = *else_condition; + let else_value = *else_value; + + let result = ValueMerger::merge_numeric_values( + dfg, + block, + then_condition, + else_condition, + then_value, + else_value, + ); + SimplifiedTo(result) + } else { + None + } + } } } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index a8365ffef39b..3d7cb478f64c 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -354,7 +354,9 @@ fn simplify_slice_push_back( slice_sizes.insert(set_last_slice_value, slice_size / element_size); slice_sizes.insert(new_slice, slice_size / element_size); - let mut value_merger = ValueMerger::new(dfg, block, &mut slice_sizes); + let unknown = &mut HashMap::default(); + let mut value_merger = ValueMerger::new(dfg, block, &mut slice_sizes, unknown, None); + let new_slice = value_merger.merge_values( len_not_equals_capacity, len_equals_capacity, diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 3e924985185b..58c593b0ad63 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -181,7 +181,7 @@ fn display_instruction_inner( let index = show(*index); let value = show(*value); let mutable = if *mutable { " mut" } else { "" }; - writeln!(f, "array_set{mutable} {array}, index {index}, value {value}",) + writeln!(f, "array_set{mutable} {array}, index {index}, value {value}") } Instruction::IncrementRc { value } => { writeln!(f, "inc_rc {}", show(*value)) @@ -192,6 +192,16 @@ fn display_instruction_inner( Instruction::RangeCheck { value, max_bit_size, .. } => { writeln!(f, "range_check {} to {} bits", show(*value), *max_bit_size,) } + Instruction::IfElse { then_condition, then_value, else_condition, else_value } => { + let then_condition = show(*then_condition); + let then_value = show(*then_value); + let else_condition = show(*else_condition); + let else_value = show(*else_value); + writeln!( + f, + "if {then_condition} then {then_value} else if {else_condition} then {else_value}" + ) + } } } 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 5a7134f34864..ac2f64243328 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,9 +6,8 @@ //! 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 is [pure][Instruction::is_pure()] -//! and there exists a duplicate instruction earlier in the same block. -//! If so, the instruction can be replaced with the results of this previous instruction. +//! - 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 //! without the need for multiple passes. @@ -257,9 +256,9 @@ impl Context { } } - // If the instruction doesn't have side-effects, cache the results so we can reuse them if - // the same instruction appears again later in the block. - if instruction.is_pure(dfg) { + // 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. + if instruction.can_be_deduplicated(dfg) { instruction_result_cache.insert(instruction, instruction_results); } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs index d1b3e1e83f50..d045762f9e9f 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -108,12 +108,12 @@ impl Context { fn is_unused(&self, instruction_id: InstructionId, function: &Function) -> bool { let instruction = &function.dfg[instruction_id]; - if instruction.has_side_effects(&function.dfg) { - // If the instruction has side effects we should never remove it. - false - } else { + if instruction.can_eliminate_if_unused(&function.dfg) { let results = function.dfg.instruction_results(instruction_id); results.iter().all(|result| !self.used_values.contains(result)) + } else { + // If the instruction has side effects we should never remove it. + false } } 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 07771397ce8c..0f8b49b40ec9 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 @@ -155,9 +155,6 @@ mod branch_analysis; mod capacity_tracker; pub(crate) mod value_merger; -use capacity_tracker::SliceCapacityTracker; -use value_merger::ValueMerger; - impl Ssa { /// Flattens the control flow graph of main such that the function is left with a /// single block containing all instructions and no more control-flow. @@ -311,18 +308,6 @@ impl<'f> Context<'f> { if self.inserter.function.entry_block() == block { // we do not inline the entry block into itself // for the outer block before we start inlining - let outer_block_instructions = self.inserter.function.dfg[block].instructions(); - let mut capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); - for instruction in outer_block_instructions { - let results = self.inserter.function.dfg.instruction_results(*instruction); - let instruction = &self.inserter.function.dfg[*instruction]; - capacity_tracker.collect_slice_information( - instruction, - &mut self.slice_sizes, - results.to_vec(), - ); - } - return; } @@ -333,14 +318,7 @@ impl<'f> Context<'f> { // unnecessary, when removing it actually causes an aliasing/mutability error. let instructions = self.inserter.function.dfg[block].instructions().to_vec(); for instruction in instructions.iter() { - let results = self.push_instruction(*instruction); - let (instruction, _) = self.inserter.map_instruction(*instruction); - let mut capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); - capacity_tracker.collect_slice_information( - &instruction, - &mut self.slice_sizes, - results, - ); + self.push_instruction(*instruction); } } @@ -543,24 +521,19 @@ impl<'f> Context<'f> { let block = self.inserter.function.entry_block(); - // Make sure we have tracked the slice capacities of any block arguments - let capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); - for (then_arg, else_arg) in args.iter() { - capacity_tracker.compute_slice_capacity(*then_arg, &mut self.slice_sizes); - capacity_tracker.compute_slice_capacity(*else_arg, &mut self.slice_sizes); - } - - let mut value_merger = - ValueMerger::new(&mut self.inserter.function.dfg, block, &mut self.slice_sizes); - // Cannot include this in the previous vecmap since it requires exclusive access to self let args = vecmap(args, |(then_arg, else_arg)| { - value_merger.merge_values( - cond_context.then_branch.condition, - cond_context.else_branch.clone().unwrap().condition, - then_arg, - else_arg, - ) + let instruction = Instruction::IfElse { + then_condition: cond_context.then_branch.condition, + then_value: then_arg, + else_condition: cond_context.else_branch.as_ref().unwrap().condition, + else_value: else_arg, + }; + self.inserter + .function + .dfg + .insert_instruction_and_results(instruction, block, None, CallStack::new()) + .first() }); self.merge_stores(cond_context.then_branch, cond_context.else_branch); @@ -643,15 +616,6 @@ impl<'f> Context<'f> { } } - // Most slice information is collected when instructions are inlined. - // We need to collect information on slice values here as we may possibly merge stores - // before any inlining occurs. - let capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); - for (then_case, else_case, _) in new_map.values() { - capacity_tracker.compute_slice_capacity(*then_case, &mut self.slice_sizes); - capacity_tracker.compute_slice_capacity(*else_case, &mut self.slice_sizes); - } - let then_condition = then_branch.condition; let else_condition = if let Some(branch) = else_branch { branch.condition @@ -660,13 +624,22 @@ impl<'f> Context<'f> { }; let block = self.inserter.function.entry_block(); - let mut value_merger = - ValueMerger::new(&mut self.inserter.function.dfg, block, &mut self.slice_sizes); // Merging must occur in a separate loop as we cannot borrow `self` as mutable while `value_merger` does let mut new_values = HashMap::default(); for (address, (then_case, else_case, _)) in &new_map { - let value = - value_merger.merge_values(then_condition, else_condition, *then_case, *else_case); + let instruction = Instruction::IfElse { + then_condition, + then_value: *then_case, + else_condition, + else_value: *else_case, + }; + let value = self + .inserter + .function + .dfg + .insert_instruction_and_results(instruction, block, None, CallStack::new()) + .first(); + new_values.insert(address, value); } @@ -683,16 +656,6 @@ impl<'f> Context<'f> { .insert(address, Store { old_value: *old_value, new_value: value }); } } - - // Collect any potential slice information on the stores we are merging - for (address, (_, _, _)) in &new_map { - let value = new_values[address]; - let address = *address; - let instruction = Instruction::Store { address, value }; - - let mut capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); - capacity_tracker.collect_slice_information(&instruction, &mut self.slice_sizes, vec![]); - } } fn remember_store(&mut self, address: ValueId, new_value: ValueId) { @@ -706,14 +669,6 @@ impl<'f> Context<'f> { let old_value = self.insert_instruction_with_typevars(load.clone(), load_type).first(); - // Need this or else we will be missing a the previous value of a slice that we wish to merge - let mut capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); - capacity_tracker.collect_slice_information( - &load, - &mut self.slice_sizes, - vec![old_value], - ); - self.store_values.insert(address, Store { old_value, new_value }); } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs index 93e525422780..4fc19acd2ac8 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs @@ -19,10 +19,10 @@ impl<'a> SliceCapacityTracker<'a> { /// Determine how the slice sizes map needs to be updated according to the provided instruction. pub(crate) fn collect_slice_information( - &mut self, + &self, instruction: &Instruction, slice_sizes: &mut HashMap, - results: Vec, + results: &[ValueId], ) { match instruction { Instruction::ArrayGet { array, .. } => { diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs index 0a351148fa38..c47d594545c8 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -1,20 +1,25 @@ use acvm::FieldElement; -use fxhash::FxHashMap as HashMap; +use fxhash::{FxHashMap as HashMap, FxHashSet}; use crate::ssa::ir::{ basic_block::BasicBlockId, - dfg::{CallStack, DataFlowGraph}, + dfg::{CallStack, DataFlowGraph, InsertInstructionResult}, instruction::{BinaryOp, Instruction}, types::Type, - value::ValueId, + value::{Value, ValueId}, }; pub(crate) struct ValueMerger<'a> { dfg: &'a mut DataFlowGraph, block: BasicBlockId, + + current_condition: Option, + // Maps SSA array values with a slice type to their size. // This must be computed before merging values. slice_sizes: &'a mut HashMap, + + array_set_conditionals: &'a mut HashMap, } impl<'a> ValueMerger<'a> { @@ -22,8 +27,10 @@ impl<'a> ValueMerger<'a> { dfg: &'a mut DataFlowGraph, block: BasicBlockId, slice_sizes: &'a mut HashMap, + array_set_conditionals: &'a mut HashMap, + current_condition: Option, ) -> Self { - ValueMerger { dfg, block, slice_sizes } + ValueMerger { dfg, block, slice_sizes, array_set_conditionals, current_condition } } /// Merge two values a and b from separate basic blocks to a single value. @@ -42,9 +49,14 @@ impl<'a> ValueMerger<'a> { else_value: ValueId, ) -> ValueId { match self.dfg.type_of_value(then_value) { - Type::Numeric(_) => { - self.merge_numeric_values(then_condition, else_condition, then_value, else_value) - } + Type::Numeric(_) => 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) } @@ -59,58 +71,57 @@ impl<'a> ValueMerger<'a> { /// Merge two numeric values a and b from separate basic blocks to a single value. This /// function would return the result of `if c { a } else { b }` as `c*a + (!c)*b`. pub(crate) fn merge_numeric_values( - &mut self, + dfg: &mut DataFlowGraph, + block: BasicBlockId, then_condition: ValueId, else_condition: ValueId, then_value: ValueId, else_value: ValueId, ) -> ValueId { - let then_type = self.dfg.type_of_value(then_value); - let else_type = self.dfg.type_of_value(else_value); + let then_type = dfg.type_of_value(then_value); + let else_type = dfg.type_of_value(else_value); assert_eq!( then_type, else_type, "Expected values merged to be of the same type but found {then_type} and {else_type}" ); - let then_call_stack = self.dfg.get_value_call_stack(then_value); - let else_call_stack = self.dfg.get_value_call_stack(else_value); + if then_value == else_value { + return then_value; + } + + let then_call_stack = dfg.get_value_call_stack(then_value); + let else_call_stack = dfg.get_value_call_stack(else_value); let call_stack = if then_call_stack.is_empty() { else_call_stack } else { then_call_stack }; // We must cast the bool conditions to the actual numeric type used by each value. - let then_condition = self - .dfg + let then_condition = dfg .insert_instruction_and_results( Instruction::Cast(then_condition, then_type), - self.block, + block, None, call_stack.clone(), ) .first(); - let else_condition = self - .dfg + let else_condition = dfg .insert_instruction_and_results( Instruction::Cast(else_condition, else_type), - self.block, + block, None, call_stack.clone(), ) .first(); let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value); - let then_value = self - .dfg - .insert_instruction_and_results(mul, self.block, None, call_stack.clone()) - .first(); + let then_value = + dfg.insert_instruction_and_results(mul, block, None, call_stack.clone()).first(); let mul = Instruction::binary(BinaryOp::Mul, else_condition, else_value); - let else_value = self - .dfg - .insert_instruction_and_results(mul, self.block, None, call_stack.clone()) - .first(); + let else_value = + dfg.insert_instruction_and_results(mul, block, None, call_stack.clone()).first(); let add = Instruction::binary(BinaryOp::Add, then_value, else_value); - self.dfg.insert_instruction_and_results(add, self.block, None, call_stack).first() + dfg.insert_instruction_and_results(add, block, None, call_stack).first() } /// Given an if expression that returns an array: `if c { array1 } else { array2 }`, @@ -131,6 +142,18 @@ impl<'a> ValueMerger<'a> { _ => panic!("Expected array type"), }; + let actual_length = len * element_types.len(); + + if let Some(result) = self.try_merge_only_changed_indices( + then_condition, + else_condition, + then_value, + else_value, + actual_length, + ) { + return result; + } + for i in 0..len { for (element_index, element_type) in element_types.iter().enumerate() { let index = ((i * element_types.len() + element_index) as u128).into(); @@ -175,12 +198,18 @@ impl<'a> ValueMerger<'a> { _ => panic!("Expected slice type"), }; - let then_len = *self.slice_sizes.get(&then_value_id).unwrap_or_else(|| { - panic!("ICE: Merging values during flattening encountered slice {then_value_id} without a preset size"); + let then_len = self.slice_sizes.get(&then_value_id).copied().unwrap_or_else(|| { + let (slice, typ) = self.dfg.get_array_constant(then_value_id).unwrap_or_else(|| { + panic!("ICE: Merging values during flattening encountered slice {then_value_id} without a preset size"); + }); + slice.len() / typ.element_types().len() }); - let else_len = *self.slice_sizes.get(&else_value_id).unwrap_or_else(|| { - panic!("ICE: Merging values during flattening encountered slice {else_value_id} without a preset size"); + let else_len = self.slice_sizes.get(&else_value_id).copied().unwrap_or_else(|| { + let (slice, typ) = self.dfg.get_array_constant(else_value_id).unwrap_or_else(|| { + panic!("ICE: Merging values during flattening encountered slice {else_value_id} without a preset size"); + }); + slice.len() / typ.element_types().len() }); let len = then_len.max(else_len); @@ -260,4 +289,157 @@ impl<'a> ValueMerger<'a> { } } } + + fn try_merge_only_changed_indices( + &mut self, + then_condition: ValueId, + else_condition: ValueId, + then_value: ValueId, + else_value: ValueId, + array_length: usize, + ) -> Option { + let mut found = false; + let current_condition = self.current_condition?; + + let mut current_then = then_value; + let mut current_else = else_value; + + // Arbitrarily limit this to looking at at most 10 past ArraySet operations. + // If there are more than that, we assume 2 completely separate arrays are being merged. + let max_iters = 1; + let mut seen_then = Vec::with_capacity(max_iters); + let mut seen_else = Vec::with_capacity(max_iters); + + // We essentially have a tree of ArraySets and want to find a common + // ancestor if it exists, alone with the path to it from each starting node. + // This path will be the indices that were changed to create each result array. + for _ in 0..max_iters { + if current_then == else_value { + seen_else.clear(); + found = true; + break; + } + + if current_else == then_value { + seen_then.clear(); + found = true; + break; + } + + if let Some(index) = seen_then.iter().position(|(elem, _, _, _)| *elem == current_else) + { + seen_else.truncate(index); + found = true; + break; + } + + if let Some(index) = seen_else.iter().position(|(elem, _, _, _)| *elem == current_then) + { + seen_then.truncate(index); + found = true; + break; + } + + current_then = self.find_previous_array_set(current_then, &mut seen_then); + current_else = self.find_previous_array_set(current_else, &mut seen_else); + } + + let changed_indices: FxHashSet<_> = seen_then + .into_iter() + .map(|(_, index, typ, condition)| (index, typ, condition)) + .chain(seen_else.into_iter().map(|(_, index, typ, condition)| (index, typ, condition))) + .collect(); + + if !found || changed_indices.len() >= array_length { + return None; + } + + let mut array = then_value; + + for (index, element_type, condition) in changed_indices { + let typevars = Some(vec![element_type.clone()]); + + let instruction = Instruction::EnableSideEffects { condition }; + self.insert_instruction(instruction); + + let mut get_element = |array, typevars| { + let get = Instruction::ArrayGet { array, index }; + self.dfg + .insert_instruction_and_results(get, self.block, typevars, CallStack::new()) + .first() + }; + + let then_element = get_element(then_value, typevars.clone()); + let else_element = get_element(else_value, typevars); + + let value = + self.merge_values(then_condition, else_condition, then_element, else_element); + + array = self.insert_array_set(array, index, value, Some(condition)).first(); + } + + let instruction = Instruction::EnableSideEffects { condition: current_condition }; + self.insert_instruction(instruction); + Some(array) + } + + fn insert_instruction(&mut self, instruction: Instruction) -> InsertInstructionResult { + self.dfg.insert_instruction_and_results(instruction, self.block, None, CallStack::new()) + } + + fn insert_array_set( + &mut self, + array: ValueId, + index: ValueId, + value: ValueId, + condition: Option, + ) -> InsertInstructionResult { + let instruction = Instruction::ArraySet { array, index, value, mutable: false }; + let result = self.dfg.insert_instruction_and_results( + instruction, + self.block, + None, + CallStack::new(), + ); + + if let Some(condition) = condition { + let result_index = if result.len() == 1 { + 0 + } else { + // Slices return (length, slice) + assert_eq!(result.len(), 2); + 1 + }; + + let result_value = result[result_index]; + self.array_set_conditionals.insert(result_value, condition); + } + + result + } + + fn find_previous_array_set( + &self, + result: ValueId, + changed_indices: &mut Vec<(ValueId, ValueId, Type, ValueId)>, + ) -> ValueId { + match &self.dfg[result] { + Value::Instruction { instruction, .. } => match &self.dfg[*instruction] { + Instruction::ArraySet { array, index, value, .. } => { + let condition = + *self.array_set_conditionals.get(&result).unwrap_or_else(|| { + panic!( + "Expected to have conditional for array set {result}\n{:?}", + self.array_set_conditionals + ) + }); + let element_type = self.dfg.type_of_value(*value); + changed_indices.push((result, *index, element_type, condition)); + *array + } + _ => result, + }, + _ => result, + } + } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 4452840a28cd..27536d59ea59 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -16,5 +16,6 @@ mod mem2reg; mod rc; mod remove_bit_shifts; mod remove_enable_side_effects; +mod remove_if_else; mod simplify_cfg; mod unrolling; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index 8535dc2661fd..02b9202b209f 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -125,6 +125,7 @@ impl Context { | Truncate { .. } | Constrain(..) | RangeCheck { .. } + | IfElse { .. } | IncrementRc { .. } | DecrementRc { .. } => false, diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs new file mode 100644 index 000000000000..fc915756110d --- /dev/null +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -0,0 +1,236 @@ +use std::collections::hash_map::Entry; + +use acvm::FieldElement; +use fxhash::FxHashMap as HashMap; + +use crate::ssa::ir::value::ValueId; +use crate::ssa::{ + ir::{ + dfg::DataFlowGraph, + function::Function, + instruction::{Instruction, Intrinsic}, + types::Type, + value::Value, + }, + opt::flatten_cfg::value_merger::ValueMerger, + Ssa, +}; + +impl Ssa { + /// This pass removes `inc_rc` and `dec_rc` instructions + /// as long as there are no `array_set` instructions to an array + /// of the same type in between. + /// + /// Note that this pass is very conservative since the array_set + /// instruction does not need to be to the same array. This is because + /// 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 { + for function in self.functions.values_mut() { + // This should match the check in flatten_cfg + if let crate::ssa::ir::function::RuntimeType::Brillig = function.runtime() { + continue; + } + + Context::default().remove_if_else(function); + } + self + } +} + +#[derive(Default)] +struct Context { + slice_sizes: HashMap, + + // Maps array_set result -> element that was overwritten by that instruction. + // Used to undo array_sets while merging values + prev_array_set_elem_values: HashMap, + + // Maps array_set result -> enable_side_effects_if value which was active during it. + array_set_conditionals: HashMap, +} + +impl Context { + fn remove_if_else(&mut self, function: &mut Function) { + let block = function.entry_block(); + let instructions = function.dfg[block].take_instructions(); + let mut current_conditional = function.dfg.make_constant(FieldElement::one(), Type::bool()); + + for instruction in instructions { + match &function.dfg[instruction] { + Instruction::IfElse { then_condition, then_value, else_condition, else_value } => { + let then_condition = *then_condition; + let then_value = *then_value; + let else_condition = *else_condition; + let else_value = *else_value; + + let typ = function.dfg.type_of_value(then_value); + assert!(!matches!(typ, Type::Numeric(_))); + + let mut value_merger = ValueMerger::new( + &mut function.dfg, + block, + &mut self.slice_sizes, + &mut self.array_set_conditionals, + Some(current_conditional), + ); + + let value = value_merger.merge_values( + then_condition, + else_condition, + then_value, + else_value, + ); + + let _typ = function.dfg.type_of_value(value); + let results = function.dfg.instruction_results(instruction); + 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:?}"), + // }; + + function.dfg.set_value_from_id(result, value); + self.array_set_conditionals.insert(result, current_conditional); + } + Instruction::Call { func, arguments } => { + if let Value::Intrinsic(intrinsic) = function.dfg[*func] { + let results = function.dfg.instruction_results(instruction); + + match slice_capacity_change(&function.dfg, intrinsic, arguments, results) { + SizeChange::None => (), + SizeChange::SetTo(value, new_capacity) => { + self.slice_sizes.insert(value, new_capacity); + } + SizeChange::Inc { old, new } => { + let old_capacity = self.get_or_find_capacity(&function.dfg, old); + self.slice_sizes.insert(new, old_capacity + 1); + } + SizeChange::Dec { old, new } => { + let old_capacity = self.get_or_find_capacity(&function.dfg, old); + self.slice_sizes.insert(new, old_capacity - 1); + } + } + } + function.dfg[block].instructions_mut().push(instruction); + } + Instruction::ArraySet { array, .. } => { + let results = function.dfg.instruction_results(instruction); + let result = if results.len() == 2 { results[1] } else { results[0] }; + + self.array_set_conditionals.insert(result, current_conditional); + + let old_capacity = self.get_or_find_capacity(&function.dfg, *array); + self.slice_sizes.insert(result, old_capacity); + function.dfg[block].instructions_mut().push(instruction); + } + Instruction::EnableSideEffects { condition } => { + current_conditional = *condition; + function.dfg[block].instructions_mut().push(instruction); + } + _ => { + function.dfg[block].instructions_mut().push(instruction); + } + } + } + } + + fn get_or_find_capacity(&mut self, dfg: &DataFlowGraph, value: ValueId) -> usize { + match self.slice_sizes.entry(value) { + Entry::Occupied(entry) => return *entry.get(), + Entry::Vacant(entry) => { + if let Some((array, typ)) = dfg.get_array_constant(value) { + let length = array.len() / typ.element_types().len(); + return *entry.insert(length); + } + + if let Type::Array(_, length) = dfg.type_of_value(value) { + return *entry.insert(length); + } + } + } + + let dbg_value = &dfg[value]; + unreachable!("No size for slice {value} = {dbg_value:?}") + } +} + +enum SizeChange { + None, + SetTo(ValueId, usize), + + // These two variants store the old and new slice ids + // not their lengths which should be old_len = new_len +/- 1 + Inc { old: ValueId, new: ValueId }, + Dec { old: ValueId, new: ValueId }, +} + +/// Find the change to a slice's capacity an instruction would have +fn slice_capacity_change( + dfg: &DataFlowGraph, + intrinsic: Intrinsic, + arguments: &[ValueId], + results: &[ValueId], +) -> SizeChange { + match intrinsic { + Intrinsic::SlicePushBack | Intrinsic::SlicePushFront | Intrinsic::SliceInsert => { + // Expecting: len, slice = ... + assert_eq!(results.len(), 2); + let old = arguments[1]; + let new = results[1]; + assert!(matches!(dfg.type_of_value(old), Type::Slice(_))); + assert!(matches!(dfg.type_of_value(new), Type::Slice(_))); + SizeChange::Inc { old, new } + } + + Intrinsic::SlicePopBack | Intrinsic::SliceRemove => { + let old = arguments[1]; + let new = results[1]; + assert!(matches!(dfg.type_of_value(old), Type::Slice(_))); + assert!(matches!(dfg.type_of_value(new), Type::Slice(_))); + SizeChange::Dec { old, new } + } + + Intrinsic::SlicePopFront => { + let old = arguments[1]; + let new = results[results.len() - 1]; + assert!(matches!(dfg.type_of_value(old), Type::Slice(_))); + assert!(matches!(dfg.type_of_value(new), Type::Slice(_))); + SizeChange::Dec { old, new } + } + + Intrinsic::ToBits(_) => { + assert_eq!(results.len(), 2); + // Some tests fail this check, returning an array instead somehow: + // assert!(matches!(dfg.type_of_value(results[1]), Type::Slice(_))); + SizeChange::SetTo(results[1], FieldElement::max_num_bits() as usize) + } + // ToRadix seems to assume it is to bytes + Intrinsic::ToRadix(_) => { + assert_eq!(results.len(), 2); + assert!(matches!(dfg.type_of_value(results[1]), Type::Slice(_))); + SizeChange::SetTo(results[1], FieldElement::max_num_bytes() as usize) + } + Intrinsic::AsSlice => { + assert_eq!(arguments.len(), 1); + assert_eq!(results.len(), 2); + let length = match dfg.type_of_value(arguments[0]) { + Type::Array(_, length) => length, + other => unreachable!("slice_capacity_change expected array, found {other:?}"), + }; + assert!(matches!(dfg.type_of_value(results[1]), Type::Slice(_))); + SizeChange::SetTo(results[1], length) + } + + // These cases don't affect slice capacities + Intrinsic::AssertConstant + | Intrinsic::ApplyRangeConstraint + | Intrinsic::ArrayLen + | Intrinsic::StrAsBytes + | Intrinsic::BlackBox(_) + | Intrinsic::FromField + | Intrinsic::AsField => SizeChange::None, + } +} diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/stmt.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/stmt.rs index f5f6e1e8180e..0760749c9e0e 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/stmt.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/stmt.rs @@ -348,8 +348,10 @@ impl<'interner> TypeChecker<'interner> { if annotated_type.is_unsigned() { self.lint_overflowing_uint(&rhs_expr, &annotated_type); } + annotated_type + } else { + expr_type } - expr_type } /// Check if an assignment is overflowing with respect to `annotated_type` diff --git a/noir/noir-repo/test_programs/execution_success/fold_complex_outputs/Nargo.toml b/noir/noir-repo/test_programs/execution_success/fold_complex_outputs/Nargo.toml new file mode 100644 index 000000000000..f00c6520b4ab --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/fold_complex_outputs/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "fold_complex_outputs" +type = "bin" +authors = [""] +compiler_version = ">=0.28.0" + +[dependencies] \ No newline at end of file diff --git a/noir/noir-repo/test_programs/execution_success/fold_complex_outputs/Prover.toml b/noir/noir-repo/test_programs/execution_success/fold_complex_outputs/Prover.toml new file mode 100644 index 000000000000..a26b97d6471c --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/fold_complex_outputs/Prover.toml @@ -0,0 +1,2 @@ +x = "5" +y = "3" diff --git a/noir/noir-repo/test_programs/execution_success/fold_complex_outputs/src/main.nr b/noir/noir-repo/test_programs/execution_success/fold_complex_outputs/src/main.nr new file mode 100644 index 000000000000..309d9747598c --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/fold_complex_outputs/src/main.nr @@ -0,0 +1,72 @@ +struct MyStruct { + x: u32, + y: u32, + z: u32, + nested_struct: InnerStruct +} + +struct InnerStruct { + small_array: [u32; 2], + big_array: [u32; 5], +} + +struct ParentStruct { + basic_array: [Field; 3], + id: u32, + my_structs: [MyStruct; 2], +} + +fn main(x: u32, y: pub u32) { + let nested_struct = InnerStruct { small_array: [1 as u32; 2], big_array: [0 as u32; 5] }; + let s = MyStruct { x, y, z: x + y, nested_struct }; + let parent = ParentStruct { basic_array: [1; 3], id: 100, my_structs: [s, s] }; + let new_parent = map_fields(parent); + + // Now check that the outputs are as we expect them to be + assert(new_parent.basic_array[0] == 1); + assert(new_parent.basic_array[1] == 18); + assert(new_parent.basic_array[2] == 1); + + let struct_0 = new_parent.my_structs[0]; + assert(struct_0.x == 5); + assert(struct_0.y == 3); + assert(struct_0.z == 8); + assert(struct_0.nested_struct.small_array == nested_struct.small_array); + assert(struct_0.nested_struct.big_array == nested_struct.big_array); + + let struct_1 = new_parent.my_structs[1]; + assert(struct_1.x == 50); + assert(struct_1.y == 30); + assert(struct_1.z == 80); + assert(struct_1.nested_struct.small_array == [5, 10]); + assert(struct_1.nested_struct.big_array == [15, 20, 25, 30, 35]); +} + +// Meaningless mapping to test whether the values returned are what we expect +#[fold] +fn map_fields(mut input: ParentStruct) -> ParentStruct { + let current_struct = input.my_structs[0]; + let mut sum = 0; + for value in current_struct.nested_struct.small_array { + sum += value; + } + for value in current_struct.nested_struct.big_array { + sum += value; + } + sum += (current_struct.x + current_struct.y + current_struct.z); + + input.basic_array[1] = sum as Field; + + input.my_structs[1].nested_struct.small_array = [5, 10]; + input.my_structs[1].nested_struct.big_array = [15, 20, 25, 30, 35]; + + // LHS input.my_structs[1].x == 50 + input.my_structs[1].x = input.my_structs[1].x * 10; + // LHS input.my_structs[1].y == 30 + input.my_structs[1].y = input.my_structs[1].y * 10; + // LHS input.my_structs[1].x == 80 + input.my_structs[1].z = input.my_structs[1].x + input.my_structs[1].y; + + input +} + diff --git a/noir/noir-repo/test_programs/execution_success/nested_array_dynamic_simple/Nargo.toml b/noir/noir-repo/test_programs/execution_success/nested_array_dynamic_simple/Nargo.toml new file mode 100644 index 000000000000..50ba1d194a62 --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/nested_array_dynamic_simple/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "nested_array_dynamic_simple" +type = "bin" +authors = [""] +compiler_version = ">=0.28.0" + +[dependencies] \ No newline at end of file diff --git a/noir/noir-repo/test_programs/execution_success/nested_array_dynamic_simple/Prover.toml b/noir/noir-repo/test_programs/execution_success/nested_array_dynamic_simple/Prover.toml new file mode 100644 index 000000000000..07890234a19b --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/nested_array_dynamic_simple/Prover.toml @@ -0,0 +1 @@ +x = "3" diff --git a/noir/noir-repo/test_programs/execution_success/nested_array_dynamic_simple/src/main.nr b/noir/noir-repo/test_programs/execution_success/nested_array_dynamic_simple/src/main.nr new file mode 100644 index 000000000000..3b1908a463be --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/nested_array_dynamic_simple/src/main.nr @@ -0,0 +1,9 @@ +fn main(x: Field) { + // x = 3 + let array: [[(Field, [Field; 1], [Field; 1]); 1]; 1] = [[(1, [2], [3])]]; + + let fetched_value = array[x - 3]; + assert(fetched_value[0].0 == 1); + assert(fetched_value[0].1[0] == 2); + assert(fetched_value[0].2[0] == 3); +} diff --git a/noir/noir-repo/test_programs/execution_success/slice_init_with_complex_type/Nargo.toml b/noir/noir-repo/test_programs/execution_success/slice_init_with_complex_type/Nargo.toml new file mode 100644 index 000000000000..a372caf92e96 --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/slice_init_with_complex_type/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "slice_init_with_complex_type" +type = "bin" +authors = [""] +compiler_version = ">=0.28.0" + +[dependencies] \ No newline at end of file diff --git a/noir/noir-repo/test_programs/noir_test_success/bounded_vec/Prover.toml b/noir/noir-repo/test_programs/execution_success/slice_init_with_complex_type/Prover.toml similarity index 100% rename from noir/noir-repo/test_programs/noir_test_success/bounded_vec/Prover.toml rename to noir/noir-repo/test_programs/execution_success/slice_init_with_complex_type/Prover.toml diff --git a/noir/noir-repo/test_programs/execution_success/slice_init_with_complex_type/src/main.nr b/noir/noir-repo/test_programs/execution_success/slice_init_with_complex_type/src/main.nr new file mode 100644 index 000000000000..01ccf2fdeff2 --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/slice_init_with_complex_type/src/main.nr @@ -0,0 +1,17 @@ +struct strct1 { + elem1: Field, +} + +fn main() { + let var1: [[i32; 1]] = [[0]]; + let var2: [[i32; 1]] = var1; + + let var1: [(i32, u8)] = [(1, 2)]; + let var2: [(i32, u8)] = var1; + + let var3: [strct1] = [strct1 { elem1: 1321351 }]; + let var4: [strct1] = var3; + + let var1: [i32; 1] = [0]; + let var2: [[i32; 1]] = [var1]; +} diff --git a/noir/noir-repo/test_programs/noir_test_success/brillig_overflow_checks/Prover.toml b/noir/noir-repo/test_programs/noir_test_success/brillig_overflow_checks/Prover.toml deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/noir/noir-repo/test_programs/noir_test_success/field_comparisons/Prover.toml b/noir/noir-repo/test_programs/noir_test_success/field_comparisons/Prover.toml deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/noir/noir-repo/test_programs/noir_test_success/ignored_oracle/Nargo.toml b/noir/noir-repo/test_programs/noir_test_success/ignored_oracle/Nargo.toml new file mode 100644 index 000000000000..0d9b77c01d77 --- /dev/null +++ b/noir/noir-repo/test_programs/noir_test_success/ignored_oracle/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "ignored_oracle" +type = "bin" +authors = [""] +compiler_version = ">=0.23.0" + +[dependencies] diff --git a/noir/noir-repo/test_programs/noir_test_success/ignored_oracle/src/main.nr b/noir/noir-repo/test_programs/noir_test_success/ignored_oracle/src/main.nr new file mode 100644 index 000000000000..9e0bc1899397 --- /dev/null +++ b/noir/noir-repo/test_programs/noir_test_success/ignored_oracle/src/main.nr @@ -0,0 +1,23 @@ +// In `nargo test` we want to avoid the need for an external oracle resolver service to be required in the situation +// where its existence doesn't affect whether the tests will pass or fail. We then want to be able to handle any +// oracles which return zero field elements. + +// Note that this custom oracle doesn't return any new values into the program. +// We can then safely continue execution even in the case where there is no oracle resolver to handle it. +#[oracle(custom_debug)] +unconstrained fn custom_debug() {} + +// However this oracle call should return a field element. We expect the ACVM to raise an error when it +// doesn't receive this value. +#[oracle(custom_getter)] +unconstrained fn custom_getter() -> Field {} + +#[test] +unconstrained fn unit_return_oracle_ignored() { + custom_debug(); +} + +#[test(should_fail_with = "0 output values were provided as a foreign call result for 1 destination slots")] +unconstrained fn field_return_oracle_fails() { + let _ = custom_getter(); +} diff --git a/noir/noir-repo/test_programs/noir_test_success/mock_oracle/Prover.toml b/noir/noir-repo/test_programs/noir_test_success/mock_oracle/Prover.toml deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/noir/noir-repo/test_programs/noir_test_success/out_of_bounds_alignment/Prover.toml b/noir/noir-repo/test_programs/noir_test_success/out_of_bounds_alignment/Prover.toml deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/noir/noir-repo/test_programs/noir_test_success/should_fail_with_matches/Prover.toml b/noir/noir-repo/test_programs/noir_test_success/should_fail_with_matches/Prover.toml deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/noir/noir-repo/tooling/debugger/ignored-tests.txt b/noir/noir-repo/tooling/debugger/ignored-tests.txt index cbef395e65c4..d08f5609645e 100644 --- a/noir/noir-repo/tooling/debugger/ignored-tests.txt +++ b/noir/noir-repo/tooling/debugger/ignored-tests.txt @@ -22,3 +22,5 @@ no_predicates_numeric_generic_poseidon regression_4709 fold_distinct_return fold_fibonacci +fold_complex_outputs +slice_init_with_complex_type diff --git a/noir/noir-repo/tooling/nargo/src/ops/foreign_calls.rs b/noir/noir-repo/tooling/nargo/src/ops/foreign_calls.rs index 90f6659ad284..c314a230cef2 100644 --- a/noir/noir-repo/tooling/nargo/src/ops/foreign_calls.rs +++ b/noir/noir-repo/tooling/nargo/src/ops/foreign_calls.rs @@ -249,43 +249,49 @@ impl ForeignCallExecutor for DefaultForeignCallExecutor { .iter() .position(|response| response.matches(foreign_call_name, &foreign_call.inputs)); - match (mock_response_position, &self.external_resolver) { - (Some(response_position), _) => { - let mock = self - .mocked_responses - .get_mut(response_position) - .expect("Invalid position of mocked response"); - - mock.last_called_params = Some(foreign_call.inputs.clone()); - - let result = mock.result.values.clone(); - - if let Some(times_left) = &mut mock.times_left { - *times_left -= 1; - if *times_left == 0 { - self.mocked_responses.remove(response_position); - } - } + if let Some(response_position) = mock_response_position { + // If the program has registered a mocked response to this oracle call then we prefer responding + // with that. + + let mock = self + .mocked_responses + .get_mut(response_position) + .expect("Invalid position of mocked response"); + + mock.last_called_params = Some(foreign_call.inputs.clone()); + + let result = mock.result.values.clone(); - Ok(result.into()) + if let Some(times_left) = &mut mock.times_left { + *times_left -= 1; + if *times_left == 0 { + self.mocked_responses.remove(response_position); + } } - (None, Some(external_resolver)) => { - let encoded_params: Vec<_> = - foreign_call.inputs.iter().map(build_json_rpc_arg).collect(); - let req = - external_resolver.build_request(foreign_call_name, &encoded_params); + Ok(result.into()) + } else if let Some(external_resolver) = &self.external_resolver { + // If the user has registered an external resolver then we forward any remaining oracle calls there. - let response = external_resolver.send_request(req)?; + let encoded_params: Vec<_> = + foreign_call.inputs.iter().map(build_json_rpc_arg).collect(); - let parsed_response: ForeignCallResult = response.result()?; + let req = external_resolver.build_request(foreign_call_name, &encoded_params); - Ok(parsed_response) - } - (None, None) => panic!( - "No mock for foreign call {}({:?})", - foreign_call_name, &foreign_call.inputs - ), + let response = external_resolver.send_request(req)?; + + let parsed_response: ForeignCallResult = response.result()?; + + Ok(parsed_response) + } else { + // If there's no registered mock oracle response and no registered resolver then we cannot + // return a correct response to the ACVM. The best we can do is to return an empty response, + // this allows us to ignore any foreign calls which exist solely to pass information from inside + // the circuit to the environment (e.g. custom logging) as the execution will still be able to progress. + // + // We optimistically return an empty response for all oracle calls as the ACVM will error + // should a response have been required. + Ok(ForeignCallResult::default()) } } } diff --git a/noir/noir-repo/tooling/noir_js/scripts/compile_test_programs.sh b/noir/noir-repo/tooling/noir_js/scripts/compile_test_programs.sh index b9f12b5deb1c..0642159aa695 100755 --- a/noir/noir-repo/tooling/noir_js/scripts/compile_test_programs.sh +++ b/noir/noir-repo/tooling/noir_js/scripts/compile_test_programs.sh @@ -3,4 +3,5 @@ rm -rf ./test/noir_compiled_examples/**/target nargo --program-dir ./test/noir_compiled_examples/assert_lt compile --force nargo --program-dir ./test/noir_compiled_examples/assert_msg_runtime compile --force +nargo --program-dir ./test/noir_compiled_examples/fold_fibonacci compile --force nargo --program-dir ./test/noir_compiled_examples/assert_raw_payload compile --force diff --git a/noir/noir-repo/tooling/noir_js/src/program.ts b/noir/noir-repo/tooling/noir_js/src/program.ts index 8d80ec3a247e..86aa0f60ddfc 100644 --- a/noir/noir-repo/tooling/noir_js/src/program.ts +++ b/noir/noir-repo/tooling/noir_js/src/program.ts @@ -2,7 +2,7 @@ import { Backend, CompiledCircuit, ProofData } from '@noir-lang/types'; import { generateWitness } from './witness_generation.js'; import initAbi, { abiDecode, InputMap, InputValue } from '@noir-lang/noirc_abi'; -import initACVM, { compressWitness, ForeignCallHandler } from '@noir-lang/acvm_js'; +import initACVM, { compressWitnessStack, ForeignCallHandler } from '@noir-lang/acvm_js'; export class Noir { constructor( @@ -55,9 +55,10 @@ export class Noir { foreignCallHandler?: ForeignCallHandler, ): Promise<{ witness: Uint8Array; returnValue: InputValue }> { await this.init(); - const witness = await generateWitness(this.circuit, inputs, foreignCallHandler); - const { return_value: returnValue } = abiDecode(this.circuit.abi, witness); - return { witness: compressWitness(witness), returnValue }; + const witness_stack = await generateWitness(this.circuit, inputs, foreignCallHandler); + const main_witness = witness_stack[0].witness; + const { return_value: returnValue } = abiDecode(this.circuit.abi, main_witness); + return { witness: compressWitnessStack(witness_stack), returnValue }; } /** diff --git a/noir/noir-repo/tooling/noir_js/src/witness_generation.ts b/noir/noir-repo/tooling/noir_js/src/witness_generation.ts index a22b06870403..7d018c81d53a 100644 --- a/noir/noir-repo/tooling/noir_js/src/witness_generation.ts +++ b/noir/noir-repo/tooling/noir_js/src/witness_generation.ts @@ -1,12 +1,12 @@ import { abiDecodeError, abiEncode, InputMap } from '@noir-lang/noirc_abi'; import { base64Decode } from './base64_decode.js'; import { - WitnessMap, + WitnessStack, ForeignCallHandler, ForeignCallInput, createBlackBoxSolver, WasmBlackBoxFunctionSolver, - executeCircuitWithBlackBoxSolver, + executeProgramWithBlackBoxSolver, ExecutionError, } from '@noir-lang/acvm_js'; import { Abi, CompiledCircuit } from '@noir-lang/types'; @@ -63,14 +63,14 @@ export async function generateWitness( compiledProgram: CompiledCircuit, inputs: InputMap, foreignCallHandler: ForeignCallHandler = defaultForeignCallHandler, -): Promise { +): Promise { // Throws on ABI encoding error const witnessMap = abiEncode(compiledProgram.abi, inputs); // Execute the circuit to generate the rest of the witnesses and serialize // them into a Uint8Array. try { - const solvedWitness = await executeCircuitWithBlackBoxSolver( + const solvedWitness = await executeProgramWithBlackBoxSolver( await getSolver(), base64Decode(compiledProgram.bytecode), witnessMap, diff --git a/noir/noir-repo/tooling/noir_js/test/node/e2e.test.ts b/noir/noir-repo/tooling/noir_js/test/node/e2e.test.ts index 979841c47e6f..dbb9abcc964c 100644 --- a/noir/noir-repo/tooling/noir_js/test/node/e2e.test.ts +++ b/noir/noir-repo/tooling/noir_js/test/node/e2e.test.ts @@ -1,10 +1,12 @@ import { expect } from 'chai'; import assert_lt_json from '../noir_compiled_examples/assert_lt/target/assert_lt.json' assert { type: 'json' }; +import fold_fibonacci_json from '../noir_compiled_examples/fold_fibonacci/target/fold_fibonacci.json' assert { type: 'json' }; import { Noir } from '@noir-lang/noir_js'; import { BarretenbergBackend as Backend, BarretenbergVerifier as Verifier } from '@noir-lang/backend_barretenberg'; import { CompiledCircuit } from '@noir-lang/types'; const assert_lt_program = assert_lt_json as CompiledCircuit; +const fold_fibonacci_program = fold_fibonacci_json as CompiledCircuit; it('end-to-end proof creation and verification (outer)', async () => { // Noir.Js part @@ -149,3 +151,24 @@ it('[BUG] -- bb.js null function or function signature mismatch (outer-inner) ', const isValidInner = await prover.verifyProof(_proofInner); expect(isValidInner).to.be.true; }); + +it('end-to-end proof creation and verification for multiple ACIR circuits (inner)', async () => { + // Noir.Js part + const inputs = { + x: '10', + }; + + const program = new Noir(fold_fibonacci_program); + + const { witness } = await program.execute(inputs); + + // bb.js part + // + // Proof creation + const backend = new Backend(fold_fibonacci_program); + const proof = await backend.generateProof(witness); + + // Proof verification + const isValid = await backend.verifyProof(proof); + expect(isValid).to.be.true; +}); diff --git a/noir/noir-repo/tooling/noir_js/test/node/execute.test.ts b/noir/noir-repo/tooling/noir_js/test/node/execute.test.ts index b7aa4f3135cb..54a42d40b60b 100644 --- a/noir/noir-repo/tooling/noir_js/test/node/execute.test.ts +++ b/noir/noir-repo/tooling/noir_js/test/node/execute.test.ts @@ -1,5 +1,6 @@ import assert_lt_json from '../noir_compiled_examples/assert_lt/target/assert_lt.json' assert { type: 'json' }; import assert_msg_json from '../noir_compiled_examples/assert_msg_runtime/target/assert_msg_runtime.json' assert { type: 'json' }; +import fold_fibonacci_json from '../noir_compiled_examples/fold_fibonacci/target/fold_fibonacci.json' assert { type: 'json' }; import assert_raw_payload_json from '../noir_compiled_examples/assert_raw_payload/target/assert_raw_payload.json' assert { type: 'json' }; import { Noir, ErrorWithPayload } from '@noir-lang/noir_js'; @@ -8,6 +9,7 @@ import { expect } from 'chai'; const assert_lt_program = assert_lt_json as CompiledCircuit; const assert_msg_runtime = assert_msg_json as CompiledCircuit; +const fold_fibonacci_program = fold_fibonacci_json as CompiledCircuit; it('returns the return value of the circuit', async () => { const inputs = { @@ -49,3 +51,45 @@ it('circuit with a raw assert payload should fail with the decoded payload', asy }); } }); + +it('successfully executes a program with multiple acir circuits', async () => { + const inputs = { + x: '10', + }; + try { + await new Noir(fold_fibonacci_program).execute(inputs); + } catch (error) { + const knownError = error as Error; + expect(knownError.message).to.equal('Circuit execution failed: Expected x < y but got 10 < 5'); + } +}); + +it('circuit with a raw assert payload should fail with the decoded payload', async () => { + const inputs = { + x: '7', + y: '5', + }; + try { + await new Noir(assert_raw_payload_json).execute(inputs); + } catch (error) { + const knownError = error as ErrorWithPayload; + const invalidXYErrorSelector = Object.keys(assert_raw_payload_json.abi.error_types)[0]; + expect(knownError.rawAssertionPayload!.selector).to.equal(invalidXYErrorSelector); + expect(knownError.decodedAssertionPayload).to.deep.equal({ + x: '0x07', + y: '0x05', + }); + } +}); + +it('successfully executes a program with multiple acir circuits', async () => { + const inputs = { + x: '10', + }; + try { + await new Noir(fold_fibonacci_program).execute(inputs); + } catch (error) { + const knownError = error as Error; + expect(knownError.message).to.equal('Circuit execution failed: Error: Cannot satisfy constraint'); + } +}); diff --git a/noir/noir-repo/tooling/noir_js/test/noir_compiled_examples/fold_fibonacci/Nargo.toml b/noir/noir-repo/tooling/noir_js/test/noir_compiled_examples/fold_fibonacci/Nargo.toml new file mode 100644 index 000000000000..6d8214689b06 --- /dev/null +++ b/noir/noir-repo/tooling/noir_js/test/noir_compiled_examples/fold_fibonacci/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "fold_fibonacci" +type = "bin" +authors = [""] +compiler_version = ">=0.28.0" + +[dependencies] \ No newline at end of file diff --git a/noir/noir-repo/tooling/noir_js/test/noir_compiled_examples/fold_fibonacci/src/main.nr b/noir/noir-repo/tooling/noir_js/test/noir_compiled_examples/fold_fibonacci/src/main.nr new file mode 100644 index 000000000000..e150a586086f --- /dev/null +++ b/noir/noir-repo/tooling/noir_js/test/noir_compiled_examples/fold_fibonacci/src/main.nr @@ -0,0 +1,12 @@ +fn main(x: u32) { + assert(fibonacci(x) == 55); +} + +#[fold] +fn fibonacci(x: u32) -> u32 { + if x <= 1 { + x + } else { + fibonacci(x - 1) + fibonacci(x - 2) + } +}