From d0b7d823129f8d04f2674912e10b7290cec70c54 Mon Sep 17 00:00:00 2001 From: Denis Merigoux Date: Tue, 1 Aug 2017 18:35:38 -0700 Subject: [PATCH 1/6] Added last instruction information to loop analysis Loop analysis now performs ebb splitting in case of multi-loop ebbs --- lib/cretonne/src/flowgraph.rs | 4 +- lib/cretonne/src/loop_analysis.rs | 272 ++++++++++++++++++++++++------ 2 files changed, 224 insertions(+), 52 deletions(-) diff --git a/lib/cretonne/src/flowgraph.rs b/lib/cretonne/src/flowgraph.rs index f7cb0c972..9634eca6c 100644 --- a/lib/cretonne/src/flowgraph.rs +++ b/lib/cretonne/src/flowgraph.rs @@ -118,8 +118,8 @@ impl ControlFlowGraph { } fn add_edge(&mut self, from: BasicBlock, to: Ebb) { - self.data[from.0].successors.push(to); - self.data[to].predecessors.push(from); + self.data.ensure(from.0).successors.push(to); + self.data.ensure(to).predecessors.push(from); } /// Get the CFG predecessor basic blocks to `ebb`. diff --git a/lib/cretonne/src/loop_analysis.rs b/lib/cretonne/src/loop_analysis.rs index 5a4607ba7..1cb952694 100644 --- a/lib/cretonne/src/loop_analysis.rs +++ b/lib/cretonne/src/loop_analysis.rs @@ -4,7 +4,8 @@ use dominator_tree::DominatorTree; use entity_map::{EntityMap, PrimaryEntityData, Keys}; use flowgraph::ControlFlowGraph; -use ir::{Function, Ebb, Layout}; +use ir::{Function, Ebb, Layout, Cursor, InstBuilder}; +use ir::entities::Inst; use packed_option::PackedOption; /// A opaque reference to a code loop. @@ -18,7 +19,7 @@ entity_impl!(Loop, "loop"); /// its eventual parent in the loop tree and all the EBB belonging to the loop. pub struct LoopAnalysis { loops: EntityMap, - ebb_loop_map: EntityMap>, + ebb_loop_map: EntityMap, } struct LoopData { @@ -30,7 +31,7 @@ impl PrimaryEntityData for LoopData {} impl LoopData { /// Creates a `LoopData` object with the loop header and its eventual parent in the loop tree. - pub fn new(header: Ebb, parent: Option) -> LoopData { + fn new(header: Ebb, parent: Option) -> LoopData { LoopData { header: header, parent: parent.into(), @@ -38,6 +39,25 @@ impl LoopData { } } +/// If an `Ebb` is part of a loop, then we record two things: the id of the loop it's part of +/// and the last instruction in the `Ebb` pertaining to the loop. If the `Ebb` is part of multiple +/// loops, then we make sure by splitting the `Ebb` that it is part of at most two loops, one being +/// the direct child of the other, and the parent loop should contain all the `Ebb` up to its +/// terminator instruction. +#[derive(Clone,Debug)] +enum EbbLoopData { + NoLoop(), + Loop { loop_id: Loop, last_inst: Inst }, +} + +impl PrimaryEntityData for EbbLoopData {} + +impl Default for EbbLoopData { + fn default() -> EbbLoopData { + EbbLoopData::NoLoop() + } +} + /// Methods for querying the loop analysis. impl LoopAnalysis { /// Allocate a new blank loop analysis struct. Use `compute` to compute the loop analysis for @@ -67,14 +87,32 @@ impl LoopAnalysis { self.loops[lp].parent.expand() } - /// Determine if an Ebb belongs to a loop by running a finger along the loop tree. + /// Determine if an `Ebb` belongs to a loop by running a finger along the loop tree. /// /// Returns `true` if `ebb` is in loop `lp`. pub fn is_in_loop(&self, ebb: Ebb, lp: Loop) -> bool { - let ebb_loop = self.ebb_loop_map[ebb]; - match ebb_loop.expand() { - None => false, - Some(ebb_loop) => self.is_child_loop(ebb_loop, lp), + match self.ebb_loop_map[ebb] { + EbbLoopData::NoLoop() => false, + EbbLoopData::Loop { loop_id, .. } => self.is_child_loop(loop_id, lp), + } + } + + /// Determine which region of an `Ebb` belongs to a particular loop. + /// + /// If `ebb` belongs to `lp`, it returns `None` if the whole `ebb` belongs to `lp` or + /// `Some(inst)` where `inst` is the last instruction of `ebb` to belong to `lp`. + pub fn last_loop_instruction(&self, ebb: Ebb, lp: Loop) -> Result, ()> { + match self.ebb_loop_map[ebb] { + EbbLoopData::NoLoop() => Err(()), + EbbLoopData::Loop { loop_id, last_inst } => { + if lp == loop_id { + Ok(Some(last_inst)) + } else if self.is_child_loop(loop_id, lp) { + Ok(None) + } else { + Err(()) + } + } } } @@ -96,12 +134,15 @@ impl LoopAnalysis { impl LoopAnalysis { /// Detects the loops in a function. Needs the control flow graph and the dominator tree. - pub fn compute(&mut self, func: &Function, cfg: &ControlFlowGraph, domtree: &DominatorTree) { + pub fn compute(&mut self, + func: &mut Function, + cfg: &mut ControlFlowGraph, + domtree: &mut DominatorTree) { self.loops.clear(); self.ebb_loop_map.clear(); self.ebb_loop_map.resize(func.dfg.num_ebbs()); self.find_loop_headers(cfg, domtree, &func.layout); - self.discover_loop_blocks(cfg, domtree, &func.layout) + self.discover_loop_blocks(cfg, domtree, func) } // Traverses the CFG in reverse postorder and create a loop object for every EBB having a @@ -116,8 +157,7 @@ impl LoopAnalysis { // If the ebb dominates one of its predecessors it is a back edge if domtree.dominates(ebb, pred_inst, layout) { // This ebb is a loop header, so we create its associated loop - let lp = self.loops.push(LoopData::new(ebb, None)); - self.ebb_loop_map[ebb] = lp.into(); + self.loops.push(LoopData::new(ebb, None)); break; // We break because we only need one back edge to identify a loop header. } @@ -129,70 +169,148 @@ impl LoopAnalysis { // discovers all the ebb belonging to the loop and its inner loops. After a call to this // function, the loop tree is fully constructed. fn discover_loop_blocks(&mut self, - cfg: &ControlFlowGraph, - domtree: &DominatorTree, - layout: &Layout) { - let mut stack: Vec = Vec::new(); + cfg: &mut ControlFlowGraph, + domtree: &mut DominatorTree, + func: &mut Function) { + let mut stack: Vec<(Ebb, Inst)> = Vec::new(); // We handle each loop header in reverse order, corresponding to a pesudo postorder // traversal of the graph. for lp in self.loops().rev() { for &(pred, pred_inst) in cfg.get_predecessors(self.loops[lp].header) { - // We follow the back edges - if domtree.dominates(self.loops[lp].header, pred_inst, layout) { - stack.push(pred); + // We follow the back edges only + if domtree.dominates(self.loops[lp].header, pred_inst, &func.layout) { + stack.push((pred, pred_inst)); } } - while let Some(node) = stack.pop() { + // Then we proceed to discover loop blocks by doing a "reverse" DFS + while let Some((ebb, loop_edge_inst)) = stack.pop() { let continue_dfs: Option; - match self.ebb_loop_map[node].expand() { - None => { - // The node hasn't been visited yet, we tag it as part of the loop - self.ebb_loop_map[node] = PackedOption::from(lp); - continue_dfs = Some(node); + match self.ebb_loop_map[ebb] { + EbbLoopData::NoLoop() => { + // The ebb hasn't been visited yet, we tag it as part of the loop + self.ebb_loop_map[ebb] = EbbLoopData::Loop { + loop_id: lp, + last_inst: loop_edge_inst, + }; + // We stop the DFS if the block is the header of the loop + if ebb != self.loops[lp].header { + continue_dfs = Some(ebb); + } else { + continue_dfs = None; + } } - Some(node_loop) => { - // We copy the node_loop into a mutable reference passed along the while - let mut node_loop = node_loop; - // The node is part of a loop, which can be lp or an inner loop - let mut node_loop_parent_option = self.loops[node_loop].parent; - while let Some(node_loop_parent) = node_loop_parent_option.expand() { - if node_loop_parent == lp { + EbbLoopData::Loop { + loop_id, + last_inst: _, + } => { + // So here the ebb we're visiting is already tagged as being part of a loop + // But since we're considering the loop headers in postorder, that loop + // can only be an inner loop of the loop we're considering. + // If this is the first time we reach this inner loop, we declare our loop + // the parent loop and continue the DFS at the header block of the inner + // loop. + // If we have already visited this inner loop, we can stop the DFS here. + let mut current_loop = loop_id; + let mut current_loop_parent_option = self.loops[current_loop].parent; + while let Some(current_loop_parent) = current_loop_parent_option.expand() { + if current_loop_parent == lp { // We have encounterd lp so we stop (already visited) break; } else { - // - node_loop = node_loop_parent; + current_loop = current_loop_parent; // We lookup the parent loop - node_loop_parent_option = self.loops[node_loop].parent; + current_loop_parent_option = self.loops[current_loop].parent; } } - // Now node_loop_parent is either: - // - None and node_loop is an new inner loop of lp - // - Some(...) and the initial node_loop was a known inner loop of lp - match node_loop_parent_option.expand() { + // Now current_loop_parent is either: + // - None and current_loop is an new inner loop of lp + // - Some(...) and the initial node loop_id was a known inner loop of lp, + // in this case we have already visited it and we stop the DFS + match current_loop_parent_option.expand() { Some(_) => continue_dfs = None, None => { - if node_loop != lp { - self.loops[node_loop].parent = lp.into(); - continue_dfs = Some(self.loops[node_loop].header) - } else { - // If lp is a one-block loop then we make sure we stop + let inner_loop = current_loop; + let inner_loop_header = self.loops[inner_loop].header; + if inner_loop == lp { + // If the inner loop is lp it means that we have reached a + // a block that has been previously tagged as part of lp so + // it has been visited by the DFS so we stop continue_dfs = None + } else { + // Else we proceed to mark the inner loop as child of lp + // And continue the DFS with the inner loop's header + self.loops[inner_loop].parent = lp.into(); + continue_dfs = Some(inner_loop_header); + // Here we perform a modification that's not related to + // the loop discovery algorithm but rather to the way we + // store the loop information. Indeed, for each Ebb we record + // the loop its part of and the last inst in this loop + // But here the Ebb is part of at least two loops, the child + // loop and the parent loop . So two cases arise: + // - either the entire ebb is part of the parent loop and this + // is fine; + // - either only part of the ebb is part of the parent loop and + // in that case we can't store the information of where does + // the parent loop stops. + // In the second case, we proceed to split the Ebb just before + // the parent's loop branch instruction (which is not a + // terminator instruction) so that now the original ebb is + // entirely in the parent loop, and the one we created by + // splitting is part of only one loop, the parent loop. + if func.layout + .last_inst(ebb) + .map_or(false, |ebb_last_inst| { + ebb_last_inst != loop_edge_inst + }) { + // This handles the second case + self.split_ebb_containing_two_loops(ebb, + loop_edge_inst, + lp, + func, + domtree, + cfg); + } } } } } } - // Now we have handled the popped node and need to continue the DFS by adding the - // predecessors of that node + // Now we have handled the popped Ebb and need to continue the DFS by adding the + // predecessors of that Ebb if let Some(continue_dfs) = continue_dfs { - for &(pred, _) in cfg.get_predecessors(continue_dfs) { - stack.push(pred) + for &(pre, pre_inst) in cfg.get_predecessors(continue_dfs) { + stack.push((pre, pre_inst)) } } } + } + } + // We are in the case where `ebb` belongs partially to two different loops, the child and + // the parent. `lp` here is the parent loop and we create a new Ebb so that `ebb` belongs + // in its entirety to the parent loop. + // We also update the cfd and domtree to reflect that. + fn split_ebb_containing_two_loops(&mut self, + ebb: Ebb, + split_inst: Inst, + lp: Loop, + func: &mut Function, + domtree: &mut DominatorTree, + cfg: &mut ControlFlowGraph) { + let new_ebb = func.dfg.make_ebb(); + func.layout.split_ebb(new_ebb, split_inst); + { + let cur = &mut Cursor::new(&mut func.layout); + cur.goto_bottom(ebb); + func.dfg.ins(cur).jump(new_ebb, &[]); } + *self.ebb_loop_map.ensure(new_ebb) = EbbLoopData::Loop { + loop_id: lp, + last_inst: split_inst, + }; + cfg.recompute_ebb(func, ebb); + cfg.recompute_ebb(func, new_ebb); + domtree.compute(func, cfg); } } @@ -237,7 +355,7 @@ mod test { let mut domtree = DominatorTree::new(); cfg.compute(&func); domtree.compute(&func, &cfg); - loop_analysis.compute(&func, &cfg, &domtree); + loop_analysis.compute(&mut func, &mut cfg, &mut domtree); let loops = loop_analysis.loops().collect::>(); assert_eq!(loops.len(), 2); @@ -298,7 +416,7 @@ mod test { let mut domtree = DominatorTree::new(); cfg.compute(&func); domtree.compute(&func, &cfg); - loop_analysis.compute(&func, &cfg, &domtree); + loop_analysis.compute(&mut func, &mut cfg, &mut domtree); let loops = loop_analysis.loops().collect::>(); assert_eq!(loops.len(), 3); @@ -315,4 +433,58 @@ mod test { assert_eq!(loop_analysis.is_in_loop(ebb4, loops[2]), true); assert_eq!(loop_analysis.is_in_loop(ebb5, loops[0]), true); } + + #[test] + fn ebb_splitting() { + let mut func = Function::new(); + let ebb0 = func.dfg.make_ebb(); + let ebb1 = func.dfg.make_ebb(); + let ebb2 = func.dfg.make_ebb(); + let cond = func.dfg.append_ebb_arg(ebb0, types::I32); + + let (inst0, inst1) = { + let dfg = &mut func.dfg; + let cur = &mut Cursor::new(&mut func.layout); + + cur.insert_ebb(ebb0); + dfg.ins(cur).jump(ebb1, &[]); + + cur.insert_ebb(ebb1); + let inst0 = dfg.ins(cur).brnz(cond, ebb1, &[]); + let inst1 = dfg.ins(cur).brnz(cond, ebb0, &[]); + dfg.ins(cur).jump(ebb2, &[]); + + cur.insert_ebb(ebb2); + dfg.ins(cur).return_(&[]); + (inst0, inst1) + }; + + let mut loop_analysis = LoopAnalysis::new(); + let mut cfg = ControlFlowGraph::new(); + let mut domtree = DominatorTree::new(); + cfg.compute(&func); + domtree.compute(&func, &cfg); + loop_analysis.compute(&mut func, &mut cfg, &mut domtree); + + let loops = loop_analysis.loops().collect::>(); + assert_eq!(loops.len(), 2); + assert_eq!(loop_analysis.loop_header(loops[0]), ebb0); + assert_eq!(loop_analysis.loop_header(loops[1]), ebb1); + assert_eq!(loop_analysis.loop_parent(loops[1]), Some(loops[0])); + assert_eq!(loop_analysis.is_in_loop(ebb0, loops[0]), true); + assert_eq!(loop_analysis.is_in_loop(ebb1, loops[1]), true); + assert_eq!(loop_analysis.is_in_loop(ebb2, loops[0]), false); + assert_eq!(loop_analysis.is_in_loop(Ebb::with_number(3).unwrap(), loops[0]), + true); + assert_eq!(loop_analysis + .last_loop_instruction(ebb1, loops[1]) + .unwrap() + .unwrap(), + inst0); + assert_eq!(loop_analysis + .last_loop_instruction(Ebb::with_number(3).unwrap(), loops[0]) + .unwrap() + .unwrap(), + inst1) + } } From 7100fa0475e7d2d2a45a7c85fb1d91a75572dac3 Mon Sep 17 00:00:00 2001 From: Denis Merigoux Date: Wed, 2 Aug 2017 16:26:51 -0700 Subject: [PATCH 2/6] Added ebb splitting update for dominator tree More checks for dominator tree integrity More testing for loop analysis ebb splitting --- filetests/licm/nested_loops.cton | 11 ++-- lib/cretonne/src/dominator_tree.rs | 12 ++--- lib/cretonne/src/loop_analysis.rs | 76 ++++++++++++++++++++++++--- lib/cretonne/src/verifier/cssa.rs | 10 ++-- lib/cretonne/src/verifier/liveness.rs | 8 +-- lib/cretonne/src/verifier/mod.rs | 40 +++++--------- 6 files changed, 104 insertions(+), 53 deletions(-) diff --git a/filetests/licm/nested_loops.cton b/filetests/licm/nested_loops.cton index a32dd4b49..eae0613b5 100644 --- a/filetests/licm/nested_loops.cton +++ b/filetests/licm/nested_loops.cton @@ -25,8 +25,8 @@ ebb3(v30: i32): } -; sameln:function %nested_loops(i32) -> i32 { -; nextln: ebb4(v12: i32): +; sameln: function %nested_loops(i32) -> i32 { +; nextln: ebb5(v12: i32): ; nextln: v1 = iconst.i32 1 ; nextln: v2 = iconst.i32 2 ; nextln: v3 = iadd v1, v2 @@ -35,12 +35,15 @@ ebb3(v30: i32): ; nextln: ; nextln: ebb0(v0: i32): ; nextln: v4 = isub v0, v1 -; nextln: v8 = iadd.i32 v7, v4 ; nextln: jump ebb1(v4, v4) ; nextln: ; nextln: ebb1(v5: i32, v6: i32): ; nextln: brz v6, ebb2(v5) -; nextln: v9 = isub v6, v7 +; nextln: jump ebb4 +; nextln: +; nextln: ebb4: +; nextln: v8 = iadd.i32 v7, v4 +; nextln: v9 = isub.i32 v6, v7 ; nextln: jump ebb1(v5, v9) ; nextln: ; nextln: ebb2(v10: i32): diff --git a/lib/cretonne/src/dominator_tree.rs b/lib/cretonne/src/dominator_tree.rs index d88adbd4a..519073ba9 100644 --- a/lib/cretonne/src/dominator_tree.rs +++ b/lib/cretonne/src/dominator_tree.rs @@ -317,8 +317,7 @@ impl DominatorTree { .filter(|&(pred, _)| self.nodes[pred].rpo_number > 1); // The RPO must visit at least one predecessor before this node. - let mut idom = reachable_preds - .next() + let mut idom = reachable_preds.next() .expect("EBB node must have one reachable predecessor"); for pred in reachable_preds { @@ -344,11 +343,10 @@ impl DominatorTree { } // We use the RPO comparison on the postorder list so we invert the operands of the // comparison - let old_ebb_postorder_index = - self.postorder - .as_slice() - .binary_search_by(|probe| self.rpo_cmp_ebb(old_ebb, *probe)) - .expect("the old ebb is not declared to the dominator tree"); + let old_ebb_postorder_index = self.postorder + .as_slice() + .binary_search_by(|probe| self.rpo_cmp_ebb(old_ebb, *probe)) + .expect("the old ebb is not declared to the dominator tree"); let new_ebb_rpo = self.insert_after_rpo(old_ebb, old_ebb_postorder_index, new_ebb); *self.nodes.ensure(new_ebb) = DomNode { rpo_number: new_ebb_rpo, diff --git a/lib/cretonne/src/loop_analysis.rs b/lib/cretonne/src/loop_analysis.rs index 1cb952694..1d64f1bf7 100644 --- a/lib/cretonne/src/loop_analysis.rs +++ b/lib/cretonne/src/loop_analysis.rs @@ -142,7 +142,7 @@ impl LoopAnalysis { self.ebb_loop_map.clear(); self.ebb_loop_map.resize(func.dfg.num_ebbs()); self.find_loop_headers(cfg, domtree, &func.layout); - self.discover_loop_blocks(cfg, domtree, func) + self.discover_loop_blocks(cfg, domtree, func); } // Traverses the CFG in reverse postorder and create a loop object for every EBB having a @@ -298,19 +298,25 @@ impl LoopAnalysis { domtree: &mut DominatorTree, cfg: &mut ControlFlowGraph) { let new_ebb = func.dfg.make_ebb(); - func.layout.split_ebb(new_ebb, split_inst); - { + let real_split_inst = { + let mut cur = Cursor::new(&mut func.layout); + cur.goto_inst(split_inst); + cur.next_inst() + .expect("you cannot split at the last instruction") + }; + func.layout.split_ebb(new_ebb, real_split_inst); + let middle_jump_inst = { let cur = &mut Cursor::new(&mut func.layout); cur.goto_bottom(ebb); - func.dfg.ins(cur).jump(new_ebb, &[]); - } + func.dfg.ins(cur).jump(new_ebb, &[]) + }; *self.ebb_loop_map.ensure(new_ebb) = EbbLoopData::Loop { loop_id: lp, last_inst: split_inst, }; cfg.recompute_ebb(func, ebb); cfg.recompute_ebb(func, new_ebb); - domtree.compute(func, cfg); + domtree.recompute_split_ebb(ebb, new_ebb, middle_jump_inst); } } @@ -487,4 +493,62 @@ mod test { .unwrap(), inst1) } + #[test] + fn ebb_splitting_complex() { + let mut func = Function::new(); + let ebb0 = func.dfg.make_ebb(); + let ebb1 = func.dfg.make_ebb(); + let ebb2 = func.dfg.make_ebb(); + let ebb3 = func.dfg.make_ebb(); + let cond = func.dfg.append_ebb_arg(ebb0, types::I32); + + let (inst0, inst1) = { + let dfg = &mut func.dfg; + let cur = &mut Cursor::new(&mut func.layout); + + cur.insert_ebb(ebb0); + dfg.ins(cur).jump(ebb1, &[]); + + cur.insert_ebb(ebb1); + dfg.ins(cur).brnz(cond, ebb2, &[]); + let inst0 = dfg.ins(cur).jump(ebb1, &[]); + + cur.insert_ebb(ebb2); + dfg.ins(cur).brnz(cond, ebb3, &[]); + let inst1 = dfg.ins(cur).jump(ebb0, &[]); + + cur.insert_ebb(ebb3); + dfg.ins(cur).return_(&[]); + (inst0, inst1) + }; + + let mut loop_analysis = LoopAnalysis::new(); + let mut cfg = ControlFlowGraph::new(); + let mut domtree = DominatorTree::new(); + cfg.compute(&func); + domtree.compute(&func, &cfg); + loop_analysis.compute(&mut func, &mut cfg, &mut domtree); + let loops = loop_analysis.loops().collect::>(); + assert_eq!(loops.len(), 2); + assert_eq!(loop_analysis.loop_header(loops[0]), ebb0); + assert_eq!(loop_analysis.loop_header(loops[1]), ebb1); + assert_eq!(loop_analysis.loop_parent(loops[1]), Some(loops[0])); + assert_eq!(loop_analysis.is_in_loop(ebb0, loops[0]), true); + assert_eq!(loop_analysis.is_in_loop(ebb1, loops[1]), true); + assert_eq!(loop_analysis.is_in_loop(ebb1, loops[0]), true); + assert_eq!(loop_analysis.is_in_loop(ebb2, loops[0]), true); + assert_eq!(loop_analysis.is_in_loop(ebb2, loops[1]), false); + assert_eq!(loop_analysis.is_in_loop(Ebb::with_number(4).unwrap(), loops[0]), + true); + assert_eq!(loop_analysis + .last_loop_instruction(ebb1, loops[1]) + .unwrap() + .unwrap(), + inst0); + assert_eq!(loop_analysis + .last_loop_instruction(ebb2, loops[0]) + .unwrap() + .unwrap(), + inst1) + } } diff --git a/lib/cretonne/src/verifier/cssa.rs b/lib/cretonne/src/verifier/cssa.rs index fe9a2fe18..8ee444b74 100644 --- a/lib/cretonne/src/verifier/cssa.rs +++ b/lib/cretonne/src/verifier/cssa.rs @@ -29,11 +29,11 @@ pub fn verify_cssa(func: &Function, virtregs: &VirtRegs) -> Result { let verifier = CssaVerifier { - func, - cfg, - domtree, - virtregs, - liveness, + func: func, + cfg: cfg, + domtree: domtree, + virtregs: virtregs, + liveness: liveness, }; verifier.check_virtregs()?; verifier.check_cssa()?; diff --git a/lib/cretonne/src/verifier/liveness.rs b/lib/cretonne/src/verifier/liveness.rs index 2dff4a6e3..0c413ebae 100644 --- a/lib/cretonne/src/verifier/liveness.rs +++ b/lib/cretonne/src/verifier/liveness.rs @@ -27,10 +27,10 @@ pub fn verify_liveness(isa: &TargetIsa, liveness: &Liveness) -> Result { let verifier = LivenessVerifier { - isa, - func, - cfg, - liveness, + isa: isa, + func: func, + cfg: cfg, + liveness: liveness, }; verifier.check_ebbs()?; verifier.check_insts()?; diff --git a/lib/cretonne/src/verifier/mod.rs b/lib/cretonne/src/verifier/mod.rs index d3f05ef4c..fa0d9541f 100644 --- a/lib/cretonne/src/verifier/mod.rs +++ b/lib/cretonne/src/verifier/mod.rs @@ -143,10 +143,10 @@ impl<'a> Verifier<'a> { let cfg = ControlFlowGraph::with_function(func); let domtree = DominatorTree::with_function(func, &cfg); Verifier { - func, - cfg, - domtree, - isa, + func: func, + cfg: cfg, + domtree: domtree, + isa: isa, } } @@ -237,21 +237,9 @@ impl<'a> Verifier<'a> { MultiAry { ref args, .. } => { self.verify_value_list(inst, args)?; } - Jump { - destination, - ref args, - .. - } | - Branch { - destination, - ref args, - .. - } | - BranchIcmp { - destination, - ref args, - .. - } => { + Jump { destination, ref args, .. } | + Branch { destination, ref args, .. } | + BranchIcmp { destination, ref args, .. } => { self.verify_ebb(inst, destination)?; self.verify_value_list(inst, args)?; } @@ -370,7 +358,7 @@ impl<'a> Verifier<'a> { // Defining instruction dominates the instruction that uses the value. if self.domtree.is_reachable(self.func.layout.pp_ebb(loc_inst)) && !self.domtree - .dominates(def_inst, loc_inst, &self.func.layout) { + .dominates(def_inst, loc_inst, &self.func.layout) { return err!(loc_inst, "uses value from non-dominating {}", def_inst); } } @@ -774,10 +762,9 @@ impl<'a> Verifier<'a> { let encoding = self.func.encodings.get_or_default(inst); if encoding.is_legal() { - let verify_encoding = - isa.encode(&self.func.dfg, - &self.func.dfg[inst], - self.func.dfg.ctrl_typevar(inst)); + let verify_encoding = isa.encode(&self.func.dfg, + &self.func.dfg[inst], + self.func.dfg.ctrl_typevar(inst)); match verify_encoding { Ok(verify_encoding) => { if verify_encoding != encoding { @@ -879,9 +866,8 @@ mod tests { let mut func = Function::new(); let ebb0 = func.dfg.make_ebb(); func.layout.append_ebb(ebb0); - let nullary_with_bad_opcode = - func.dfg - .make_inst(InstructionData::Nullary { opcode: Opcode::Jump }); + let nullary_with_bad_opcode = func.dfg + .make_inst(InstructionData::Nullary { opcode: Opcode::Jump }); func.layout.append_inst(nullary_with_bad_opcode, ebb0); let verifier = Verifier::new(&func, None); assert_err_with_msg!(verifier.run(), "instruction format"); From ea1959899a628d3e606bd0ba47d337410629764d Mon Sep 17 00:00:00 2001 From: Denis Merigoux Date: Thu, 3 Aug 2017 10:38:07 -0700 Subject: [PATCH 3/6] First round of fixes after review Not done yet --- lib/cretonne/src/loop_analysis.rs | 211 ++++++++++++++++-------------- 1 file changed, 116 insertions(+), 95 deletions(-) diff --git a/lib/cretonne/src/loop_analysis.rs b/lib/cretonne/src/loop_analysis.rs index 1d64f1bf7..fbb2245bc 100644 --- a/lib/cretonne/src/loop_analysis.rs +++ b/lib/cretonne/src/loop_analysis.rs @@ -50,8 +50,6 @@ enum EbbLoopData { Loop { loop_id: Loop, last_inst: Inst }, } -impl PrimaryEntityData for EbbLoopData {} - impl Default for EbbLoopData { fn default() -> EbbLoopData { EbbLoopData::NoLoop() @@ -99,8 +97,12 @@ impl LoopAnalysis { /// Determine which region of an `Ebb` belongs to a particular loop. /// - /// If `ebb` belongs to `lp`, it returns `None` if the whole `ebb` belongs to `lp` or - /// `Some(inst)` where `inst` is the last instruction of `ebb` to belong to `lp`. + /// Three cases arise: + /// + /// - either `ebb` doesn't belong to `lp` and `Err(())` is returned; + /// - or `ebb` belongs entirely to `lp` and `Ok(None)` is returned; + /// - or `ebb` belongs partially to `lp`, from the beginning until a specific branch + /// instruction `inst` and `Ok(Some(inst))` is returned. pub fn last_loop_instruction(&self, ebb: Ebb, lp: Loop) -> Result, ()> { match self.ebb_loop_map[ebb] { EbbLoopData::NoLoop() => Err(()), @@ -134,6 +136,12 @@ impl LoopAnalysis { impl LoopAnalysis { /// Detects the loops in a function. Needs the control flow graph and the dominator tree. + /// + /// Because of loop information storage efficiency, the loop analysis only allows one `Ebb` to + /// be part of at most two loops: an inner loop which can end in the middle of the `Ebb` and + /// an outer loop that has to contain the full `Ebb`. All `Ebb`s who don't match this criteria + /// are split until they do so. That is why `compute` mutates the function, control flow + /// graph and moniator tree. pub fn compute(&mut self, func: &mut Function, cfg: &mut ControlFlowGraph, @@ -184,97 +192,7 @@ impl LoopAnalysis { } // Then we proceed to discover loop blocks by doing a "reverse" DFS while let Some((ebb, loop_edge_inst)) = stack.pop() { - let continue_dfs: Option; - match self.ebb_loop_map[ebb] { - EbbLoopData::NoLoop() => { - // The ebb hasn't been visited yet, we tag it as part of the loop - self.ebb_loop_map[ebb] = EbbLoopData::Loop { - loop_id: lp, - last_inst: loop_edge_inst, - }; - // We stop the DFS if the block is the header of the loop - if ebb != self.loops[lp].header { - continue_dfs = Some(ebb); - } else { - continue_dfs = None; - } - } - EbbLoopData::Loop { - loop_id, - last_inst: _, - } => { - // So here the ebb we're visiting is already tagged as being part of a loop - // But since we're considering the loop headers in postorder, that loop - // can only be an inner loop of the loop we're considering. - // If this is the first time we reach this inner loop, we declare our loop - // the parent loop and continue the DFS at the header block of the inner - // loop. - // If we have already visited this inner loop, we can stop the DFS here. - let mut current_loop = loop_id; - let mut current_loop_parent_option = self.loops[current_loop].parent; - while let Some(current_loop_parent) = current_loop_parent_option.expand() { - if current_loop_parent == lp { - // We have encounterd lp so we stop (already visited) - break; - } else { - current_loop = current_loop_parent; - // We lookup the parent loop - current_loop_parent_option = self.loops[current_loop].parent; - } - } - // Now current_loop_parent is either: - // - None and current_loop is an new inner loop of lp - // - Some(...) and the initial node loop_id was a known inner loop of lp, - // in this case we have already visited it and we stop the DFS - match current_loop_parent_option.expand() { - Some(_) => continue_dfs = None, - None => { - let inner_loop = current_loop; - let inner_loop_header = self.loops[inner_loop].header; - if inner_loop == lp { - // If the inner loop is lp it means that we have reached a - // a block that has been previously tagged as part of lp so - // it has been visited by the DFS so we stop - continue_dfs = None - } else { - // Else we proceed to mark the inner loop as child of lp - // And continue the DFS with the inner loop's header - self.loops[inner_loop].parent = lp.into(); - continue_dfs = Some(inner_loop_header); - // Here we perform a modification that's not related to - // the loop discovery algorithm but rather to the way we - // store the loop information. Indeed, for each Ebb we record - // the loop its part of and the last inst in this loop - // But here the Ebb is part of at least two loops, the child - // loop and the parent loop . So two cases arise: - // - either the entire ebb is part of the parent loop and this - // is fine; - // - either only part of the ebb is part of the parent loop and - // in that case we can't store the information of where does - // the parent loop stops. - // In the second case, we proceed to split the Ebb just before - // the parent's loop branch instruction (which is not a - // terminator instruction) so that now the original ebb is - // entirely in the parent loop, and the one we created by - // splitting is part of only one loop, the parent loop. - if func.layout - .last_inst(ebb) - .map_or(false, |ebb_last_inst| { - ebb_last_inst != loop_edge_inst - }) { - // This handles the second case - self.split_ebb_containing_two_loops(ebb, - loop_edge_inst, - lp, - func, - domtree, - cfg); - } - } - } - } - } - } + let continue_dfs = self.visit_loop_ebb(lp, ebb, loop_edge_inst, func, domtree, cfg); // Now we have handled the popped Ebb and need to continue the DFS by adding the // predecessors of that Ebb if let Some(continue_dfs) = continue_dfs { @@ -286,6 +204,109 @@ impl LoopAnalysis { } } + fn visit_loop_ebb(&mut self, + lp: Loop, + ebb: Ebb, + loop_edge_inst: Inst, + func: &mut Function, + domtree: &mut DominatorTree, + cfg: &mut ControlFlowGraph) + -> Option { + let continue_dfs: Option; + match self.ebb_loop_map[ebb] { + EbbLoopData::NoLoop() => { + // The ebb hasn't been visited yet, we tag it as part of the loop + self.ebb_loop_map[ebb] = EbbLoopData::Loop { + loop_id: lp, + last_inst: loop_edge_inst, + }; + // We stop the DFS if the block is the header of the loop + if ebb != self.loops[lp].header { + continue_dfs = Some(ebb); + } else { + continue_dfs = None; + } + } + EbbLoopData::Loop { + loop_id, + last_inst: _, + } => { + // So here the ebb we are visiting is already tagged as being part of a + // loop. But since we're considering the loop headers in postorder, that + // loop can only be an inner loop of the loop we're considering. + // + // If this is the first time we reach this inner loop, we declare our loop + // the parent loop and continue the DFS at the header block of the inner + // loop. + // + // If we have already visited this inner loop, we can stop the DFS here. + let mut current_loop = loop_id; + let mut current_loop_parent_option = self.loops[current_loop].parent; + while let Some(current_loop_parent) = current_loop_parent_option.expand() { + if current_loop_parent == lp { + // We have encounterd lp so we stop (already visited) + break; + } else { + current_loop = current_loop_parent; + // We lookup the parent loop + current_loop_parent_option = self.loops[current_loop].parent; + } + } + // Now current_loop_parent is either: + // - None and current_loop is an new inner loop of lp + // - Some(...) and the initial node loop_id was a known inner loop of lp, + // in this case we have already visited it and we stop the DFS + match current_loop_parent_option.expand() { + Some(_) => continue_dfs = None, + None => { + let inner_loop = current_loop; + let inner_loop_header = self.loops[inner_loop].header; + if inner_loop == lp { + // If the inner loop is lp it means that we have reached a + // a block that has been previously tagged as part of lp so + // it has been visited by the DFS so we stop + continue_dfs = None + } else { + // Else we proceed to mark the inner loop as child of lp + // And continue the DFS with the inner loop's header + self.loops[inner_loop].parent = lp.into(); + continue_dfs = Some(inner_loop_header); + // Here we perform a modification that's not related to + // the loop discovery algorithm but rather to the way we + // store the loop information. Indeed, for each Ebb we record + // the loop it's part of and the last inst in this loop. + // But here the Ebb is part of at least two loops, the child + // loop and the parent loop. So two cases arise: + // - either the entire ebb is part of the parent loop and this + // is fine; + // - or only part of the ebb is part of the parent loop and + // in that case we can't store the information of where the + // parent loop stops. + // In the second case, we proceed to split the Ebb just before + // the parent's loop branch instruction (which is not a + // terminator instruction) so that now the original ebb is + // entirely in the parent loop, and the one we created by + // splitting is part of only one loop, the parent loop. + if func.layout + .last_inst(ebb) + .map_or(false, + |ebb_last_inst| ebb_last_inst != loop_edge_inst) { + // This handles the second case + self.split_ebb_containing_two_loops(ebb, + loop_edge_inst, + lp, + func, + domtree, + cfg); + } + } + } + } + } + } + continue_dfs + } + // We are in the case where `ebb` belongs partially to two different loops, the child and // the parent. `lp` here is the parent loop and we create a new Ebb so that `ebb` belongs // in its entirety to the parent loop. From fc18e9555bd21f2c68b5c3eaa66a677018f71d8e Mon Sep 17 00:00:00 2001 From: Denis Merigoux Date: Thu, 3 Aug 2017 15:01:32 -0700 Subject: [PATCH 4/6] Fixed bug in loop analysis ebb splitting algorithm --- filetests/licm/nested_loops.cton | 11 +- lib/cretonne/src/dominator_tree.rs | 12 +- lib/cretonne/src/ir/layout.rs | 5 + lib/cretonne/src/loop_analysis.rs | 219 +++++++++++++++----------- lib/cretonne/src/verifier/cssa.rs | 10 +- lib/cretonne/src/verifier/liveness.rs | 8 +- lib/cretonne/src/verifier/mod.rs | 40 +++-- 7 files changed, 182 insertions(+), 123 deletions(-) diff --git a/filetests/licm/nested_loops.cton b/filetests/licm/nested_loops.cton index eae0613b5..a32dd4b49 100644 --- a/filetests/licm/nested_loops.cton +++ b/filetests/licm/nested_loops.cton @@ -25,8 +25,8 @@ ebb3(v30: i32): } -; sameln: function %nested_loops(i32) -> i32 { -; nextln: ebb5(v12: i32): +; sameln:function %nested_loops(i32) -> i32 { +; nextln: ebb4(v12: i32): ; nextln: v1 = iconst.i32 1 ; nextln: v2 = iconst.i32 2 ; nextln: v3 = iadd v1, v2 @@ -35,15 +35,12 @@ ebb3(v30: i32): ; nextln: ; nextln: ebb0(v0: i32): ; nextln: v4 = isub v0, v1 +; nextln: v8 = iadd.i32 v7, v4 ; nextln: jump ebb1(v4, v4) ; nextln: ; nextln: ebb1(v5: i32, v6: i32): ; nextln: brz v6, ebb2(v5) -; nextln: jump ebb4 -; nextln: -; nextln: ebb4: -; nextln: v8 = iadd.i32 v7, v4 -; nextln: v9 = isub.i32 v6, v7 +; nextln: v9 = isub v6, v7 ; nextln: jump ebb1(v5, v9) ; nextln: ; nextln: ebb2(v10: i32): diff --git a/lib/cretonne/src/dominator_tree.rs b/lib/cretonne/src/dominator_tree.rs index 519073ba9..d88adbd4a 100644 --- a/lib/cretonne/src/dominator_tree.rs +++ b/lib/cretonne/src/dominator_tree.rs @@ -317,7 +317,8 @@ impl DominatorTree { .filter(|&(pred, _)| self.nodes[pred].rpo_number > 1); // The RPO must visit at least one predecessor before this node. - let mut idom = reachable_preds.next() + let mut idom = reachable_preds + .next() .expect("EBB node must have one reachable predecessor"); for pred in reachable_preds { @@ -343,10 +344,11 @@ impl DominatorTree { } // We use the RPO comparison on the postorder list so we invert the operands of the // comparison - let old_ebb_postorder_index = self.postorder - .as_slice() - .binary_search_by(|probe| self.rpo_cmp_ebb(old_ebb, *probe)) - .expect("the old ebb is not declared to the dominator tree"); + let old_ebb_postorder_index = + self.postorder + .as_slice() + .binary_search_by(|probe| self.rpo_cmp_ebb(old_ebb, *probe)) + .expect("the old ebb is not declared to the dominator tree"); let new_ebb_rpo = self.insert_after_rpo(old_ebb, old_ebb_postorder_index, new_ebb); *self.nodes.ensure(new_ebb) = DomNode { rpo_number: new_ebb_rpo, diff --git a/lib/cretonne/src/ir/layout.rs b/lib/cretonne/src/ir/layout.rs index 3f31d34f2..229a7089b 100644 --- a/lib/cretonne/src/ir/layout.rs +++ b/lib/cretonne/src/ir/layout.rs @@ -358,6 +358,11 @@ impl Layout { pub fn next_ebb(&self, ebb: Ebb) -> Option { self.ebbs[ebb].next.expand() } + + /// Get the instruction following `inst` in the layout order. + pub fn next_inst(&self, inst: Inst) -> Option { + self.insts[inst].next.expand() + } } #[derive(Clone, Debug, Default)] diff --git a/lib/cretonne/src/loop_analysis.rs b/lib/cretonne/src/loop_analysis.rs index fbb2245bc..cc2aba288 100644 --- a/lib/cretonne/src/loop_analysis.rs +++ b/lib/cretonne/src/loop_analysis.rs @@ -4,9 +4,10 @@ use dominator_tree::DominatorTree; use entity_map::{EntityMap, PrimaryEntityData, Keys}; use flowgraph::ControlFlowGraph; -use ir::{Function, Ebb, Layout, Cursor, InstBuilder}; +use ir::{Function, Ebb, Layout, Cursor, InstBuilder, ProgramOrder}; use ir::entities::Inst; use packed_option::PackedOption; +use std::cmp::Ordering; /// A opaque reference to a code loop. #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)] @@ -45,14 +46,17 @@ impl LoopData { /// the direct child of the other, and the parent loop should contain all the `Ebb` up to its /// terminator instruction. #[derive(Clone,Debug)] -enum EbbLoopData { - NoLoop(), - Loop { loop_id: Loop, last_inst: Inst }, +struct EbbLoopData { + loop_id: PackedOption, + last_inst: PackedOption, } impl Default for EbbLoopData { fn default() -> EbbLoopData { - EbbLoopData::NoLoop() + EbbLoopData { + loop_id: None.into(), + last_inst: None.into(), + } } } @@ -89,9 +93,9 @@ impl LoopAnalysis { /// /// Returns `true` if `ebb` is in loop `lp`. pub fn is_in_loop(&self, ebb: Ebb, lp: Loop) -> bool { - match self.ebb_loop_map[ebb] { - EbbLoopData::NoLoop() => false, - EbbLoopData::Loop { loop_id, .. } => self.is_child_loop(loop_id, lp), + match self.ebb_loop_map[ebb].loop_id.expand() { + None => false, + Some(ebb_lp) => self.is_child_loop(ebb_lp, lp), } } @@ -104,12 +108,12 @@ impl LoopAnalysis { /// - or `ebb` belongs partially to `lp`, from the beginning until a specific branch /// instruction `inst` and `Ok(Some(inst))` is returned. pub fn last_loop_instruction(&self, ebb: Ebb, lp: Loop) -> Result, ()> { - match self.ebb_loop_map[ebb] { - EbbLoopData::NoLoop() => Err(()), - EbbLoopData::Loop { loop_id, last_inst } => { - if lp == loop_id { - Ok(Some(last_inst)) - } else if self.is_child_loop(loop_id, lp) { + match self.ebb_loop_map[ebb].loop_id.expand() { + None => Err(()), + Some(ebb_lp) => { + if lp == ebb_lp { + Ok(self.ebb_loop_map[ebb].last_inst.expand()) + } else if self.is_child_loop(ebb_lp, lp) { Ok(None) } else { Err(()) @@ -141,7 +145,7 @@ impl LoopAnalysis { /// be part of at most two loops: an inner loop which can end in the middle of the `Ebb` and /// an outer loop that has to contain the full `Ebb`. All `Ebb`s who don't match this criteria /// are split until they do so. That is why `compute` mutates the function, control flow - /// graph and moniator tree. + /// graph and dominator tree. pub fn compute(&mut self, func: &mut Function, cfg: &mut ControlFlowGraph, @@ -213,12 +217,19 @@ impl LoopAnalysis { cfg: &mut ControlFlowGraph) -> Option { let continue_dfs: Option; - match self.ebb_loop_map[ebb] { - EbbLoopData::NoLoop() => { + match self.ebb_loop_map[ebb].loop_id.expand() { + None => { // The ebb hasn't been visited yet, we tag it as part of the loop - self.ebb_loop_map[ebb] = EbbLoopData::Loop { - loop_id: lp, - last_inst: loop_edge_inst, + self.ebb_loop_map[ebb] = EbbLoopData { + loop_id: lp.into(), + last_inst: func.layout + .last_inst(ebb) + .map_or(None.into(), + |ebb_last_inst| if ebb_last_inst != loop_edge_inst { + loop_edge_inst.into() + } else { + None.into() + }), }; // We stop the DFS if the block is the header of the loop if ebb != self.loops[lp].header { @@ -227,20 +238,18 @@ impl LoopAnalysis { continue_dfs = None; } } - EbbLoopData::Loop { - loop_id, - last_inst: _, - } => { - // So here the ebb we are visiting is already tagged as being part of a - // loop. But since we're considering the loop headers in postorder, that - // loop can only be an inner loop of the loop we're considering. + Some(inner_loop) => { + // So here the ebb we are visiting is already tagged as being part of a loop. + // But since we're considering the loop headers in postorder, that loop can only + // be an inner loop of the loop we're considering. // - // If this is the first time we reach this inner loop, we declare our loop - // the parent loop and continue the DFS at the header block of the inner - // loop. + // If this is the first time we reach this inner loop, we declare our loop the + // parent loop and continue the DFS at the header block of the inner loop. If we + // have already visited this inner loop, we can stop the DFS here. // - // If we have already visited this inner loop, we can stop the DFS here. - let mut current_loop = loop_id; + // But first we go up the loop tree to determine if this inner loop is a child + // of our loop or not. + let mut current_loop = inner_loop; let mut current_loop_parent_option = self.loops[current_loop].parent; while let Some(current_loop_parent) = current_loop_parent_option.expand() { if current_loop_parent == lp { @@ -254,51 +263,91 @@ impl LoopAnalysis { } // Now current_loop_parent is either: // - None and current_loop is an new inner loop of lp - // - Some(...) and the initial node loop_id was a known inner loop of lp, - // in this case we have already visited it and we stop the DFS + // - Some(...) and the initial node loop_id was a known inner loop of lp, in this + // case we have already visited it and we stop the DFS match current_loop_parent_option.expand() { Some(_) => continue_dfs = None, None => { let inner_loop = current_loop; let inner_loop_header = self.loops[inner_loop].header; if inner_loop == lp { - // If the inner loop is lp it means that we have reached a - // a block that has been previously tagged as part of lp so - // it has been visited by the DFS so we stop - continue_dfs = None + // If the inner loop is lp it means that we have reached a a block that + // has been previously tagged as part of lp so it has been visited by + // the DFS so we stop. + continue_dfs = None; + // We then proceed to update the information of the last instruction + // of the Ebb belonging to lp. + match self.ebb_loop_map[ebb].last_inst.expand() { + // If the whole ebb belonged to the loop, it doesn't change. + None => (), + // If there was a limit, we might update it. + Some(previous_last_inst) => { + if func.layout.cmp(loop_edge_inst, previous_last_inst) == + Ordering::Greater { + if loop_edge_inst == func.layout.last_inst(ebb).unwrap() { + // If the limit is the end there is no limit. + self.ebb_loop_map[ebb].last_inst = None.into(); + } else { + self.ebb_loop_map[ebb].last_inst = loop_edge_inst + .into(); + } + } + } + } } else { - // Else we proceed to mark the inner loop as child of lp - // And continue the DFS with the inner loop's header + // Else we proceed to mark the inner loop as child of lp, and continue + // the DFS with the inner loop's header. self.loops[inner_loop].parent = lp.into(); continue_dfs = Some(inner_loop_header); - // Here we perform a modification that's not related to - // the loop discovery algorithm but rather to the way we - // store the loop information. Indeed, for each Ebb we record - // the loop it's part of and the last inst in this loop. - // But here the Ebb is part of at least two loops, the child - // loop and the parent loop. So two cases arise: - // - either the entire ebb is part of the parent loop and this - // is fine; - // - or only part of the ebb is part of the parent loop and - // in that case we can't store the information of where the - // parent loop stops. - // In the second case, we proceed to split the Ebb just before - // the parent's loop branch instruction (which is not a - // terminator instruction) so that now the original ebb is - // entirely in the parent loop, and the one we created by - // splitting is part of only one loop, the parent loop. - if func.layout - .last_inst(ebb) - .map_or(false, - |ebb_last_inst| ebb_last_inst != loop_edge_inst) { - // This handles the second case - self.split_ebb_containing_two_loops(ebb, - loop_edge_inst, - lp, - func, - domtree, - cfg); - } + // Here we perform a modification that's not related to the loop + // discovery algorithm but rather to the way we store the loop + // information. Indeed, for each Ebb we record the loop it's part of + // and the last inst in this loop. But here the Ebb is part of at least + // two loops, the child loop and the parent loop. So two cases arise: + // - either the entire ebb is part of the parent loop and this is fine; + // - or only part of the ebb is part of the parent loop and in that + // case we can't store the information of where the parent loop + // stops. + // In the second case, we proceed to split the Ebb just before the + // parent's loop branch instruction (which is not a terminator + // instruction) so that now the original ebb is entirely in the parent + // loop, and the one we created by splitting is part of only one loop, + // the parent loop. + // + // To detect which case we're in we have to set conservatively the + // limit of the parent loop in the `Ebb`. We only know two things about + // it here: + // - it's after the last instruction of the inner loop; + // - it's after loop_edge_inst + // Based on that we take the linit as the max of these two. + match self.ebb_loop_map[ebb].last_inst.expand() { + None => (), + Some(last_inner_loop) => { + if func.layout + .last_inst(ebb) + .map_or(false, |ebb_last_inst| { + ebb_last_inst != loop_edge_inst + }) { + let limit = + match func.layout.cmp(loop_edge_inst, + last_inner_loop) { + Ordering::Greater | Ordering::Equal => { + loop_edge_inst + } + Ordering::Less => last_inner_loop, + }; + { + // This handles the second case + self.split_ebb_containing_two_loops(ebb, + limit, + lp, + func, + domtree, + cfg); + } + } + } + }; } } } @@ -319,21 +368,18 @@ impl LoopAnalysis { domtree: &mut DominatorTree, cfg: &mut ControlFlowGraph) { let new_ebb = func.dfg.make_ebb(); - let real_split_inst = { - let mut cur = Cursor::new(&mut func.layout); - cur.goto_inst(split_inst); - cur.next_inst() - .expect("you cannot split at the last instruction") - }; - func.layout.split_ebb(new_ebb, real_split_inst); + let split_inst_next = func.layout + .next_inst(split_inst) + .expect("you cannot split at the last instruction"); + func.layout.split_ebb(new_ebb, split_inst_next); let middle_jump_inst = { let cur = &mut Cursor::new(&mut func.layout); cur.goto_bottom(ebb); func.dfg.ins(cur).jump(new_ebb, &[]) }; - *self.ebb_loop_map.ensure(new_ebb) = EbbLoopData::Loop { - loop_id: lp, - last_inst: split_inst, + *self.ebb_loop_map.ensure(new_ebb) = EbbLoopData { + loop_id: lp.into(), + last_inst: split_inst.into(), }; cfg.recompute_ebb(func, ebb); cfg.recompute_ebb(func, new_ebb); @@ -523,7 +569,7 @@ mod test { let ebb3 = func.dfg.make_ebb(); let cond = func.dfg.append_ebb_arg(ebb0, types::I32); - let (inst0, inst1) = { + let inst1 = { let dfg = &mut func.dfg; let cur = &mut Cursor::new(&mut func.layout); @@ -532,15 +578,15 @@ mod test { cur.insert_ebb(ebb1); dfg.ins(cur).brnz(cond, ebb2, &[]); - let inst0 = dfg.ins(cur).jump(ebb1, &[]); + dfg.ins(cur).jump(ebb1, &[]); cur.insert_ebb(ebb2); - dfg.ins(cur).brnz(cond, ebb3, &[]); let inst1 = dfg.ins(cur).jump(ebb0, &[]); + dfg.ins(cur).brnz(cond, ebb3, &[]); cur.insert_ebb(ebb3); dfg.ins(cur).return_(&[]); - (inst0, inst1) + inst1 }; let mut loop_analysis = LoopAnalysis::new(); @@ -559,13 +605,8 @@ mod test { assert_eq!(loop_analysis.is_in_loop(ebb1, loops[0]), true); assert_eq!(loop_analysis.is_in_loop(ebb2, loops[0]), true); assert_eq!(loop_analysis.is_in_loop(ebb2, loops[1]), false); - assert_eq!(loop_analysis.is_in_loop(Ebb::with_number(4).unwrap(), loops[0]), - true); - assert_eq!(loop_analysis - .last_loop_instruction(ebb1, loops[1]) - .unwrap() - .unwrap(), - inst0); + assert_eq!(loop_analysis.last_loop_instruction(ebb1, loops[1]).unwrap(), + None); assert_eq!(loop_analysis .last_loop_instruction(ebb2, loops[0]) .unwrap() diff --git a/lib/cretonne/src/verifier/cssa.rs b/lib/cretonne/src/verifier/cssa.rs index 8ee444b74..fe9a2fe18 100644 --- a/lib/cretonne/src/verifier/cssa.rs +++ b/lib/cretonne/src/verifier/cssa.rs @@ -29,11 +29,11 @@ pub fn verify_cssa(func: &Function, virtregs: &VirtRegs) -> Result { let verifier = CssaVerifier { - func: func, - cfg: cfg, - domtree: domtree, - virtregs: virtregs, - liveness: liveness, + func, + cfg, + domtree, + virtregs, + liveness, }; verifier.check_virtregs()?; verifier.check_cssa()?; diff --git a/lib/cretonne/src/verifier/liveness.rs b/lib/cretonne/src/verifier/liveness.rs index 0c413ebae..2dff4a6e3 100644 --- a/lib/cretonne/src/verifier/liveness.rs +++ b/lib/cretonne/src/verifier/liveness.rs @@ -27,10 +27,10 @@ pub fn verify_liveness(isa: &TargetIsa, liveness: &Liveness) -> Result { let verifier = LivenessVerifier { - isa: isa, - func: func, - cfg: cfg, - liveness: liveness, + isa, + func, + cfg, + liveness, }; verifier.check_ebbs()?; verifier.check_insts()?; diff --git a/lib/cretonne/src/verifier/mod.rs b/lib/cretonne/src/verifier/mod.rs index fa0d9541f..d3f05ef4c 100644 --- a/lib/cretonne/src/verifier/mod.rs +++ b/lib/cretonne/src/verifier/mod.rs @@ -143,10 +143,10 @@ impl<'a> Verifier<'a> { let cfg = ControlFlowGraph::with_function(func); let domtree = DominatorTree::with_function(func, &cfg); Verifier { - func: func, - cfg: cfg, - domtree: domtree, - isa: isa, + func, + cfg, + domtree, + isa, } } @@ -237,9 +237,21 @@ impl<'a> Verifier<'a> { MultiAry { ref args, .. } => { self.verify_value_list(inst, args)?; } - Jump { destination, ref args, .. } | - Branch { destination, ref args, .. } | - BranchIcmp { destination, ref args, .. } => { + Jump { + destination, + ref args, + .. + } | + Branch { + destination, + ref args, + .. + } | + BranchIcmp { + destination, + ref args, + .. + } => { self.verify_ebb(inst, destination)?; self.verify_value_list(inst, args)?; } @@ -358,7 +370,7 @@ impl<'a> Verifier<'a> { // Defining instruction dominates the instruction that uses the value. if self.domtree.is_reachable(self.func.layout.pp_ebb(loc_inst)) && !self.domtree - .dominates(def_inst, loc_inst, &self.func.layout) { + .dominates(def_inst, loc_inst, &self.func.layout) { return err!(loc_inst, "uses value from non-dominating {}", def_inst); } } @@ -762,9 +774,10 @@ impl<'a> Verifier<'a> { let encoding = self.func.encodings.get_or_default(inst); if encoding.is_legal() { - let verify_encoding = isa.encode(&self.func.dfg, - &self.func.dfg[inst], - self.func.dfg.ctrl_typevar(inst)); + let verify_encoding = + isa.encode(&self.func.dfg, + &self.func.dfg[inst], + self.func.dfg.ctrl_typevar(inst)); match verify_encoding { Ok(verify_encoding) => { if verify_encoding != encoding { @@ -866,8 +879,9 @@ mod tests { let mut func = Function::new(); let ebb0 = func.dfg.make_ebb(); func.layout.append_ebb(ebb0); - let nullary_with_bad_opcode = func.dfg - .make_inst(InstructionData::Nullary { opcode: Opcode::Jump }); + let nullary_with_bad_opcode = + func.dfg + .make_inst(InstructionData::Nullary { opcode: Opcode::Jump }); func.layout.append_inst(nullary_with_bad_opcode, ebb0); let verifier = Verifier::new(&func, None); assert_err_with_msg!(verifier.run(), "instruction format"); From 17215848cb500cd47e8ea268c9525b4e99768ae2 Mon Sep 17 00:00:00 2001 From: Denis Merigoux Date: Mon, 7 Aug 2017 09:35:23 -0700 Subject: [PATCH 5/6] Adapted PR to changes in master --- lib/cretonne/src/loop_analysis.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cretonne/src/loop_analysis.rs b/lib/cretonne/src/loop_analysis.rs index cc2aba288..83da227d7 100644 --- a/lib/cretonne/src/loop_analysis.rs +++ b/lib/cretonne/src/loop_analysis.rs @@ -4,7 +4,7 @@ use dominator_tree::DominatorTree; use entity_map::{EntityMap, PrimaryEntityData, Keys}; use flowgraph::ControlFlowGraph; -use ir::{Function, Ebb, Layout, Cursor, InstBuilder, ProgramOrder}; +use ir::{Function, Ebb, Layout, Cursor, CursorBase, InstBuilder, ProgramOrder}; use ir::entities::Inst; use packed_option::PackedOption; use std::cmp::Ordering; @@ -390,7 +390,7 @@ impl LoopAnalysis { #[cfg(test)] mod test { - use ir::{Function, InstBuilder, Cursor, CursorBase, types}; + use ir::{Function, InstBuilder, Cursor, CursorBase, Ebb, types}; use loop_analysis::{Loop, LoopAnalysis}; use flowgraph::ControlFlowGraph; use dominator_tree::DominatorTree; From 3ccc9dd9524f3c54e228e8adc565616533ad1696 Mon Sep 17 00:00:00 2001 From: Denis Merigoux Date: Thu, 10 Aug 2017 11:40:17 -0700 Subject: [PATCH 6/6] Bugfixes after test with real-world code --- lib/cretonne/src/context.rs | 8 +- lib/cretonne/src/dominator_tree.rs | 3 +- lib/cretonne/src/flowgraph.rs | 2 +- lib/cretonne/src/loop_analysis.rs | 159 +++++++++++++++++++++++------ lib/cretonne/src/verifier/mod.rs | 4 +- 5 files changed, 143 insertions(+), 33 deletions(-) diff --git a/lib/cretonne/src/context.rs b/lib/cretonne/src/context.rs index 9967f1e11..cf7b6be45 100644 --- a/lib/cretonne/src/context.rs +++ b/lib/cretonne/src/context.rs @@ -121,7 +121,13 @@ impl Context { self.verify(None).map_err(Into::into) } - /// Perform LICM on the function. + /// Recompute the loop analysis of the function. Needs the flowgraph to be computed first. + pub fn loops(&mut self) { + self.loop_analysis + .compute(&mut self.func, &mut self.cfg, &mut self.domtree) + } + + /// Perform LICM on the function. Needs the loop analysis to be computed first. pub fn licm(&mut self) -> CtonResult { do_licm(&mut self.func, &mut self.cfg, diff --git a/lib/cretonne/src/dominator_tree.rs b/lib/cretonne/src/dominator_tree.rs index d88adbd4a..b44440394 100644 --- a/lib/cretonne/src/dominator_tree.rs +++ b/lib/cretonne/src/dominator_tree.rs @@ -12,7 +12,7 @@ use std::cmp::Ordering; const STRIDE: u32 = 4; // Dominator tree node. We keep one of these per EBB. -#[derive(Clone, Default)] +#[derive(Clone, Default, Debug)] struct DomNode { // Number of this node in a reverse post-order traversal of the CFG, starting from 1. // This number is monotonic in the reverse postorder but not contiguous, since we leave @@ -29,6 +29,7 @@ struct DomNode { } /// The dominator tree for a single function. +#[derive(Debug)] pub struct DominatorTree { nodes: EntityMap, diff --git a/lib/cretonne/src/flowgraph.rs b/lib/cretonne/src/flowgraph.rs index 9634eca6c..46a0e7841 100644 --- a/lib/cretonne/src/flowgraph.rs +++ b/lib/cretonne/src/flowgraph.rs @@ -98,7 +98,7 @@ impl ControlFlowGraph { // Temporarily take ownership because we need mutable access to self.data inside the loop. // Unfortunately borrowck cannot see that our mut accesses to predecessors don't alias // our iteration over successors. - let mut successors = mem::replace(&mut self.data[ebb].successors, Vec::new()); + let mut successors = mem::replace(&mut self.data.ensure(ebb).successors, Vec::new()); for suc in successors.iter().cloned() { self.data[suc].predecessors.retain(|&(e, _)| e != ebb); } diff --git a/lib/cretonne/src/loop_analysis.rs b/lib/cretonne/src/loop_analysis.rs index 83da227d7..8ebf9f771 100644 --- a/lib/cretonne/src/loop_analysis.rs +++ b/lib/cretonne/src/loop_analysis.rs @@ -40,6 +40,19 @@ impl LoopData { } } +// The loop analysis can split an Ebb that belongs partially to two loops or more. +// We have to record that because splitting involves incrementally updating the dominator tree, +// putting it out of sync with a dominator tree recomputed from scratch. +struct SideEffects { + ebb_splitted: bool, +} + +impl SideEffects { + fn propagate(&mut self, other: SideEffects) { + self.ebb_splitted = self.ebb_splitted || other.ebb_splitted; + } +} + /// If an `Ebb` is part of a loop, then we record two things: the id of the loop it's part of /// and the last instruction in the `Ebb` pertaining to the loop. If the `Ebb` is part of multiple /// loops, then we make sure by splitting the `Ebb` that it is part of at most two loops, one being @@ -99,6 +112,31 @@ impl LoopAnalysis { } } + /// Returns the inner-most loop of which this `Ebb` is a part of. + pub fn base_loop_ebb(&self, ebb: Ebb) -> Option { + self.ebb_loop_map[ebb].loop_id.expand() + } + + /// Returns the inner-most loop of which this `Inst` is a part of. + pub fn base_loop_inst(&self, inst: Inst, layout: &Layout) -> Option { + let ebb = layout.inst_ebb(inst).expect("inst should be inserted"); + let (lp, last_lp_inst) = match self.ebb_loop_map[ebb].loop_id.expand() { + None => return None, + Some(lp) => (lp, self.ebb_loop_map[ebb].last_inst), + }; + if last_lp_inst.is_none() || layout.cmp(inst, last_lp_inst.unwrap()) != Ordering::Greater { + Some(lp) + } else { + // If the instruction is beyond the inner-most loop limit + // then by construction it either in the parent loop or in + // no loop at all if the parent loop doesn't exist + match self.loop_parent(lp) { + None => None, + Some(lp_parent) => Some(lp_parent), + } + } + } + /// Determine which region of an `Ebb` belongs to a particular loop. /// /// Three cases arise: @@ -136,6 +174,40 @@ impl LoopAnalysis { } false } + + /// Returns the outermost loop of which `lp` is a child of (goes all the way up the loop tree). + /// If `limit` is not `None`, returns the outermost loop which is not `limit`. When `limit` + /// is a parent of `lp`, the returned loop is a direct child of `limit`. + pub fn outermost_loop(&self, lp: Loop, limit: Option) -> Loop { + let mut finger = Some(lp); + let mut parent = lp; + while let Some(finger_loop) = finger { + match limit { + None => parent = finger_loop, + Some(limit) => { + if finger_loop == limit { + return parent; + } else { + parent = finger_loop; + } + } + } + finger = self.loop_parent(finger_loop); + } + parent + } + + /// Returns the least common ancestor of two loops in the loop tree, if they share one. + pub fn least_common_ancestor(&self, lp1: Loop, lp2: Loop) -> Option { + let mut finger = Some(lp1); + while let Some(finger_loop) = finger { + if self.is_child_loop(lp2, finger_loop) { + return Some(finger_loop); + } + finger = self.loop_parent(finger_loop); + } + None + } } impl LoopAnalysis { @@ -154,7 +226,17 @@ impl LoopAnalysis { self.ebb_loop_map.clear(); self.ebb_loop_map.resize(func.dfg.num_ebbs()); self.find_loop_headers(cfg, domtree, &func.layout); - self.discover_loop_blocks(cfg, domtree, func); + let side_effects = self.discover_loop_blocks(cfg, domtree, func); + // During the loop block discovery, the loop analysis can split ebbs. While doing so, + // it incrementally updates the dominator tree so that the algorithm can continue its work. + // However, the incremental update of the dominator tree breaks the property that if we + // recompute the dominator tree from scratch, it will be exactly the same as the one we + // have. + // So if we have splitted an Ebb we have to recompute it from scratch now, to make sure + // it passes verifier::verify_context. + if side_effects.ebb_splitted { + domtree.compute(func, cfg); + } } // Traverses the CFG in reverse postorder and create a loop object for every EBB having a @@ -183,8 +265,10 @@ impl LoopAnalysis { fn discover_loop_blocks(&mut self, cfg: &mut ControlFlowGraph, domtree: &mut DominatorTree, - func: &mut Function) { + func: &mut Function) + -> SideEffects { let mut stack: Vec<(Ebb, Inst)> = Vec::new(); + let mut global_side_effects = SideEffects { ebb_splitted: false }; // We handle each loop header in reverse order, corresponding to a pesudo postorder // traversal of the graph. for lp in self.loops().rev() { @@ -196,7 +280,8 @@ impl LoopAnalysis { } // Then we proceed to discover loop blocks by doing a "reverse" DFS while let Some((ebb, loop_edge_inst)) = stack.pop() { - let continue_dfs = self.visit_loop_ebb(lp, ebb, loop_edge_inst, func, domtree, cfg); + let (continue_dfs, side_effects) = + self.visit_loop_ebb(lp, ebb, loop_edge_inst, func, domtree, cfg); // Now we have handled the popped Ebb and need to continue the DFS by adding the // predecessors of that Ebb if let Some(continue_dfs) = continue_dfs { @@ -204,8 +289,11 @@ impl LoopAnalysis { stack.push((pre, pre_inst)) } } + // We also propagate the side effects + global_side_effects.propagate(side_effects); } } + global_side_effects } fn visit_loop_ebb(&mut self, @@ -215,8 +303,9 @@ impl LoopAnalysis { func: &mut Function, domtree: &mut DominatorTree, cfg: &mut ControlFlowGraph) - -> Option { + -> (Option, SideEffects) { let continue_dfs: Option; + let mut split_ebb = false; match self.ebb_loop_map[ebb].loop_id.expand() { None => { // The ebb hasn't been visited yet, we tag it as part of the loop @@ -323,28 +412,23 @@ impl LoopAnalysis { match self.ebb_loop_map[ebb].last_inst.expand() { None => (), Some(last_inner_loop) => { + let limit = + match func.layout.cmp(loop_edge_inst, last_inner_loop) { + Ordering::Greater | Ordering::Equal => loop_edge_inst, + Ordering::Less => last_inner_loop, + }; if func.layout .last_inst(ebb) - .map_or(false, |ebb_last_inst| { - ebb_last_inst != loop_edge_inst - }) { - let limit = - match func.layout.cmp(loop_edge_inst, - last_inner_loop) { - Ordering::Greater | Ordering::Equal => { - loop_edge_inst - } - Ordering::Less => last_inner_loop, - }; - { - // This handles the second case - self.split_ebb_containing_two_loops(ebb, - limit, - lp, - func, - domtree, - cfg); - } + .map_or(false, + |ebb_last_inst| ebb_last_inst != limit) { + // This handles the second case + self.split_ebb_containing_two_loops(ebb, + limit, + lp, + func, + domtree, + cfg); + split_ebb = true; } } }; @@ -353,7 +437,7 @@ impl LoopAnalysis { } } } - continue_dfs + (continue_dfs, SideEffects { ebb_splitted: split_ebb }) } // We are in the case where `ebb` belongs partially to two different loops, the child and @@ -367,11 +451,16 @@ impl LoopAnalysis { func: &mut Function, domtree: &mut DominatorTree, cfg: &mut ControlFlowGraph) { + if func.layout.inst_ebb(split_inst).unwrap() != ebb { + // This is a tricky edge case. Basically this function can be called twice with the + // same arguments, in some pathological situations where an ebb can be splitted for + // two different reasons and the iterator in visit_loop_ebb is not modified to ignore + // the second reason after the ebb split. + // Because of that, we simply do nothing the second time. + return; + } let new_ebb = func.dfg.make_ebb(); - let split_inst_next = func.layout - .next_inst(split_inst) - .expect("you cannot split at the last instruction"); - func.layout.split_ebb(new_ebb, split_inst_next); + func.layout.split_ebb(new_ebb, split_inst); let middle_jump_inst = { let cur = &mut Cursor::new(&mut func.layout); cur.goto_bottom(ebb); @@ -387,6 +476,18 @@ impl LoopAnalysis { } } +impl LoopAnalysis { + /// Updates the loop analysis information when a loop pre-header is created. + pub fn recompute_loop_preheader(&mut self, pre_header: Ebb, header: Ebb) { + let header_lp = self.base_loop_ebb(header) + .expect("the header should belong to a loop"); + *self.ebb_loop_map.ensure(pre_header) = EbbLoopData { + loop_id: self.loop_parent(header_lp).into(), + last_inst: None.into(), + } + } +} + #[cfg(test)] mod test { diff --git a/lib/cretonne/src/verifier/mod.rs b/lib/cretonne/src/verifier/mod.rs index d3f05ef4c..718c6245e 100644 --- a/lib/cretonne/src/verifier/mod.rs +++ b/lib/cretonne/src/verifier/mod.rs @@ -414,7 +414,9 @@ impl<'a> Verifier<'a> { // We also verify if the postorder defined by `DominatorTree` is sane if self.domtree.cfg_postorder().len() != domtree.cfg_postorder().len() { return err!(AnyEntity::Function, - "incorrect number of Ebbs in postorder traversal"); + "incorrect number of Ebbs in postorder traversal: {}, should be {}", + domtree.cfg_postorder().len(), + self.domtree.cfg_postorder().len()); } for (index, (&true_ebb, &test_ebb)) in self.domtree