diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 4ea42f1e48d..1e97e96f15b 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -304,7 +304,10 @@ impl DataFlowGraph { return InsertInstructionResult::InstructionRemoved; } - match simplify(&instruction, self, block, ctrl_typevars.clone(), call_stack) { + let simplify_result = + simplify(&instruction, self, block, ctrl_typevars.clone(), call_stack); + + match simplify_result { SimplifyResult::SimplifiedTo(simplification) => { InsertInstructionResult::SimplifiedTo(simplification) } @@ -315,8 +318,19 @@ impl DataFlowGraph { result @ (SimplifyResult::SimplifiedToInstruction(_) | SimplifyResult::SimplifiedToInstructionMultiple(_) | SimplifyResult::None) => { - let instructions = result.instructions(); - if instructions.is_none() { + let is_simplified = match &result { + SimplifyResult::SimplifiedToInstruction(i) => { + // Binary can simplify to itself instead of None + *i != instruction + } + SimplifyResult::SimplifiedToInstructionMultiple(is) => { + // Constrain can simplify to a multiple of with a single item of itself. + is.len() != 1 || is[0] != instruction + } + SimplifyResult::None => false, + _ => true, + }; + if !is_simplified { if let Some(id) = existing_id { if self[id] == instruction { // Just (re)insert into the block, no need to redefine. @@ -328,7 +342,7 @@ impl DataFlowGraph { } } } - let mut instructions = instructions.unwrap_or(vec![instruction]); + let mut instructions = result.instructions().unwrap_or(vec![instruction]); assert!( !instructions.is_empty(), "`SimplifyResult::SimplifiedToInstructionMultiple` must not return empty vector" diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify.rs index 9bc31898327..3d76c464385 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg/simplify.rs @@ -27,6 +27,7 @@ pub(crate) use call::constant_to_radix; /// Contains the result to Instruction::simplify, specifying how the instruction /// should be simplified. +#[derive(Debug)] pub(crate) enum SimplifyResult { /// Replace this function's result with the given value SimplifiedTo(ValueId), diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs index ab4a2895963..0b42ad922c2 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding/mod.rs @@ -14,7 +14,10 @@ //! //! This is the only pass which removes duplicated pure [`Instruction`]s however and so is needed when //! different blocks are merged, i.e. after the [`flatten_cfg`][super::flatten_cfg] pass. -use std::{collections::BTreeMap, io::Empty}; +use std::{ + collections::{BTreeMap, BTreeSet, HashSet}, + io::Empty, +}; use acvm::{FieldElement, acir::AcirField}; use iter_extended::vecmap; @@ -44,6 +47,10 @@ use interpret::try_interpret_call; use result_cache::{CacheResult, InstructionResultCache}; use simplification_cache::{ConstraintSimplificationCache, SimplificationCache}; +/// Maximum number of times we can trigger revisiting other blocks +/// after hoisting an instruction from a given block. +const MAX_REVISIT_TRIGGER_PER_BLOCK: usize = 3; + impl Ssa { /// Performs constant folding on each instruction. /// @@ -54,7 +61,7 @@ impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn fold_constants(mut self) -> Ssa { for function in self.functions.values_mut() { - function.constant_fold(false, &mut None); + function.constant_fold(false, true, &mut None); } self } @@ -67,7 +74,7 @@ impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn fold_constants_using_constraints(mut self) -> Ssa { for function in self.functions.values_mut() { - function.constant_fold(true, &mut None); + function.constant_fold(true, false, &mut None); } self } @@ -98,7 +105,7 @@ impl Ssa { }; for function in self.functions.values_mut() { - function.constant_fold(false, &mut interpreter); + function.constant_fold(false, true, &mut interpreter); } self @@ -111,9 +118,17 @@ impl Function { pub(crate) fn constant_fold( &mut self, use_constraint_info: bool, + revisit_hoisted: bool, interpreter: &mut Option>, ) { - let mut context = Context::new(use_constraint_info); + // Trying to revisit blocks when using constraint info can lead to constraints being removed. + // For example if we see `constrain v1 == 5`, then if we visit this block again it would be + // resolved to `constrain 5 == 5` + assert!( + !use_constraint_info || !revisit_hoisted, + "Don't revisit blocks when using constraints." + ); + let mut context = Context::new(use_constraint_info, revisit_hoisted); let mut dom = DominatorTree::with_function(self); context.block_queue.push_back(self.entry_block()); @@ -149,6 +164,16 @@ struct Context { /// ``` use_constraint_info: bool, + /// Whether to revisit blocks we hoist cached instruction from, looking for new + /// instructions that can be deduplicated and unlock further opportunities. + revisit_hoisted: bool, + + /// Number of times we triggered a revisit of an origin after hoisting from a block. + revisit_triggered: HashMap, + + /// All blocks we ever visited, to detect revisits. + blocks_visited: HashSet, + /// Contains sets of values which are constrained to be equivalent to each other. /// /// The mapping's structure is `side_effects_enabled_var => (constrained_value => simplified_value)`. @@ -168,9 +193,12 @@ struct Context { } impl Context { - fn new(use_constraint_info: bool) -> Self { + fn new(use_constraint_info: bool, revisit_hoisted: bool) -> Self { Self { use_constraint_info, + revisit_hoisted, + revisit_triggered: Default::default(), + blocks_visited: Default::default(), block_queue: Default::default(), constraint_simplification_mappings: Default::default(), cached_instruction_results: Default::default(), @@ -185,14 +213,20 @@ impl Context { block_id: BasicBlockId, interpreter: &mut Option>, ) { + let is_revisit = !self.blocks_visited.insert(block_id); let instructions = dfg[block_id].take_instructions(); // Default side effect condition variable with an enabled state. let mut side_effects_enabled_var = dfg.make_constant(FieldElement::one(), NumericType::bool()); + // Collect any block we hoisted instructions from. They will have instructions + // which can subsequently be deduplicated, potentially unlocking new opportunities. + let mut origins = CacheOrigins::default(); + for instruction_id in instructions { let instruction = &mut dfg[instruction_id]; + instruction.replace_values(&self.values_to_replace); self.fold_constants_into_instruction( @@ -201,6 +235,7 @@ impl Context { block_id, instruction_id, &mut side_effects_enabled_var, + &mut origins, interpreter, ); } @@ -227,9 +262,42 @@ impl Context { dfg[block_id].set_terminator(terminator); dfg.data_bus.map_values_mut(resolve_cache); - self.block_queue.extend(dfg[block_id].successors()); + // If we replaced an instruction in this block with something from the cache, + // we have to potentially update the values in all of the blocks this one dominates. + // For simplicity, just forget everything we visited, and go through successors. + if is_revisit && !origins.reused.is_empty() { + self.block_queue.clear_all_visited(); + } + + // If we hoisted from blocks, revisit them, to deduplicate what was hoisted, + // and potentially replace the values in the next instructions, then revisit + // this block, to take advantage of further hoisting opportunities. + // Do this before any other blocks in the queue, so we minimize revisits. + if self.revisit_hoisted + && !origins.hoisted.is_empty() + && self.can_revisit_after_hoist(block_id) + { + self.block_queue.clear_visited(&block_id); + self.block_queue.push_front(block_id); + for origin in origins.hoisted.into_iter().rev() { + self.block_queue.clear_visited(&origin); + self.block_queue.push_front(origin); + } + } else { + // Otherwise proceed to the (yet to be visited) successors. + self.block_queue.extend(dfg[block_id].successors()); + } + } + + /// Check if + fn can_revisit_after_hoist(&mut self, block_id: BasicBlockId) -> bool { + let num_triggers = self.revisit_triggered.entry(block_id).or_default(); + let can_trigger = *num_triggers < MAX_REVISIT_TRIGGER_PER_BLOCK; + *num_triggers += 1; + can_trigger } + #[allow(clippy::too_many_arguments)] fn fold_constants_into_instruction( &mut self, dfg: &mut DataFlowGraph, @@ -237,6 +305,7 @@ impl Context { mut block: BasicBlockId, id: InstructionId, side_effects_enabled_var: &mut ValueId, + origins: &mut CacheOrigins, interpreter: &mut Option>, ) { let constraint_simplification_mapping = @@ -254,7 +323,10 @@ impl Context { self.cached_instruction_results.get(dfg, dom, id, &instruction, predicate, block) { match cache_result { - CacheResult::Cached(cached) => { + CacheResult::Cached { instruction_id, .. } if instruction_id == id => { + // This is a revisit, we just need to reinsert the instruction as-is. + } + CacheResult::Cached { origin, results: cached, .. } => { // We track whether we may mutate `MakeArray` instructions before we deduplicate // them but we still need to issue an extra inc_rc in case they're mutated afterward. // @@ -276,14 +348,30 @@ impl Context { } self.values_to_replace.batch_insert(&old_results, cached); + + // Because we removed some instruction from this block, we will have to (re)visit its successors. + if origin != block { + origins.reused.insert(origin); + } + return; } - CacheResult::NeedToHoistToCommonBlock(dominator) => { + CacheResult::NeedToHoistToCommonBlock { dominator, .. } if dominator == block => { + // This is a revisit; we seem to want to hoist into self from a dominated block, which could cause an infinite loop of trying. + } + CacheResult::NeedToHoistToCommonBlock { origin, dominator } => { // Just change the block to insert in the common dominator instead. // This will only move the current instance of the instruction right now. // When constant folding is run a second time later on, it'll catch // that the previous instance can be deduplicated to this instance. + // Another effect is going to be that the cache should be updated to + // point at the dominator, so subsequent blocks can use the result. block = dominator; + + // Because we hoisted an instruction, we can revisit the origin to deduplicate. + origins.hoisted.insert(origin); + // Effectively we will be reusing a result from the dominator, becoming dependant on it. + origins.reused.insert(dominator); } } }; @@ -302,7 +390,15 @@ impl Context { self.values_to_replace.batch_insert(&old_results, &new_results); - self.cache_instruction(&instruction, new_results, dfg, *side_effects_enabled_var, block); + self.cache_instruction( + id, + &instruction, + new_results, + dfg, + dom, + *side_effects_enabled_var, + block, + ); // If we just inserted an `Instruction::EnableSideEffectsIf`, we need to update `side_effects_enabled_var` // so that we use the correct set of constrained values in future. @@ -358,11 +454,14 @@ impl Context { new_results } + #[allow(clippy::too_many_arguments)] fn cache_instruction( &mut self, + instruction_id: InstructionId, instruction: &Instruction, instruction_results: Vec, dfg: &DataFlowGraph, + dom: &mut DominatorTree, side_effects_enabled_var: ValueId, block: BasicBlockId, ) { @@ -397,7 +496,14 @@ impl Context { let array_get = Instruction::ArrayGet { array: instruction_results[0], index: *index }; // If we encounter an array_get for this address, we know what the result will be. - self.cached_instruction_results.cache(array_get, predicate, block, vec![*value]); + self.cached_instruction_results.cache( + dom, + instruction_id, + array_get, + predicate, + block, + vec![*value], + ); } self.cached_instruction_results @@ -415,6 +521,8 @@ impl Context { let predicate = self.cache_predicate(side_effects_enabled_var, instruction, dfg); // If we see this make_array again, we can reuse the current result. self.cached_instruction_results.cache( + dom, + instruction_id, instruction.clone(), predicate, block, @@ -471,6 +579,19 @@ fn resolve_cache( } } +/// Blocks from which we reused cached instruction results. +/// +/// In both cases we need to revisit some blocks after. +#[derive(Default)] +struct CacheOrigins { + /// Blocks that contain duplicates of instructions we hoisted into the dominator. + hoisted: BTreeSet, + /// Blocks that dominated this block and we could reuse their instructions as-is, + /// removing the instruction from the current block. + reused: BTreeSet, +} + +#[derive(Debug)] enum CanBeDeduplicated { /// This instruction has no side effects so we can substitute the results for those of the same instruction elsewhere. Always, @@ -825,6 +946,10 @@ mod test { let instructions = main.dfg[main.entry_block()].instructions(); assert_eq!(instructions.len(), 15); + // The `array_get` instructions after `enable_side_effects v1` is deduplicated + // with the one under `enable_side_effects v0` because it doesn't require a predicate, + // but the `array_set` is not, because it does require a predicate, and the subsequent + // `array_get` uses a different input, so it's not a duplicate of anything. let expected = " acir(inline) fn main f0 { b0(v0: u1, v1: u1, v2: [Field; 2]): @@ -933,8 +1058,8 @@ mod test { return } "; - let ssa = Ssa::from_str(src).unwrap(); - let ssa = ssa.fold_constants_using_constraints(); + let mut ssa = Ssa::from_str(src).unwrap(); + ssa.main_mut().constant_fold(true, false, &mut None); // v4 has been hoisted, although: // - v5 has not yet been removed since it was encountered earlier in the program @@ -963,6 +1088,394 @@ mod test { "); } + #[test] + fn repeatedly_hoist_and_deduplicate() { + // Repeating the same block 3x times. + let src = " + brillig(inline) predicate_pure fn main f0 { + b0(v0: u1, v1: i8): + v2 = allocate -> &mut i8 + store i8 0 at v2 + jmpif v0 then: b1, else: b2 + b1(): + v5 = unchecked_mul v1, i8 127 + v6 = cast v5 as u16 + v7 = truncate v6 to 8 bits, max_bit_size: 16 + v8 = cast v7 as i8 + store v8 at v2 + jmp b2() + b2(): + jmpif v0 then: b3, else: b4 + b3(): + v9 = unchecked_mul v1, i8 127 + v10 = cast v9 as u16 + v11 = truncate v10 to 8 bits, max_bit_size: 16 + v12 = cast v11 as i8 + store v12 at v2 + jmp b4() + b4(): + jmpif v0 then: b5, else: b6 + b5(): + v13 = unchecked_mul v1, i8 127 + v14 = cast v13 as u16 + v15 = truncate v14 to 8 bits, max_bit_size: 16 + v16 = cast v15 as i8 + store v16 at v2 + jmp b6() + b6(): + v17 = load v2 -> i8 + return v17 + } + "; + + let mut ssa = Ssa::from_str(src).unwrap(); + + // First demonstrate what happens if we don't revisit. + ssa.main_mut().constant_fold(false, false, &mut None); + + // 1. v9 is a duplicate of v5 -> hoisted to b0 + // 2. v13 is a duplicate of v9 -> immediately deduplicated because it's now in b0 + // 3. v14 is a duplicate of v10 -> hoisted to b2 + assert_ssa_snapshot!(ssa, @r" + brillig(inline) predicate_pure fn main f0 { + b0(v0: u1, v1: i8): + v2 = allocate -> &mut i8 + store i8 0 at v2 + v5 = unchecked_mul v1, i8 127 + jmpif v0 then: b1, else: b2 + b1(): + v6 = unchecked_mul v1, i8 127 + v7 = cast v6 as u16 + v8 = truncate v7 to 8 bits, max_bit_size: 16 + v9 = cast v8 as i8 + store v9 at v2 + jmp b2() + b2(): + v10 = cast v5 as u16 + jmpif v0 then: b3, else: b4 + b3(): + v11 = cast v5 as u16 + v12 = truncate v11 to 8 bits, max_bit_size: 16 + v13 = cast v12 as i8 + store v13 at v2 + jmp b4() + b4(): + jmpif v0 then: b5, else: b6 + b5(): + v14 = truncate v10 to 8 bits, max_bit_size: 16 + v15 = cast v14 as i8 + store v15 at v2 + jmp b6() + b6(): + v16 = load v2 -> i8 + return v16 + } + "); + + // Now with revisit. + let ssa = Ssa::from_str(src).unwrap(); + let ssa = ssa.fold_constants(); + + // All duplicates hoisted into b0. + assert_ssa_snapshot!(ssa, @r" + brillig(inline) predicate_pure fn main f0 { + b0(v0: u1, v1: i8): + v2 = allocate -> &mut i8 + store i8 0 at v2 + v5 = unchecked_mul v1, i8 127 + v6 = cast v5 as u16 + v7 = truncate v6 to 8 bits, max_bit_size: 16 + v8 = cast v7 as i8 + jmpif v0 then: b1, else: b2 + b1(): + store v8 at v2 + jmp b2() + b2(): + jmpif v0 then: b3, else: b4 + b3(): + store v8 at v2 + jmp b4() + b4(): + jmpif v0 then: b5, else: b6 + b5(): + store v8 at v2 + jmp b6() + b6(): + v9 = load v2 -> i8 + return v9 + } + "); + } + + #[test] + fn avoid_cycles_during_revisits() { + // We have program structure like this: + // `if a { A[(b[0].0 + b[0].0]) % 2 } else { A[(b[0].0 + b[0].0]) % 2 }` + // so the 'then' and the 'else' are the same and instructions are repeated within the block. + let src = r#" + g0 = make_array [] : [u1; 0] + g1 = make_array [] : [u1; 0] + g2 = make_array [g0, g1] : [[u1; 0]; 2] + + brillig(inline) predicate_pure fn main f0 { + b0(v3: u1, v4: [(u1, u1, u1, [u8; 2]); 4]): + jmpif v3 then: b1, else: b2 + b1(): + v20 = array_get v4, index u32 0 -> u1 + v21 = array_get v4, index u32 3 -> [u8; 2] + inc_rc v21 + v22 = cast v20 as u32 + v23 = array_get v4, index u32 0 -> u1 + v24 = array_get v4, index u32 3 -> [u8; 2] + inc_rc v24 + v25 = cast v23 as u32 + v26 = add v22, v25 + v27 = truncate v26 to 1 bits, max_bit_size: 32 + v28 = lt v27, u32 2 + constrain v28 == u1 1, "Index out of bounds" + v29 = array_get g2, index v27 -> [u1; 0] + inc_rc v29 + jmp b3(v29) + b2(): + v7 = array_get v4, index u32 0 -> u1 + v9 = array_get v4, index u32 3 -> [u8; 2] + inc_rc v9 + v10 = cast v7 as u32 + v11 = array_get v4, index u32 0 -> u1 + v12 = array_get v4, index u32 3 -> [u8; 2] + inc_rc v12 + v13 = cast v11 as u32 + v14 = add v10, v13 + v15 = truncate v14 to 1 bits, max_bit_size: 32 + v17 = lt v15, u32 2 + constrain v17 == u1 1, "Index out of bounds" + v19 = array_get g2, index v15 -> [u1; 0] + inc_rc v19 + jmp b3(v19) + b3(v5: [u1; 0]): + return v5 + } + "#; + + let mut ssa = Ssa::from_str(src).unwrap(); + ssa.main_mut().constant_fold(false, true, &mut None); + + // Duplicated array_get and cast are hoisted to b0. + assert_ssa_snapshot!(ssa, @r#" + g0 = make_array [] : [u1; 0] + g1 = make_array [] : [u1; 0] + g2 = make_array [g0, g1] : [[u1; 0]; 2] + + brillig(inline) predicate_pure fn main f0 { + b0(v3: u1, v4: [(u1, u1, u1, [u8; 2]); 4]): + v7 = array_get v4, index u32 0 -> u1 + v9 = array_get v4, index u32 3 -> [u8; 2] + v10 = cast v7 as u32 + jmpif v3 then: b1, else: b2 + b1(): + inc_rc v9 + inc_rc v9 + v17 = add v10, v10 + v18 = truncate v17 to 1 bits, max_bit_size: 32 + v19 = lt v18, u32 2 + constrain v19 == u1 1, "Index out of bounds" + v20 = array_get g2, index v18 -> [u1; 0] + inc_rc v20 + jmp b3(v20) + b2(): + inc_rc v9 + inc_rc v9 + v11 = add v10, v10 + v12 = truncate v11 to 1 bits, max_bit_size: 32 + v14 = lt v12, u32 2 + constrain v14 == u1 1, "Index out of bounds" + v16 = array_get g2, index v12 -> [u1; 0] + inc_rc v16 + jmp b3(v16) + b3(v5: [u1; 0]): + return v5 + } + "#); + } + + #[test] + fn avoid_unmapped_instructions_during_revisit() { + // We have program calling `lambdas_in_array_literal(x)` and `lambdas_in_array_literal(x-1)`, + // with 1 of the 2 potential lambdas called in `lambdas_in_array_literal` based on the index. + let src = r#" + brillig(inline) predicate_pure fn main f0 { + b0(v0: u32): + v8 = sub v0, u32 1 + v13 = make_array [Field 2, Field 3, Field 4, Field 5] : [(Field, Field); 2] + v15 = lt v8, u32 2 + constrain v15 == u1 1, "Index out of bounds" + v17 = unchecked_mul v8, u32 2 + v18 = unchecked_add v17, u32 1 + v19 = array_get v13, index v18 -> Field + v20 = eq v19, Field 2 + jmpif v20 then: b1, else: b2 + b1(): + v32 = make_array b"ABC" + jmp b3(v32) + b2(): + v21 = eq v19, Field 3 + jmpif v21 then: b4, else: b5 + b3(v1: [u8; 3]): + v33 = make_array [Field 2, Field 3, Field 4, Field 5] : [(Field, Field); 2] + v34 = lt v0, u32 2 + constrain v34 == u1 1, "Index out of bounds" + v35 = unchecked_mul v0, u32 2 + v36 = unchecked_add v35, u32 1 + v37 = array_get v33, index v36 -> Field + v38 = eq v37, Field 2 + jmpif v38 then: b6, else: b7 + b4(): + v31 = make_array b"ABC" + jmp b8(v31) + b5(): + v22 = eq v19, Field 4 + jmpif v22 then: b9, else: b10 + b6(): + v44 = make_array b"ABC" + jmp b11(v44) + b7(): + v39 = eq v37, Field 3 + jmpif v39 then: b12, else: b13 + b8(v2: [u8; 3]): + jmp b3(v2) + b9(): + v27 = make_array b"DEF" + jmp b19() + b19(): + inc_rc v27 + jmp b14(v27) + b10(): + constrain v19 == Field 5 + v26 = make_array b"DEF" + jmp b20() + b20(): + inc_rc v26 + jmp b14(v26) + b11(v3: [u8; 3]): + return v1, v3 + b12(): + v43 = make_array b"ABC" + jmp b15(v43) + b13(): + v40 = eq v37, Field 4 + jmpif v40 then: b16, else: b17 + b14(v4: [u8; 3]): + jmp b8(v4) + b15(v5: [u8; 3]): + jmp b11(v5) + b16(): + v42 = make_array b"DEF" + jmp b18(v42) + b17(): + constrain v37 == Field 5 + v41 = make_array b"DEF" + jmp b18(v41) + b18(v6: [u8; 3]): + jmp b15(v6) + } + "#; + + let mut ssa = Ssa::from_str(src).unwrap(); + ssa.main_mut().constant_fold(false, true, &mut None); + + // The hoisting of "DEF" will happen in multiple stages: + // * Appears first in b9 + // * Duplicate of b9 in b10 -> hoisted from b10 to b5 + // * Revisited in b9 -> reused from b5 + // * Duplicate of b5 in b16 -> hoisted from b16 to b0 + // * Revisited in b5 -> reused from b0 + // * Duplicate of b0 in b17 -> reused from b0 + // The crucial bit is that b9 and b10 has to be revisited as well, as they contain a reuse from b5, + // which needs to be updated to point at b0 instead, otherwise trying to normalize the IDs will panic. + // Another tripwire is b19 and b20: they refer to values in b9 and b10, and so if we revisit those + // and update the IDs after hoisting from b5 to b0, we also have to revisit their successors, even + // though they did not interact with the cache per-se. + + // All make_array hoisted into b0 + assert_ssa_snapshot!(ssa, @r#" + brillig(inline) predicate_pure fn main f0 { + b0(v0: u32): + v8 = sub v0, u32 1 + v13 = make_array [Field 2, Field 3, Field 4, Field 5] : [(Field, Field); 2] + v15 = lt v8, u32 2 + constrain v15 == u1 1, "Index out of bounds" + v17 = unchecked_mul v8, u32 2 + v18 = unchecked_add v17, u32 1 + v19 = array_get v13, index v18 -> Field + v20 = eq v19, Field 2 + v24 = make_array b"ABC" + v28 = make_array b"DEF" + jmpif v20 then: b1, else: b2 + b1(): + inc_rc v24 + jmp b3(v24) + b2(): + v29 = eq v19, Field 3 + jmpif v29 then: b4, else: b5 + b3(v1: [u8; 3]): + inc_rc v13 + v31 = lt v0, u32 2 + constrain v31 == u1 1, "Index out of bounds" + v32 = unchecked_mul v0, u32 2 + v33 = unchecked_add v32, u32 1 + v34 = array_get v13, index v33 -> Field + v35 = eq v34, Field 2 + jmpif v35 then: b6, else: b7 + b4(): + jmp b8(v24) + b5(): + v30 = eq v19, Field 4 + inc_rc v28 + jmpif v30 then: b9, else: b11 + b6(): + inc_rc v24 + jmp b13(v24) + b7(): + v36 = eq v34, Field 3 + jmpif v36 then: b14, else: b15 + b8(v2: [u8; 3]): + jmp b3(v2) + b9(): + inc_rc v28 + jmp b10() + b10(): + inc_rc v28 + jmp b16(v28) + b11(): + constrain v19 == Field 5 + jmp b12() + b12(): + inc_rc v28 + jmp b16(v28) + b13(v3: [u8; 3]): + return v1, v3 + b14(): + inc_rc v24 + jmp b17(v24) + b15(): + v37 = eq v34, Field 4 + jmpif v37 then: b18, else: b19 + b16(v4: [u8; 3]): + jmp b8(v4) + b17(v5: [u8; 3]): + jmp b13(v5) + b18(): + jmp b20(v28) + b19(): + constrain v34 == Field 5 + inc_rc v28 + jmp b20(v28) + b20(v6: [u8; 3]): + jmp b17(v6) + } + "#); + } + #[test] fn inlines_brillig_call_without_arguments() { let src = " @@ -1496,19 +2009,23 @@ mod test { b0(v0: Field): v1 = call to_le_radix(v0, u32 256) -> [u8; 2] v2 = call to_le_radix(v0, u32 256) -> [u8; 3] - v3 = call to_le_radix(v0, u32 256) -> [u8; 2] + v3 = call to_le_radix(v0, u32 256) -> [u8; 3] + v4 = call to_le_radix(v0, u32 256) -> [u8; 2] return } "; let ssa = Ssa::from_str(src).unwrap(); + // These intrinsic calls can only be deduplicated when using constraints. let ssa = ssa.fold_constants_using_constraints(); + // Only the first one is cached at the moment. assert_ssa_snapshot!(ssa, @r" brillig(inline) predicate_pure fn main f0 { b0(v0: Field): v3 = call to_le_radix(v0, u32 256) -> [u8; 2] v4 = call to_le_radix(v0, u32 256) -> [u8; 3] + v5 = call to_le_radix(v0, u32 256) -> [u8; 3] inc_rc v3 return } diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding/result_cache.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding/result_cache.rs index 154e416729f..e3c95908fb4 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding/result_cache.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding/result_cache.rs @@ -46,14 +46,14 @@ impl InstructionResultCache { // This is a hacky solution to https://github.com/noir-lang/noir/issues/9477 // We explicitly check that the cached result values are of the same type as expected by the instruction // being checked against the cache and reject if they differ. - if let CacheResult::Cached(results) = results { + // They can also be the exact same value after a re-visit, which we don't want as it creates a cycle. + if let CacheResult::Cached { results, .. } = results { let old_results = dfg.instruction_results(id).to_vec(); results.len() == old_results.len() - && old_results - .iter() - .zip(results.iter()) - .all(|(old, new)| dfg.type_of_value(*old) == dfg.type_of_value(*new)) + && old_results.iter().zip(results.iter()).all(|(old, new)| { + new != old && dfg.type_of_value(*old) == dfg.type_of_value(*new) + }) } else { true } @@ -62,12 +62,19 @@ impl InstructionResultCache { pub(super) fn cache( &mut self, + dom: &mut DominatorTree, + instruction_id: InstructionId, instruction: Instruction, predicate: Option, block: BasicBlockId, results: Vec, ) { - self.0.entry(instruction).or_default().entry(predicate).or_default().cache(block, results); + self.0.entry(instruction).or_default().entry(predicate).or_default().cache( + block, + dom, + instruction_id, + results, + ); } pub(super) fn remove( @@ -151,15 +158,26 @@ impl InstructionResultCache { /// Records the results of all duplicate [`Instruction`]s along with the blocks in which they sit. /// /// For more information see [`InstructionResultCache`]. -#[derive(Default)] +#[derive(Default, Debug)] pub(super) struct ResultCache { - result: Option<(BasicBlockId, Vec)>, + result: Option<(BasicBlockId, InstructionId, Vec)>, } impl ResultCache { /// Records that an `Instruction` in block `block` produced the result values `results`. - fn cache(&mut self, block: BasicBlockId, results: Vec) { - if self.result.is_none() { - self.result = Some((block, results)); + fn cache( + &mut self, + block: BasicBlockId, + dom: &mut DominatorTree, + instruction_id: InstructionId, + results: Vec, + ) { + let overwrite = match self.result { + None => true, + Some((origin, _, _)) => origin != block && dom.dominates(block, origin), + }; + + if overwrite { + self.result = Some((block, instruction_id, results)); } } @@ -175,13 +193,17 @@ impl ResultCache { dom: &mut DominatorTree, has_side_effects: bool, ) -> Option { - self.result.as_ref().and_then(|(origin_block, results)| { - if dom.dominates(*origin_block, block) { - Some(CacheResult::Cached(results)) + self.result.as_ref().and_then(|(origin, instruction_id, results)| { + if dom.dominates(*origin, block) { + Some(CacheResult::Cached { + origin: *origin, + instruction_id: *instruction_id, + results, + }) } else if !has_side_effects { // Insert a copy of this instruction in the common dominator - let dominator = dom.common_dominator(*origin_block, block); - Some(CacheResult::NeedToHoistToCommonBlock(dominator)) + let dominator = dom.common_dominator(*origin, block); + Some(CacheResult::NeedToHoistToCommonBlock { origin: *origin, dominator }) } else { None } @@ -191,6 +213,17 @@ impl ResultCache { #[derive(Debug)] pub(super) enum CacheResult<'a> { - Cached(&'a [ValueId]), - NeedToHoistToCommonBlock(BasicBlockId), + /// The result of an earlier instruction can be readily reused, because it was found + /// in a block that dominates the one where the current instruction is. We can drop + /// the current instruction and redefine its results in terms of the existing values. + Cached { origin: BasicBlockId, instruction_id: InstructionId, results: &'a [ValueId] }, + /// We found an identical instruction in a non-dominating block, so we cannot directly + /// reuse its results, because they are not visible in the current block. However, we + /// can hoist the instruction into the common dominator, and deduplicate later. + NeedToHoistToCommonBlock { + /// The block in which we found the identical instruction. + origin: BasicBlockId, + /// The common dominator where we can hoist the current instruction. + dominator: BasicBlockId, + }, } diff --git a/compiler/noirc_evaluator/src/ssa/visit_once_deque.rs b/compiler/noirc_evaluator/src/ssa/visit_once_deque.rs index de54d795c54..4ec33c78c17 100644 --- a/compiler/noirc_evaluator/src/ssa/visit_once_deque.rs +++ b/compiler/noirc_evaluator/src/ssa/visit_once_deque.rs @@ -1,53 +1,89 @@ -use std::collections::{HashSet, VecDeque}; +use std::{ + collections::{HashSet, VecDeque}, + hash::Hash, +}; use crate::ssa::ir::basic_block::BasicBlockId; -/// A wrapper around `VecDeque` to ensure that we never process the same [`BasicBlockId`] more than once. -#[derive(Debug, Default)] -pub(crate) struct VisitOnceDeque { - visited_blocks: HashSet, - block_queue: VecDeque, +/// A wrapper around `VecDeque` to ensure that we never process the same item more than once. +#[derive(Debug)] +pub(crate) struct VisitOnceDeque { + visited_blocks: HashSet, + block_queue: VecDeque, } -impl VisitOnceDeque { - pub(crate) fn push_back(&mut self, item: BasicBlockId) { +impl VisitOnceDeque { + pub(crate) fn new() -> Self { + Self { visited_blocks: HashSet::new(), block_queue: VecDeque::new() } + } + + pub(crate) fn push_back(&mut self, item: T) { self.block_queue.push_back(item); } - pub(crate) fn extend>(&mut self, items: T) { + pub(crate) fn push_front(&mut self, item: T) { + self.block_queue.push_front(item); + } + + pub(crate) fn extend(&mut self, items: impl IntoIterator) { self.block_queue.extend(items); } - pub(crate) fn pop_front(&mut self) -> Option { + pub(crate) fn pop_front(&mut self) -> Option { let item = self.block_queue.pop_front()?; if self.visited_blocks.insert(item) { Some(item) } else { self.pop_front() } } - pub(crate) fn pop_back(&mut self) -> Option { + pub(crate) fn pop_back(&mut self) -> Option { let item = self.block_queue.pop_back()?; if self.visited_blocks.insert(item) { Some(item) } else { self.pop_back() } } + + /// Forget whether we visited a specific item. + pub(crate) fn clear_visited(&mut self, item: &T) { + self.visited_blocks.remove(item); + } + + /// Forget all visits. + pub(crate) fn clear_all_visited(&mut self) { + self.visited_blocks.clear(); + } +} + +impl Default for VisitOnceDeque { + fn default() -> Self { + Self::new() + } } #[cfg(test)] mod tests { - use crate::ssa::{ir::basic_block::BasicBlockId, visit_once_deque::VisitOnceDeque}; + use crate::ssa::visit_once_deque::VisitOnceDeque; #[test] fn does_not_return_duplicates() { let mut deque = VisitOnceDeque::default(); - deque.extend([ - BasicBlockId::test_new(0), - BasicBlockId::test_new(1), - BasicBlockId::test_new(2), - BasicBlockId::test_new(0), - BasicBlockId::test_new(1), - ]); - - assert_eq!(deque.pop_front(), Some(BasicBlockId::test_new(0))); - assert_eq!(deque.pop_back(), Some(BasicBlockId::test_new(1))); - assert_eq!(deque.pop_front(), Some(BasicBlockId::test_new(2))); + deque.extend([0, 1, 2, 0, 1]); + + assert_eq!(deque.pop_front(), Some(0)); + assert_eq!(deque.pop_back(), Some(1)); + assert_eq!(deque.pop_front(), Some(2)); assert_eq!(deque.pop_front(), None); assert_eq!(deque.pop_back(), None); } + + #[test] + fn can_clear_visited() { + let mut deque = VisitOnceDeque::default(); + deque.extend([0, 1, 2]); + + assert_eq!(deque.pop_front(), Some(0)); + deque.push_front(0); + assert_eq!(deque.pop_front(), Some(1)); + deque.clear_visited(&1); + deque.push_front(1); + assert_eq!(deque.pop_front(), Some(1)); + assert_eq!(deque.pop_front(), Some(2)); + assert_eq!(deque.pop_front(), None); + } }