diff --git a/Cargo.lock b/Cargo.lock index 15ad7806a17..e3ce4ca8748 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -633,7 +633,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "40723b8fb387abc38f4f4a37c09073622e41dd12327033091ef8950659e6dc0c" dependencies = [ "memchr", - "regex-automata 0.4.8", + "regex-automata 0.4.9", "serde", ] @@ -712,9 +712,9 @@ checksum = "37b2a672a2cb129a2e41c10b1224bb368f9f37a2b16b612598138befd7b37eb5" [[package]] name = "cc" -version = "1.1.36" +version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "baee610e9452a8f6f0a1b6194ec09ff9e2d85dea54432acdae41aa0761c95d70" +checksum = "1aeb932158bd710538c73702db6945cb68a8fb08c519e6e12706b94263b36db8" dependencies = [ "shlex", ] @@ -775,9 +775,9 @@ dependencies = [ [[package]] name = "clap" -version = "4.5.20" +version = "4.5.21" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b97f376d85a664d5837dbae44bf546e6477a679ff6610010f17276f686d867e8" +checksum = "fb3b4b9e5a7c7514dfa52869339ee98b3156b0bfb4e8a77c4ff4babb64b1604f" dependencies = [ "clap_builder", "clap_derive", @@ -793,9 +793,9 @@ dependencies = [ [[package]] name = "clap_builder" -version = "4.5.20" +version = "4.5.21" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "19bc80abd44e4bed93ca373a0704ccbd1b710dc5749406201bb018272808dc54" +checksum = "b17a95aa67cc7b5ebd32aa5370189aa0d79069ef1c64ce893bd30fb24bff20ec" dependencies = [ "anstream", "anstyle", @@ -805,9 +805,9 @@ dependencies = [ [[package]] name = "clap_complete" -version = "4.5.37" +version = "4.5.38" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "11611dca53440593f38e6b25ec629de50b14cdfa63adc0fb856115a2c6d97595" +checksum = "d9647a559c112175f17cf724dc72d3645680a883c58481332779192b0d8e7a01" dependencies = [ "clap", ] @@ -826,9 +826,9 @@ dependencies = [ [[package]] name = "clap_lex" -version = "0.7.2" +version = "0.7.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1462739cb27611015575c0c11df5df7601141071f07518d56fcc1be504cbec97" +checksum = "afb84c814227b90d6895e01398aee0d8033c00e7466aca416fb6a8e0eb19d8a7" [[package]] name = "clipboard-win" @@ -1004,9 +1004,9 @@ dependencies = [ [[package]] name = "cpufeatures" -version = "0.2.14" +version = "0.2.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "608697df725056feaccfa42cffdaeeec3fccc4ffc38358ecd19b243e716a78e0" +checksum = "0ca741a962e1b0bff6d724a1a0958b686406e853bb14061f218562e1896f95e6" dependencies = [ "libc", ] @@ -1120,9 +1120,9 @@ dependencies = [ [[package]] name = "csv" -version = "1.3.0" +version = "1.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ac574ff4d437a7b5ad237ef331c17ccca63c46479e5b5453eb8e10bb99a759fe" +checksum = "acdc4883a9c96732e4733212c01447ebd805833b7275a73ca3ee080fd77afdaf" dependencies = [ "csv-core", "itoa", @@ -1769,7 +1769,7 @@ dependencies = [ "aho-corasick", "bstr", "log", - "regex-automata 0.4.8", + "regex-automata 0.4.9", "regex-syntax 0.8.5", ] @@ -2180,7 +2180,7 @@ dependencies = [ "globset", "log", "memchr", - "regex-automata 0.4.8", + "regex-automata 0.4.9", "same-file", "walkdir", "winapi-util", @@ -3145,6 +3145,7 @@ dependencies = [ "noirc_frontend", "num-bigint", "num-traits", + "petgraph", "proptest", "rayon", "serde", @@ -3956,7 +3957,7 @@ checksum = "b544ef1b4eac5dc2db33ea63606ae9ffcfac26c1416a2806ae0bf5f56b201191" dependencies = [ "aho-corasick", "memchr", - "regex-automata 0.4.8", + "regex-automata 0.4.9", "regex-syntax 0.8.5", ] @@ -3971,9 +3972,9 @@ dependencies = [ [[package]] name = "regex-automata" -version = "0.4.8" +version = "0.4.9" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "368758f23274712b504848e9d5a6f010445cc8b87a7cdb4d7cbee666c1288da3" +checksum = "809e8dc61f6de73b46c85f4c96486310fe304c434cfa43669d7b40f711150908" dependencies = [ "aho-corasick", "memchr", @@ -4082,9 +4083,9 @@ dependencies = [ [[package]] name = "rustix" -version = "0.38.39" +version = "0.38.40" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "375116bee2be9ed569afe2154ea6a99dfdffd257f533f187498c2a8f5feaf4ee" +checksum = "99e4ea3e1cdc4b559b8e5650f9c8e5998e3e5c1343b4eaf034565f32318d63c0" dependencies = [ "bitflags 2.6.0", "errno", @@ -4250,9 +4251,9 @@ dependencies = [ [[package]] name = "serde" -version = "1.0.214" +version = "1.0.215" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f55c3193aca71c12ad7890f1785d2b73e1b9f63a0bbc353c08ef26fe03fc56b5" +checksum = "6513c1ad0b11a9376da888e3e0baa0077f1aed55c17f50e7b2397136129fb88f" dependencies = [ "serde_derive", ] @@ -4293,9 +4294,9 @@ dependencies = [ [[package]] name = "serde_derive" -version = "1.0.214" +version = "1.0.215" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "de523f781f095e28fa605cdce0f8307e451cc0fd14e2eb4cd2e98a355b147766" +checksum = "ad1e866f866923f252f05c889987993144fb74e722403468a4ebd70c3cd756c0" dependencies = [ "proc-macro2", "quote", @@ -4754,18 +4755,18 @@ dependencies = [ [[package]] name = "thiserror" -version = "1.0.68" +version = "1.0.69" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "02dd99dc800bbb97186339685293e1cc5d9df1f8fae2d0aecd9ff1c77efea892" +checksum = "b6aaf5339b578ea85b50e080feb250a3e8ae8cfcdff9a461c9ec2904bc923f52" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "1.0.68" +version = "1.0.69" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a7c61ec9a6f64d2793d8a45faba21efbe3ced62a886d44c36a009b2b519b4c7e" +checksum = "4fee6c4efc90059e10f81e6d42c60a18f76588c3d74cb83a0b242a2b6c7504c1" dependencies = [ "proc-macro2", "quote", diff --git a/Cargo.toml b/Cargo.toml index 214ed9a5bcb..4ed8b2626ef 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -163,6 +163,7 @@ tracing = "0.1.40" tracing-web = "0.1.3" tracing-subscriber = { version = "0.3.18", features = ["env-filter"] } rust-embed = "6.6.0" +petgraph = "0.6" [profile.dev] # This is required to be able to run `cargo test` in acvm_js due to the `locals exceeds maximum` error. diff --git a/compiler/noirc_evaluator/Cargo.toml b/compiler/noirc_evaluator/Cargo.toml index aac07339dd6..884b649989c 100644 --- a/compiler/noirc_evaluator/Cargo.toml +++ b/compiler/noirc_evaluator/Cargo.toml @@ -29,6 +29,7 @@ tracing.workspace = true chrono = "0.4.37" rayon.workspace = true cfg-if.workspace = true +petgraph.workspace = true [dev-dependencies] proptest.workspace = true diff --git a/compiler/noirc_evaluator/src/ssa/ir/function.rs b/compiler/noirc_evaluator/src/ssa/ir/function.rs index e8245ff6036..0412f47018e 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/function.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/function.rs @@ -2,8 +2,12 @@ use std::collections::BTreeSet; use iter_extended::vecmap; use noirc_frontend::monomorphization::ast::InlineType; +use petgraph::graph::NodeIndex; +use petgraph::prelude::DiGraph; use serde::{Deserialize, Serialize}; +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; + use super::basic_block::BasicBlockId; use super::dfg::DataFlowGraph; use super::instruction::TerminatorInstruction; @@ -187,6 +191,46 @@ impl Function { unreachable!("SSA Function {} has no reachable return instruction!", self.id()) } + + /// Return each SCC (strongly-connected component) of the CFG of this function + /// in postorder. In practice this is identical to a normal CFG except that + /// blocks within loops will all be grouped together in the same SCC. + pub(crate) fn postorder_scc_cfg(&self) -> Vec> { + let mut cfg = DiGraph::new(); + let mut stack = vec![self.entry_block]; + + let mut visited = HashSet::default(); + let mut block_to_index = HashMap::default(); + let mut index_to_block = HashMap::default(); + + // Add or create a block node in the cfg + let mut get_block_index = |cfg: &mut DiGraph<_, _>, block| { + block_to_index.get(&block).copied().unwrap_or_else(|| { + let index = cfg.add_node(block); + block_to_index.insert(block, index); + index_to_block.insert(index, block); + index + }) + }; + + // Populate each reachable block & edges between them + while let Some(block) = stack.pop() { + if visited.insert(block) { + let block_index = get_block_index(&mut cfg, block); + + for successor in self.dfg[block].successors() { + stack.push(successor); + let successor_index = get_block_index(&mut cfg, successor); + cfg.add_edge(block_index, successor_index, ()); + } + } + } + + // Perform tarjan_scc to get strongly connected components. + // Lucky for us, this already returns SCCs in postorder. + let postorder: Vec> = petgraph::algo::tarjan_scc(&cfg); + vecmap(postorder, |indices| vecmap(indices, |index| index_to_block[&index])) + } } impl std::fmt::Display for RuntimeType { diff --git a/compiler/noirc_evaluator/src/ssa/ir/post_order.rs b/compiler/noirc_evaluator/src/ssa/ir/post_order.rs index 94ff96ba1d7..cb0112f32a0 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/post_order.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/post_order.rs @@ -15,6 +15,12 @@ enum Visit { pub(crate) struct PostOrder(Vec); +/// An SCC Post Order is a post-order visit of each strongly-connected +/// component in a function's control flow graph. In practice this is +/// identical to a normal post order except that blocks within loops +/// will all be grouped together within the same visit step. +pub(crate) struct SccPostOrder(Vec>); + impl PostOrder { pub(crate) fn as_slice(&self) -> &[BasicBlockId] { self.0.as_slice() @@ -24,49 +30,49 @@ impl PostOrder { impl PostOrder { /// Allocate and compute a function's block post-order. Pos pub(crate) fn with_function(func: &Function) -> Self { - PostOrder(Self::compute_post_order(func)) + PostOrder(compute_post_order(func)) } pub(crate) fn into_vec(self) -> Vec { self.0 } +} - // Computes the post-order of the function by doing a depth-first traversal of the - // function's entry block's previously unvisited children. Each block is sequenced according - // to when the traversal exits it. - fn compute_post_order(func: &Function) -> Vec { - let mut stack = vec![(Visit::First, func.entry_block())]; - let mut visited: HashSet = HashSet::new(); - let mut post_order: Vec = Vec::new(); - - while let Some((visit, block_id)) = stack.pop() { - match visit { - Visit::First => { - if !visited.contains(&block_id) { - // This is the first time we pop the block, so we need to scan its - // successors and then revisit it. - visited.insert(block_id); - stack.push((Visit::Last, block_id)); - // Stack successors for visiting. Because items are taken from the top of the - // stack, we push the item that's due for a visit first to the top. - for successor_id in func.dfg[block_id].successors().rev() { - if !visited.contains(&successor_id) { - // This not visited check would also be cover by the next - // iteration, but checking here two saves an iteration per successor. - stack.push((Visit::First, successor_id)); - } +// Computes the post-order of the function by doing a depth-first traversal of the +// function's entry block's previously unvisited children. Each block is sequenced according +// to when the traversal exits it. +fn compute_post_order(func: &Function) -> Vec { + let mut stack = vec![(Visit::First, func.entry_block())]; + let mut visited: HashSet = HashSet::new(); + let mut post_order: Vec = Vec::new(); + + while let Some((visit, block_id)) = stack.pop() { + match visit { + Visit::First => { + if !visited.contains(&block_id) { + // This is the first time we pop the block, so we need to scan its + // successors and then revisit it. + visited.insert(block_id); + stack.push((Visit::Last, block_id)); + // Stack successors for visiting. Because items are taken from the top of the + // stack, we push the item that's due for a visit first to the top. + for successor_id in func.dfg[block_id].successors() { + if !visited.contains(&successor_id) { + // This not visited check would also be cover by the next + // iteration, but checking here two saves an iteration per successor. + stack.push((Visit::First, successor_id)); } } } + } - Visit::Last => { - // We've finished all this node's successors. - post_order.push(block_id); - } + Visit::Last => { + // We've finished all this node's successors. + post_order.push(block_id); } } - post_order } + post_order } #[cfg(test)] @@ -104,7 +110,7 @@ mod tests { // D (seen) // } -> push(A) // Result: - // D, F, E, B, A, (C dropped as unreachable) + // F, E, B, D, A, (C dropped as unreachable) let func_id = Id::test_new(0); let mut builder = FunctionBuilder::new("func".into(), func_id); @@ -145,6 +151,6 @@ mod tests { let func = ssa.main(); let post_order = PostOrder::with_function(func); let block_a_id = func.entry_block(); - assert_eq!(post_order.0, [block_d_id, block_f_id, block_e_id, block_b_id, block_a_id]); + assert_eq!(post_order.0, [block_f_id, block_e_id, block_b_id, block_d_id, block_a_id]); } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs index 96de22600a4..f1c7c523795 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs @@ -208,15 +208,15 @@ mod tests { jmp b1(u32 0) b1(v0: u32): v8 = lt v0, u32 5 - jmpif v8 then: b3, else: b2 - b3(): + jmpif v8 then: b2, else: b5 + b2(): v9 = eq v0, u32 5 - jmpif v9 then: b4, else: b5 - b4(): + jmpif v9 then: b3, else: b4 + b3(): v10 = load v4 -> [[Field; 5]; 2] store v10 at v5 - jmp b5() - b5(): + jmp b4() + b4(): v11 = load v4 -> [[Field; 5]; 2] v12 = array_get v11, index Field 0 -> [Field; 5] v14 = array_set v12, index v0, value Field 20 @@ -224,7 +224,7 @@ mod tests { store v15 at v4 v17 = add v0, u32 1 jmp b1(v17) - b2(): + b5(): return } "; diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index d8f33f6494d..7a92a39989c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -10,7 +10,6 @@ use crate::ssa::{ dfg::DataFlowGraph, function::Function, instruction::{BinaryOp, Instruction, InstructionId, Intrinsic}, - post_order::PostOrder, types::Type, value::{Value, ValueId}, }, @@ -44,15 +43,20 @@ impl Function { context.mark_used_instruction_results(&self.dfg, call_data.array_id); } + let postorder_scc_cfg = self.postorder_scc_cfg(); let mut inserted_out_of_bounds_checks = false; - let blocks = PostOrder::with_function(self); - for block in blocks.as_slice() { - inserted_out_of_bounds_checks |= context.remove_unused_instructions_in_block( - self, - *block, - insert_out_of_bounds_checks, - ); + for scc in postorder_scc_cfg { + if scc.len() == 1 { + inserted_out_of_bounds_checks |= context + .scan_and_remove_unused_instructions_in_block( + self, + scc[0], + insert_out_of_bounds_checks, + ); + } else { + context.die_in_scc(scc, self); + } } // If we inserted out of bounds check, let's run the pass again with those new @@ -97,7 +101,7 @@ impl Context { /// removing unused instructions and return `true`. The idea then is to later call this /// function again with `insert_out_of_bounds_checks` set to false to effectively remove /// unused instructions but leave the out of bounds checks. - fn remove_unused_instructions_in_block( + fn scan_and_remove_unused_instructions_in_block( &mut self, function: &mut Function, block_id: BasicBlockId, @@ -106,15 +110,13 @@ impl Context { let block = &function.dfg[block_id]; self.mark_terminator_values_as_used(function, block); - let instructions_len = block.instructions().len(); - let mut rc_tracker = RcTracker::default(); // Indexes of instructions that might be out of bounds. // We'll remove those, but before that we'll insert bounds checks for them. let mut possible_index_out_of_bounds_indexes = Vec::new(); - for (instruction_index, instruction_id) in block.instructions().iter().rev().enumerate() { + for (instruction_index, instruction_id) in block.instructions().iter().enumerate().rev() { let instruction = &function.dfg[*instruction_id]; if self.is_unused(*instruction_id, function) { @@ -123,8 +125,7 @@ impl Context { if insert_out_of_bounds_checks && instruction_might_result_in_out_of_bounds(function, instruction) { - possible_index_out_of_bounds_indexes - .push(instructions_len - instruction_index - 1); + possible_index_out_of_bounds_indexes.push(instruction_index); } } else { use Instruction::*; @@ -158,13 +159,83 @@ impl Context { } } - function.dfg[block_id] - .instructions_mut() - .retain(|instruction| !self.instructions_to_remove.contains(instruction)); - + self.remove_unused_instructions_in_block(function, block_id); false } + fn die_in_scc(&mut self, scc: Vec, function: &mut Function) { + let mut used_values_len = self.used_values.len(); + + // Continue to mark used values while we have at least one change + while { + for block in &scc { + self.mark_used_values(*block, function); + } + let len_has_changed = self.used_values.len() != used_values_len; + used_values_len = self.used_values.len(); + len_has_changed + } {} + + for block in scc { + self.populate_instructions_to_remove(function, block); + self.remove_unused_instructions_in_block(function, block); + } + } + + fn remove_unused_instructions_in_block(&self, function: &mut Function, block: BasicBlockId) { + let instructions = function.dfg[block].instructions_mut(); + instructions.retain(|instruction| !self.instructions_to_remove.contains(instruction)); + } + + fn mark_used_values(&mut self, block_id: BasicBlockId, function: &Function) { + let block = &function.dfg[block_id]; + self.mark_terminator_values_as_used(function, block); + + for instruction_id in block.instructions() { + use Instruction::*; + let instruction = &function.dfg[*instruction_id]; + // TODO: Insert OOB checks for unused array operations + + // We're just marking so avoid removing instructions that may need bounds checks + if !self.is_unused(*instruction_id, function) { + if matches!(instruction, IncrementRc { .. } | DecrementRc { .. }) { + // self.rc_instructions.push((*instruction_id, block_id)); + } else { + instruction.for_each_value(|value| { + self.mark_used_instruction_results(&function.dfg, value); + }); + } + } else if instruction_might_result_in_out_of_bounds(function, instruction) { + // Even if the array is unused, mark the index as used since we'll insert + // an OOB check on it later. + if let ArrayGet { index, .. } | ArraySet { index, .. } = instruction { + self.used_values.insert(*index); + } + } + } + } + + /// Go through a block and populate the instructions to remove list, assuming + /// `self.used_values` is already populated. + fn populate_instructions_to_remove(&mut self, function: &Function, block_id: BasicBlockId) { + let block = &function.dfg[block_id]; + for instruction_id in block.instructions() { + use Instruction::*; + let instruction = &function.dfg[*instruction_id]; + + if self.is_unused(*instruction_id, function) { + self.instructions_to_remove.insert(*instruction_id); + + // We can remove side-effectful instructions here since we marked beforehand we know + // `value` isn't used anywhere by this point. + } else if let IncrementRc { value } | DecrementRc { value } = instruction { + if !self.used_values.contains(value) { + self.instructions_to_remove.insert(*instruction_id); + } + } + } + } + /// Returns true if an instruction can be removed. /// /// An instruction can be removed as long as it has no side-effects, and none of its result diff --git a/compiler/noirc_frontend/Cargo.toml b/compiler/noirc_frontend/Cargo.toml index 581d7f1b61d..2257df29bfd 100644 --- a/compiler/noirc_frontend/Cargo.toml +++ b/compiler/noirc_frontend/Cargo.toml @@ -28,7 +28,7 @@ small-ord-set = "0.1.3" regex = "1.9.1" cfg-if.workspace = true tracing.workspace = true -petgraph = "0.6" +petgraph.workspace = true rangemap = "1.4.0" strum = "0.24" strum_macros = "0.24"