diff --git a/compiler/noirc_evaluator/src/ssa/ir/types.rs b/compiler/noirc_evaluator/src/ssa/ir/types.rs index 6b8817fb3fa..0648f2b5f2f 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/types.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/types.rs @@ -269,15 +269,6 @@ impl Type { } } - /// Retrieves the array or slice type within this type, or panics if there is none. - pub(crate) fn get_contained_array(&self) -> &Type { - match self { - Type::Numeric(_) | Type::Function => panic!("Expected an array type"), - Type::Array(_, _) | Type::Slice(_) => self, - Type::Reference(element) => element.get_contained_array(), - } - } - pub(crate) fn element_types(self) -> Arc> { match self { Type::Array(element_types, _) | Type::Slice(element_types) => element_types, diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index eaf4467fbf9..fe995341da6 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -52,15 +52,12 @@ use crate::ssa::{ function::{Function, FunctionId}, instruction::{BinaryOp, Instruction, InstructionId, Intrinsic, TerminatorInstruction}, post_order::PostOrder, - types::Type, value::{Value, ValueId}, }, opt::{die::array_oob_checks::should_insert_oob_check, pure::Purity}, ssa_gen::Ssa, }; -use super::rc::{RcInstruction, pop_rc_for}; - mod array_oob_checks; mod prune_dead_parameters; @@ -173,8 +170,6 @@ impl Function { ) -> HashMap> { let mut context = Context { flattened, ..Default::default() }; - context.mark_function_parameter_arrays_as_used(self); - for call_data in &self.dfg.data_bus.call_data { context.mark_used_instruction_results(&self.dfg, call_data.array_id); } @@ -239,9 +234,6 @@ struct Context { /// them just yet. flattened: bool, - /// Track IncrementRc instructions per block to determine whether they are useless. - rc_tracker: RcTracker, - /// A per-block list indicating which block parameters are still considered alive. /// /// Each entry maps a [BasicBlockId] to a `Vec`, where the `i`th boolean corresponds to @@ -281,9 +273,6 @@ impl Context { let block = &function.dfg[block_id]; self.mark_terminator_values_as_used(function, block); - self.rc_tracker.new_block(); - self.rc_tracker.mark_terminator_arrays_as_used(function, block); - let instructions_len = block.instructions().len(); // Indexes of instructions that might be out of bounds. @@ -319,13 +308,8 @@ impl Context { }); } } - - self.rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); } - self.instructions_to_remove.extend(self.rc_tracker.get_non_mutated_arrays(&function.dfg)); - self.instructions_to_remove.extend(self.rc_tracker.rc_pairs_to_remove.drain()); - // If there are some instructions that might trigger an out of bounds error, // first add constrain checks. Then run the DIE pass again, which will remove those // but leave the constrains (any any value needed by those constrains) @@ -395,23 +379,6 @@ impl Context { } } - /// Mark any array parameters to the function itself as possibly mutated. - fn mark_function_parameter_arrays_as_used(&mut self, function: &Function) { - for parameter in function.parameters() { - let typ = function.dfg.type_of_value(*parameter); - if typ.contains_an_array() { - let typ = typ.get_contained_array(); - // Want to store the array type which is being referenced, - // because it's the underlying array that the `inc_rc` is associated with. - self.add_mutated_array_type(typ.clone()); - } - } - } - - fn add_mutated_array_type(&mut self, typ: Type) { - self.rc_tracker.mutated_array_types.insert(typ.get_contained_array().clone()); - } - /// Go through the RC instructions collected when we figured out which values were unused; /// for each RC that refers to an unused value, remove the RC as well. fn remove_rc_instructions(&self, dfg: &mut DataFlowGraph) { @@ -535,161 +502,6 @@ fn can_be_eliminated_if_unused( } } -#[derive(Default)] -/// Per block RC tracker. -struct RcTracker { - // We can track IncrementRc instructions per block to determine whether they are useless. - // IncrementRc and DecrementRc instructions are normally side effectual instructions, but we remove - // them if their value is not used anywhere in the function. However, even when their value is used, their existence - // is pointless logic if there is no array set between the increment and the decrement of the reference counter. - // We track per block whether an IncrementRc instruction has a paired DecrementRc instruction - // with the same value but no array set in between. - // If we see an inc/dec RC pair within a block we can safely remove both instructions. - rcs_with_possible_pairs: HashMap>, - // Tracks repeated RC instructions: if there are two `inc_rc` for the same value in a row, the 2nd one is redundant. - rc_pairs_to_remove: HashSet, - // We also separately track all IncrementRc instructions and all array types which have been mutably borrowed. - // If an array is the same type as one of those non-mutated array types, we can safely remove all IncrementRc instructions on that array. - inc_rcs: HashMap>, - // Mutated arrays shared across the blocks of the function. - // When tracking mutations we consider arrays with the same type as all being possibly mutated. - mutated_array_types: HashSet, - // The SSA often creates patterns where after simplifications we end up with repeat - // IncrementRc instructions on the same value. We track whether the previous instruction was an IncrementRc, - // and if the current instruction is also an IncrementRc on the same value we remove the current instruction. - // `None` if the previous instruction was anything other than an IncrementRc - previous_inc_rc: Option, -} - -impl RcTracker { - fn new_block(&mut self) { - self.rcs_with_possible_pairs.clear(); - self.rc_pairs_to_remove.clear(); - self.inc_rcs.clear(); - self.previous_inc_rc = Default::default(); - } - - fn mark_terminator_arrays_as_used(&mut self, function: &Function, block: &BasicBlock) { - block.unwrap_terminator().for_each_value(|value| { - self.handle_value_for_mutated_array_types(value, &function.dfg); - }); - } - - fn handle_value_for_mutated_array_types(&mut self, value: ValueId, dfg: &DataFlowGraph) { - let typ = dfg.type_of_value(value); - if !matches!(&typ, Type::Array(_, _) | Type::Slice(_)) { - return; - } - - self.mutated_array_types.insert(typ); - - if dfg.is_global(value) { - return; - } - - // Also check if the value is a MakeArray instruction. If so, do the same check for all of its values. - let Value::Instruction { instruction, .. } = &dfg[value] else { - return; - }; - let Instruction::MakeArray { elements, typ: _ } = &dfg[*instruction] else { - return; - }; - - for element in elements { - self.handle_value_for_mutated_array_types(*element, dfg); - } - } - - fn track_inc_rcs_to_remove(&mut self, instruction_id: InstructionId, function: &Function) { - let instruction = &function.dfg[instruction_id]; - - // Deduplicate IncRC instructions. - if let Instruction::IncrementRc { value } = instruction { - if let Some(previous_value) = self.previous_inc_rc { - if previous_value == *value { - self.rc_pairs_to_remove.insert(instruction_id); - } - } - self.previous_inc_rc = Some(*value); - } else { - // Reset the deduplication. - self.previous_inc_rc = None; - } - - // DIE loops over a block in reverse order, so we insert an RC instruction for possible removal - // when we see a DecrementRc and check whether it was possibly mutated when we see an IncrementRc. - match instruction { - Instruction::IncrementRc { value } => { - // Get any RC instruction recorded further down the block for this array; - // if it exists and not marked as mutated, then both RCs can be removed. - if let Some(inc_rc) = - pop_rc_for(*value, function, &mut self.rcs_with_possible_pairs) - { - if !inc_rc.possibly_mutated { - self.rc_pairs_to_remove.insert(inc_rc.id); - self.rc_pairs_to_remove.insert(instruction_id); - } - } - // Remember that this array was RC'd by this instruction. - self.inc_rcs.entry(*value).or_default().insert(instruction_id); - } - Instruction::DecrementRc { value, .. } => { - let typ = function.dfg.type_of_value(*value); - - // We assume arrays aren't mutated until we find an array_set - let dec_rc = - RcInstruction { id: instruction_id, array: *value, possibly_mutated: false }; - self.rcs_with_possible_pairs.entry(typ).or_default().push(dec_rc); - } - Instruction::ArraySet { array, value, .. } => { - let typ = function.dfg.type_of_value(*array); - // We mark all RCs that refer to arrays with a matching type as the one being set, as possibly mutated. - if let Some(dec_rcs) = self.rcs_with_possible_pairs.get_mut(&typ) { - for dec_rc in dec_rcs { - dec_rc.possibly_mutated = true; - } - } - self.mutated_array_types.insert(typ); - - let value_typ = function.dfg.type_of_value(*value); - if value_typ.is_array() { - self.mutated_array_types.insert(value_typ); - } - } - Instruction::Store { value, .. } => { - // We are very conservative and say that any store of an array type means it has the potential to be mutated. - self.handle_value_for_mutated_array_types(*value, &function.dfg); - } - Instruction::Call { arguments, .. } => { - // Treat any array-type arguments to calls as possible sources of mutation. - // During the preprocessing of functions in isolation we don't want to - // get rid of IncRCs arrays that can potentially be mutated outside. - for arg in arguments { - self.handle_value_for_mutated_array_types(*arg, &function.dfg); - } - } - _ => {} - } - } - - /// Get all RC instructions which work on arrays whose type has not been marked as mutated. - fn get_non_mutated_arrays(&self, dfg: &DataFlowGraph) -> HashSet { - self.inc_rcs - .keys() - .filter_map(|value| { - let typ = dfg.type_of_value(*value); - if !self.mutated_array_types.contains(&typ) { - Some(&self.inc_rcs[value]) - } else { - None - } - }) - .flatten() - .copied() - .collect() - } -} - /// Store instructions must be removed by DIE in acir code, any load /// instructions should already be unused by that point. /// @@ -908,37 +720,6 @@ mod test { assert_eq!(main.dfg[b1].instructions().len(), 2); } - #[test] - fn keep_inc_rc_on_borrowed_array_set() { - let src = " - brillig(inline) fn main f0 { - b0(v0: [u32; 2]): - inc_rc v0 - v3 = array_set v0, index u32 0, value u32 1 - inc_rc v0 - inc_rc v0 - inc_rc v0 - v4 = array_get v3, index u32 1 -> u32 - return v4 - } - "; - let ssa = Ssa::from_str(src).unwrap(); - let ssa = ssa.dead_instruction_elimination(); - - // We expect the output to be unchanged - // Except for the repeated inc_rc instructions - assert_ssa_snapshot!(ssa, @r" - brillig(inline) fn main f0 { - b0(v0: [u32; 2]): - inc_rc v0 - v3 = array_set v0, index u32 0, value u32 1 - inc_rc v0 - v4 = array_get v3, index u32 1 -> u32 - return v4 - } - "); - } - #[test] fn does_not_remove_inc_or_dec_rc_of_if_they_are_loaded_from_a_reference() { let src = " @@ -958,66 +739,6 @@ mod test { assert_ssa_does_not_change(src, Ssa::dead_instruction_elimination); } - #[test] - fn does_not_remove_inc_rcs_that_are_never_mutably_borrowed() { - let src = " - brillig(inline) fn main f0 { - b0(v0: [Field; 2]): - inc_rc v0 - inc_rc v0 - inc_rc v0 - v2 = array_get v0, index u32 0 -> Field - inc_rc v0 - return v2 - } - "; - - let ssa = Ssa::from_str(src).unwrap(); - let main = ssa.main(); - - // The instruction count never includes the terminator instruction - assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5); - - let ssa = ssa.dead_instruction_elimination(); - assert_ssa_snapshot!(ssa, @r" - brillig(inline) fn main f0 { - b0(v0: [Field; 2]): - inc_rc v0 - v2 = array_get v0, index u32 0 -> Field - inc_rc v0 - return v2 - } - "); - } - - #[test] - fn do_not_remove_inc_rcs_for_arrays_in_terminator() { - let src = " - brillig(inline) fn main f0 { - b0(v0: [Field; 2]): - inc_rc v0 - inc_rc v0 - inc_rc v0 - v2 = array_get v0, index u32 0 -> Field - inc_rc v0 - return v0, v2 - } - "; - - let ssa = Ssa::from_str(src).unwrap(); - - let ssa = ssa.dead_instruction_elimination(); - assert_ssa_snapshot!(ssa, @r" - brillig(inline) fn main f0 { - b0(v0: [Field; 2]): - inc_rc v0 - v2 = array_get v0, index u32 0 -> Field - inc_rc v0 - return v0, v2 - } - "); - } - #[test] fn do_not_remove_inc_rc_if_used_as_call_arg() { // We do not want to remove inc_rc instructions on values diff --git a/compiler/noirc_evaluator/src/ssa/opt/die/prune_dead_parameters.rs b/compiler/noirc_evaluator/src/ssa/opt/die/prune_dead_parameters.rs index f212c4f68b8..7cc2503a8fc 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die/prune_dead_parameters.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die/prune_dead_parameters.rs @@ -240,6 +240,7 @@ mod tests { brillig(inline) predicate_pure fn main f0 { b0(v1: [[u1; 4]; 4]): v3 = array_get v1, index u32 0 -> [u1; 4] + inc_rc v3 v5 = array_get v3, index u32 3 -> u1 jmpif v5 then: b1, else: b2 b1(): diff --git a/tooling/nargo_cli/tests/execute.rs b/tooling/nargo_cli/tests/execute.rs index b703c2ea82b..cd0ba0f9510 100644 --- a/tooling/nargo_cli/tests/execute.rs +++ b/tooling/nargo_cli/tests/execute.rs @@ -28,6 +28,20 @@ mod tests { #[derive(Debug, Clone, Copy)] struct Inliner(pub i64); + // These tests fail with stack too deep errors when debug assertions are active + const IGNORED_BRILLIG_DEBUG_ASSERTIONS_TESTS: [&str; 1] = ["ski_calculus"]; + + const IGNORED_PEDANTIC_SOLVING_TESTS: [&str; 3] = [ + // TODO(https://github.com/noir-lang/noir/issues/8098): all of these are failing with: + // ``` + // Failed to solve program: + // \'Failed to solve blackbox function: embedded_curve_add, reason: Infinite input: embedded_curve_add(infinity, infinity)\' + // ``` + "execution_success/multi_scalar_mul", + "execution_success/regression_5045", + "execution_success/regression_7744", + ]; + fn setup_nargo( test_program_dir: &Path, test_command: &str, @@ -38,24 +52,19 @@ mod tests { nargo.arg("--program-dir").arg(test_program_dir); nargo.arg(test_command).arg("--force"); nargo.arg("--inliner-aggressiveness").arg(inliner_aggressiveness.0.to_string()); - // Allow more bytecode in exchange to catch illegal states. - nargo.arg("--enable-brillig-debug-assertions"); + let skip_brillig_debug_assertions = IGNORED_BRILLIG_DEBUG_ASSERTIONS_TESTS + .into_iter() + .any(|test_to_skip| test_program_dir.ends_with(test_to_skip)); + if !skip_brillig_debug_assertions { + // Allow more bytecode in exchange to catch illegal states. + nargo.arg("--enable-brillig-debug-assertions"); + } // Enable pedantic solving - let skip_pedantic_solving = [ - // TODO(https://github.com/noir-lang/noir/issues/8098): all of these are failing with: - // ``` - // Failed to solve program: - // \'Failed to solve blackbox function: embedded_curve_add, reason: Infinite input: embedded_curve_add(infinity, infinity)\' - // ``` - "execution_success/multi_scalar_mul", - "execution_success/regression_5045", - "execution_success/regression_7744", - ]; - if !skip_pedantic_solving + let skip_pedantic_solving = IGNORED_PEDANTIC_SOLVING_TESTS .into_iter() - .any(|test_to_skip| test_program_dir.ends_with(test_to_skip)) - { + .any(|test_to_skip| test_program_dir.ends_with(test_to_skip)); + if !skip_pedantic_solving { nargo.arg("--pedantic-solving"); }