From c573b12a197da0e3cc2704f949274d19409d100b Mon Sep 17 00:00:00 2001 From: Tom French Date: Tue, 1 Oct 2024 23:54:03 +0100 Subject: [PATCH 1/4] feat: remove orphaned blocks from cfg --- compiler/noirc_evaluator/src/ssa/ir/cfg.rs | 20 +++++++++++++++---- .../src/ssa/opt/simplify_cfg.rs | 10 +++++----- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs index b9166bf1d56..24505989142 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs @@ -59,7 +59,10 @@ impl ControlFlowGraph { /// Clears out a given block's successors. This also removes the given block from /// being a predecessor of any of its previous successors. - fn invalidate_block_successors(&mut self, basic_block_id: BasicBlockId) { + fn invalidate_block_successors( + &mut self, + basic_block_id: BasicBlockId, + ) -> BTreeSet { let node = self .data .get_mut(&basic_block_id) @@ -67,13 +70,15 @@ impl ControlFlowGraph { let old_successors = std::mem::take(&mut node.successors); - for successor_id in old_successors { + for successor_id in &old_successors { self.data - .get_mut(&successor_id) + .get_mut(successor_id) .expect("ICE: Cfg node successor doesn't exist.") .predecessors .remove(&basic_block_id); } + + old_successors } /// Recompute the control flow graph of `block`. @@ -81,9 +86,16 @@ impl ControlFlowGraph { /// This is for use after modifying instructions within a specific block. It recomputes all edges /// from `basic_block_id` while leaving edges to `basic_block_id` intact. pub(crate) fn recompute_block(&mut self, func: &Function, basic_block_id: BasicBlockId) { - self.invalidate_block_successors(basic_block_id); + let old_successors = self.invalidate_block_successors(basic_block_id); let basic_block = &func.dfg[basic_block_id]; self.compute_block(basic_block_id, basic_block); + + // If we've made any of the old successors unreachable, we should remove it as a predecessor from its successors. + for orphaned_successor in old_successors.difference(&basic_block.successors().collect()) { + if self.predecessors(*orphaned_successor).len() == 0 { + self.invalidate_block_successors(*orphaned_successor); + } + } } /// Add a directed edge making `from` a predecessor of `to`. diff --git a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index b77bc8c72f3..dbd7e96a55d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -51,16 +51,16 @@ impl Function { let mut visited = HashSet::new(); while let Some(block) = stack.pop() { - if visited.insert(block) { - stack.extend(self.dfg[block].successors().filter(|block| !visited.contains(block))); - } - // This call is before try_inline_into_predecessor so that if it succeeds in changing a // jmpif into a jmp, the block may then be inlined entirely into its predecessor in try_inline_into_predecessor. check_for_constant_jmpif(self, block, &mut cfg); - let mut predecessors = cfg.predecessors(block); + // We extend the stack after checking for constant jmpifs to avoid pushing an unreachable block onto the stack. + if visited.insert(block) { + stack.extend(self.dfg[block].successors().filter(|block| !visited.contains(block))); + } + let mut predecessors = cfg.predecessors(block); if predecessors.len() == 1 { let predecessor = predecessors.next().expect("Already checked length of predecessors"); From 343d2190bb200ebc6e4addb6a133acb093ce2d68 Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 2 Oct 2024 00:48:55 +0100 Subject: [PATCH 2/4] chore: conditional orphan detection --- compiler/noirc_evaluator/src/ssa/ir/cfg.rs | 24 ++++++++++++------- .../src/ssa/opt/simplify_cfg.rs | 10 ++++---- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs index 24505989142..9ddaa891041 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs @@ -85,15 +85,23 @@ impl ControlFlowGraph { /// /// This is for use after modifying instructions within a specific block. It recomputes all edges /// from `basic_block_id` while leaving edges to `basic_block_id` intact. - pub(crate) fn recompute_block(&mut self, func: &Function, basic_block_id: BasicBlockId) { + pub(crate) fn recompute_block( + &mut self, + func: &Function, + basic_block_id: BasicBlockId, + check_for_orphans: bool, + ) { let old_successors = self.invalidate_block_successors(basic_block_id); let basic_block = &func.dfg[basic_block_id]; self.compute_block(basic_block_id, basic_block); - // If we've made any of the old successors unreachable, we should remove it as a predecessor from its successors. - for orphaned_successor in old_successors.difference(&basic_block.successors().collect()) { - if self.predecessors(*orphaned_successor).len() == 0 { - self.invalidate_block_successors(*orphaned_successor); + if check_for_orphans { + // If we've made any of the old successors unreachable, we should remove it as a predecessor from its successors. + for orphaned_successor in old_successors.difference(&basic_block.successors().collect()) + { + if self.predecessors(*orphaned_successor).len() == 0 { + self.invalidate_block_successors(*orphaned_successor); + } } } } @@ -253,9 +261,9 @@ mod tests { }); // Recompute new and changed blocks - cfg.recompute_block(&func, block0_id); - cfg.recompute_block(&func, block2_id); - cfg.recompute_block(&func, ret_block_id); + cfg.recompute_block(&func, block0_id, false); + cfg.recompute_block(&func, block2_id, false); + cfg.recompute_block(&func, ret_block_id, false); #[allow(clippy::needless_collect)] { diff --git a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index dbd7e96a55d..e57a13ad67b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -106,7 +106,7 @@ fn check_for_constant_jmpif( let call_stack = call_stack.clone(); let jmp = TerminatorInstruction::Jmp { destination, arguments, call_stack }; function.dfg[block].set_terminator(jmp); - cfg.recompute_block(function, block); + cfg.recompute_block(function, block, true); } } } @@ -171,9 +171,9 @@ fn check_for_double_jmp(function: &mut Function, block: BasicBlockId, cfg: &mut }; function.dfg[predecessor_block].set_terminator(redirected_terminator_instruction); - cfg.recompute_block(function, predecessor_block); + cfg.recompute_block(function, predecessor_block, false); } - cfg.recompute_block(function, block); + cfg.recompute_block(function, block, false); } /// If the given block has block parameters, replace them with the jump arguments from the predecessor. @@ -221,8 +221,8 @@ fn try_inline_into_predecessor( drop(successors); function.dfg.inline_block(block, predecessor); - cfg.recompute_block(function, block); - cfg.recompute_block(function, predecessor); + cfg.recompute_block(function, block, false); + cfg.recompute_block(function, predecessor, false); true } else { false From 48466fe388e28eab6889f35bc2fc79eed5561b5b Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 2 Oct 2024 11:53:39 +0100 Subject: [PATCH 3/4] chore: cleanup code --- compiler/noirc_evaluator/src/ssa/ir/cfg.rs | 36 +++++-------------- .../src/ssa/opt/simplify_cfg.rs | 32 ++++++++++------- 2 files changed, 28 insertions(+), 40 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs index 9ddaa891041..38e6efa5b9a 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs @@ -59,10 +59,7 @@ impl ControlFlowGraph { /// Clears out a given block's successors. This also removes the given block from /// being a predecessor of any of its previous successors. - fn invalidate_block_successors( - &mut self, - basic_block_id: BasicBlockId, - ) -> BTreeSet { + pub(crate) fn invalidate_block_successors(&mut self, basic_block_id: BasicBlockId) { let node = self .data .get_mut(&basic_block_id) @@ -70,40 +67,23 @@ impl ControlFlowGraph { let old_successors = std::mem::take(&mut node.successors); - for successor_id in &old_successors { + for successor_id in old_successors { self.data - .get_mut(successor_id) + .get_mut(&successor_id) .expect("ICE: Cfg node successor doesn't exist.") .predecessors .remove(&basic_block_id); } - - old_successors } /// Recompute the control flow graph of `block`. /// /// This is for use after modifying instructions within a specific block. It recomputes all edges /// from `basic_block_id` while leaving edges to `basic_block_id` intact. - pub(crate) fn recompute_block( - &mut self, - func: &Function, - basic_block_id: BasicBlockId, - check_for_orphans: bool, - ) { - let old_successors = self.invalidate_block_successors(basic_block_id); + pub(crate) fn recompute_block(&mut self, func: &Function, basic_block_id: BasicBlockId) { + self.invalidate_block_successors(basic_block_id); let basic_block = &func.dfg[basic_block_id]; self.compute_block(basic_block_id, basic_block); - - if check_for_orphans { - // If we've made any of the old successors unreachable, we should remove it as a predecessor from its successors. - for orphaned_successor in old_successors.difference(&basic_block.successors().collect()) - { - if self.predecessors(*orphaned_successor).len() == 0 { - self.invalidate_block_successors(*orphaned_successor); - } - } - } } /// Add a directed edge making `from` a predecessor of `to`. @@ -261,9 +241,9 @@ mod tests { }); // Recompute new and changed blocks - cfg.recompute_block(&func, block0_id, false); - cfg.recompute_block(&func, block2_id, false); - cfg.recompute_block(&func, ret_block_id, false); + cfg.recompute_block(&func, block0_id); + cfg.recompute_block(&func, block2_id); + cfg.recompute_block(&func, ret_block_id); #[allow(clippy::needless_collect)] { diff --git a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index e57a13ad67b..07f6d124fb9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -51,15 +51,14 @@ impl Function { let mut visited = HashSet::new(); while let Some(block) = stack.pop() { - // This call is before try_inline_into_predecessor so that if it succeeds in changing a - // jmpif into a jmp, the block may then be inlined entirely into its predecessor in try_inline_into_predecessor. - check_for_constant_jmpif(self, block, &mut cfg); - - // We extend the stack after checking for constant jmpifs to avoid pushing an unreachable block onto the stack. if visited.insert(block) { stack.extend(self.dfg[block].successors().filter(|block| !visited.contains(block))); } + // This call is before try_inline_into_predecessor so that if it succeeds in changing a + // jmpif into a jmp, the block may then be inlined entirely into its predecessor in try_inline_into_predecessor. + check_for_constant_jmpif(self, block, &mut cfg); + let mut predecessors = cfg.predecessors(block); if predecessors.len() == 1 { let predecessor = @@ -99,14 +98,23 @@ fn check_for_constant_jmpif( }) = function.dfg[block].terminator() { if let Some(constant) = function.dfg.get_numeric_constant(*condition) { - let destination = - if constant.is_zero() { *else_destination } else { *then_destination }; + let (destination, unchosen_destination) = if constant.is_zero() { + (*else_destination, *then_destination) + } else { + (*then_destination, *else_destination) + }; let arguments = Vec::new(); let call_stack = call_stack.clone(); let jmp = TerminatorInstruction::Jmp { destination, arguments, call_stack }; function.dfg[block].set_terminator(jmp); - cfg.recompute_block(function, block, true); + cfg.recompute_block(function, block); + + // If `block` was the only predecessor to `unchose_destination` then it's no long reachable through the CFG, + // we can then invalidate it successors as it's an invalid predecessor. + if cfg.predecessors(unchosen_destination).len() == 0 { + cfg.invalidate_block_successors(unchosen_destination); + } } } } @@ -171,9 +179,9 @@ fn check_for_double_jmp(function: &mut Function, block: BasicBlockId, cfg: &mut }; function.dfg[predecessor_block].set_terminator(redirected_terminator_instruction); - cfg.recompute_block(function, predecessor_block, false); + cfg.recompute_block(function, predecessor_block); } - cfg.recompute_block(function, block, false); + cfg.recompute_block(function, block); } /// If the given block has block parameters, replace them with the jump arguments from the predecessor. @@ -221,8 +229,8 @@ fn try_inline_into_predecessor( drop(successors); function.dfg.inline_block(block, predecessor); - cfg.recompute_block(function, block, false); - cfg.recompute_block(function, predecessor, false); + cfg.recompute_block(function, block); + cfg.recompute_block(function, predecessor); true } else { false From 580928bcdda8f644d8257d577e57162081cd99dd Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Wed, 2 Oct 2024 11:54:28 +0100 Subject: [PATCH 4/4] Update compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs --- compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index 07f6d124fb9..46941775c5e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -110,7 +110,7 @@ fn check_for_constant_jmpif( function.dfg[block].set_terminator(jmp); cfg.recompute_block(function, block); - // If `block` was the only predecessor to `unchose_destination` then it's no long reachable through the CFG, + // If `block` was the only predecessor to `unchosen_destination` then it's no long reachable through the CFG, // we can then invalidate it successors as it's an invalid predecessor. if cfg.predecessors(unchosen_destination).len() == 0 { cfg.invalidate_block_successors(unchosen_destination);