From 3ccc9dd9524f3c54e228e8adc565616533ad1696 Mon Sep 17 00:00:00 2001 From: Denis Merigoux Date: Thu, 10 Aug 2017 11:40:17 -0700 Subject: [PATCH] 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