From 7add2547d5236efea19d9b9958a859bdc761eec0 Mon Sep 17 00:00:00 2001 From: guipublic Date: Wed, 25 Jun 2025 13:35:51 +0000 Subject: [PATCH 1/5] Compute the dominance frontier of the reverse cfg on the extended reverse cfg --- compiler/noirc_evaluator/src/ssa/ir/cfg.rs | 42 +++++++++++++++++++ .../src/ssa/opt/loop_invariant.rs | 2 +- .../Nargo.toml | 7 ++++ .../src/main.nr | 17 ++++++++ .../src_hash.txt | 1 + 5 files changed, 68 insertions(+), 1 deletion(-) create mode 100644 test_programs/compile_success_no_bug/post_order_for_unreachable_blocks/Nargo.toml create mode 100644 test_programs/compile_success_no_bug/post_order_for_unreachable_blocks/src/main.nr create mode 100644 test_programs/compile_success_no_bug/post_order_for_unreachable_blocks/src_hash.txt diff --git a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs index 87a43d66c03..9c9b32126d1 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs @@ -148,6 +148,48 @@ impl ControlFlowGraph { pub(crate) fn compute_entry_blocks(&self) -> Vec { self.data.keys().filter(|&&block| self.predecessors(block).len() == 0).copied().collect() } + + /// Computes the reverse graph of the extended CFG. + /// The extended CFG is the CFG with an additional unique exit node (if there is none) + /// such that there is a path from every block to the exit node. + pub(crate) fn extended_reverse(func: &mut Function) -> Self { + let mut cfg = Self::with_function(func); + // Exit blocks are the ones having no successor + let exit_nodes: Vec = + cfg.data.keys().filter(|&&block| cfg.successors(block).len() == 0).copied().collect(); + // Traverse the reverse CFG from the exit blocks + let reverse = cfg.reverse(); + let post_order = crate::ssa::ir::post_order::PostOrder::with_cfg(&reverse); + // Extract blocks that are not reachable from the exit blocks + let dead_blocks: Vec = cfg + .data + .keys() + .filter(|&block| !post_order.as_slice().contains(block)) + .copied() + .collect(); + + // If some blocks, that we call 'dead' blocks, are not in the post-order traversal of the reverse CFG, + // or if there are multiple exit nodes, then the reverse CFG is not a CFG because + // it does not have a single entry node and so we will not be able to apply the dominance frontier algorithm. + // In that case, we extend the CFG with a new 'exit' node and connect the exit blocks and the 'dead' blocks to it. + if exit_nodes.len() > 1 || !dead_blocks.is_empty() { + // Create a fake 'exit' block + let exit = func.dfg.make_block(); + cfg.data.insert(exit, CfgNode::default()); + // Connect the exit nodes to it + for e in exit_nodes { + cfg.add_edge(e, exit); + } + // Connect the 'dead' blocks to it + for block in dead_blocks { + if !post_order.as_slice().contains(&block) { + cfg.add_edge(block, exit); + } + } + } + // We can now reverse the extended CFG + cfg.reverse() + } } #[cfg(test)] diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 1550aaec171..ce56b134a1b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -192,7 +192,7 @@ struct LoopInvariantContext<'f> { impl<'f> LoopInvariantContext<'f> { fn new(function: &'f mut Function) -> Self { let cfg = ControlFlowGraph::with_function(function); - let reversed_cfg = cfg.reverse(); + let reversed_cfg = ControlFlowGraph::extended_reverse(function); 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); diff --git a/test_programs/compile_success_no_bug/post_order_for_unreachable_blocks/Nargo.toml b/test_programs/compile_success_no_bug/post_order_for_unreachable_blocks/Nargo.toml new file mode 100644 index 00000000000..b7bccda97fb --- /dev/null +++ b/test_programs/compile_success_no_bug/post_order_for_unreachable_blocks/Nargo.toml @@ -0,0 +1,7 @@ + + [package] + name = "post_order_for_unreachable_blocks" + type = "bin" + authors = [""] + + [dependencies] \ No newline at end of file diff --git a/test_programs/compile_success_no_bug/post_order_for_unreachable_blocks/src/main.nr b/test_programs/compile_success_no_bug/post_order_for_unreachable_blocks/src/main.nr new file mode 100644 index 00000000000..915865e282d --- /dev/null +++ b/test_programs/compile_success_no_bug/post_order_for_unreachable_blocks/src/main.nr @@ -0,0 +1,17 @@ +fn main() { + // Safety: test + unsafe { func_2(true) }; +} + +unconstrained fn func_2(cond: bool) { + loop { + if cond { + break; + } + loop { + if false { + break + } + } + } +} diff --git a/test_programs/compile_success_no_bug/post_order_for_unreachable_blocks/src_hash.txt b/test_programs/compile_success_no_bug/post_order_for_unreachable_blocks/src_hash.txt new file mode 100644 index 00000000000..5a383f0e9d6 --- /dev/null +++ b/test_programs/compile_success_no_bug/post_order_for_unreachable_blocks/src_hash.txt @@ -0,0 +1 @@ +32904228461474130 \ No newline at end of file From db1b7f1481ea6f35c74d1a0b51982f01f3891c5d Mon Sep 17 00:00:00 2001 From: guipublic Date: Wed, 25 Jun 2025 14:19:09 +0000 Subject: [PATCH 2/5] snapshot --- .../execute__tests__expanded.snap | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 tooling/nargo_cli/tests/snapshots/compile_success_no_bug/post_order_for_unreachable_blocks/execute__tests__expanded.snap diff --git a/tooling/nargo_cli/tests/snapshots/compile_success_no_bug/post_order_for_unreachable_blocks/execute__tests__expanded.snap b/tooling/nargo_cli/tests/snapshots/compile_success_no_bug/post_order_for_unreachable_blocks/execute__tests__expanded.snap new file mode 100644 index 00000000000..0dd440a8ff4 --- /dev/null +++ b/tooling/nargo_cli/tests/snapshots/compile_success_no_bug/post_order_for_unreachable_blocks/execute__tests__expanded.snap @@ -0,0 +1,19 @@ +--- +source: tooling/nargo_cli/tests/execute.rs +expression: expanded_code +--- +fn main() { + // Safety: comment added by `nargo expand` + unsafe { func_2(true) }; +} + +unconstrained fn func_2(cond: bool) { + loop { + if cond { break; }; + loop { + if false { + break; + } + } + } +} From 1f98d3d0da61d4253ddbf3e7cac72403b1126415 Mon Sep 17 00:00:00 2001 From: guipublic Date: Wed, 25 Jun 2025 14:32:55 +0000 Subject: [PATCH 3/5] code review --- compiler/noirc_evaluator/src/ssa/ir/cfg.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs index 9c9b32126d1..bcf6544bd65 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs @@ -5,6 +5,7 @@ use super::{ function::Function, }; use fxhash::FxHashMap as HashMap; +use std::collections::HashSet; /// A container for the successors and predecessors of some Block. #[derive(Clone, Default)] @@ -161,12 +162,9 @@ impl ControlFlowGraph { let reverse = cfg.reverse(); let post_order = crate::ssa::ir::post_order::PostOrder::with_cfg(&reverse); // Extract blocks that are not reachable from the exit blocks - let dead_blocks: Vec = cfg - .data - .keys() - .filter(|&block| !post_order.as_slice().contains(block)) - .copied() - .collect(); + let rpo_traversal: HashSet = HashSet::from_iter(post_order.into_vec()); + let dead_blocks: Vec = + cfg.data.keys().filter(|&block| !rpo_traversal.contains(block)).copied().collect(); // If some blocks, that we call 'dead' blocks, are not in the post-order traversal of the reverse CFG, // or if there are multiple exit nodes, then the reverse CFG is not a CFG because @@ -182,9 +180,7 @@ impl ControlFlowGraph { } // Connect the 'dead' blocks to it for block in dead_blocks { - if !post_order.as_slice().contains(&block) { - cfg.add_edge(block, exit); - } + cfg.add_edge(block, exit); } } // We can now reverse the extended CFG From f0813c4c45babe4891609a9e5a448e38c952f4db Mon Sep 17 00:00:00 2001 From: guipublic Date: Wed, 25 Jun 2025 14:57:52 +0000 Subject: [PATCH 4/5] code review --- compiler/noirc_evaluator/src/ssa/ir/cfg.rs | 32 ++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs index bcf6544bd65..db4cd216211 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs @@ -153,6 +153,38 @@ impl ControlFlowGraph { /// Computes the reverse graph of the extended CFG. /// The extended CFG is the CFG with an additional unique exit node (if there is none) /// such that there is a path from every block to the exit node. + /// Ex: below the forward CFG has one exit node: b2 + /// However, there is no path from b5 to b2 + /// forward reverse + /// ------- ------- + /// b0* b0 + /// | ^ + /// v | + /// b1 b1 + /// / \ ^ ^ + /// v v / \ + /// b3 b4 b3 b4 + /// | | ^ ^ + /// v v | | + /// b2 b5 <-| b2* b5 <-| + /// \___| \___| + /// + /// The extended CFG is the forward CFG with a new 'exit' node: + /// extended extended reverse + /// ------- ------- + /// b0* b0 + /// | ^ + /// v | + /// b1 b1 + /// / \ ^ ^ + /// v v / \ + /// b3 b4 b3 b4 + /// | | ^ ^ + /// v v | | + /// b2 b5 <-| b2* b5 <-| + /// \ /\___| ^ ^\___| + /// v v \ / + /// exit exit pub(crate) fn extended_reverse(func: &mut Function) -> Self { let mut cfg = Self::with_function(func); // Exit blocks are the ones having no successor From d071d7eaf0bd73ea1c312262fc66dbfeaa0ee61b Mon Sep 17 00:00:00 2001 From: guipublic Date: Wed, 25 Jun 2025 15:03:28 +0000 Subject: [PATCH 5/5] code review --- compiler/noirc_evaluator/src/ssa/ir/cfg.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs index db4cd216211..77bccc004c1 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs @@ -181,10 +181,10 @@ impl ControlFlowGraph { /// b3 b4 b3 b4 /// | | ^ ^ /// v v | | - /// b2 b5 <-| b2* b5 <-| + /// b2 b5 <-| b2 b5 <-| /// \ /\___| ^ ^\___| /// v v \ / - /// exit exit + /// exit exit* pub(crate) fn extended_reverse(func: &mut Function) -> Self { let mut cfg = Self::with_function(func); // Exit blocks are the ones having no successor