diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 8dc7ccb9ac0..ff8d33c0fdc 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -631,7 +631,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); @@ -712,6 +712,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/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 89ef74c7406..ffe3b380c1a 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -59,6 +59,7 @@ pub(crate) fn optimize_into_acir( .run_pass(Ssa::remove_bit_shifts, "After Removing Bit Shifts:") // Run mem2reg once more with the flattened CFG to catch any remaining loads/stores .run_pass(Ssa::mem2reg, "After Mem2Reg:") + .run_pass(Ssa::remove_if_else, "After Remove IfElse:") // 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:") diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 65cd9c05992..55c11e06ca3 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -686,6 +686,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()); @@ -1009,6 +1012,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/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 6b950c327cf..85630b75614 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/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/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index b6f11f45c07..11e3fc940b6 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -1,6 +1,8 @@ use acvm::{acir::BlackBoxFunc, FieldElement}; use iter_extended::vecmap; +use crate::ssa::opt::flatten_cfg::value_merger::ValueMerger; + use super::{ basic_block::BasicBlockId, dfg::{CallStack, DataFlowGraph}, @@ -206,6 +208,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 { @@ -219,10 +229,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 { .. } @@ -270,6 +282,7 @@ impl Instruction { | Cast(_, _) | Not(_) | Truncate { .. } + | IfElse { .. } | ArrayGet { .. } | ArraySet { .. } => !self.requires_acir_gen_predicate(dfg), } @@ -295,6 +308,7 @@ impl Instruction { | Allocate | Load { .. } | ArrayGet { .. } + | IfElse { .. } | ArraySet { .. } => true, Constrain(..) @@ -349,6 +363,7 @@ impl Instruction { | Instruction::Allocate | Instruction::Load { .. } | Instruction::Store { .. } + | Instruction::IfElse { .. } | Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. } => false, } @@ -416,6 +431,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), + } + } } } @@ -473,6 +496,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); + } } } @@ -624,6 +653,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/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 1187ea8cb07..dcde0aa79be 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/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/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index d17d2989341..23395617c85 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -184,7 +184,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)) @@ -195,6 +195,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/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 07771397ce8..0f8b49b40ec 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/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/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs index 93e52542278..4fc19acd2ac 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs +++ b/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/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs index 0a351148fa3..c47d594545c 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 @@ -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/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 4452840a28c..27536d59ea5 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/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/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index 8535dc2661f..02b9202b209 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/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/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs new file mode 100644 index 00000000000..fc915756110 --- /dev/null +++ b/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/test_programs/execution_success/nested_array_dynamic_simple/Nargo.toml b/test_programs/execution_success/nested_array_dynamic_simple/Nargo.toml new file mode 100644 index 00000000000..50ba1d194a6 --- /dev/null +++ b/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/test_programs/execution_success/nested_array_dynamic_simple/Prover.toml b/test_programs/execution_success/nested_array_dynamic_simple/Prover.toml new file mode 100644 index 00000000000..07890234a19 --- /dev/null +++ b/test_programs/execution_success/nested_array_dynamic_simple/Prover.toml @@ -0,0 +1 @@ +x = "3" diff --git a/test_programs/execution_success/nested_array_dynamic_simple/src/main.nr b/test_programs/execution_success/nested_array_dynamic_simple/src/main.nr new file mode 100644 index 00000000000..3b1908a463b --- /dev/null +++ b/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); +}