From b5508281837b784b9f90b55aef57fb09dbcaf3ba Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 11 Mar 2025 13:52:33 +0000 Subject: [PATCH 01/24] control dependent LICM, only checking whether there are any conditionals between the current block and the pre-header, can be made more advanced --- compiler/noirc_evaluator/src/ssa/ir/cfg.rs | 5 - compiler/noirc_evaluator/src/ssa/ir/dom.rs | 8 +- .../src/ssa/opt/loop_invariant.rs | 137 +++++++++++++++++- .../noirc_evaluator/src/ssa/opt/unrolling.rs | 2 +- cspell.json | 13 +- .../loop_invariant_regression/src/main.nr | 2 +- 6 files changed, 149 insertions(+), 18 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs index 8badcc5fa29..1c1da4bf2f0 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs @@ -89,10 +89,6 @@ impl ControlFlowGraph { /// Add a directed edge making `from` a predecessor of `to`. fn add_edge(&mut self, from: BasicBlockId, to: BasicBlockId) { let predecessor_node = self.data.entry(from).or_default(); - assert!( - predecessor_node.successors.len() < 2, - "ICE: A cfg node cannot have more than two successors" - ); predecessor_node.successors.insert(to); let successor_node = self.data.entry(to).or_default(); successor_node.predecessors.insert(from); @@ -125,7 +121,6 @@ impl ControlFlowGraph { } /// Reverse the control flow graph - #[cfg(test)] pub(crate) fn reverse(&self) -> Self { let mut reversed_cfg = ControlFlowGraph::default(); diff --git a/compiler/noirc_evaluator/src/ssa/ir/dom.rs b/compiler/noirc_evaluator/src/ssa/ir/dom.rs index c9e5a95650c..05b139b6efc 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dom.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dom.rs @@ -178,10 +178,10 @@ impl DominatorTree { /// "Simple, Fast Dominator Algorithm." fn compute_dominator_tree(&mut self, cfg: &ControlFlowGraph, post_order: &PostOrder) { // We'll be iterating over a reverse post-order of the CFG, skipping the entry block. - let (entry_block_id, entry_free_post_order) = post_order - .as_slice() - .split_last() - .expect("ICE: functions always have at least one block"); + let Some((entry_block_id, entry_free_post_order)) = post_order.as_slice().split_last() + else { + return; + }; // Do a first pass where we assign reverse post-order indices to all reachable nodes. The // entry block will be the only node with no immediate dominator. diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index eb65fa077ab..68e43f13c76 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -14,6 +14,8 @@ use crate::ssa::{ Ssa, ir::{ basic_block::BasicBlockId, + cfg::ControlFlowGraph, + dom::DominatorTree, function::Function, function_inserter::FunctionInserter, instruction::{ @@ -103,10 +105,20 @@ struct LoopInvariantContext<'f> { // This stores the current loop's pre-header block. // It is wrapped in an Option as our SSA `Id` does not allow dummy values. current_pre_header: Option, + + post_dom: DominatorTree, + + cfg: ControlFlowGraph, + + current_block_control_dependent: bool, } impl<'f> LoopInvariantContext<'f> { fn new(function: &'f mut Function) -> Self { + let cfg = ControlFlowGraph::with_function(function); + let reversed_cfg = cfg.reverse(); + let post_order = PostOrder::with_cfg(&reversed_cfg); + let post_dom = DominatorTree::with_cfg_and_post_order(&reversed_cfg, &post_order); Self { inserter: FunctionInserter::new(function), defined_in_loop: HashSet::default(), @@ -114,6 +126,9 @@ impl<'f> LoopInvariantContext<'f> { current_induction_variables: HashMap::default(), outer_induction_variables: HashMap::default(), current_pre_header: None, + post_dom, + cfg, + current_block_control_dependent: false, } } @@ -125,6 +140,23 @@ impl<'f> LoopInvariantContext<'f> { self.set_values_defined_in_loop(loop_); for block in loop_.blocks.iter() { + let mut all_predecessors = + Loop::find_blocks_in_loop(self.pre_header(), *block, &self.cfg).blocks; + all_predecessors.remove(block); + all_predecessors.remove(&self.pre_header()); + + // Need to accurately determine whether the current block is dependent on any blocks between + // the current block and the loop header + for predecessor in all_predecessors { + if predecessor == loop_.header { + continue; + } + if self.is_control_dependent(predecessor, *block) { + self.current_block_control_dependent = true; + break; + } + } + for instruction_id in self.inserter.function.dfg[*block].take_instructions() { self.transform_to_unchecked_from_loop_bounds(instruction_id); @@ -159,11 +191,34 @@ impl<'f> LoopInvariantContext<'f> { } self.extend_values_defined_in_loop_and_invariants(instruction_id, hoist_invariant); } + + self.current_block_control_dependent = false; } self.set_induction_var_bounds(loop_, false); } + /// Jeanne Ferrante, Karl J. Ottenstein, and Joe D. Warren. 1987. + /// The program dependence graph and its use in optimization. ACM + /// Trans. Program. Lang. Syst. 9, 3 (July 1987), 319–349. + /// https://doi.org/10.1145/24039.24041 + /// + /// Definition 3 from the linked paper above. + fn is_control_dependent(&mut self, parent_block: BasicBlockId, block: BasicBlockId) -> bool { + let mut all_predecessors = Loop::find_blocks_in_loop(parent_block, block, &self.cfg).blocks; + all_predecessors.remove(&parent_block); + all_predecessors.remove(&block); + + let mut first_control_cond = false; + for predecessor in all_predecessors { + if self.post_dom.dominates(predecessor, block) { + first_control_cond = true; + break; + } + } + first_control_cond && !self.post_dom.dominates(block, parent_block) + } + /// Gather the variables declared within the loop fn set_values_defined_in_loop(&mut self, loop_: &Loop) { // Clear any values that may be defined in previous loops, as the context is per function. @@ -228,6 +283,8 @@ impl<'f> LoopInvariantContext<'f> { let can_be_deduplicated = instruction.can_be_deduplicated(self.inserter.function, false) || matches!(instruction, Instruction::MakeArray { .. }) + // TODO: improve this control dependence check + || (!matches!(instruction, Instruction::Call { .. }) && instruction.requires_acir_gen_predicate(&self.inserter.function.dfg) && !self.current_block_control_dependent) || self.can_be_deduplicated_from_loop_bound(&instruction); is_loop_invariant && can_be_deduplicated @@ -901,7 +958,7 @@ mod test { #[test] fn do_not_hoist_unsafe_div() { // This test is similar to `nested_loop_invariant_code_motion`, the operation - // in question we are trying to hoist is `v9 = div i32 10, v0`. + // in question we are trying to hoist is `v7 = div i32 10, v0`. // Check that the lower bound of the outer loop it checked and that we not // hoist an operation that can potentially error with a division by zero. let src = " @@ -995,3 +1052,81 @@ mod test { assert_normalized_ssa_equals(ssa, expected); } } + +#[cfg(test)] +mod control_dependence { + use crate::ssa::{opt::assert_normalized_ssa_equals, ssa_gen::Ssa}; + + #[test] + fn do_not_hoist_unsafe_mul_in_control_dependent_block() { + let src = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v4 = eq v0, u32 5 + jmp b1(u32 0) + b1(v2: u32): + v7 = lt v2, u32 4 + jmpif v7 then: b2, else: b3 + b2(): + jmpif v4 then: b4, else: b5 + b3(): + return + b4(): + v8 = mul v0, v1 + constrain v8 == u32 12 + jmp b5() + b5(): + v11 = unchecked_add v2, u32 1 + jmp b1(v11) + } + "; + + let ssa = Ssa::from_str(src).unwrap(); + + let ssa = ssa.loop_invariant_code_motion(); + assert_normalized_ssa_equals(ssa, src); + } + + #[test] + fn hoist_safe_mul_that_is_not_control_dependent() { + let src = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + jmp b1(u32 0) + b1(v2: u32): + v3 = lt v2, u32 4 + jmpif v3 then: b2, else: b3 + b2(): + v6 = mul v0, v1 + v7 = mul v6, v0 + constrain v7 == u32 12 + v10 = unchecked_add v2, u32 1 + jmp b1(v10) + b3(): + return + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let ssa = ssa.loop_invariant_code_motion(); + + let expected = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v3 = mul v0, v1 + v4 = mul v3, v0 + jmp b1(u32 0) + b1(v2: u32): + v7 = lt v2, u32 4 + jmpif v7 then: b2, else: b3 + b2(): + constrain v4 == u32 12 + v10 = unchecked_add v2, u32 1 + jmp b1(v10) + b3(): + return + } + "; + + assert_normalized_ssa_equals(ssa, expected); + } +} diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 5b7e34c8e0d..e2f896c801b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -253,7 +253,7 @@ impl Loops { impl Loop { /// Return each block that is in a loop starting in the given header block. /// Expects back_edge_start -> header to be the back edge of the loop. - fn find_blocks_in_loop( + pub(crate) fn find_blocks_in_loop( header: BasicBlockId, back_edge_start: BasicBlockId, cfg: &ControlFlowGraph, diff --git a/cspell.json b/cspell.json index 8093ee50816..fb593a2ab46 100644 --- a/cspell.json +++ b/cspell.json @@ -93,6 +93,8 @@ "endianness", "envrc", "EXPONENTIATE", + "Ferrante", + "flamegraph", "Flamegraph", "flamegraphs", "flate", @@ -122,10 +124,10 @@ "impls", "indexmap", "injective", - "interners", "Inlines", "instrumenter", "interner", + "interners", "intrinsics", "jmp", "jmpif", @@ -139,6 +141,7 @@ "krate", "libc", "Linea", + "lookback", "losslessly", "lvalue", "Maddiaa", @@ -173,8 +176,9 @@ "nomicfoundation", "noncanonical", "nouner", - "oneshot", "oneof", + "oneshot", + "Ottenstein", "overflowing", "pedersen", "peekable", @@ -268,10 +272,7 @@ "whitespaces", "YOLO", "zkhash", - "zshell", - "flamegraph", - "flamegraphs", - "lookback" + "zshell" ], "ignorePaths": [ "./**/node_modules/**", diff --git a/test_programs/execution_success/loop_invariant_regression/src/main.nr b/test_programs/execution_success/loop_invariant_regression/src/main.nr index d126b7729c1..e672f5ab007 100644 --- a/test_programs/execution_success/loop_invariant_regression/src/main.nr +++ b/test_programs/execution_success/loop_invariant_regression/src/main.nr @@ -19,7 +19,7 @@ fn simple_loop(upper_bound: u32, x: u32, y: u32) { fn loop_with_predicate(upper_bound: u32, x: u32, y: u32) { for _ in 0..upper_bound { if x == 5 { - let mut z = U32_MAX * y; + let mut z = x * y; assert_eq(z, 12); } } From 0d8a0b25a7f1117b23b660c99ca4a5fcf6a5d559 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 11 Mar 2025 14:42:25 +0000 Subject: [PATCH 02/24] revert test change --- .../execution_success/loop_invariant_regression/src/main.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test_programs/execution_success/loop_invariant_regression/src/main.nr b/test_programs/execution_success/loop_invariant_regression/src/main.nr index e672f5ab007..d126b7729c1 100644 --- a/test_programs/execution_success/loop_invariant_regression/src/main.nr +++ b/test_programs/execution_success/loop_invariant_regression/src/main.nr @@ -19,7 +19,7 @@ fn simple_loop(upper_bound: u32, x: u32, y: u32) { fn loop_with_predicate(upper_bound: u32, x: u32, y: u32) { for _ in 0..upper_bound { if x == 5 { - let mut z = x * y; + let mut z = U32_MAX * y; assert_eq(z, 12); } } From 5def4427a32579beb4ad2d1a5b905c2d33928332 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 11 Mar 2025 15:02:12 +0000 Subject: [PATCH 03/24] fixup do_not_hoist_unsafe_div test --- .../src/ssa/opt/loop_invariant.rs | 124 +++++++++++------- 1 file changed, 73 insertions(+), 51 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 68e43f13c76..ece56ec4f9e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -957,32 +957,44 @@ mod test { #[test] fn do_not_hoist_unsafe_div() { - // This test is similar to `nested_loop_invariant_code_motion`, the operation - // in question we are trying to hoist is `v7 = div i32 10, v0`. - // Check that the lower bound of the outer loop it checked and that we not + // This test is similar to `nested_loop_invariant_code_motion`, except that + // the loop logic is under a dynamic predicate. + // Divisions are only reliant upon predicates and do not have other side effects. + // If an unsafe division occurs in a loop block that is not control dependent, + // we can still safely hoist that division as that instruction is always going to be hit. + // Thus, we place the unsafe division under a predicate to ensure that we are testing + // division hoisting based upon loop bounds and nothing else. + // + // The operation in question we are trying to hoist is `v12 = div u32 10, v1`. + // Check that the lower bound of the outer loop is checked and that we do not // hoist an operation that can potentially error with a division by zero. let src = " brillig(inline) fn main f0 { - b0(): - jmp b1(i32 0) - b1(v0: i32): - v4 = lt v0, i32 4 - jmpif v4 then: b3, else: b2 + b0(v0: u32): + v4 = eq v0, u32 5 + jmp b1(u32 0) + b1(v1: u32): + v7 = lt v1, u32 4 + jmpif v7 then: b2, else: b3 b2(): - return + jmp b4(u32 0) b3(): - jmp b4(i32 0) - b4(v1: i32): - v5 = lt v1, i32 4 - jmpif v5 then: b6, else: b5 + return + b4(v2: u32): + v8 = lt v2, u32 4 + jmpif v8 then: b5, else: b6 b5(): - v11 = unchecked_add v0, i32 1 - jmp b1(v11) + jmpif v4 then: b7, else: b8 b6(): - v7 = div i32 10, v0 - constrain v7 == i32 6 - v10 = unchecked_add v1, i32 1 - jmp b4(v10) + v10 = unchecked_add v1, u32 1 + jmp b1(v10) + b7(): + v12 = div u32 10, v1 + constrain v12 == u32 6 + jmp b8() + b8(): + v14 = unchecked_add v2, u32 1 + jmp b4(v14) } "; @@ -998,26 +1010,31 @@ mod test { // in this test starts with a lower bound of `1`. let src = " brillig(inline) fn main f0 { - b0(): - jmp b1(i32 1) - b1(v0: i32): - v4 = lt v0, i32 4 - jmpif v4 then: b3, else: b2 + b0(v0: u32): + v4 = eq v0, u32 5 + jmp b1(u32 1) + b1(v1: u32): + v7 = lt v1, u32 4 + jmpif v7 then: b2, else: b3 b2(): - return + jmp b4(u32 0) b3(): - jmp b4(i32 0) - b4(v1: i32): - v5 = lt v1, i32 4 - jmpif v5 then: b6, else: b5 + return + b4(v2: u32): + v9 = lt v2, u32 4 + jmpif v9 then: b5, else: b6 b5(): - v7 = unchecked_add v0, i32 1 - jmp b1(v7) + jmpif v4 then: b7, else: b8 b6(): - v9 = div i32 10, v0 - constrain v9 == i32 6 - v11 = unchecked_add v1, i32 1 - jmp b4(v11) + v10 = unchecked_add v1, u32 1 + jmp b1(v10) + b7(): + v12 = div u32 10, v1 + constrain v12 == u32 6 + jmp b8() + b8(): + v14 = unchecked_add v2, u32 1 + jmp b4(v14) } "; @@ -1026,26 +1043,31 @@ mod test { let ssa = ssa.loop_invariant_code_motion(); let expected = " brillig(inline) fn main f0 { - b0(): - jmp b1(i32 1) - b1(v0: i32): - v4 = lt v0, i32 4 - jmpif v4 then: b3, else: b2 + b0(v0: u32): + v4 = eq v0, u32 5 + jmp b1(u32 1) + b1(v1: u32): + v7 = lt v1, u32 4 + jmpif v7 then: b2, else: b3 b2(): - return + v9 = div u32 10, v1 + jmp b4(u32 0) b3(): - v6 = div i32 10, v0 - jmp b4(i32 0) - b4(v1: i32): - v8 = lt v1, i32 4 - jmpif v8 then: b6, else: b5 + return + b4(v2: u32): + v11 = lt v2, u32 4 + jmpif v11 then: b5, else: b6 b5(): - v11 = unchecked_add v0, i32 1 - jmp b1(v11) + jmpif v4 then: b7, else: b8 b6(): - constrain v6 == i32 6 - v10 = unchecked_add v1, i32 1 - jmp b4(v10) + v12 = unchecked_add v1, u32 1 + jmp b1(v12) + b7(): + constrain v9 == u32 6 + jmp b8() + b8(): + v14 = unchecked_add v2, u32 1 + jmp b4(v14) } "; From 2ec7af99303a2e5d512305f06f18793e97b0eb35 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 11 Mar 2025 15:59:59 +0000 Subject: [PATCH 04/24] block checking licm control dependence in acir --- .../src/ssa/opt/loop_invariant.rs | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index ece56ec4f9e..4fada0a5cf7 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -140,21 +140,25 @@ impl<'f> LoopInvariantContext<'f> { self.set_values_defined_in_loop(loop_); for block in loop_.blocks.iter() { - let mut all_predecessors = - Loop::find_blocks_in_loop(self.pre_header(), *block, &self.cfg).blocks; - all_predecessors.remove(block); - all_predecessors.remove(&self.pre_header()); - - // Need to accurately determine whether the current block is dependent on any blocks between - // the current block and the loop header - for predecessor in all_predecessors { - if predecessor == loop_.header { - continue; - } - if self.is_control_dependent(predecessor, *block) { - self.current_block_control_dependent = true; - break; + if self.inserter.function.dfg.runtime().is_brillig() { + let mut all_predecessors = + Loop::find_blocks_in_loop(self.pre_header(), *block, &self.cfg).blocks; + all_predecessors.remove(block); + all_predecessors.remove(&self.pre_header()); + + // Need to accurately determine whether the current block is dependent on any blocks between + // the current block and the loop header + for predecessor in all_predecessors { + if predecessor == loop_.header { + continue; + } + if self.is_control_dependent(predecessor, *block) { + self.current_block_control_dependent = true; + break; + } } + } else { + self.current_block_control_dependent = true; } for instruction_id in self.inserter.function.dfg[*block].take_instructions() { From 3b657b2749dee4f52f887cdff3bcd4d0549655b8 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 11 Mar 2025 18:35:24 +0000 Subject: [PATCH 05/24] store map of predecessors which have been marked control dependent as to avoid repeat work, allow control dependence checks on acir again --- .../src/ssa/opt/loop_invariant.rs | 64 +++++++++++++------ 1 file changed, 44 insertions(+), 20 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 4fada0a5cf7..37c3c28f7fb 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -106,11 +106,20 @@ struct LoopInvariantContext<'f> { // It is wrapped in an Option as our SSA `Id` does not allow dummy values. current_pre_header: Option, + // Post-Dominator Tree post_dom: DominatorTree, cfg: ControlFlowGraph, + // Stores whether the current block being processed is control dependent current_block_control_dependent: bool, + // Set of all control dependent blocks in a loop. + // Maintaining this set lets us optimize checking whether a block is control dependent. + // When checking whether a block is control dependent, we can check this set for + // whether any of its predecessors have already been marked as control dependent. + // If a predecessor has been marked as control dependent it means the block being + // checked must also be control dependent. + control_dependent_blocks: HashSet, } impl<'f> LoopInvariantContext<'f> { @@ -129,6 +138,7 @@ impl<'f> LoopInvariantContext<'f> { post_dom, cfg, current_block_control_dependent: false, + control_dependent_blocks: HashSet::default(), } } @@ -140,26 +150,7 @@ impl<'f> LoopInvariantContext<'f> { self.set_values_defined_in_loop(loop_); for block in loop_.blocks.iter() { - if self.inserter.function.dfg.runtime().is_brillig() { - let mut all_predecessors = - Loop::find_blocks_in_loop(self.pre_header(), *block, &self.cfg).blocks; - all_predecessors.remove(block); - all_predecessors.remove(&self.pre_header()); - - // Need to accurately determine whether the current block is dependent on any blocks between - // the current block and the loop header - for predecessor in all_predecessors { - if predecessor == loop_.header { - continue; - } - if self.is_control_dependent(predecessor, *block) { - self.current_block_control_dependent = true; - break; - } - } - } else { - self.current_block_control_dependent = true; - } + self.is_control_dependent_post_pre_header(loop_, *block); for instruction_id in self.inserter.function.dfg[*block].take_instructions() { self.transform_to_unchecked_from_loop_bounds(instruction_id); @@ -202,6 +193,37 @@ impl<'f> LoopInvariantContext<'f> { self.set_induction_var_bounds(loop_, false); } + /// Checks whether a `block` is control dependent on any blocks after + /// the given loop's pre-header. + fn is_control_dependent_post_pre_header(&mut self, loop_: &Loop, block: BasicBlockId) { + let mut all_predecessors = + Loop::find_blocks_in_loop(self.pre_header(), block, &self.cfg).blocks; + all_predecessors.remove(&block); + all_predecessors.remove(&self.pre_header()); + + // Need to accurately determine whether the current block is dependent on any blocks between + // the current block and the loop header + for predecessor in all_predecessors { + if predecessor == loop_.header { + continue; + } + + if self.control_dependent_blocks.contains(&predecessor) { + self.current_block_control_dependent = true; + self.control_dependent_blocks.insert(predecessor); + break; + } + + if self.is_control_dependent(predecessor, block) { + self.current_block_control_dependent = true; + self.control_dependent_blocks.insert(block); + break; + } + } + } + + /// Checks whether a `block` is control dependent on a `parent_block` + /// /// Jeanne Ferrante, Karl J. Ottenstein, and Joe D. Warren. 1987. /// The program dependence graph and its use in optimization. ACM /// Trans. Program. Lang. Syst. 9, 3 (July 1987), 319–349. @@ -235,6 +257,8 @@ impl<'f> LoopInvariantContext<'f> { // set the new current induction variable. self.current_induction_variables.clear(); self.set_induction_var_bounds(loop_, true); + // Reset the blocks that have been marked as control dependent + self.control_dependent_blocks.clear(); for block in loop_.blocks.iter() { let params = self.inserter.function.dfg.block_parameters(*block); From 86a391b132429e5879a53b1443173cd6e310f5bf Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 11 Mar 2025 18:57:55 +0000 Subject: [PATCH 06/24] still block control dep checks on acir and add comments --- compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 37c3c28f7fb..dc384b4b268 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -150,7 +150,14 @@ impl<'f> LoopInvariantContext<'f> { self.set_values_defined_in_loop(loop_); for block in loop_.blocks.iter() { - self.is_control_dependent_post_pre_header(loop_, *block); + // The CFG will ultimately be entirely flattened in ACIR. + // We can leave hoisting of instructions based upon predicates as part + // of constant folding once we we have a flat CFG. + // Thus, we forego checking control dependence on ACIR as to + // avoid greater compilation cost. + if self.inserter.function.runtime().is_brillig() { + self.is_control_dependent_post_pre_header(loop_, *block); + } for instruction_id in self.inserter.function.dfg[*block].take_instructions() { self.transform_to_unchecked_from_loop_bounds(instruction_id); From 73e84ebed5d9d171dddb2ab81f9a5581cc930fa7 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 11 Mar 2025 19:12:22 +0000 Subject: [PATCH 07/24] move where current_block_control_dependent is set to false --- compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index dc384b4b268..d20bd64b048 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -150,6 +150,7 @@ impl<'f> LoopInvariantContext<'f> { self.set_values_defined_in_loop(loop_); for block in loop_.blocks.iter() { + self.current_block_control_dependent = false; // The CFG will ultimately be entirely flattened in ACIR. // We can leave hoisting of instructions based upon predicates as part // of constant folding once we we have a flat CFG. @@ -193,8 +194,6 @@ impl<'f> LoopInvariantContext<'f> { } self.extend_values_defined_in_loop_and_invariants(instruction_id, hoist_invariant); } - - self.current_block_control_dependent = false; } self.set_induction_var_bounds(loop_, false); From f67e28883bd1125945cce2bc98a59c8e278c0e80 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 11 Mar 2025 19:42:34 +0000 Subject: [PATCH 08/24] current_block_control_dependent always true for acir --- compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index d20bd64b048..380322495a9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -158,6 +158,8 @@ impl<'f> LoopInvariantContext<'f> { // avoid greater compilation cost. if self.inserter.function.runtime().is_brillig() { self.is_control_dependent_post_pre_header(loop_, *block); + } else { + self.current_block_control_dependent = true; } for instruction_id in self.inserter.function.dfg[*block].take_instructions() { From e0848711a9ee1a042a542c4c4cfc974122172445 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 11 Mar 2025 19:52:48 +0000 Subject: [PATCH 09/24] ugh bumping noir_bigcurve timeout --- EXTERNAL_NOIR_LIBRARIES.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/EXTERNAL_NOIR_LIBRARIES.yml b/EXTERNAL_NOIR_LIBRARIES.yml index 30c2a9e3817..4d5156bc795 100644 --- a/EXTERNAL_NOIR_LIBRARIES.yml +++ b/EXTERNAL_NOIR_LIBRARIES.yml @@ -26,7 +26,7 @@ libraries: timeout: 90 noir_bigcurve: repo: noir-lang/noir_bigcurve - timeout: 250 + timeout: 350 noir_base64: repo: noir-lang/noir_base64 timeout: 5 From 60a01babd57f4db84ec5e7b3d35e9f47a2ab7a38 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 11 Mar 2025 20:10:21 +0000 Subject: [PATCH 10/24] more --- EXTERNAL_NOIR_LIBRARIES.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/EXTERNAL_NOIR_LIBRARIES.yml b/EXTERNAL_NOIR_LIBRARIES.yml index 4d5156bc795..ab0aa8a8ae7 100644 --- a/EXTERNAL_NOIR_LIBRARIES.yml +++ b/EXTERNAL_NOIR_LIBRARIES.yml @@ -26,7 +26,7 @@ libraries: timeout: 90 noir_bigcurve: repo: noir-lang/noir_bigcurve - timeout: 350 + timeout: 400 noir_base64: repo: noir-lang/noir_base64 timeout: 5 From 536e1a5b04cb4e4a81dd96450f4baa8675d99a76 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 12 Mar 2025 13:00:53 +0000 Subject: [PATCH 11/24] rev order we check blocks between preheader and block that may be control dependent --- compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 380322495a9..3a8f42c125f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -211,7 +211,7 @@ impl<'f> LoopInvariantContext<'f> { // Need to accurately determine whether the current block is dependent on any blocks between // the current block and the loop header - for predecessor in all_predecessors { + for predecessor in all_predecessors.into_iter().rev() { if predecessor == loop_.header { continue; } From ae69d802c8f32d0433c1b00be47bcbc89639e91e Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 12 Mar 2025 15:15:52 +0000 Subject: [PATCH 12/24] try to reduce bigcurve timeout --- EXTERNAL_NOIR_LIBRARIES.yml | 2 +- .../src/ssa/opt/loop_invariant.rs | 35 +++++++++---------- 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/EXTERNAL_NOIR_LIBRARIES.yml b/EXTERNAL_NOIR_LIBRARIES.yml index 5fb1d8e8d66..f0047c153b3 100644 --- a/EXTERNAL_NOIR_LIBRARIES.yml +++ b/EXTERNAL_NOIR_LIBRARIES.yml @@ -26,7 +26,7 @@ libraries: timeout: 90 noir_bigcurve: repo: noir-lang/noir_bigcurve - timeout: 400 + timeout: 300 noir_base64: repo: noir-lang/noir_base64 timeout: 5 diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 3a8f42c125f..6e964511130 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -206,27 +206,21 @@ impl<'f> LoopInvariantContext<'f> { fn is_control_dependent_post_pre_header(&mut self, loop_: &Loop, block: BasicBlockId) { let mut all_predecessors = Loop::find_blocks_in_loop(self.pre_header(), block, &self.cfg).blocks; + all_predecessors.remove(&block); all_predecessors.remove(&self.pre_header()); + all_predecessors.remove(&loop_.header); // Need to accurately determine whether the current block is dependent on any blocks between // the current block and the loop header - for predecessor in all_predecessors.into_iter().rev() { - if predecessor == loop_.header { - continue; - } - - if self.control_dependent_blocks.contains(&predecessor) { + for predecessor in all_predecessors { + if self.control_dependent_blocks.contains(&predecessor) + || self.is_control_dependent(predecessor, block) + { self.current_block_control_dependent = true; self.control_dependent_blocks.insert(predecessor); break; } - - if self.is_control_dependent(predecessor, block) { - self.current_block_control_dependent = true; - self.control_dependent_blocks.insert(block); - break; - } } } @@ -238,18 +232,21 @@ impl<'f> LoopInvariantContext<'f> { /// https://doi.org/10.1145/24039.24041 /// /// Definition 3 from the linked paper above. + /// ```text + /// Let G be a control flow graph. Let X and Y be nodes in G. Y is + // control dependent on X iff + // (1) there exists a directed path P from X to Y with any 2 in P (excluding X + // and Y) post-dominated by Y and + // (2) X is not post-dominated by Y. + /// ``` fn is_control_dependent(&mut self, parent_block: BasicBlockId, block: BasicBlockId) -> bool { let mut all_predecessors = Loop::find_blocks_in_loop(parent_block, block, &self.cfg).blocks; all_predecessors.remove(&parent_block); all_predecessors.remove(&block); - let mut first_control_cond = false; - for predecessor in all_predecessors { - if self.post_dom.dominates(predecessor, block) { - first_control_cond = true; - break; - } - } + let first_control_cond = + all_predecessors.iter().any(|&pred| self.post_dom.dominates(pred, block)); + first_control_cond && !self.post_dom.dominates(block, parent_block) } From 4e9a828b4093290eacf9dc9be0427f428c8f4270 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 12 Mar 2025 18:02:29 +0000 Subject: [PATCH 13/24] use reverse dom frontiers to move from n^2 control dep check to n --- compiler/noirc_evaluator/src/ssa/ir/dom.rs | 71 ++++++++++++++++++- .../src/ssa/opt/loop_invariant.rs | 25 +++---- 2 files changed, 81 insertions(+), 15 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dom.rs b/compiler/noirc_evaluator/src/ssa/ir/dom.rs index be42fbc713e..058c4064f3f 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dom.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dom.rs @@ -9,7 +9,7 @@ use std::cmp::Ordering; use super::{ basic_block::BasicBlockId, cfg::ControlFlowGraph, function::Function, post_order::PostOrder, }; -use fxhash::FxHashMap as HashMap; +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; /// Dominator tree node. We keep one of these per reachable block. #[derive(Clone, Default)] @@ -276,6 +276,49 @@ impl DominatorTree { debug_assert_eq!(block_a_id, block_b_id, "Unreachable block passed to common_dominator?"); block_a_id } + + /// Computes the dominance frontier for all blocks in the dominator tree. + /// + /// We expect a standard CFG (non-reversed) even if we are operating over + /// a dominator tree or a post-dominator tree. + /// Calling this method on a dominator tree will return the CFG's dominance frontiers, + /// while on a post-dominator tree the method will return the CFG's reverse dominance frontiers. + /// + /// + pub(crate) fn compute_dominance_frontiers( + &mut self, + cfg: &ControlFlowGraph, + ) -> HashMap> { + let mut dominance_frontiers: HashMap> = + HashMap::default(); + + let nodes = self.nodes.keys().into_iter().copied().collect::>(); + for block_id in nodes { + let predecessors = cfg.predecessors(block_id); + // Dominance frontier nodes must have more than one predecessor + if predecessors.len() > 1 { + // Iterate over the predecessors of the current block + for pred_id in predecessors { + let mut runner = pred_id; + // We start by checking if the current block dominates the predecessor + while let Some(immediate_dominator) = self.immediate_dominator(block_id) { + if immediate_dominator != runner { + dominance_frontiers.entry(runner).or_default().insert(block_id); + let Some(runner_immediate_dom) = self.immediate_dominator(runner) + else { + break; + }; + runner = runner_immediate_dom; + } else { + break; + } + } + } + } + } + + dominance_frontiers + } } #[cfg(test)] @@ -285,8 +328,9 @@ mod tests { use crate::ssa::{ function_builder::FunctionBuilder, ir::{ - basic_block::BasicBlockId, call_stack::CallStackId, dom::DominatorTree, - function::Function, instruction::TerminatorInstruction, map::Id, types::Type, + basic_block::BasicBlockId, call_stack::CallStackId, cfg::ControlFlowGraph, + dom::DominatorTree, function::Function, instruction::TerminatorInstruction, map::Id, + types::Type, }, }; @@ -542,6 +586,27 @@ mod tests { assert!(post_dom.dominates(block2_id, block2_id)); } + #[test] + fn dom_frontiers_backwards_layout() { + let (func, ..) = backwards_layout_setup(); + let mut dt = DominatorTree::with_function(&func); + + let cfg = ControlFlowGraph::with_function(&func); + let dom_frontiers = dt.compute_dominance_frontiers(&cfg); + assert!(dom_frontiers.is_empty()); + } + + #[test] + fn post_dom_frontiers_backwards_layout() { + let (func, ..) = backwards_layout_setup(); + let mut post_dom = DominatorTree::with_function_post_dom(&func); + + let cfg = ControlFlowGraph::with_function(&func); + let reversed_cfg = cfg.reverse(); + let dom_frontiers = post_dom.compute_dominance_frontiers(&cfg); + assert!(dom_frontiers.is_empty()); + } + #[test] fn test_find_map_dominator() { let (dt, b0, b1, b2, _b3) = unreachable_node_setup(); diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 6e964511130..13ed4fcad7c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -107,6 +107,7 @@ struct LoopInvariantContext<'f> { current_pre_header: Option, // Post-Dominator Tree + #[allow(dead_code)] post_dom: DominatorTree, cfg: ControlFlowGraph, @@ -120,6 +121,8 @@ struct LoopInvariantContext<'f> { // If a predecessor has been marked as control dependent it means the block being // checked must also be control dependent. control_dependent_blocks: HashSet, + + reverse_dom_frontiers: HashMap>, } impl<'f> LoopInvariantContext<'f> { @@ -127,7 +130,8 @@ impl<'f> LoopInvariantContext<'f> { let cfg = ControlFlowGraph::with_function(function); let reversed_cfg = cfg.reverse(); let post_order = PostOrder::with_cfg(&reversed_cfg); - let post_dom = DominatorTree::with_cfg_and_post_order(&reversed_cfg, &post_order); + let mut post_dom = DominatorTree::with_cfg_and_post_order(&reversed_cfg, &post_order); + let reverse_dom_frontiers = post_dom.compute_dominance_frontiers(&reversed_cfg); Self { inserter: FunctionInserter::new(function), defined_in_loop: HashSet::default(), @@ -139,6 +143,7 @@ impl<'f> LoopInvariantContext<'f> { cfg, current_block_control_dependent: false, control_dependent_blocks: HashSet::default(), + reverse_dom_frontiers, } } @@ -214,9 +219,9 @@ impl<'f> LoopInvariantContext<'f> { // Need to accurately determine whether the current block is dependent on any blocks between // the current block and the loop header for predecessor in all_predecessors { - if self.control_dependent_blocks.contains(&predecessor) - || self.is_control_dependent(predecessor, block) - { + // if self.control_dependent_blocks.contains(&predecessor) + // || self.is_control_dependent(predecessor, block) + if self.is_control_dependent(predecessor, block) { self.current_block_control_dependent = true; self.control_dependent_blocks.insert(predecessor); break; @@ -240,14 +245,10 @@ impl<'f> LoopInvariantContext<'f> { // (2) X is not post-dominated by Y. /// ``` fn is_control_dependent(&mut self, parent_block: BasicBlockId, block: BasicBlockId) -> bool { - let mut all_predecessors = Loop::find_blocks_in_loop(parent_block, block, &self.cfg).blocks; - all_predecessors.remove(&parent_block); - all_predecessors.remove(&block); - - let first_control_cond = - all_predecessors.iter().any(|&pred| self.post_dom.dominates(pred, block)); - - first_control_cond && !self.post_dom.dominates(block, parent_block) + match self.reverse_dom_frontiers.get(&block) { + Some(dependent_blocks) => dependent_blocks.contains(&parent_block), + None => false, + } } /// Gather the variables declared within the loop From 870b6693f8fc3df0bd910ad1e177c343d6c87f9b Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 12 Mar 2025 18:08:05 +0000 Subject: [PATCH 14/24] clippy --- compiler/noirc_evaluator/src/ssa/ir/dom.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dom.rs b/compiler/noirc_evaluator/src/ssa/ir/dom.rs index 058c4064f3f..9c3c0c965d9 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dom.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dom.rs @@ -292,7 +292,7 @@ impl DominatorTree { let mut dominance_frontiers: HashMap> = HashMap::default(); - let nodes = self.nodes.keys().into_iter().copied().collect::>(); + let nodes = self.nodes.keys().copied().collect::>(); for block_id in nodes { let predecessors = cfg.predecessors(block_id); // Dominance frontier nodes must have more than one predecessor From 1bc58e86d0dd22949dfc54d360df1b398a5fc842 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 12 Mar 2025 18:19:35 +0000 Subject: [PATCH 15/24] just check dom frontiers and not control dep blocks map --- compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 13ed4fcad7c..29718f35fe1 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -220,7 +220,8 @@ impl<'f> LoopInvariantContext<'f> { // the current block and the loop header for predecessor in all_predecessors { // if self.control_dependent_blocks.contains(&predecessor) - // || self.is_control_dependent(predecessor, block) + // || self.is_control_dependent(predecessor, block) + // { if self.is_control_dependent(predecessor, block) { self.current_block_control_dependent = true; self.control_dependent_blocks.insert(predecessor); From 571fa37c8ab1b6d1c3365ed50b59f81fc37c95ba Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 12 Mar 2025 18:21:36 +0000 Subject: [PATCH 16/24] one more clippy --- compiler/noirc_evaluator/src/ssa/ir/dom.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dom.rs b/compiler/noirc_evaluator/src/ssa/ir/dom.rs index 9c3c0c965d9..ec4e9eeb499 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dom.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dom.rs @@ -602,7 +602,6 @@ mod tests { let mut post_dom = DominatorTree::with_function_post_dom(&func); let cfg = ControlFlowGraph::with_function(&func); - let reversed_cfg = cfg.reverse(); let dom_frontiers = post_dom.compute_dominance_frontiers(&cfg); assert!(dom_frontiers.is_empty()); } From 5784c627d5c9b82a64153c9395b5f22c8466a2c3 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 12 Mar 2025 18:41:45 +0000 Subject: [PATCH 17/24] big curve timeout back to 250 --- EXTERNAL_NOIR_LIBRARIES.yml | 2 +- compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/EXTERNAL_NOIR_LIBRARIES.yml b/EXTERNAL_NOIR_LIBRARIES.yml index 6bc62d6f00e..786b2f51a7d 100644 --- a/EXTERNAL_NOIR_LIBRARIES.yml +++ b/EXTERNAL_NOIR_LIBRARIES.yml @@ -26,7 +26,7 @@ libraries: timeout: 90 noir_bigcurve: repo: noir-lang/noir_bigcurve - timeout: 300 + timeout: 250 noir_base64: repo: noir-lang/noir_base64 timeout: 5 diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 29718f35fe1..bf0ee2ad9e1 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -219,9 +219,6 @@ impl<'f> LoopInvariantContext<'f> { // Need to accurately determine whether the current block is dependent on any blocks between // the current block and the loop header for predecessor in all_predecessors { - // if self.control_dependent_blocks.contains(&predecessor) - // || self.is_control_dependent(predecessor, block) - // { if self.is_control_dependent(predecessor, block) { self.current_block_control_dependent = true; self.control_dependent_blocks.insert(predecessor); From c4e79373fa560ab0a523cc02afd7ad11227802bd Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 12 Mar 2025 18:51:30 +0000 Subject: [PATCH 18/24] expand instructions that can be hoisted with same predicate in licm --- .../src/ssa/opt/loop_invariant.rs | 30 +++++-------------- 1 file changed, 7 insertions(+), 23 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index bf0ee2ad9e1..e37f3dd1f1a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -106,23 +106,12 @@ struct LoopInvariantContext<'f> { // It is wrapped in an Option as our SSA `Id` does not allow dummy values. current_pre_header: Option, - // Post-Dominator Tree - #[allow(dead_code)] - post_dom: DominatorTree, - cfg: ControlFlowGraph, // Stores whether the current block being processed is control dependent current_block_control_dependent: bool, - // Set of all control dependent blocks in a loop. - // Maintaining this set lets us optimize checking whether a block is control dependent. - // When checking whether a block is control dependent, we can check this set for - // whether any of its predecessors have already been marked as control dependent. - // If a predecessor has been marked as control dependent it means the block being - // checked must also be control dependent. - control_dependent_blocks: HashSet, - - reverse_dom_frontiers: HashMap>, + + post_dom_frontiers: HashMap>, } impl<'f> LoopInvariantContext<'f> { @@ -131,7 +120,7 @@ impl<'f> LoopInvariantContext<'f> { let reversed_cfg = cfg.reverse(); let post_order = PostOrder::with_cfg(&reversed_cfg); let mut post_dom = DominatorTree::with_cfg_and_post_order(&reversed_cfg, &post_order); - let reverse_dom_frontiers = post_dom.compute_dominance_frontiers(&reversed_cfg); + let post_dom_frontiers = post_dom.compute_dominance_frontiers(&reversed_cfg); Self { inserter: FunctionInserter::new(function), defined_in_loop: HashSet::default(), @@ -139,11 +128,9 @@ impl<'f> LoopInvariantContext<'f> { current_induction_variables: HashMap::default(), outer_induction_variables: HashMap::default(), current_pre_header: None, - post_dom, cfg, current_block_control_dependent: false, - control_dependent_blocks: HashSet::default(), - reverse_dom_frontiers, + post_dom_frontiers, } } @@ -221,7 +208,6 @@ impl<'f> LoopInvariantContext<'f> { for predecessor in all_predecessors { if self.is_control_dependent(predecessor, block) { self.current_block_control_dependent = true; - self.control_dependent_blocks.insert(predecessor); break; } } @@ -243,7 +229,7 @@ impl<'f> LoopInvariantContext<'f> { // (2) X is not post-dominated by Y. /// ``` fn is_control_dependent(&mut self, parent_block: BasicBlockId, block: BasicBlockId) -> bool { - match self.reverse_dom_frontiers.get(&block) { + match self.post_dom_frontiers.get(&block) { Some(dependent_blocks) => dependent_blocks.contains(&parent_block), None => false, } @@ -261,8 +247,6 @@ impl<'f> LoopInvariantContext<'f> { // set the new current induction variable. self.current_induction_variables.clear(); self.set_induction_var_bounds(loop_, true); - // Reset the blocks that have been marked as control dependent - self.control_dependent_blocks.clear(); for block in loop_.blocks.iter() { let params = self.inserter.function.dfg.block_parameters(*block); @@ -315,8 +299,8 @@ impl<'f> LoopInvariantContext<'f> { let can_be_deduplicated = instruction.can_be_deduplicated(self.inserter.function, false) || matches!(instruction, Instruction::MakeArray { .. }) - // TODO: improve this control dependence check - || (!matches!(instruction, Instruction::Call { .. }) && instruction.requires_acir_gen_predicate(&self.inserter.function.dfg) && !self.current_block_control_dependent) + || (instruction.can_be_deduplicated(self.inserter.function, true) + && !self.current_block_control_dependent) || self.can_be_deduplicated_from_loop_bound(&instruction); is_loop_invariant && can_be_deduplicated From 90e9a99e6eb98a82f36c65c530c1b8659d428a13 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 12 Mar 2025 19:26:55 +0000 Subject: [PATCH 19/24] fix licm unit tests --- .../src/ssa/opt/loop_invariant.rs | 37 +++++++++++-------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index e37f3dd1f1a..c39391107fa 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -474,19 +474,24 @@ mod test { let instructions = main.dfg[main.entry_block()].instructions(); assert_eq!(instructions.len(), 0); // The final return is not counted - // `v6 = mul v0, v1` in b3 should now be `v3 = mul v0, v1` in b0 + // From b3: + // `v6 = mul v0, v1` + // `constrain v6 == i32 6` + // To b0: + // `v3 = mul v0, v1` + // `constrain v3 == i32 6` let expected = " brillig(inline) fn main f0 { b0(v0: i32, v1: i32): v3 = mul v0, v1 + constrain v3 == i32 6 jmp b1(i32 0) b1(v2: i32): - v6 = lt v2, i32 4 - jmpif v6 then: b3, else: b2 + v7 = lt v2, i32 4 + jmpif v7 then: b3, else: b2 b2(): return b3(): - constrain v3 == i32 6 v9 = unchecked_add v2, i32 1 jmp b1(v9) } @@ -543,15 +548,15 @@ mod test { b2(): return b3(): + constrain v4 == i32 6 jmp b4(i32 0) b4(v3: i32): - v8 = lt v3, i32 4 - jmpif v8 then: b6, else: b5 + v9 = lt v3, i32 4 + jmpif v9 then: b6, else: b5 b5(): v12 = unchecked_add v2, i32 1 jmp b1(v12) b6(): - constrain v4 == i32 6 v11 = unchecked_add v3, i32 1 jmp b4(v11) } @@ -605,6 +610,7 @@ mod test { v3 = mul v0, v1 v4 = mul v3, v0 v6 = eq v4, i32 12 + constrain v4 == i32 12 jmp b1(i32 0) b1(v2: i32): v9 = lt v2, i32 4 @@ -612,7 +618,6 @@ mod test { b2(): return b3(): - constrain v4 == i32 12 v11 = unchecked_add v2, i32 1 jmp b1(v11) } @@ -746,8 +751,10 @@ mod test { v19 = unchecked_add v2, u32 1 jmp b1(v19) b6(): + constrain v10 == v0 v13 = array_get v6, index v3 -> u32 v14 = eq v13, v0 + constrain v13 == v0 jmp b7(u32 0) b7(v4: u32): v15 = lt v4, u32 4 @@ -756,8 +763,6 @@ mod test { v18 = unchecked_add v3, u32 1 jmp b4(v18) b9(): - constrain v10 == v0 - constrain v13 == v0 v17 = unchecked_add v4, u32 1 jmp b7(v17) } @@ -885,14 +890,14 @@ mod test { brillig(inline) fn main f0 { b0(v0: i32, v1: i32): v3 = mul v0, v1 + constrain v3 == i32 6 jmp b1(i32 0) b1(v2: i32): - v6 = lt v2, i32 4 - jmpif v6 then: b3, else: b2 + v7 = lt v2, i32 4 + jmpif v7 then: b3, else: b2 b2(): return b3(): - constrain v3 == i32 6 v9 = unchecked_add v2, i32 1 jmp b1(v9) } @@ -1152,12 +1157,12 @@ mod control_dependence { b0(v0: u32, v1: u32): v3 = mul v0, v1 v4 = mul v3, v0 + constrain v4 == u32 12 jmp b1(u32 0) b1(v2: u32): - v7 = lt v2, u32 4 - jmpif v7 then: b2, else: b3 + v8 = lt v2, u32 4 + jmpif v8 then: b2, else: b3 b2(): - constrain v4 == u32 12 v10 = unchecked_add v2, u32 1 jmp b1(v10) b3(): From fbebe85564336388cfd29ea091c054133c490f40 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 12 Mar 2025 19:35:29 +0000 Subject: [PATCH 20/24] bump timeout --- .github/benchmark_projects.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/benchmark_projects.yml b/.github/benchmark_projects.yml index 27eac1efa63..625d08f62e0 100644 --- a/.github/benchmark_projects.yml +++ b/.github/benchmark_projects.yml @@ -5,7 +5,7 @@ projects: ref: *AZ_COMMIT path: noir-projects/noir-protocol-circuits/crates/private-kernel-inner num_runs: 5 - compilation-timeout: 2.5 + compilation-timeout: 3 execution-timeout: 0.08 compilation-memory-limit: 350 execution-memory-limit: 250 From 14c86323b885d5660cdd081476f0534e0a877b79 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 12 Mar 2025 20:35:18 +0000 Subject: [PATCH 21/24] dom ffrontier tests --- compiler/noirc_evaluator/src/ssa/ir/dom.rs | 143 ++++++++++++++++++++- 1 file changed, 142 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dom.rs b/compiler/noirc_evaluator/src/ssa/ir/dom.rs index ec4e9eeb499..c77946b8050 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dom.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dom.rs @@ -325,13 +325,16 @@ impl DominatorTree { mod tests { use std::cmp::Ordering; + use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; + use crate::ssa::{ function_builder::FunctionBuilder, ir::{ basic_block::BasicBlockId, call_stack::CallStackId, cfg::ControlFlowGraph, dom::DominatorTree, function::Function, instruction::TerminatorInstruction, map::Id, - types::Type, + post_order::PostOrder, types::Type, }, + ssa_gen::Ssa, }; #[test] @@ -606,6 +609,144 @@ mod tests { assert!(dom_frontiers.is_empty()); } + fn loop_with_conditional() -> Ssa { + let src = " + brillig(inline) predicate_pure fn main f0 { + b0(v1: u32, v2: u32): + v5 = eq v1, u32 5 + jmp b1(u32 0) + b1(v3: u32): + v8 = lt v3, u32 4 + jmpif v8 then: b2, else: b3 + b2(): + jmpif v5 then: b4, else: b5 + b3(): + return + b4(): + v9 = mul u32 4294967295, v2 + constrain v9 == u32 12 + jmp b5() + b5(): + v12 = unchecked_add v3, u32 1 + jmp b1(v12) + } + "; + Ssa::from_str(src).unwrap() + } + + #[test] + fn dom_frontiers() { + let ssa = loop_with_conditional(); + let main = ssa.main(); + + let cfg = ControlFlowGraph::with_function(main); + let post_order = PostOrder::with_cfg(&cfg); + + let mut dt = DominatorTree::with_cfg_and_post_order(&cfg, &post_order); + let dom_frontiers = dt.compute_dominance_frontiers(&cfg); + dbg!(dom_frontiers.clone()); + + // Convert BasicBlockIds to their underlying u32 values for easy comparisons + let dom_frontiers = dom_frontiers + .into_iter() + .map(|(block, frontier)| { + ( + block.to_u32(), + frontier.into_iter().map(|block| block.to_u32()).collect::>(), + ) + }) + .collect::>(); + + // b0 is the entry block which dominates all other blocks + // Thus, it has an empty set for its dominance frontier + assert!(dom_frontiers.get(&0).is_none()); + + // b1 is in its own DF due to the loop b5 -> b1 + // b1 dominates b5 which is a predecessor of b1, but b1 does not strictly dominate b1 + let b1_df = &dom_frontiers[&1]; + assert_eq!(b1_df.len(), 1); + assert!(b1_df.contains(&1)); + + // b2 has DF { b1 } also due to the loop b5 -> b1. + // b2 dominates b5 which is a predecessor of b1, but b2 does not strictly dominate b1 + let b2_df = &dom_frontiers[&2]; + assert_eq!(b2_df.len(), 1); + assert!(b2_df.contains(&1)); + + // b3 is the exit block which does not dominate any blocks + assert!(dom_frontiers.get(&3).is_none()); + + // b4 has DF { b5 } because b4 jumps to b5 (thus being a predecessor to b5) + // b4 dominates itself but b5 is not strictly dominated by b4. + let b4_df = &dom_frontiers[&4]; + assert_eq!(b4_df.len(), 1); + assert!(b4_df.contains(&5)); + + // b5 has DF { b1 } also due to the loop b5 -> b1 + let b5_df = &dom_frontiers[&5]; + assert_eq!(b5_df.len(), 1); + assert!(b5_df.contains(&1)); + } + + #[test] + fn post_dom_frontiers() { + let ssa = loop_with_conditional(); + let main = ssa.main(); + + let cfg = ControlFlowGraph::with_function(main); + let reversed_cfg = cfg.reverse(); + let post_order = PostOrder::with_cfg(&reversed_cfg); + + let mut post_dom = DominatorTree::with_cfg_and_post_order(&reversed_cfg, &post_order); + let post_dom_frontiers = post_dom.compute_dominance_frontiers(&reversed_cfg); + dbg!(post_dom_frontiers.clone()); + + // Convert BasicBlockIds to their underlying u32 values for easy comparisons + let post_dom_frontiers = post_dom_frontiers + .into_iter() + .map(|(block, frontier)| { + ( + block.to_u32(), + frontier.into_iter().map(|block| block.to_u32()).collect::>(), + ) + }) + .collect::>(); + + // Another way to think about the post-dominator frontier for a node n, + // is that we can reach a block in the PDF during execution without going through n. + + // b0 is the entry node of the program and the exit block of the post-dominator tree. + // Thus, it has an empty set for its post-dominance frontier (PDF) + assert!(post_dom_frontiers.get(&0).is_none()); + + // b1 is in its own PDF due to the loop b5 -> b1 + // b1 post-dominates b2 and b5. b1 post-dominates itself but not strictly post-dominate itself. + let b1_pdf = &post_dom_frontiers[&1]; + assert_eq!(b1_pdf.len(), 1); + assert!(b1_pdf.contains(&1)); + + // b2 has DF { b1 } also due to the loop b5 -> b1. + // b1 post-dominates itself is a predecessor of b2, but b1 does not strictly post-dominate b2. + let b2_pdf = &post_dom_frontiers[&2]; + assert_eq!(b2_pdf.len(), 1); + assert!(b2_pdf.contains(&1)); + + // b3 is the exit block of the program, but the starting node of the post-dominator tree + // Thus, it has an empty PDF + assert!(post_dom_frontiers.get(&3).is_none()); + + // b4 has DF { b2 } because b2 post-dominates itself and is a predecessor to b4. + // b2 does not strictly post-dominate b4. + let b4_pdf = &post_dom_frontiers[&4]; + assert_eq!(b4_pdf.len(), 1); + assert!(b4_pdf.contains(&2)); + + // b5 has DF { b1 } also due to the loop b5 -> b1 + let b5_pdf = &post_dom_frontiers[&5]; + assert_eq!(b5_pdf.len(), 1); + assert!(b5_pdf.contains(&1)); + } + #[test] fn test_find_map_dominator() { let (dt, b0, b1, b2, _b3) = unreachable_node_setup(); From b33626fb73cac4a775a3c6185e83a2046a3168f8 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 12 Mar 2025 20:46:16 +0000 Subject: [PATCH 22/24] improve comments for dom frontiers --- compiler/noirc_evaluator/src/ssa/ir/dom.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dom.rs b/compiler/noirc_evaluator/src/ssa/ir/dom.rs index c77946b8050..2d1095913fa 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dom.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dom.rs @@ -279,12 +279,14 @@ impl DominatorTree { /// Computes the dominance frontier for all blocks in the dominator tree. /// - /// We expect a standard CFG (non-reversed) even if we are operating over - /// a dominator tree or a post-dominator tree. - /// Calling this method on a dominator tree will return the CFG's dominance frontiers, - /// while on a post-dominator tree the method will return the CFG's reverse dominance frontiers. - /// + /// This method uses the algorithm specified under Cooper, Keith D. et al. “A Simple, Fast Dominance Algorithm.” (1999). + /// As referenced in the paper a dominance frontier is the set of all CFG nodes, y, such that + /// b dominates a predecessor of y but does not strictly d. /// + /// This method expects the appropriate CFG depending on whether we are operating over + /// a dominator tree (standard CFG) or a post-dominator tree (reversed CFG). + /// Calling this method on a dominator tree will return a function's dominance frontiers, + /// while on a post-dominator tree the method will return the function's reverse (or post) dominance frontiers. pub(crate) fn compute_dominance_frontiers( &mut self, cfg: &ControlFlowGraph, @@ -659,7 +661,7 @@ mod tests { // b0 is the entry block which dominates all other blocks // Thus, it has an empty set for its dominance frontier - assert!(dom_frontiers.get(&0).is_none()); + assert!(!dom_frontiers.contains_key(&0)); // b1 is in its own DF due to the loop b5 -> b1 // b1 dominates b5 which is a predecessor of b1, but b1 does not strictly dominate b1 @@ -674,7 +676,7 @@ mod tests { assert!(b2_df.contains(&1)); // b3 is the exit block which does not dominate any blocks - assert!(dom_frontiers.get(&3).is_none()); + assert!(!dom_frontiers.contains_key(&3)); // b4 has DF { b5 } because b4 jumps to b5 (thus being a predecessor to b5) // b4 dominates itself but b5 is not strictly dominated by b4. @@ -717,7 +719,7 @@ mod tests { // b0 is the entry node of the program and the exit block of the post-dominator tree. // Thus, it has an empty set for its post-dominance frontier (PDF) - assert!(post_dom_frontiers.get(&0).is_none()); + assert!(!post_dom_frontiers.contains_key(&0)); // b1 is in its own PDF due to the loop b5 -> b1 // b1 post-dominates b2 and b5. b1 post-dominates itself but not strictly post-dominate itself. @@ -733,7 +735,7 @@ mod tests { // b3 is the exit block of the program, but the starting node of the post-dominator tree // Thus, it has an empty PDF - assert!(post_dom_frontiers.get(&3).is_none()); + assert!(!post_dom_frontiers.contains_key(&3)); // b4 has DF { b2 } because b2 post-dominates itself and is a predecessor to b4. // b2 does not strictly post-dominate b4. From 096dcb39e6fed46cf682a1580bdb7c5bba70abac Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 12 Mar 2025 21:07:35 +0000 Subject: [PATCH 23/24] add doc comments regarding licm control dep and start checking control dep blocks after the header not the preheader --- .../src/ssa/opt/loop_invariant.rs | 62 +++++++++++++------ 1 file changed, 44 insertions(+), 18 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index c39391107fa..4205d4d3ccc 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -7,6 +7,43 @@ //! - Already marked as loop invariants //! //! We also check that we are not hoisting instructions with side effects. +//! However, there are certain instructions whose side effects are only activated +//! under a predicate (e.g. an array out of bounds error on a dynamic index). +//! Thus, we also track the control dependence of loop blocks to determine +//! whether these "pure with predicate instructions" can be hoisted. +//! We use post-dominance frontiers to determine control dependence. +//! +//! Let's look at definition 3 from the following paper: +//! Jeanne Ferrante, Karl J. Ottenstein, and Joe D. Warren. 1987. +//! The program dependence graph and its use in optimization. ACM +//! Trans. Program. Lang. Syst. 9, 3 (July 1987), 319–349. +//! https://doi.org/10.1145/24039.24041 +//! +//! ```text +//! Let G be a control flow graph. Let X and Y be nodes in G. Y is +//! control dependent on X iff +//! (1) there exists a directed path P from X to Y with any 2 in P (excluding X +//! and Y) post-dominated by Y and +//! (2) X is not post-dominated by Y. +//! ``` +//! +//! Verifying these conditions for every loop block would be quite inefficient. +//! For example, let's say we just want to check whether a given loop block is control dependent at all +//! after the loop preheader. We would have to to verify the conditions above for every block between the loop preheader +//! and the given loop block. This is n^2 complexity in the worst case. +//! To optimize the control dependence checks, we can use post-dominance frontiers (PDF). +//! +//! From Cooper, Keith D. et al. “A Simple, Fast Dominance Algorithm.” (1999). +//! ```text +//! A dominance frontier is the set of all CFG nodes, y, such that +//! b dominates a predecessor of y but does not strictly dominate y. +//! ``` +//! Reversing this for post-dominance we can see that the conditions for control dependence +//! are the same as those for post-dominance frontiers. +//! Thus, we rewrite our control dependence condition as Y is control dependent on X iff Y is in PDF(Y). +//! +//! We then can store the PDFs for every block as part of the context of this pass, and use it for checking control dependence. +//! Using PDFs gets us from a worst case n^2 complexity to a worst case n. use acvm::{FieldElement, acir::AcirField}; use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; @@ -111,6 +148,8 @@ struct LoopInvariantContext<'f> { // Stores whether the current block being processed is control dependent current_block_control_dependent: bool, + // Maps a block to its post-dominance frontiers + // This map should be precomputed a single time and used for checking control dependence. post_dom_frontiers: HashMap>, } @@ -194,13 +233,11 @@ impl<'f> LoopInvariantContext<'f> { } /// Checks whether a `block` is control dependent on any blocks after - /// the given loop's pre-header. + /// the given loop's header. fn is_control_dependent_post_pre_header(&mut self, loop_: &Loop, block: BasicBlockId) { - let mut all_predecessors = - Loop::find_blocks_in_loop(self.pre_header(), block, &self.cfg).blocks; + let mut all_predecessors = Loop::find_blocks_in_loop(loop_.header, block, &self.cfg).blocks; all_predecessors.remove(&block); - all_predecessors.remove(&self.pre_header()); all_predecessors.remove(&loop_.header); // Need to accurately determine whether the current block is dependent on any blocks between @@ -214,20 +251,9 @@ impl<'f> LoopInvariantContext<'f> { } /// Checks whether a `block` is control dependent on a `parent_block` - /// - /// Jeanne Ferrante, Karl J. Ottenstein, and Joe D. Warren. 1987. - /// The program dependence graph and its use in optimization. ACM - /// Trans. Program. Lang. Syst. 9, 3 (July 1987), 319–349. - /// https://doi.org/10.1145/24039.24041 - /// - /// Definition 3 from the linked paper above. - /// ```text - /// Let G be a control flow graph. Let X and Y be nodes in G. Y is - // control dependent on X iff - // (1) there exists a directed path P from X to Y with any 2 in P (excluding X - // and Y) post-dominated by Y and - // (2) X is not post-dominated by Y. - /// ``` + /// Uses post-dominance frontiers to determine control dependence. + /// Reference the doc comments at the top of the this module for more information + /// regarding post-dominance frontiers and control dependence. fn is_control_dependent(&mut self, parent_block: BasicBlockId, block: BasicBlockId) -> bool { match self.post_dom_frontiers.get(&block) { Some(dependent_blocks) => dependent_blocks.contains(&parent_block), From 61e5f0ed1cb3e2087756e8070b700f43557d3d27 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 13 Mar 2025 03:30:56 +0000 Subject: [PATCH 24/24] fixup dom frontiers --- compiler/noirc_evaluator/src/ssa/ir/dom.rs | 47 +++++----------------- 1 file changed, 10 insertions(+), 37 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dom.rs b/compiler/noirc_evaluator/src/ssa/ir/dom.rs index 2d1095913fa..f6ec6c7a298 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dom.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dom.rs @@ -281,7 +281,7 @@ impl DominatorTree { /// /// This method uses the algorithm specified under Cooper, Keith D. et al. “A Simple, Fast Dominance Algorithm.” (1999). /// As referenced in the paper a dominance frontier is the set of all CFG nodes, y, such that - /// b dominates a predecessor of y but does not strictly d. + /// b dominates a predecessor of y but does not strictly dominate y. /// /// This method expects the appropriate CFG depending on whether we are operating over /// a dominator tree (standard CFG) or a post-dominator tree (reversed CFG). @@ -304,7 +304,7 @@ impl DominatorTree { let mut runner = pred_id; // We start by checking if the current block dominates the predecessor while let Some(immediate_dominator) = self.immediate_dominator(block_id) { - if immediate_dominator != runner { + if immediate_dominator != runner && !self.dominates(block_id, runner) { dominance_frontiers.entry(runner).or_default().insert(block_id); let Some(runner_immediate_dom) = self.immediate_dominator(runner) else { @@ -646,7 +646,6 @@ mod tests { let mut dt = DominatorTree::with_cfg_and_post_order(&cfg, &post_order); let dom_frontiers = dt.compute_dominance_frontiers(&cfg); - dbg!(dom_frontiers.clone()); // Convert BasicBlockIds to their underlying u32 values for easy comparisons let dom_frontiers = dom_frontiers @@ -662,19 +661,8 @@ mod tests { // b0 is the entry block which dominates all other blocks // Thus, it has an empty set for its dominance frontier assert!(!dom_frontiers.contains_key(&0)); - - // b1 is in its own DF due to the loop b5 -> b1 - // b1 dominates b5 which is a predecessor of b1, but b1 does not strictly dominate b1 - let b1_df = &dom_frontiers[&1]; - assert_eq!(b1_df.len(), 1); - assert!(b1_df.contains(&1)); - - // b2 has DF { b1 } also due to the loop b5 -> b1. - // b2 dominates b5 which is a predecessor of b1, but b2 does not strictly dominate b1 - let b2_df = &dom_frontiers[&2]; - assert_eq!(b2_df.len(), 1); - assert!(b2_df.contains(&1)); - + assert!(!dom_frontiers.contains_key(&1)); + assert!(!dom_frontiers.contains_key(&2)); // b3 is the exit block which does not dominate any blocks assert!(!dom_frontiers.contains_key(&3)); @@ -684,10 +672,7 @@ mod tests { assert_eq!(b4_df.len(), 1); assert!(b4_df.contains(&5)); - // b5 has DF { b1 } also due to the loop b5 -> b1 - let b5_df = &dom_frontiers[&5]; - assert_eq!(b5_df.len(), 1); - assert!(b5_df.contains(&1)); + assert!(!dom_frontiers.contains_key(&5)); } #[test] @@ -701,7 +686,6 @@ mod tests { let mut post_dom = DominatorTree::with_cfg_and_post_order(&reversed_cfg, &post_order); let post_dom_frontiers = post_dom.compute_dominance_frontiers(&reversed_cfg); - dbg!(post_dom_frontiers.clone()); // Convert BasicBlockIds to their underlying u32 values for easy comparisons let post_dom_frontiers = post_dom_frontiers @@ -720,18 +704,9 @@ mod tests { // b0 is the entry node of the program and the exit block of the post-dominator tree. // Thus, it has an empty set for its post-dominance frontier (PDF) assert!(!post_dom_frontiers.contains_key(&0)); - - // b1 is in its own PDF due to the loop b5 -> b1 - // b1 post-dominates b2 and b5. b1 post-dominates itself but not strictly post-dominate itself. - let b1_pdf = &post_dom_frontiers[&1]; - assert_eq!(b1_pdf.len(), 1); - assert!(b1_pdf.contains(&1)); - - // b2 has DF { b1 } also due to the loop b5 -> b1. - // b1 post-dominates itself is a predecessor of b2, but b1 does not strictly post-dominate b2. - let b2_pdf = &post_dom_frontiers[&2]; - assert_eq!(b2_pdf.len(), 1); - assert!(b2_pdf.contains(&1)); + // We must go through b1 and b2 to reach the exist node + assert!(!post_dom_frontiers.contains_key(&1)); + assert!(!post_dom_frontiers.contains_key(&2)); // b3 is the exit block of the program, but the starting node of the post-dominator tree // Thus, it has an empty PDF @@ -743,10 +718,8 @@ mod tests { assert_eq!(b4_pdf.len(), 1); assert!(b4_pdf.contains(&2)); - // b5 has DF { b1 } also due to the loop b5 -> b1 - let b5_pdf = &post_dom_frontiers[&5]; - assert_eq!(b5_pdf.len(), 1); - assert!(b5_pdf.contains(&1)); + // Must go through b5 to reach the exit node + assert!(!post_dom_frontiers.contains_key(&5)); } #[test]