From 60a28e60aa6eb0ed074fa5e6875e60cb2f038605 Mon Sep 17 00:00:00 2001 From: James Miller Date: Fri, 11 Mar 2016 13:14:51 +1300 Subject: [PATCH 1/7] Add some standard traversal iterators for MIR Adds Preorder, Postorder and Reverse Postorder traversal iterators. Also makes trans/mir use Reverse Postorder traversal for blocks. --- src/librustc_data_structures/bitvec.rs | 1 + src/librustc_mir/lib.rs | 1 + src/librustc_mir/traversal.rs | 276 +++++++++++++++++++++++++ src/librustc_trans/mir/mod.rs | 8 +- 4 files changed, 284 insertions(+), 2 deletions(-) create mode 100644 src/librustc_mir/traversal.rs diff --git a/src/librustc_data_structures/bitvec.rs b/src/librustc_data_structures/bitvec.rs index 3677c8c5e59..e45c6cfc6dc 100644 --- a/src/librustc_data_structures/bitvec.rs +++ b/src/librustc_data_structures/bitvec.rs @@ -9,6 +9,7 @@ // except according to those terms. /// A very simple BitVector type. +#[derive(Clone)] pub struct BitVector { data: Vec, } diff --git a/src/librustc_mir/lib.rs b/src/librustc_mir/lib.rs index 67bac196f48..cbbad057cf9 100644 --- a/src/librustc_mir/lib.rs +++ b/src/librustc_mir/lib.rs @@ -40,3 +40,4 @@ mod hair; pub mod mir_map; pub mod pretty; pub mod transform; +pub mod traversal; diff --git a/src/librustc_mir/traversal.rs b/src/librustc_mir/traversal.rs new file mode 100644 index 00000000000..fc53881bd81 --- /dev/null +++ b/src/librustc_mir/traversal.rs @@ -0,0 +1,276 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use std::vec; + +use rustc_data_structures::bitvec::BitVector; + +use rustc::mir::repr::*; + +/// Preorder traversal of a graph. +/// +/// Preorder traversal is when each node is visited before an of it's +/// successors +/// +/// A +/// / \ +/// / \ +/// B C +/// \ / +/// \ / +/// D +/// +/// A preorder traversal of this graph is either `A B D C` or `A C D B` +#[derive(Clone)] +pub struct Preorder<'a, 'tcx: 'a> { + mir: &'a Mir<'tcx>, + visited: BitVector, + worklist: Vec, +} + +impl<'a, 'tcx> Preorder<'a, 'tcx> { + pub fn new(mir: &'a Mir<'tcx>, root: BasicBlock) -> Preorder<'a, 'tcx> { + let worklist = vec![root]; + + Preorder { + mir: mir, + visited: BitVector::new(mir.basic_blocks.len()), + worklist: worklist + } + } +} + +pub fn preorder<'a, 'tcx>(mir: &'a Mir<'tcx>) -> Preorder<'a, 'tcx> { + Preorder::new(mir, START_BLOCK) +} + +impl<'a, 'tcx> Iterator for Preorder<'a, 'tcx> { + type Item = (BasicBlock, &'a BasicBlockData<'tcx>); + + fn next(&mut self) -> Option<(BasicBlock, &'a BasicBlockData<'tcx>)> { + while let Some(idx) = self.worklist.pop() { + if !self.visited.insert(idx.index()) { + continue; + } + + let data = self.mir.basic_block_data(idx); + + if let Some(ref term) = data.terminator { + for &succ in term.successors().iter() { + self.worklist.push(succ); + } + } + + return Some((idx, data)); + } + + None + } +} + +/// Postorder traversal of a graph. +/// +/// Postorder traversal is when each node is visited after all of it's +/// successors, except when the successor is only reachable by a back-edge +/// +/// A +/// / \ +/// / \ +/// B C +/// \ / +/// \ / +/// D +/// +/// A Postorder traversal of this graph is `D B C A` or `D C B A` +pub struct Postorder<'a, 'tcx: 'a> { + mir: &'a Mir<'tcx>, + visited: BitVector, + visit_stack: Vec<(BasicBlock, vec::IntoIter)> +} + +impl<'a, 'tcx> Postorder<'a, 'tcx> { + pub fn new(mir: &'a Mir<'tcx>, root: BasicBlock) -> Postorder<'a, 'tcx> { + let mut po = Postorder { + mir: mir, + visited: BitVector::new(mir.basic_blocks.len()), + visit_stack: Vec::new() + }; + + + let data = po.mir.basic_block_data(root); + + if let Some(ref term) = data.terminator { + po.visited.insert(root.index()); + + let succs = term.successors().into_owned().into_iter(); + + po.visit_stack.push((root, succs)); + po.traverse_successor(); + } + + po + } + + fn traverse_successor(&mut self) { + // This is quite a complex loop due to 1. the borrow checker not liking it much + // and 2. what exactly is going on is not clear + // + // It does the actual traversal of the graph, while the `next` method on the iterator + // just pops off of the stack. `visit_stack` is a stack containing pairs of nodes and + // iterators over the sucessors of those nodes. Each iteration attempts to get the next + // node from the top of the stack, then pushes that node and an iterator over the + // successors to the top of the stack. This loop only grows `visit_stack`, stopping when + // we reach a child that has no children that we haven't already visited. + // + // For a graph that looks like this: + // + // A + // / \ + // / \ + // B C + // | | + // | | + // D | + // \ / + // \ / + // E + // + // The state of the stack starts out with just the root node (`A` in this case); + // [(A, [B, C])] + // + // When the first call to `traverse_sucessor` happens, the following happens: + // + // [(B, [D]), // `B` taken from the successors of `A`, pushed to the + // // top of the stack along with the successors of `B` + // (A, [C])] + // + // [(D, [E]), // `D` taken from successors of `B`, pushed to stack + // (B, []), + // (A, [C])] + // + // [(E, []), // `E` taken from successors of `D`, pushed to stack + // (D, []), + // (B, []), + // (A, [C])] + // + // Now that the top of the stack has no successors we can traverse, each item will + // be popped off during iteration until we get back to `A`. This yeilds [E, D, B]. + // + // When we yeild `B` and call `traverse_successor`, We push `C` to the stack, but + // since we've already visited `E`, that child isn't added to the stack. The last + // two iterations yield `C` and finally `A` for a final traversal of [E, D, B, C, A] + loop { + let bb = if let Some(&mut (_, ref mut iter)) = self.visit_stack.last_mut() { + if let Some(bb) = iter.next() { + bb + } else { + break; + } + } else { + break; + }; + + if self.visited.insert(bb.index()) { + let data = self.mir.basic_block_data(bb); + + if let Some(ref term) = data.terminator { + let succs = term.successors().into_owned().into_iter(); + self.visit_stack.push((bb, succs)); + } + } + } + } +} + +pub fn postorder<'a, 'tcx>(mir: &'a Mir<'tcx>) -> Postorder<'a, 'tcx> { + Postorder::new(mir, START_BLOCK) +} + +impl<'a, 'tcx> Iterator for Postorder<'a, 'tcx> { + type Item = (BasicBlock, &'a BasicBlockData<'tcx>); + + fn next(&mut self) -> Option<(BasicBlock, &'a BasicBlockData<'tcx>)> { + let next = self.visit_stack.pop(); + if next.is_some() { + self.traverse_successor(); + } + + next.map(|(bb, _)| { + let data = self.mir.basic_block_data(bb); + (bb, data) + }) + } +} + +/// Reverse postorder traversal of a graph +/// +/// Reverse postorder is the reverse order of a postorder traversal. +/// This is different to a preorder traversal and represents a natural +/// linearisation of control-flow. +/// +/// A +/// / \ +/// / \ +/// B C +/// \ / +/// \ / +/// D +/// +/// A reverse postorder traversal of this graph is either `A B C D` or `A C B D` +/// Note that for a graph containing no loops (i.e. A DAG), this is equivalent to +/// a topological sort. +/// +/// Construction of a `ReversePostorder` traversal requires doing a full +/// postorder traversal of the graph, therefore this traversal should be +/// constructed as few times as possible. Use the `reset` method to be able +/// to re-use the traversal +#[derive(Clone)] +pub struct ReversePostorder<'a, 'tcx: 'a> { + mir: &'a Mir<'tcx>, + blocks: Vec, + idx: usize +} + +impl<'a, 'tcx> ReversePostorder<'a, 'tcx> { + pub fn new(mir: &'a Mir<'tcx>, root: BasicBlock) -> ReversePostorder<'a, 'tcx> { + let blocks : Vec<_> = Postorder::new(mir, root).map(|(bb, _)| bb).collect(); + + let len = blocks.len(); + + ReversePostorder { + mir: mir, + blocks: blocks, + idx: len + } + } + + pub fn reset(&mut self) { + self.idx = self.blocks.len(); + } +} + + +pub fn reverse_postorder<'a, 'tcx>(mir: &'a Mir<'tcx>) -> ReversePostorder<'a, 'tcx> { + ReversePostorder::new(mir, START_BLOCK) +} + +impl<'a, 'tcx> Iterator for ReversePostorder<'a, 'tcx> { + type Item = (BasicBlock, &'a BasicBlockData<'tcx>); + + fn next(&mut self) -> Option<(BasicBlock, &'a BasicBlockData<'tcx>)> { + if self.idx == 0 { return None; } + self.idx -= 1; + + self.blocks.get(self.idx).map(|&bb| { + let data = self.mir.basic_block_data(bb); + (bb, data) + }) + } +} diff --git a/src/librustc_trans/mir/mod.rs b/src/librustc_trans/mir/mod.rs index 61536f3ecae..152834a07ba 100644 --- a/src/librustc_trans/mir/mod.rs +++ b/src/librustc_trans/mir/mod.rs @@ -20,6 +20,9 @@ use std::ops::Deref; use std::rc::Rc; use self::lvalue::{LvalueRef, get_dataptr, get_meta}; +use rustc_mir::traversal; + +use self::lvalue::LvalueRef; use self::operand::OperandRef; #[derive(Clone)] @@ -152,8 +155,9 @@ pub fn trans_mir<'blk, 'tcx>(fcx: &'blk FunctionContext<'blk, 'tcx>) { args: args, }; - // Translate the body of each block - for &bb in &mir_blocks { + let rpo = traversal::reverse_postorder(mir); + // Translate the body of each block using reverse postorder + for (bb, _) in rpo { mircx.trans_block(bb); } From eee7f3c73298ed77f61ad15cdca552528d6f3783 Mon Sep 17 00:00:00 2001 From: James Miller Date: Fri, 11 Mar 2016 18:00:52 +1300 Subject: [PATCH 2/7] Add and use a break critical edges transform This is a fairly standard transform that inserts blocks along critical edges so code can be inserted along the edge without it affecting other edges. The main difference is that it considers a Drop or Call terminator that would require an `invoke` instruction in LLVM a critical edge. This is because we can't actually insert code after an invoke, so it ends up looking similar to a critical edge anyway. The transform is run just before translation right now. --- src/librustc_driver/driver.rs | 9 +- .../transform/break_critical_edges.rs | 180 ++++++++++++++++++ src/librustc_mir/transform/mod.rs | 1 + src/librustc_trans/mir/block.rs | 46 ++--- src/librustc_trans/mir/mod.rs | 33 +++- 5 files changed, 227 insertions(+), 42 deletions(-) create mode 100644 src/librustc_mir/transform/break_critical_edges.rs diff --git a/src/librustc_driver/driver.rs b/src/librustc_driver/driver.rs index 468dc7b12c1..03f6ce355b8 100644 --- a/src/librustc_driver/driver.rs +++ b/src/librustc_driver/driver.rs @@ -932,7 +932,7 @@ pub fn phase_3_run_analysis_passes<'tcx, F, R>(sess: &'tcx Session, /// Run the translation phase to LLVM, after which the AST and analysis can pub fn phase_4_translate_to_llvm<'tcx>(tcx: &TyCtxt<'tcx>, - mir_map: MirMap<'tcx>, + mut mir_map: MirMap<'tcx>, analysis: ty::CrateAnalysis) -> trans::CrateTranslation { let time_passes = tcx.sess.time_passes(); @@ -941,6 +941,13 @@ pub fn phase_4_translate_to_llvm<'tcx>(tcx: &TyCtxt<'tcx>, "resolving dependency formats", || dependency_format::calculate(&tcx.sess)); + time(time_passes, + "erasing regions from MIR", + || mir::transform::erase_regions::erase_regions(tcx, &mut mir_map)); + + time(time_passes, "breaking critical edges", + || mir::transform::break_critical_edges::break_critical_edges(&mut mir_map)); + // Option dance to work around the lack of stack once closures. time(time_passes, "translation", diff --git a/src/librustc_mir/transform/break_critical_edges.rs b/src/librustc_mir/transform/break_critical_edges.rs new file mode 100644 index 00000000000..5e64da0ec90 --- /dev/null +++ b/src/librustc_mir/transform/break_critical_edges.rs @@ -0,0 +1,180 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. +use std::mem; + +use rustc_back::slice; +use rustc::mir::repr::*; +use rustc::mir::mir_map::MirMap; + +use traversal; + +/** + * Breaks critical edges in the MIR. + * + * Critical edges are edges that are neither the only edge leaving a + * block, nor the only edge entering one. + * + * When you want something to happen "along" an edge, you can either + * do at the end of the predecessor block, or at the start of the + * successor block. Critical edges have to be broken in order to prevent + * "edge actions" from affecting other edges. + * + * This function will break those edges by inserting new blocks along them. + * + * A special case is Drop and Call terminators with unwind/cleanup successors, + * They use `invoke` in LLVM, which terminates a block, meaning that code cannot + * be inserted after them, so even if an edge is the only edge leaving a block + * like that, we still insert blocks if the edge is one of many entering the + * target. + * + * NOTE: Simplify CFG will happily undo most of the work this pass does. + * + */ +pub fn break_critical_edges<'tcx>(mir_map: &mut MirMap<'tcx>) { + for (_, mir) in &mut mir_map.map { + break_critical_edges_fn(mir); + } +} + +/* + * Predecessor map for tracking the predecessors of a block + */ +struct PredMap { + preds: Vec +} + +/** + * Most blocks only have one predecessor, so we can cut down on + * some allocation by not using Vec until we have more than one. + */ +#[derive(Clone)] +enum BlockPredecessors { + None, + One(BasicBlock), + Some(Vec) +} + +impl PredMap { + pub fn new(n: usize) -> PredMap { + let preds = vec![BlockPredecessors::None; n]; + + PredMap { + preds: preds + } + } + + fn ensure_len(&mut self, bb: BasicBlock) { + let idx = bb.index(); + while self.preds.len() <= idx { + self.preds.push(BlockPredecessors::None); + } + } + + pub fn add_pred(&mut self, target: BasicBlock, pred: BasicBlock) { + self.ensure_len(target); + + let preds = mem::replace(&mut self.preds[target.index()], BlockPredecessors::None); + match preds { + BlockPredecessors::None => { + self.preds[target.index()] = BlockPredecessors::One(pred); + } + BlockPredecessors::One(bb) => { + self.preds[target.index()] = BlockPredecessors::Some(vec![bb, pred]); + } + BlockPredecessors::Some(mut preds) => { + preds.push(pred); + self.preds[target.index()] = BlockPredecessors::Some(preds); + } + } + } + + pub fn remove_pred(&mut self, target: BasicBlock, pred: BasicBlock) { + self.ensure_len(target); + + let preds = mem::replace(&mut self.preds[target.index()], BlockPredecessors::None); + match preds { + BlockPredecessors::None => {} + BlockPredecessors::One(bb) if bb == pred => {} + + BlockPredecessors::One(bb) => { + self.preds[target.index()] = BlockPredecessors::One(bb); + } + + BlockPredecessors::Some(mut preds) => { + preds.retain(|&bb| bb != pred); + self.preds[target.index()] = BlockPredecessors::Some(preds); + } + } + } + + pub fn get_preds(&self, bb: BasicBlock) -> &[BasicBlock] { + match self.preds[bb.index()] { + BlockPredecessors::None => &[], + BlockPredecessors::One(ref bb) => slice::ref_slice(bb), + BlockPredecessors::Some(ref bbs) => &bbs[..] + } + } +} + + +fn break_critical_edges_fn(mir: &mut Mir) { + let mut pred_map = PredMap::new(mir.basic_blocks.len()); + + // Build the precedecessor map for the MIR + for (pred, data) in traversal::preorder(mir) { + if let Some(ref term) = data.terminator { + for &tgt in term.successors().iter() { + pred_map.add_pred(tgt, pred); + } + } + } + + // We need a place to store the new blocks generated + let mut new_blocks = Vec::new(); + + let bbs = mir.all_basic_blocks(); + let cur_len = mir.basic_blocks.len(); + + for &bb in &bbs { + let data = mir.basic_block_data_mut(bb); + + if let Some(ref mut term) = data.terminator { + let is_invoke = term_is_invoke(term); + let succs = term.successors_mut(); + if succs.len() > 1 || (succs.len() > 0 && is_invoke) { + for tgt in succs { + let num_preds = pred_map.get_preds(*tgt).len(); + if num_preds > 1 { + // It's a critical edge, break it + let goto = Terminator::Goto { target: *tgt }; + let data = BasicBlockData::new(Some(goto)); + // Get the index it will be when inserted into the MIR + let idx = cur_len + new_blocks.len(); + new_blocks.push(data); + *tgt = BasicBlock::new(idx); + } + } + } + } + } + + debug!("Broke {} N edges", new_blocks.len()); + + mir.basic_blocks.extend_from_slice(&new_blocks); +} + +// Returns true if the terminator would use an invoke in LLVM. +fn term_is_invoke(term: &Terminator) -> bool { + match *term { + Terminator::Call { cleanup: Some(_), .. } | + Terminator::Drop { unwind: Some(_), .. } => true, + _ => false + } +} diff --git a/src/librustc_mir/transform/mod.rs b/src/librustc_mir/transform/mod.rs index 57690caeccb..a52a8edc211 100644 --- a/src/librustc_mir/transform/mod.rs +++ b/src/librustc_mir/transform/mod.rs @@ -13,3 +13,4 @@ pub mod simplify_cfg; pub mod erase_regions; pub mod no_landing_pads; pub mod type_check; +pub mod break_critical_edges; diff --git a/src/librustc_trans/mir/block.rs b/src/librustc_trans/mir/block.rs index a60b03dd724..1efbe5df162 100644 --- a/src/librustc_trans/mir/block.rs +++ b/src/librustc_trans/mir/block.rs @@ -300,33 +300,8 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { // Many different ways to call a function handled here if let Some(cleanup) = cleanup.map(|bb| self.bcx(bb)) { - // We translate the copy into a temporary block. The temporary block is - // necessary because the current block has already been terminated (by - // `invoke`) and we cannot really translate into the target block - // because: - // * The target block may have more than a single precedesor; - // * Some LLVM insns cannot have a preceeding store insn (phi, - // cleanuppad), and adding/prepending the store now may render - // those other instructions invalid. - // - // NB: This approach still may break some LLVM code. For example if the - // target block starts with a `phi` (which may only match on immediate - // precedesors), it cannot know about this temporary block thus - // resulting in an invalid code: - // - // this: - // … - // %0 = … - // %1 = invoke to label %temp … - // temp: - // store ty %1, ty* %dest - // br label %actualtargetblock - // actualtargetblock: ; preds: %temp, … - // phi … [%this, …], [%0, …] ; ERROR: phi requires to match only on - // ; immediate precedesors - - let ret_bcx = if destination.is_some() { - self.fcx.new_block("", None) + let ret_bcx = if let Some((_, target)) = *destination { + self.blocks[target.index()] } else { self.unreachable_block() }; @@ -343,15 +318,16 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { self.set_operand_dropped(bcx, op); }); - if let Some((_, target)) = *destination { + if destination.is_some() { let ret_bcx = ret_bcx.build(); - if let Some(ret_dest) = ret_dest { - fn_ty.ret.store(&ret_bcx, invokeret, ret_dest.llval); - } - for op in args { - self.set_operand_dropped(&ret_bcx, op); - } - ret_bcx.br(self.llblock(target)); + ret_bcx.at_start(|ret_bcx| { + if let Some(ret_dest) = ret_dest { + fn_ty.ret.store(&ret_bcx, invokeret, ret_dest.llval); + } + for op in args { + self.set_operand_dropped(&ret_bcx, op); + } + }); } } else { let llret = bcx.call(fn_ptr, &llargs, cleanup_bundle.as_ref()); diff --git a/src/librustc_trans/mir/mod.rs b/src/librustc_trans/mir/mod.rs index 152834a07ba..27e09d04a9d 100644 --- a/src/librustc_trans/mir/mod.rs +++ b/src/librustc_trans/mir/mod.rs @@ -19,10 +19,11 @@ use common::{self, Block, BlockAndBuilder, FunctionContext}; use std::ops::Deref; use std::rc::Rc; +use rustc_data_structures::bitvec::BitVector; + use self::lvalue::{LvalueRef, get_dataptr, get_meta}; use rustc_mir::traversal; -use self::lvalue::LvalueRef; use self::operand::OperandRef; #[derive(Clone)] @@ -98,7 +99,7 @@ enum TempRef<'tcx> { /////////////////////////////////////////////////////////////////////////// -pub fn trans_mir<'blk, 'tcx>(fcx: &'blk FunctionContext<'blk, 'tcx>) { +pub fn trans_mir<'blk, 'tcx: 'blk>(fcx: &'blk FunctionContext<'blk, 'tcx>) { let bcx = fcx.init(false, None).build(); let mir = bcx.mir(); @@ -135,8 +136,13 @@ pub fn trans_mir<'blk, 'tcx>(fcx: &'blk FunctionContext<'blk, 'tcx>) { let block_bcxs: Vec> = mir_blocks.iter() .map(|&bb|{ - // FIXME(#30941) this doesn't handle msvc-style exceptions - fcx.new_block(&format!("{:?}", bb), None) + if bb == mir::START_BLOCK { + fcx.new_block("start", None) + } else if bb == mir::END_BLOCK { + fcx.new_block("end", None) + } else { + fcx.new_block(&format!("{:?}", bb), None) + } }) .collect(); @@ -145,7 +151,7 @@ pub fn trans_mir<'blk, 'tcx>(fcx: &'blk FunctionContext<'blk, 'tcx>) { bcx.br(start_bcx.llbb); let mut mircx = MirContext { - mir: mir, + mir: mir.clone(), fcx: fcx, llpersonalityslot: None, blocks: block_bcxs, @@ -155,13 +161,28 @@ pub fn trans_mir<'blk, 'tcx>(fcx: &'blk FunctionContext<'blk, 'tcx>) { args: args, }; - let rpo = traversal::reverse_postorder(mir); + let mut visited = BitVector::new(mir_blocks.len()); + + let rpo = traversal::reverse_postorder(&mir); // Translate the body of each block using reverse postorder for (bb, _) in rpo { + visited.insert(bb.index()); mircx.trans_block(bb); } + // Add unreachable instructions at the end of unreachable blocks + // so they're actually terminated. + // TODO: Remove the blocks from the function + for &bb in &mir_blocks { + if !visited.contains(bb.index()) { + mircx.blocks[bb.index()].build().unreachable(); + } + } + + fcx.cleanup(); + + debug!("trans_mir: {:?}", ::trans::value::Value(fcx.llfn)); } /// Produce, for each argument, a `ValueRef` pointing at the From c70bc3a5daa2ce46aad7c230004ade7a404c12f1 Mon Sep 17 00:00:00 2001 From: James Miller Date: Fri, 11 Mar 2016 19:42:26 +1300 Subject: [PATCH 3/7] Don't build a map of predecessors, just count them instead --- .../transform/break_critical_edges.rs | 92 +------------------ 1 file changed, 4 insertions(+), 88 deletions(-) diff --git a/src/librustc_mir/transform/break_critical_edges.rs b/src/librustc_mir/transform/break_critical_edges.rs index 5e64da0ec90..6ce9f348917 100644 --- a/src/librustc_mir/transform/break_critical_edges.rs +++ b/src/librustc_mir/transform/break_critical_edges.rs @@ -7,9 +7,6 @@ // , at your // option. This file may not be copied, modified, or distributed // except according to those terms. -use std::mem; - -use rustc_back::slice; use rustc::mir::repr::*; use rustc::mir::mir_map::MirMap; @@ -43,95 +40,14 @@ pub fn break_critical_edges<'tcx>(mir_map: &mut MirMap<'tcx>) { } } -/* - * Predecessor map for tracking the predecessors of a block - */ -struct PredMap { - preds: Vec -} - -/** - * Most blocks only have one predecessor, so we can cut down on - * some allocation by not using Vec until we have more than one. - */ -#[derive(Clone)] -enum BlockPredecessors { - None, - One(BasicBlock), - Some(Vec) -} - -impl PredMap { - pub fn new(n: usize) -> PredMap { - let preds = vec![BlockPredecessors::None; n]; - - PredMap { - preds: preds - } - } - - fn ensure_len(&mut self, bb: BasicBlock) { - let idx = bb.index(); - while self.preds.len() <= idx { - self.preds.push(BlockPredecessors::None); - } - } - - pub fn add_pred(&mut self, target: BasicBlock, pred: BasicBlock) { - self.ensure_len(target); - - let preds = mem::replace(&mut self.preds[target.index()], BlockPredecessors::None); - match preds { - BlockPredecessors::None => { - self.preds[target.index()] = BlockPredecessors::One(pred); - } - BlockPredecessors::One(bb) => { - self.preds[target.index()] = BlockPredecessors::Some(vec![bb, pred]); - } - BlockPredecessors::Some(mut preds) => { - preds.push(pred); - self.preds[target.index()] = BlockPredecessors::Some(preds); - } - } - } - - pub fn remove_pred(&mut self, target: BasicBlock, pred: BasicBlock) { - self.ensure_len(target); - - let preds = mem::replace(&mut self.preds[target.index()], BlockPredecessors::None); - match preds { - BlockPredecessors::None => {} - BlockPredecessors::One(bb) if bb == pred => {} - - BlockPredecessors::One(bb) => { - self.preds[target.index()] = BlockPredecessors::One(bb); - } - - BlockPredecessors::Some(mut preds) => { - preds.retain(|&bb| bb != pred); - self.preds[target.index()] = BlockPredecessors::Some(preds); - } - } - } - - pub fn get_preds(&self, bb: BasicBlock) -> &[BasicBlock] { - match self.preds[bb.index()] { - BlockPredecessors::None => &[], - BlockPredecessors::One(ref bb) => slice::ref_slice(bb), - BlockPredecessors::Some(ref bbs) => &bbs[..] - } - } -} - - fn break_critical_edges_fn(mir: &mut Mir) { - let mut pred_map = PredMap::new(mir.basic_blocks.len()); + let mut pred_count = vec![0u32; mir.basic_blocks.len()]; // Build the precedecessor map for the MIR - for (pred, data) in traversal::preorder(mir) { + for (_, data) in traversal::preorder(mir) { if let Some(ref term) = data.terminator { for &tgt in term.successors().iter() { - pred_map.add_pred(tgt, pred); + pred_count[tgt.index()] += 1; } } } @@ -150,7 +66,7 @@ fn break_critical_edges_fn(mir: &mut Mir) { let succs = term.successors_mut(); if succs.len() > 1 || (succs.len() > 0 && is_invoke) { for tgt in succs { - let num_preds = pred_map.get_preds(*tgt).len(); + let num_preds = pred_count[tgt.index()]; if num_preds > 1 { // It's a critical edge, break it let goto = Terminator::Goto { target: *tgt }; From 02cb1d4ef1e187494f7f089e114515dc74fabd10 Mon Sep 17 00:00:00 2001 From: James Miller Date: Fri, 11 Mar 2016 19:42:47 +1300 Subject: [PATCH 4/7] Delete obviously-unreachable blocks Some blocks won't be translated at all because they aren't reachable at the LLVM level, these need to be dealt with because they lack a terminator and therefore trigger an LLVM assertion. Other blocks aren't reachable because of codegen-time optimistions, for example not dropping types that don't need it, often resulting in blocks with no predecessors. We'll clean those up as well. --- src/librustc_trans/basic_block.rs | 6 ++++++ src/librustc_trans/mir/mod.rs | 17 ++++++++++------- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/librustc_trans/basic_block.rs b/src/librustc_trans/basic_block.rs index 919ea49b392..60bd3fb8ef1 100644 --- a/src/librustc_trans/basic_block.rs +++ b/src/librustc_trans/basic_block.rs @@ -49,4 +49,10 @@ impl BasicBlock { _ => None } } + + pub fn delete(self) { + unsafe { + llvm::LLVMDeleteBasicBlock(self.0); + } + } } diff --git a/src/librustc_trans/mir/mod.rs b/src/librustc_trans/mir/mod.rs index 27e09d04a9d..e1a0d33fa13 100644 --- a/src/librustc_trans/mir/mod.rs +++ b/src/librustc_trans/mir/mod.rs @@ -19,6 +19,8 @@ use common::{self, Block, BlockAndBuilder, FunctionContext}; use std::ops::Deref; use std::rc::Rc; +use trans::basic_block::BasicBlock; + use rustc_data_structures::bitvec::BitVector; use self::lvalue::{LvalueRef, get_dataptr, get_meta}; @@ -170,19 +172,20 @@ pub fn trans_mir<'blk, 'tcx: 'blk>(fcx: &'blk FunctionContext<'blk, 'tcx>) { mircx.trans_block(bb); } - // Add unreachable instructions at the end of unreachable blocks - // so they're actually terminated. - // TODO: Remove the blocks from the function + // Remove blocks that haven't been visited, or have no + // predecessors. for &bb in &mir_blocks { + let block = mircx.blocks[bb.index()]; + let block = BasicBlock(block.llbb); + // Unreachable block if !visited.contains(bb.index()) { - mircx.blocks[bb.index()].build().unreachable(); + block.delete(); + } else if block.pred_iter().count() == 0 { + block.delete(); } } - fcx.cleanup(); - - debug!("trans_mir: {:?}", ::trans::value::Value(fcx.llfn)); } /// Produce, for each argument, a `ValueRef` pointing at the From 6935782b07c1caf2cc78c44faf28cc4cf0658fe8 Mon Sep 17 00:00:00 2001 From: James Miller Date: Sat, 12 Mar 2016 20:18:26 +1300 Subject: [PATCH 5/7] Add a test --- src/test/run-pass/mir_trans_critical_edge.rs | 53 ++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 src/test/run-pass/mir_trans_critical_edge.rs diff --git a/src/test/run-pass/mir_trans_critical_edge.rs b/src/test/run-pass/mir_trans_critical_edge.rs new file mode 100644 index 00000000000..320f4017592 --- /dev/null +++ b/src/test/run-pass/mir_trans_critical_edge.rs @@ -0,0 +1,53 @@ +// Copyright 2015 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// This code produces a CFG with critical edges that, if we don't +// handle properly, will cause invalid codegen. + +#![feature(rustc_attrs)] + +enum State { + Both, + Front, + Back +} + +pub struct Foo { + state: State, + a: A, + b: B +} + +impl Foo +where A: Iterator, B: Iterator +{ + // This is the function we care about + #[rustc_mir] + fn next(&mut self) -> Option { + match self.state { + State::Both => match self.a.next() { + elt @ Some(..) => elt, + None => { + self.state = State::Back; + self.b.next() + } + }, + State::Front => self.a.next(), + State::Back => self.b.next(), + } + } +} + +// Make sure we actually translate a version of the function +pub fn do_stuff(mut f: Foo>, Box>>) { + let _x = f.next(); +} + +fn main() {} From 63321ca19390535795780ce15991b6238fb67db4 Mon Sep 17 00:00:00 2001 From: James Miller Date: Wed, 30 Mar 2016 14:46:02 +1300 Subject: [PATCH 6/7] Turn break critical edges into a MIR pass Also adds a new set of passes to run just before translation that "prepare" the MIR for codegen. Removal of landing pads, region erasure and break critical edges are run in this pass. Also fixes some merge/rebase errors. --- src/librustc_driver/driver.rs | 23 +++++++------ .../transform/break_critical_edges.rs | 32 +++++++++++++------ src/librustc_mir/traversal.rs | 2 +- src/librustc_trans/mir/mod.rs | 2 +- 4 files changed, 36 insertions(+), 23 deletions(-) diff --git a/src/librustc_driver/driver.rs b/src/librustc_driver/driver.rs index 03f6ce355b8..fbb185303e0 100644 --- a/src/librustc_driver/driver.rs +++ b/src/librustc_driver/driver.rs @@ -876,10 +876,7 @@ pub fn phase_3_run_analysis_passes<'tcx, F, R>(sess: &'tcx Session, passes.push_pass(box mir::transform::remove_dead_blocks::RemoveDeadBlocks); passes.push_pass(box mir::transform::type_check::TypeckMir); passes.push_pass(box mir::transform::simplify_cfg::SimplifyCfg); - // Late passes - passes.push_pass(box mir::transform::no_landing_pads::NoLandingPads); passes.push_pass(box mir::transform::remove_dead_blocks::RemoveDeadBlocks); - passes.push_pass(box mir::transform::erase_regions::EraseRegions); // And run everything. passes.run_passes(tcx, &mut mir_map); }); @@ -933,22 +930,24 @@ pub fn phase_3_run_analysis_passes<'tcx, F, R>(sess: &'tcx Session, /// Run the translation phase to LLVM, after which the AST and analysis can pub fn phase_4_translate_to_llvm<'tcx>(tcx: &TyCtxt<'tcx>, mut mir_map: MirMap<'tcx>, - analysis: ty::CrateAnalysis) - -> trans::CrateTranslation { + analysis: ty::CrateAnalysis) -> trans::CrateTranslation { let time_passes = tcx.sess.time_passes(); time(time_passes, "resolving dependency formats", || dependency_format::calculate(&tcx.sess)); - time(time_passes, - "erasing regions from MIR", - || mir::transform::erase_regions::erase_regions(tcx, &mut mir_map)); + // Run the passes that transform the MIR into a more suitable for translation + // to LLVM code. + time(time_passes, "Prepare MIR codegen passes", || { + let mut passes = ::rustc::mir::transform::Passes::new(); + passes.push_pass(box mir::transform::no_landing_pads::NoLandingPads); + passes.push_pass(box mir::transform::remove_dead_blocks::RemoveDeadBlocks); + passes.push_pass(box mir::transform::erase_regions::EraseRegions); + passes.push_pass(box mir::transform::break_critical_edges::BreakCriticalEdges); + passes.run_passes(tcx, &mut mir_map); + }); - time(time_passes, "breaking critical edges", - || mir::transform::break_critical_edges::break_critical_edges(&mut mir_map)); - - // Option dance to work around the lack of stack once closures. time(time_passes, "translation", move || trans::trans_crate(tcx, &mir_map, analysis)) diff --git a/src/librustc_mir/transform/break_critical_edges.rs b/src/librustc_mir/transform/break_critical_edges.rs index 6ce9f348917..e78c8e8fd73 100644 --- a/src/librustc_mir/transform/break_critical_edges.rs +++ b/src/librustc_mir/transform/break_critical_edges.rs @@ -7,11 +7,16 @@ // , at your // option. This file may not be copied, modified, or distributed // except according to those terms. + +use rustc::ty::TyCtxt; use rustc::mir::repr::*; -use rustc::mir::mir_map::MirMap; +use rustc::mir::transform::{MirPass, Pass}; +use syntax::ast::NodeId; use traversal; +pub struct BreakCriticalEdges; + /** * Breaks critical edges in the MIR. * @@ -34,13 +39,16 @@ use traversal; * NOTE: Simplify CFG will happily undo most of the work this pass does. * */ -pub fn break_critical_edges<'tcx>(mir_map: &mut MirMap<'tcx>) { - for (_, mir) in &mut mir_map.map { - break_critical_edges_fn(mir); + +impl<'tcx> MirPass<'tcx> for BreakCriticalEdges { + fn run_pass(&mut self, _: &TyCtxt<'tcx>, _: NodeId, mir: &mut Mir<'tcx>) { + break_critical_edges(mir); } } -fn break_critical_edges_fn(mir: &mut Mir) { +impl Pass for BreakCriticalEdges {} + +fn break_critical_edges(mir: &mut Mir) { let mut pred_count = vec![0u32; mir.basic_blocks.len()]; // Build the precedecessor map for the MIR @@ -63,13 +71,19 @@ fn break_critical_edges_fn(mir: &mut Mir) { if let Some(ref mut term) = data.terminator { let is_invoke = term_is_invoke(term); + let term_span = term.span; + let term_scope = term.scope; let succs = term.successors_mut(); if succs.len() > 1 || (succs.len() > 0 && is_invoke) { for tgt in succs { let num_preds = pred_count[tgt.index()]; if num_preds > 1 { // It's a critical edge, break it - let goto = Terminator::Goto { target: *tgt }; + let goto = Terminator { + span: term_span, + scope: term_scope, + kind: TerminatorKind::Goto { target: *tgt } + }; let data = BasicBlockData::new(Some(goto)); // Get the index it will be when inserted into the MIR let idx = cur_len + new_blocks.len(); @@ -88,9 +102,9 @@ fn break_critical_edges_fn(mir: &mut Mir) { // Returns true if the terminator would use an invoke in LLVM. fn term_is_invoke(term: &Terminator) -> bool { - match *term { - Terminator::Call { cleanup: Some(_), .. } | - Terminator::Drop { unwind: Some(_), .. } => true, + match term.kind { + TerminatorKind::Call { cleanup: Some(_), .. } | + TerminatorKind::Drop { unwind: Some(_), .. } => true, _ => false } } diff --git a/src/librustc_mir/traversal.rs b/src/librustc_mir/traversal.rs index fc53881bd81..8b6821136f5 100644 --- a/src/librustc_mir/traversal.rs +++ b/src/librustc_mir/traversal.rs @@ -163,7 +163,7 @@ impl<'a, 'tcx> Postorder<'a, 'tcx> { // Now that the top of the stack has no successors we can traverse, each item will // be popped off during iteration until we get back to `A`. This yeilds [E, D, B]. // - // When we yeild `B` and call `traverse_successor`, We push `C` to the stack, but + // When we yield `B` and call `traverse_successor`, we push `C` to the stack, but // since we've already visited `E`, that child isn't added to the stack. The last // two iterations yield `C` and finally `A` for a final traversal of [E, D, B, C, A] loop { diff --git a/src/librustc_trans/mir/mod.rs b/src/librustc_trans/mir/mod.rs index e1a0d33fa13..91577fc654a 100644 --- a/src/librustc_trans/mir/mod.rs +++ b/src/librustc_trans/mir/mod.rs @@ -19,7 +19,7 @@ use common::{self, Block, BlockAndBuilder, FunctionContext}; use std::ops::Deref; use std::rc::Rc; -use trans::basic_block::BasicBlock; +use basic_block::BasicBlock; use rustc_data_structures::bitvec::BitVector; From 605bc042646ef0dc0bd6e0420e6bd5a4715c93df Mon Sep 17 00:00:00 2001 From: James Miller Date: Sun, 3 Apr 2016 14:58:34 +1200 Subject: [PATCH 7/7] Use a BitVector instead of Vec for recording cleanup blocks Also adds a FromIterator impl for BitVector to allow construction of a BitVector from an iterator yeilding bools. --- src/librustc_data_structures/bitvec.rs | 27 ++++++++++++++++++- .../transform/break_critical_edges.rs | 9 ++++++- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/librustc_data_structures/bitvec.rs b/src/librustc_data_structures/bitvec.rs index e45c6cfc6dc..092b406ae9e 100644 --- a/src/librustc_data_structures/bitvec.rs +++ b/src/librustc_data_structures/bitvec.rs @@ -8,6 +8,8 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use std::iter::FromIterator; + /// A very simple BitVector type. #[derive(Clone)] pub struct BitVector { @@ -51,7 +53,9 @@ impl BitVector { pub fn grow(&mut self, num_bits: usize) { let num_words = u64s(num_bits); let extra_words = self.data.len() - num_words; - self.data.extend((0..extra_words).map(|_| 0)); + if extra_words > 0 { + self.data.extend((0..extra_words).map(|_| 0)); + } } /// Iterates over indexes of set bits in a sorted order @@ -94,6 +98,27 @@ impl<'a> Iterator for BitVectorIter<'a> { } } +impl FromIterator for BitVector { + fn from_iter(iter: I) -> BitVector where I: IntoIterator { + let iter = iter.into_iter(); + let (len, _) = iter.size_hint(); + // Make the minimum length for the bitvector 64 bits since that's + // the smallest non-zero size anyway. + let len = if len < 64 { 64 } else { len }; + let mut bv = BitVector::new(len); + for (idx, val) in iter.enumerate() { + if idx > len { + bv.grow(idx); + } + if val { + bv.insert(idx); + } + } + + bv + } +} + /// A "bit matrix" is basically a square matrix of booleans /// represented as one gigantic bitvector. In other words, it is as if /// you have N bitvectors, each of length N. Note that `elements` here is `N`/ diff --git a/src/librustc_mir/transform/break_critical_edges.rs b/src/librustc_mir/transform/break_critical_edges.rs index e78c8e8fd73..e1fb5dfd437 100644 --- a/src/librustc_mir/transform/break_critical_edges.rs +++ b/src/librustc_mir/transform/break_critical_edges.rs @@ -13,6 +13,8 @@ use rustc::mir::repr::*; use rustc::mir::transform::{MirPass, Pass}; use syntax::ast::NodeId; +use rustc_data_structures::bitvec::BitVector; + use traversal; pub struct BreakCriticalEdges; @@ -60,6 +62,9 @@ fn break_critical_edges(mir: &mut Mir) { } } + let cleanup_map : BitVector = mir.basic_blocks + .iter().map(|bb| bb.is_cleanup).collect(); + // We need a place to store the new blocks generated let mut new_blocks = Vec::new(); @@ -84,7 +89,9 @@ fn break_critical_edges(mir: &mut Mir) { scope: term_scope, kind: TerminatorKind::Goto { target: *tgt } }; - let data = BasicBlockData::new(Some(goto)); + let mut data = BasicBlockData::new(Some(goto)); + data.is_cleanup = cleanup_map.contains(tgt.index()); + // Get the index it will be when inserted into the MIR let idx = cur_len + new_blocks.len(); new_blocks.push(data);