From 7fa612c837ddd62fda74341f8e2e8c544bcb6627 Mon Sep 17 00:00:00 2001 From: oflatt Date: Wed, 22 May 2024 11:01:09 -0700 Subject: [PATCH 1/3] improve block fusing --- src/rvsdg/optimize_direct_jumps.rs | 83 +++++++++---------- ...mize_direct_jumps__add_block_ind_test.snap | 2 +- ...state_mem_to_cfg_more_blocks_snapshot.snap | 12 +-- ...ests__rvsdg_state_mem_to_cfg_snapshot.snap | 2 +- 4 files changed, 47 insertions(+), 52 deletions(-) diff --git a/src/rvsdg/optimize_direct_jumps.rs b/src/rvsdg/optimize_direct_jumps.rs index a89c92748..577ea7109 100644 --- a/src/rvsdg/optimize_direct_jumps.rs +++ b/src/rvsdg/optimize_direct_jumps.rs @@ -8,7 +8,7 @@ use hashbrown::HashMap; use petgraph::{ graph::EdgeIndex, stable_graph::{NodeIndex, StableDiGraph, StableGraph}, - visit::{Bfs, EdgeRef}, + visit::{DfsPostOrder, EdgeRef}, Direction, }; @@ -19,10 +19,13 @@ use crate::Optimizer; impl SimpleCfgFunction { pub fn optimize_jumps(&self) -> Self { - self.single_in_single_out().collapse_empty_blocks() + self.fuse_down().collapse_empty_blocks() } - fn single_in_single_out(&self) -> SimpleCfgFunction { + /// Find cases where a block jumps directly to another block A -> B where B + /// A has only one outgoing edge and B has one incoming edge + /// Turn it into one block AB + fn fuse_down(&self) -> SimpleCfgFunction { let mut resulting_graph: StableGraph = StableDiGraph::new(); // a map from nodes in the old graph to nodes in the @@ -34,52 +37,53 @@ impl SimpleCfgFunction { // we use a bfs so that previous nodes are mapped to new nodes // before their children. // This ensures that `node_mapping[&previous]` succeeds. - let mut bfs = Bfs::new(&self.graph, self.entry); + let mut dfs = DfsPostOrder::new(&self.graph, self.entry); let mut edges_to_add = vec![]; // copy the graph without the edges // also choose which nodes get fused to which // by re-assigning in the node map - while let Some(node) = bfs.next(&self.graph) { - let mut collapse_node = false; - let edges = self + while let Some(node) = dfs.next(&self.graph) { + let outgoing_from_node = self .graph - .edges_directed(node, Direction::Incoming) + .edges_directed(node, Direction::Outgoing) .collect::>(); - // single incoming edge to node - if let &[single_edge] = edges.as_slice() { - let previous = single_edge.source(); - let previous_outgoing = self + let target = if let &[single_edge] = outgoing_from_node.as_slice() { + let target = single_edge.target(); + let incoming_to_next = self .graph - .edges_directed(previous, Direction::Outgoing) - .collect::>(); - // single outgoing edge from previous - // and two distinct nodes - if previous_outgoing.len() == 1 && previous != node { - let previous_new = node_mapping[&previous]; - - // this node will be mapped to the previous - node_mapping.insert(node, previous_new); - - // add instructions to the end of the previous node - resulting_graph[previous_new] - .instrs - .extend(self.graph[node].instrs.to_vec()); - resulting_graph[previous_new] - .footer - .extend(self.graph[node].footer.to_vec()); - - collapse_node = true; + .edges_directed(target, Direction::Incoming) + .count(); + if incoming_to_next == 1 { + Some(target) + } else { + None } - } - - if !collapse_node { + } else { + None + }; + // single outgoing edge + if let Some(next) = target { + let new_target = node_mapping[&next]; + + // this node will be mapped to the previous + node_mapping.insert(node, new_target); + + // add instructions to the beginning of the next node + let mut new_instrs = self.graph[node].instrs.to_vec(); + let mut new_footer = self.graph[node].footer.to_vec(); + new_instrs.extend(resulting_graph[new_target].instrs.to_vec()); + new_footer.extend(resulting_graph[new_target].footer.to_vec()); + + resulting_graph[new_target].instrs = new_instrs; + resulting_graph[new_target].footer = new_footer; + } else { // add the node let new_node = resulting_graph.add_node(self.graph[node].clone()); node_mapping.insert(node, new_node); - edges_to_add.extend(self.graph.edges_directed(node, Direction::Incoming)); + edges_to_add.extend(self.graph.edges_directed(node, Direction::Outgoing)); } } @@ -89,15 +93,6 @@ impl SimpleCfgFunction { resulting_graph.add_edge(source, target, edge.weight().clone()); } - let mut label_mapping = HashMap::::new(); - for (old, new) in node_mapping.iter() { - let old_entry = label_mapping.insert( - self.graph[*old].name.to_string(), - resulting_graph[*new].name.to_string(), - ); - assert!(old_entry.is_none(), "Duplicate labels in graph"); - } - SimpleCfgFunction { name: self.name.clone(), args: self.args.clone(), diff --git a/src/rvsdg/snapshots/eggcc__rvsdg__optimize_direct_jumps__add_block_ind_test.snap b/src/rvsdg/snapshots/eggcc__rvsdg__optimize_direct_jumps__add_block_ind_test.snap index 45a30ebfe..5c116d6e3 100644 --- a/src/rvsdg/snapshots/eggcc__rvsdg__optimize_direct_jumps__add_block_ind_test.snap +++ b/src/rvsdg/snapshots/eggcc__rvsdg__optimize_direct_jumps__add_block_ind_test.snap @@ -3,7 +3,7 @@ source: src/rvsdg/optimize_direct_jumps.rs expression: cfg.optimize_jumps().to_bril().to_string() --- @main { -.entry___: +.exit___: v0: int = const 1; v1: int = const 2; v2: int = add v0 v1; diff --git a/src/rvsdg/snapshots/eggcc__rvsdg__tests__rvsdg_state_mem_to_cfg_more_blocks_snapshot.snap b/src/rvsdg/snapshots/eggcc__rvsdg__tests__rvsdg_state_mem_to_cfg_more_blocks_snapshot.snap index 8acca5418..db37e57b9 100644 --- a/src/rvsdg/snapshots/eggcc__rvsdg__tests__rvsdg_state_mem_to_cfg_more_blocks_snapshot.snap +++ b/src/rvsdg/snapshots/eggcc__rvsdg__tests__rvsdg_state_mem_to_cfg_more_blocks_snapshot.snap @@ -3,24 +3,24 @@ source: src/rvsdg/tests.rs expression: prog.to_string() --- @main { -.__0__: +.__21__: v1: int = const 1; v4: ptr = alloc v1; v7: int = const 10; store v4 v7; v11: int = load v4; v14: bool = lt v11 v7; - br v14 .__33__ .__22__; -.__33__: + br v14 .__39__ .__32__; +.__39__: print v11; free v4; v31: int = id v7; - jmp .__40__; -.__22__: + jmp .__42__; +.__32__: v24: int = add v11 v1; free v4; print v24; v31: int = id v7; -.__40__: +.__42__: print v31; } diff --git a/src/rvsdg/snapshots/eggcc__rvsdg__tests__rvsdg_state_mem_to_cfg_snapshot.snap b/src/rvsdg/snapshots/eggcc__rvsdg__tests__rvsdg_state_mem_to_cfg_snapshot.snap index 1449554d9..b229c675d 100644 --- a/src/rvsdg/snapshots/eggcc__rvsdg__tests__rvsdg_state_mem_to_cfg_snapshot.snap +++ b/src/rvsdg/snapshots/eggcc__rvsdg__tests__rvsdg_state_mem_to_cfg_snapshot.snap @@ -3,7 +3,7 @@ source: src/rvsdg/tests.rs expression: prog.to_string() --- @main { -.__0__: +.__16__: v1: int = const 1; v4: ptr = alloc v1; v8: int = const 10; From a34c3003d291a866575177f45ce746a9e2d58ae1 Mon Sep 17 00:00:00 2001 From: oflatt Date: Wed, 22 May 2024 11:38:01 -0700 Subject: [PATCH 2/3] makefile simpler, fix bug --- Makefile | 9 ++++++--- src/rvsdg/optimize_direct_jumps.rs | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 0a05c241a..d828f989f 100644 --- a/Makefile +++ b/Makefile @@ -5,16 +5,19 @@ DIRS = . dag_in_context all: nits test test: - $(foreach dir,$(DIRS),(cd $(dir) && cargo insta test --release --unreferenced=reject) &&) : + cargo insta test --release --unreferenced=reject + cd dag_in_context && cargo insta test --release --unreferenced=reject test-clean: - $(foreach dir,$(DIRS),(cd $(dir) && cargo insta test --release --unreferenced=delete) &&) : + cargo insta test --release --unreferenced=delete + cd dag_in_context && cargo insta test --release --unreferenced=delete nits: npx prettier infra/nightly-resources/*.js --check @rustup component add clippy @rustup component add rustfmt - $(foreach dir,$(DIRS),(cd $(dir) && cargo clippy --tests -- -D warnings && cargo fmt --check) &&) : + cargo clippy --tests -- -D warnings && cargo fmt --check + cd dag_in_context && cargo clippy --tests -- -D warnings && cargo fmt --check diff --git a/src/rvsdg/optimize_direct_jumps.rs b/src/rvsdg/optimize_direct_jumps.rs index 577ea7109..7809dfd77 100644 --- a/src/rvsdg/optimize_direct_jumps.rs +++ b/src/rvsdg/optimize_direct_jumps.rs @@ -55,7 +55,7 @@ impl SimpleCfgFunction { .graph .edges_directed(target, Direction::Incoming) .count(); - if incoming_to_next == 1 { + if incoming_to_next == 1 && target != node { Some(target) } else { None From 1cf2e13094dfea8f796dc1da525d45bd5943e1f8 Mon Sep 17 00:00:00 2001 From: oflatt Date: Wed, 22 May 2024 15:37:47 -0700 Subject: [PATCH 3/3] update comment --- src/rvsdg/optimize_direct_jumps.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rvsdg/optimize_direct_jumps.rs b/src/rvsdg/optimize_direct_jumps.rs index 7809dfd77..dab0c1a28 100644 --- a/src/rvsdg/optimize_direct_jumps.rs +++ b/src/rvsdg/optimize_direct_jumps.rs @@ -34,9 +34,9 @@ impl SimpleCfgFunction { // it points to the new, fused node let mut node_mapping: HashMap = HashMap::new(); - // we use a bfs so that previous nodes are mapped to new nodes - // before their children. - // This ensures that `node_mapping[&previous]` succeeds. + // we use a dfs post order + // so dependencies are visited before parents + // This ensures that `node_mapping[&next]` succeeds. let mut dfs = DfsPostOrder::new(&self.graph, self.entry); let mut edges_to_add = vec![];