From f36d9f55ecb4c00aef3bb4fba5693aec200f7136 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 4 Apr 2024 11:35:55 -0500 Subject: [PATCH 01/14] Initial version of new pass --- .../src/brillig/brillig_gen/brillig_block.rs | 3 + compiler/noirc_evaluator/src/ssa.rs | 1 + .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 3 + .../noirc_evaluator/src/ssa/ir/instruction.rs | 61 +++++- .../noirc_evaluator/src/ssa/ir/printer.rs | 10 + .../src/ssa/opt/flatten_cfg.rs | 109 ++++------ .../ssa/opt/flatten_cfg/capacity_tracker.rs | 4 +- .../src/ssa/opt/flatten_cfg/value_merger.rs | 60 +++--- compiler/noirc_evaluator/src/ssa/opt/mod.rs | 1 + .../src/ssa/opt/remove_if_else.rs | 199 ++++++++++++++++++ 10 files changed, 349 insertions(+), 102 deletions(-) create mode 100644 compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs 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 cf2501ab1c0..4e5a8cde0c2 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -703,6 +703,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 a45cf3af608..6ecf11cf20d 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -60,6 +60,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_pass(Ssa::fold_constants, "After Constant Folding:") .run_pass(Ssa::fold_constants_using_constraints, "After Constraint Folding:") .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index e3dd4c33986..8c0f7a38ad0 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -533,6 +533,9 @@ impl Context { assert_message.clone(), )?; } + Instruction::IfElse { .. } => { + unreachable!("IfElse instruction remaining in acir-gen") + } } self.acir_context.set_call_stack(CallStack::new()); diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 2b23cc1c1e8..f7fe84ea1e3 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 { .. } @@ -254,7 +266,12 @@ impl Instruction { // 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(_) | ArrayGet { .. } | ArraySet { .. } => true, + Cast(_, _) + | Truncate { .. } + | Not(_) + | ArrayGet { .. } + | ArraySet { .. } + | IfElse { .. } => true, // These either have side-effects or interact with memory Constrain(..) @@ -293,7 +310,8 @@ impl Instruction { | Allocate | Load { .. } | ArrayGet { .. } - | ArraySet { .. } => false, + | ArraySet { .. } + | IfElse { .. } => false, Constrain(..) | Store { .. } @@ -379,6 +397,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), + } + } } } @@ -433,6 +459,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); + } } } @@ -584,6 +616,27 @@ impl Instruction { None } } + Instruction::IfElse { then_condition, then_value, else_condition, else_value } => { + let typ = dfg.type_of_value(*then_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/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index fc13ab7307a..be92984441e 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -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/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 07771397ce8..1074297a458 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,29 @@ 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); + // 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() + + // value_merger.merge_values( + // cond_context.then_branch.condition, + // cond_context.else_branch.clone().unwrap().condition, + // then_arg, + // else_arg, + // ) }); self.merge_stores(cond_context.then_branch, cond_context.else_branch); @@ -643,15 +626,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 +634,28 @@ 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); + // 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 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 +672,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 +685,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..ac869caf995 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 @@ -42,9 +42,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 +64,53 @@ 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); + 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 }`, @@ -175,12 +175,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); diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 479010b1ed8..0adc74d7e87 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -14,5 +14,6 @@ mod inlining; mod mem2reg; mod rc; mod remove_bit_shifts; +mod remove_if_else; mod simplify_cfg; mod unrolling; 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..2afafbff78e --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -0,0 +1,199 @@ +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, +} + +impl Context { + fn remove_if_else(&mut self, function: &mut Function) { + let block = function.entry_block(); + let instructions = function.dfg[block].take_instructions(); + + 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); + let value = value_merger.merge_values( + then_condition, + else_condition, + then_value, + else_value, + ); + + let result = function.dfg.instruction_results(instruction)[0]; + + function.dfg.set_value_from_id(result, value); + } + 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] }; + + 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); + } + _ => { + function.dfg[block].instructions_mut().push(instruction); + } + } + } + } + + fn get_or_find_capacity<'a>(&'a 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(elems, length) = dfg.type_of_value(value) { + let length = length / elems.len(); + 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]; + SizeChange::Inc { old, new } + } + + Intrinsic::SlicePopBack | Intrinsic::SliceRemove => { + let old = arguments[1]; + let new = results[1]; + SizeChange::Dec { old, new } + } + + Intrinsic::SlicePopFront => { + let old = arguments[1]; + let new = results[results.len() - 1]; + SizeChange::Dec { old, new } + } + + Intrinsic::ToBits(_) => { + assert_eq!(results.len(), 2); + 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); + 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:?}"), + }; + 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, + } +} From 222bc0e34953a8e95032e47013134f5080dfe017 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 4 Apr 2024 14:18:51 -0500 Subject: [PATCH 02/14] clippy --- compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index 2afafbff78e..f21c2a1b519 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -109,7 +109,7 @@ impl Context { } } - fn get_or_find_capacity<'a>(&'a mut self, dfg: &DataFlowGraph, value: ValueId) -> usize { + 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) => { From 0a39c7b3ea3d377a535371d82955cfa06a72b504 Mon Sep 17 00:00:00 2001 From: jfecher Date: Fri, 5 Apr 2024 09:10:01 -0500 Subject: [PATCH 03/14] chore: Add optimization to #4716 (#4718) # Description ## Problem\* ## Summary\* Adds the actual array set optimization to https://github.com/noir-lang/noir/pull/4716. This is separate since the other PR still has errors and I want to fix those before adding more with the actual optimization. At the same time, I've put up this PR to see the circuit size changes in CI. ## Additional Context This PR is currently failing a few additional tests. I think these are mostly array out of bounds accesses from storing which indices were changed. In some if statements, these indices may be out of bounds but not error since that if case was not run. Trying to get and store to this same index after the if then gives an OOB error. This can be fixed by only applying the optimization when constant indices are used and filtering out the OOB ones but then dynamic indices won't see this important optimization at all. ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../src/brillig/brillig_gen/brillig_block.rs | 2 +- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 5 +- .../src/ssa/ir/instruction/call.rs | 4 +- .../noirc_evaluator/src/ssa/ir/printer.rs | 2 +- .../src/ssa/opt/flatten_cfg/value_merger.rs | 117 +++++++++++++++++- .../src/ssa/opt/remove_if_else.rs | 23 +++- 6 files changed, 143 insertions(+), 10 deletions(-) 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 4e5a8cde0c2..e3d8b33837c 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -622,7 +622,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); diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 870b5e602f1..f1c817d3866 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -530,7 +530,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") } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 1187ea8cb07..78b68f6d61d 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 = 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 be92984441e..ef5ba39d0e4 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/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)) 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 ac869caf995..47953bedb72 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 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 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. @@ -131,6 +138,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(); @@ -266,4 +285,94 @@ 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 seen_then = FxHashSet::default(); + let mut seen_else = FxHashSet::default(); + let mut found = false; + + let mut current_then = then_value; + let mut current_else = else_value; + + let mut changed_indices = FxHashSet::default(); + + // 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. + for _ in 0..10 { + seen_then.insert(current_then); + seen_else.insert(current_else); + + if seen_then.contains(¤t_else) || seen_else.contains(¤t_then) { + found = true; + break; + } + + current_then = self.find_previous_array_set(current_then, &mut changed_indices)?; + current_else = self.find_previous_array_set(current_else, &mut changed_indices)?; + } + + 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); + + let instruction = Instruction::ArraySet { array, index, value, mutable: false }; + array = self.insert_instruction(instruction).first(); + } + + let instruction = Instruction::EnableSideEffects { condition: self.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 find_previous_array_set( + &self, + value: ValueId, + changed_indices: &mut FxHashSet<(ValueId, Type, ValueId)>, + ) -> Option { + match &self.dfg[value] { + Value::Instruction { instruction, .. } => match &self.dfg[*instruction] { + Instruction::ArraySet { array, index, value, .. } => { + let condition = *self.array_set_conditionals.get(index)?; + let element_type = self.dfg.type_of_value(*value); + changed_indices.insert((*index, element_type, condition)); + Some(*array) + } + _ => Some(value), + }, + _ => Some(value), + } + } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index f21c2a1b519..86e90559639 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -42,12 +42,20 @@ impl Ssa { #[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] { @@ -60,8 +68,14 @@ impl Context { 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); + let mut value_merger = ValueMerger::new( + &mut function.dfg, + block, + &mut self.slice_sizes, + &self.array_set_conditionals, + Some(current_conditional), + ); + let value = value_merger.merge_values( then_condition, else_condition, @@ -98,10 +112,15 @@ impl Context { let results = function.dfg.instruction_results(instruction); let result = if results.len() == 2 { results[1] } else { results[0] }; + self.array_set_conditionals.insert(*array, 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); } From 398e9a7b5c28d699bc848b79472d9af844e4494c Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 5 Apr 2024 11:20:59 -0500 Subject: [PATCH 04/14] Tracking changes more accurately makes performance worse --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 18 ++++ .../noirc_evaluator/src/ssa/ir/instruction.rs | 9 ++ .../src/ssa/ir/instruction/call.rs | 4 +- .../src/ssa/opt/flatten_cfg/value_merger.rs | 95 ++++++++++++++----- .../src/ssa/opt/remove_if_else.rs | 15 ++- 5 files changed, 111 insertions(+), 30 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index ce138633390..fdaf62b148e 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -586,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 f7fe84ea1e3..aceb148d9bd 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -618,6 +618,15 @@ impl Instruction { } 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; diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 78b68f6d61d..dcde0aa79be 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -354,8 +354,8 @@ 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 unknown = HashMap::default(); - let mut value_merger = ValueMerger::new(dfg, block, &mut slice_sizes, &unknown, None); + 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, 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 47953bedb72..7ac7c278c1e 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 @@ -19,7 +19,7 @@ pub(crate) struct ValueMerger<'a> { // This must be computed before merging values. slice_sizes: &'a mut HashMap, - array_set_conditionals: &'a HashMap, + array_set_conditionals: &'a mut HashMap, } impl<'a> ValueMerger<'a> { @@ -27,7 +27,7 @@ impl<'a> ValueMerger<'a> { dfg: &'a mut DataFlowGraph, block: BasicBlockId, slice_sizes: &'a mut HashMap, - array_set_conditionals: &'a HashMap, + array_set_conditionals: &'a mut HashMap, current_condition: Option, ) -> Self { ValueMerger { dfg, block, slice_sizes, array_set_conditionals, current_condition } @@ -294,30 +294,55 @@ impl<'a> ValueMerger<'a> { else_value: ValueId, array_length: usize, ) -> Option { - let mut seen_then = FxHashSet::default(); - let mut seen_else = FxHashSet::default(); let mut found = false; + let current_condition = self.current_condition?; let mut current_then = then_value; let mut current_else = else_value; - let mut changed_indices = FxHashSet::default(); - // 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. - for _ in 0..10 { - seen_then.insert(current_then); - seen_else.insert(current_else); + 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 seen_then.contains(¤t_else) || seen_else.contains(¤t_then) { + 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 changed_indices)?; - current_else = self.find_previous_array_set(current_else, &mut changed_indices)?; + 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; } @@ -343,13 +368,11 @@ impl<'a> ValueMerger<'a> { let value = self.merge_values(then_condition, else_condition, then_element, else_element); - let instruction = Instruction::ArraySet { array, index, value, mutable: false }; - array = self.insert_instruction(instruction).first(); + array = self.insert_array_set(array, index, value, Some(condition)).first(); } - let instruction = Instruction::EnableSideEffects { condition: self.current_condition? }; + let instruction = Instruction::EnableSideEffects { condition: current_condition }; self.insert_instruction(instruction); - Some(array) } @@ -357,22 +380,44 @@ impl<'a> ValueMerger<'a> { 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, - value: ValueId, - changed_indices: &mut FxHashSet<(ValueId, Type, ValueId)>, - ) -> Option { - match &self.dfg[value] { + 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(index)?; + 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.insert((*index, element_type, condition)); - Some(*array) + changed_indices.push((result, *index, element_type, condition)); + *array } - _ => Some(value), + _ => result, }, - _ => Some(value), + _ => result, } } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index 86e90559639..d3838eea59f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -72,7 +72,7 @@ impl Context { &mut function.dfg, block, &mut self.slice_sizes, - &self.array_set_conditionals, + &mut self.array_set_conditionals, Some(current_conditional), ); @@ -83,9 +83,17 @@ impl Context { else_value, ); - let result = function.dfg.instruction_results(instruction)[0]; + 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] { @@ -112,7 +120,8 @@ impl Context { let results = function.dfg.instruction_results(instruction); let result = if results.len() == 2 { results[1] } else { results[0] }; - self.array_set_conditionals.insert(*array, current_conditional); + 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); From 233290680686b2390ff7f3169d459f93057b5b59 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 5 Apr 2024 11:46:32 -0500 Subject: [PATCH 05/14] Add constant check --- .../noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs | 4 ++++ 1 file changed, 4 insertions(+) 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 7ac7c278c1e..51557ad7b61 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 @@ -85,6 +85,10 @@ impl<'a> ValueMerger<'a> { "Expected values merged to be of the same type but found {then_type} and {else_type}" ); + 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); From cf5e9f61378df037228f59e77c8c8afc36031379 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 5 Apr 2024 16:04:56 -0500 Subject: [PATCH 06/14] Debug commit. Remove before merging --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 18 ++++++++++-------- compiler/noirc_evaluator/src/ssa/ir/printer.rs | 6 ++++-- .../src/ssa/opt/remove_if_else.rs | 14 ++++++++++++-- 3 files changed, 26 insertions(+), 12 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 39d0f50c771..82ad436e50d 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -829,6 +829,7 @@ impl Context { }); } }; + 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 { @@ -1034,20 +1035,17 @@ impl Context { var_index: &mut AcirVar, ) -> Result { let one = self.acir_context.add_constant(FieldElement::one()); - match ssa_type.clone() { + let result = match ssa_type.clone() { Type::Numeric(numeric_type) => { // Read the value from the array at the specified index let read = self.acir_context.read_from_memory(block_id, var_index)?; - // Increment the var_index in case of a nested array - *var_index = self.acir_context.add_var(*var_index, one)?; - let typ = AcirType::NumericType(numeric_type); Ok(AcirValue::Var(read, typ)) } Type::Array(element_types, len) => { let mut values = Vector::new(); - for _ in 0..len { + for _ in 0 .. len { for typ in element_types.as_ref() { values.push_back(self.array_get_value(typ, block_id, var_index)?); } @@ -1055,7 +1053,11 @@ impl Context { Ok(AcirValue::Array(values)) } _ => unreachable!("ICE: Expected an array or numeric but got {ssa_type:?}"), - } + }; + + // Increment the var_index in case of a nested array + *var_index = self.acir_context.add_var(*var_index, one)?; + result } /// If `mutate_array` is: @@ -1155,8 +1157,6 @@ impl Context { AcirValue::Var(store_var, _) => { // Write the new value into the new array at the specified index self.acir_context.write_to_memory(block_id, var_index, store_var)?; - // Increment the var_index in case of a nested array - *var_index = self.acir_context.add_var(*var_index, one)?; } AcirValue::Array(values) => { for value in values { @@ -1173,6 +1173,8 @@ impl Context { self.array_set_value(&AcirValue::Array(values.into()), block_id, var_index)?; } } + // Increment the var_index in case of a nested array + *var_index = self.acir_context.add_var(*var_index, one)?; Ok(()) } diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index ef5ba39d0e4..80fae3a2e85 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -174,14 +174,16 @@ fn display_instruction_inner( writeln!(f, "enable_side_effects {}", show(*condition)) } Instruction::ArrayGet { array, index } => { - writeln!(f, "array_get {}, index {}", show(*array), show(*index)) + let t = function.dfg.type_of_value(*array); + writeln!(f, "array_get {}, index {}, array type = {t}", show(*array), show(*index)) } Instruction::ArraySet { array, index, value, mutable } => { + let t = function.dfg.type_of_value(*array); let array = show(*array); 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}, array type = {t}") } Instruction::IncrementRc { value } => { writeln!(f, "inc_rc {}", show(*value)) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index d3838eea59f..30b80f6d21f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -146,8 +146,8 @@ impl Context { return *entry.insert(length); } - if let Type::Array(elems, length) = dfg.type_of_value(value) { - let length = length / elems.len(); + if let Type::Array(_, length) = dfg.type_of_value(value) { + let length = length; return *entry.insert(length); } } @@ -181,28 +181,37 @@ fn slice_capacity_change( 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 => { @@ -212,6 +221,7 @@ fn slice_capacity_change( 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) } From 25830695522300f658f8c49e3e74d5268fb4024a Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 10 Apr 2024 10:47:24 -0400 Subject: [PATCH 07/14] Clippy --- compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index 30b80f6d21f..fc915756110 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -147,7 +147,6 @@ impl Context { } if let Type::Array(_, length) = dfg.type_of_value(value) { - let length = length; return *entry.insert(length); } } From 53ebf753c2432cf1c94ad8e1783b0204a28d6273 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 10 Apr 2024 11:27:12 -0400 Subject: [PATCH 08/14] Make ArrayGet and ArraySet impure --- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index aceb148d9bd..bf5934b5e6e 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -269,8 +269,6 @@ impl Instruction { Cast(_, _) | Truncate { .. } | Not(_) - | ArrayGet { .. } - | ArraySet { .. } | IfElse { .. } => true, // These either have side-effects or interact with memory @@ -283,6 +281,13 @@ 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, From 9e50e33a1c892ee4568c03c6e62ccb6872593a2c Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 10 Apr 2024 11:31:44 -0400 Subject: [PATCH 09/14] Format --- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index bf5934b5e6e..54a2ee893f4 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -266,10 +266,7 @@ impl Instruction { // 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(_) - | IfElse { .. } => true, + Cast(_, _) | Truncate { .. } | Not(_) | IfElse { .. } => true, // These either have side-effects or interact with memory Constrain(..) @@ -285,8 +282,7 @@ impl Instruction { // 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, + ArrayGet { .. } | ArraySet { .. } => false, Call { func, .. } => match dfg[*func] { Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(), From daaccd79da4de541543ba43a034dd27a952a0a23 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 10 Apr 2024 11:45:28 -0400 Subject: [PATCH 10/14] Remove type print debug --- compiler/noirc_evaluator/src/ssa/ir/printer.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index a8b48ac7dc2..92f64021d71 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -174,18 +174,16 @@ fn display_instruction_inner( writeln!(f, "enable_side_effects {}", show(*condition)) } Instruction::ArrayGet { array, index } => { - let t = function.dfg.type_of_value(*array); - writeln!(f, "array_get {}, index {}, array type = {t}", show(*array), show(*index)) + writeln!(f, "array_get {}, index {}", show(*array), show(*index)) } Instruction::ArraySet { array, index, value, mutable } => { - let t = function.dfg.type_of_value(*array); let array = show(*array); let index = show(*index); let value = show(*value); let mutable = if *mutable { " mut" } else { "" }; writeln!( f, - "array_set{mutable} {array}, index {index}, value {value}, array type = {t}" + "array_set{mutable} {array}, index {index}, value {value}" ) } Instruction::IncrementRc { value } => { From 5ff5d27daf495df938838ce03cd2bb13927976c4 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 10 Apr 2024 12:35:23 -0400 Subject: [PATCH 11/14] Fmt --- compiler/noirc_evaluator/src/ssa/ir/printer.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 92f64021d71..ef5ba39d0e4 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -181,10 +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)) From e3d54745ca8f3c07d203249b82c1cf666c2a1943 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Fri, 3 May 2024 14:51:35 +0000 Subject: [PATCH 12/14] fix array_get_value --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 11 +++++------ .../nested_array_dynamic_simple/Nargo.toml | 7 +++++++ .../nested_array_dynamic_simple/Prover.toml | 1 + .../nested_array_dynamic_simple/src/main.nr | 9 +++++++++ 4 files changed, 22 insertions(+), 6 deletions(-) create mode 100644 test_programs/execution_success/nested_array_dynamic_simple/Nargo.toml create mode 100644 test_programs/execution_success/nested_array_dynamic_simple/Prover.toml create mode 100644 test_programs/execution_success/nested_array_dynamic_simple/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index dc35aa28437..43e83fafeaa 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1218,11 +1218,14 @@ impl<'a> Context<'a> { var_index: &mut AcirVar, ) -> Result { let one = self.acir_context.add_constant(FieldElement::one()); - let result = match ssa_type.clone() { + match ssa_type.clone() { Type::Numeric(numeric_type) => { // Read the value from the array at the specified index let read = self.acir_context.read_from_memory(block_id, var_index)?; + // Increment the var_index in case of a nested array + *var_index = self.acir_context.add_var(*var_index, one)?; + let typ = AcirType::NumericType(numeric_type); Ok(AcirValue::Var(read, typ)) } @@ -1236,11 +1239,7 @@ impl<'a> Context<'a> { Ok(AcirValue::Array(values)) } _ => unreachable!("ICE: Expected an array or numeric but got {ssa_type:?}"), - }; - - // Increment the var_index in case of a nested array - *var_index = self.acir_context.add_var(*var_index, one)?; - result + } } /// If `mutate_array` is: 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); +} From 5a7a73b9902ed8e92737624d2779cbd1d02447f2 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Fri, 3 May 2024 15:06:34 +0000 Subject: [PATCH 13/14] fix array_set_value --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 43e83fafeaa..55c11e06ca3 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1339,6 +1339,8 @@ impl<'a> Context<'a> { AcirValue::Var(store_var, _) => { // Write the new value into the new array at the specified index self.acir_context.write_to_memory(block_id, var_index, store_var)?; + // Increment the var_index in case of a nested array + *var_index = self.acir_context.add_var(*var_index, one)?; } AcirValue::Array(values) => { for value in values { @@ -1355,8 +1357,6 @@ impl<'a> Context<'a> { self.array_set_value(&AcirValue::Array(values.into()), block_id, var_index)?; } } - // Increment the var_index in case of a nested array - *var_index = self.acir_context.add_var(*var_index, one)?; Ok(()) } From 471398bc164843728e7f47a6cc2be23030de008b Mon Sep 17 00:00:00 2001 From: vezenovm Date: Fri, 3 May 2024 15:57:57 +0000 Subject: [PATCH 14/14] comment cleanup --- .../noirc_evaluator/src/ssa/opt/flatten_cfg.rs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 1074297a458..0f8b49b40ec 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -521,9 +521,6 @@ 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); - // Cannot include this in the previous vecmap since it requires exclusive access to self let args = vecmap(args, |(then_arg, else_arg)| { let instruction = Instruction::IfElse { @@ -537,13 +534,6 @@ impl<'f> Context<'f> { .dfg .insert_instruction_and_results(instruction, block, None, CallStack::new()) .first() - - // value_merger.merge_values( - // cond_context.then_branch.condition, - // cond_context.else_branch.clone().unwrap().condition, - // then_arg, - // else_arg, - // ) }); self.merge_stores(cond_context.then_branch, cond_context.else_branch); @@ -634,15 +624,9 @@ 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,