From 261895b927a7198bf6a47e575472bc81459e3fce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Steinbrink?= Date: Sat, 14 Nov 2015 23:52:17 +0100 Subject: [PATCH 1/6] [MIR] Enhance the SimplifyCfg pass to merge consecutive blocks --- src/librustc_mir/transform/simplify_cfg.rs | 80 ++++++++++++++-------- src/librustc_mir/transform/util.rs | 68 ++++++++++++++++++ 2 files changed, 120 insertions(+), 28 deletions(-) diff --git a/src/librustc_mir/transform/simplify_cfg.rs b/src/librustc_mir/transform/simplify_cfg.rs index 558276a13a8b8..d4f6d98f36537 100644 --- a/src/librustc_mir/transform/simplify_cfg.rs +++ b/src/librustc_mir/transform/simplify_cfg.rs @@ -21,28 +21,7 @@ impl SimplifyCfg { SimplifyCfg } - fn remove_dead_blocks(&self, mir: &mut Mir) { - let mut seen = vec![false; mir.basic_blocks.len()]; - - // These blocks are always required. - seen[START_BLOCK.index()] = true; - seen[END_BLOCK.index()] = true; - seen[DIVERGE_BLOCK.index()] = true; - - let mut worklist = vec![START_BLOCK]; - while let Some(bb) = worklist.pop() { - for succ in mir.basic_block_data(bb).terminator.successors() { - if !seen[succ.index()] { - seen[succ.index()] = true; - worklist.push(*succ); - } - } - } - - util::retain_basic_blocks(mir, &seen); - } - - fn remove_goto_chains(&self, mir: &mut Mir) -> bool { + fn merge_consecutive_blocks(&self, mir: &mut Mir) -> bool { // Find the target at the end of the jump chain, return None if there is a loop fn final_target(mir: &Mir, mut target: BasicBlock) -> Option { @@ -65,25 +44,71 @@ impl SimplifyCfg { Some(target) } + let mut predecessor_map = util::build_predecessor_map(mir); + let mut changed = false; - for bb in mir.all_basic_blocks() { + let mut seen = vec![false; mir.basic_blocks.len()]; + let mut worklist = vec![START_BLOCK]; + while let Some(bb) = worklist.pop() { // Temporarily swap out the terminator we're modifying to keep borrowck happy let mut terminator = Terminator::Diverge; mem::swap(&mut terminator, &mut mir.basic_block_data_mut(bb).terminator); + // Shortcut chains of empty blocks that just jump from one to the next for target in terminator.successors_mut() { let new_target = match final_target(mir, *target) { Some(new_target) => new_target, None if mir.basic_block_data(bb).statements.is_empty() => bb, None => continue }; - changed |= *target != new_target; - *target = new_target; + + if *target != new_target { + changed = true; + predecessor_map.remove_predecessor(*target, bb); + predecessor_map.add_predecessor(new_target, bb); + *target = new_target; + } } - mir.basic_block_data_mut(bb).terminator = terminator; + // See if we can merge the target block into this one + match terminator { + Terminator::Goto { target } if target.index() > DIVERGE_BLOCK.index() && + predecessor_map.predecessors(target).len() == 1 => { + changed = true; + let mut other_data = BasicBlockData { + statements: Vec::new(), + terminator: Terminator::Goto { target: target} + }; + mem::swap(&mut other_data, mir.basic_block_data_mut(target)); + + predecessor_map.replace_predecessor(target, bb, target); + for succ in other_data.terminator.successors() { + predecessor_map.replace_predecessor(*succ, target, bb); + } + + let data = mir.basic_block_data_mut(bb); + data.statements.append(&mut other_data.statements); + mem::swap(&mut data.terminator, &mut other_data.terminator); + } + _ => mir.basic_block_data_mut(bb).terminator = terminator + } + + for succ in mir.basic_block_data(bb).terminator.successors() { + if !seen[succ.index()] { + seen[succ.index()] = true; + worklist.push(*succ); + } + } } + // These blocks must be retained, so mark them seen even if we didn't see them + seen[START_BLOCK.index()] = true; + seen[END_BLOCK.index()] = true; + seen[DIVERGE_BLOCK.index()] = true; + + // Now get rid of all the blocks we never saw + util::retain_basic_blocks(mir, &seen); + changed } @@ -125,8 +150,7 @@ impl<'tcx> MirPass<'tcx> for SimplifyCfg { let mut changed = true; while changed { changed = self.simplify_branches(mir); - changed |= self.remove_goto_chains(mir); - self.remove_dead_blocks(mir); + changed |= self.merge_consecutive_blocks(mir); } // FIXME: Should probably be moved into some kind of pass manager diff --git a/src/librustc_mir/transform/util.rs b/src/librustc_mir/transform/util.rs index 9510269454485..90bb28c81a074 100644 --- a/src/librustc_mir/transform/util.rs +++ b/src/librustc_mir/transform/util.rs @@ -9,6 +9,7 @@ // except according to those terms. use rustc::mir::repr::*; +use rustc::mir::visit::*; /// Update basic block ids in all terminators using the given replacements, /// useful e.g. after removal of several basic blocks to update all terminators @@ -49,3 +50,70 @@ pub fn retain_basic_blocks(mir: &mut Mir, keep: &[bool]) { update_basic_block_ids(mir, &replacements); } + +// A simple map to perform quick lookups of the predecessors of a BasicBlock. +// Since BasicBlocks usually only have a small number of predecessors, we use a +// simple vector. Also, if a block has the same target more than once, for +// example in a switch, it will appear in the target's predecessor list multiple +// times. This allows to update the map more easily when modifying the graph. +pub struct PredecessorMap { + map: Vec>, +} + +impl PredecessorMap { + pub fn new(num_blocks: usize) -> PredecessorMap { + PredecessorMap { + map: vec![Vec::new(); num_blocks], + } + } + + pub fn predecessors(&self, block: BasicBlock) -> &[BasicBlock] { + &self.map[block.index()] + } + + pub fn add_predecessor(&mut self, block: BasicBlock, predecessor: BasicBlock) { + self.map[block.index()].push(predecessor); + } + + pub fn remove_predecessor(&mut self, block: BasicBlock, predecessor: BasicBlock) { + let pos = self.map[block.index()].iter().position(|&p| p == predecessor).expect( + &format!("{:?} is not registered as a predecessor of {:?}", predecessor, block)); + + self.map[block.index()].swap_remove(pos); + } + + pub fn replace_predecessor(&mut self, block: BasicBlock, old: BasicBlock, new: BasicBlock) { + self.remove_predecessor(block, old); + self.add_predecessor(block, new); + } +} + +struct PredecessorVisitor { + predecessor_map: PredecessorMap, +} + +impl PredecessorVisitor { + fn new(num_blocks: usize) -> PredecessorVisitor { + PredecessorVisitor { + predecessor_map: PredecessorMap::new(num_blocks), + } + } +} + +impl<'tcx> Visitor<'tcx> for PredecessorVisitor { + fn visit_mir(&mut self, mir: &Mir<'tcx>) { + self.predecessor_map = PredecessorMap::new(mir.basic_blocks.len()); + self.super_mir(mir); + } + + fn visit_branch(&mut self, source: BasicBlock, target: BasicBlock) { + self.predecessor_map.add_predecessor(target, source); + } +} + +pub fn build_predecessor_map(mir: &Mir) -> PredecessorMap { + let mut v = PredecessorVisitor::new(mir.basic_blocks.len()); + v.visit_mir(mir); + + v.predecessor_map +} From 58db206dc57bfebd3aa53829844d16aa0860c068 Mon Sep 17 00:00:00 2001 From: Jonathan S Date: Sun, 6 Dec 2015 09:49:56 -0600 Subject: [PATCH 2/6] Make the PredecessorMap only keep track of a count of predecessors --- src/librustc_mir/transform/simplify_cfg.rs | 2 +- src/librustc_mir/transform/util.rs | 33 +++++++++------------- 2 files changed, 14 insertions(+), 21 deletions(-) diff --git a/src/librustc_mir/transform/simplify_cfg.rs b/src/librustc_mir/transform/simplify_cfg.rs index d4f6d98f36537..e8cf43d8df81b 100644 --- a/src/librustc_mir/transform/simplify_cfg.rs +++ b/src/librustc_mir/transform/simplify_cfg.rs @@ -73,7 +73,7 @@ impl SimplifyCfg { // See if we can merge the target block into this one match terminator { Terminator::Goto { target } if target.index() > DIVERGE_BLOCK.index() && - predecessor_map.predecessors(target).len() == 1 => { + predecessor_map.num_predecessors(target) == 1 => { changed = true; let mut other_data = BasicBlockData { statements: Vec::new(), diff --git a/src/librustc_mir/transform/util.rs b/src/librustc_mir/transform/util.rs index 90bb28c81a074..18de91c9c2450 100644 --- a/src/librustc_mir/transform/util.rs +++ b/src/librustc_mir/transform/util.rs @@ -51,41 +51,34 @@ pub fn retain_basic_blocks(mir: &mut Mir, keep: &[bool]) { update_basic_block_ids(mir, &replacements); } -// A simple map to perform quick lookups of the predecessors of a BasicBlock. -// Since BasicBlocks usually only have a small number of predecessors, we use a -// simple vector. Also, if a block has the same target more than once, for -// example in a switch, it will appear in the target's predecessor list multiple -// times. This allows to update the map more easily when modifying the graph. +// A simple map to perform quick lookups of the number of predecessors of a +// BasicBlock. If a block has the same target more than once, for +// example in a switch, it will appear in the target's predecessor list +// multiple times. This makes updating the map easier when modifying the graph. pub struct PredecessorMap { - map: Vec>, + map: Vec, } impl PredecessorMap { pub fn new(num_blocks: usize) -> PredecessorMap { PredecessorMap { - map: vec![Vec::new(); num_blocks], + map: vec![0; num_blocks], } } - pub fn predecessors(&self, block: BasicBlock) -> &[BasicBlock] { - &self.map[block.index()] + pub fn num_predecessors(&self, block: BasicBlock) -> usize { + self.map[block.index()] } - pub fn add_predecessor(&mut self, block: BasicBlock, predecessor: BasicBlock) { - self.map[block.index()].push(predecessor); + pub fn add_predecessor(&mut self, block: BasicBlock, _predecessor: BasicBlock) { + self.map[block.index()] += 1; } - pub fn remove_predecessor(&mut self, block: BasicBlock, predecessor: BasicBlock) { - let pos = self.map[block.index()].iter().position(|&p| p == predecessor).expect( - &format!("{:?} is not registered as a predecessor of {:?}", predecessor, block)); - - self.map[block.index()].swap_remove(pos); + pub fn remove_predecessor(&mut self, block: BasicBlock, _predecessor: BasicBlock) { + self.map[block.index()] -= 1; } - pub fn replace_predecessor(&mut self, block: BasicBlock, old: BasicBlock, new: BasicBlock) { - self.remove_predecessor(block, old); - self.add_predecessor(block, new); - } + pub fn replace_predecessor(&mut self, _block: BasicBlock, _old: BasicBlock, _new: BasicBlock) { } } struct PredecessorVisitor { From 156c061bc5e92a828ea9408a3c2cefb4b0c70da2 Mon Sep 17 00:00:00 2001 From: Jonathan S Date: Sun, 6 Dec 2015 10:04:00 -0600 Subject: [PATCH 3/6] Adjust the API of the PredecessorMap to reflect the fact that it only tracks the number of predecessors --- src/librustc_mir/transform/simplify_cfg.rs | 11 +++++------ src/librustc_mir/transform/util.rs | 10 ++++------ 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/librustc_mir/transform/simplify_cfg.rs b/src/librustc_mir/transform/simplify_cfg.rs index e8cf43d8df81b..1d858f57f349a 100644 --- a/src/librustc_mir/transform/simplify_cfg.rs +++ b/src/librustc_mir/transform/simplify_cfg.rs @@ -64,8 +64,8 @@ impl SimplifyCfg { if *target != new_target { changed = true; - predecessor_map.remove_predecessor(*target, bb); - predecessor_map.add_predecessor(new_target, bb); + predecessor_map.remove_predecessor(*target); + predecessor_map.add_predecessor(new_target); *target = new_target; } } @@ -81,10 +81,9 @@ impl SimplifyCfg { }; mem::swap(&mut other_data, mir.basic_block_data_mut(target)); - predecessor_map.replace_predecessor(target, bb, target); - for succ in other_data.terminator.successors() { - predecessor_map.replace_predecessor(*succ, target, bb); - } + // target used to have 1 predecessor (bb), and still has only one (itself) + // All the successors of target have had target replaced by bb in their + // list of predecessors, keeping the number the same. let data = mir.basic_block_data_mut(bb); data.statements.append(&mut other_data.statements); diff --git a/src/librustc_mir/transform/util.rs b/src/librustc_mir/transform/util.rs index 18de91c9c2450..ee06ee0ba41e1 100644 --- a/src/librustc_mir/transform/util.rs +++ b/src/librustc_mir/transform/util.rs @@ -70,15 +70,13 @@ impl PredecessorMap { self.map[block.index()] } - pub fn add_predecessor(&mut self, block: BasicBlock, _predecessor: BasicBlock) { + pub fn add_predecessor(&mut self, block: BasicBlock) { self.map[block.index()] += 1; } - pub fn remove_predecessor(&mut self, block: BasicBlock, _predecessor: BasicBlock) { + pub fn remove_predecessor(&mut self, block: BasicBlock) { self.map[block.index()] -= 1; } - - pub fn replace_predecessor(&mut self, _block: BasicBlock, _old: BasicBlock, _new: BasicBlock) { } } struct PredecessorVisitor { @@ -99,8 +97,8 @@ impl<'tcx> Visitor<'tcx> for PredecessorVisitor { self.super_mir(mir); } - fn visit_branch(&mut self, source: BasicBlock, target: BasicBlock) { - self.predecessor_map.add_predecessor(target, source); + fn visit_branch(&mut self, _source: BasicBlock, target: BasicBlock) { + self.predecessor_map.add_predecessor(target); } } From c3e6daa6079a07c293ad29ef04fc5a374b9efe9e Mon Sep 17 00:00:00 2001 From: Jonathan S Date: Fri, 18 Dec 2015 21:38:09 -0600 Subject: [PATCH 4/6] Fixup nits in merge_consecutive_blocks and merge multiple blocks at once to improve performance. --- src/librustc_mir/transform/simplify_cfg.rs | 43 ++++++++++++---------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/src/librustc_mir/transform/simplify_cfg.rs b/src/librustc_mir/transform/simplify_cfg.rs index 1d858f57f349a..80b7b3412db63 100644 --- a/src/librustc_mir/transform/simplify_cfg.rs +++ b/src/librustc_mir/transform/simplify_cfg.rs @@ -51,8 +51,8 @@ impl SimplifyCfg { let mut worklist = vec![START_BLOCK]; while let Some(bb) = worklist.pop() { // Temporarily swap out the terminator we're modifying to keep borrowck happy - let mut terminator = Terminator::Diverge; - mem::swap(&mut terminator, &mut mir.basic_block_data_mut(bb).terminator); + let mut terminator = mem::replace(&mut mir.basic_block_data_mut(bb).terminator, + Terminator::Diverge); // Shortcut chains of empty blocks that just jump from one to the next for target in terminator.successors_mut() { @@ -71,27 +71,30 @@ impl SimplifyCfg { } // See if we can merge the target block into this one - match terminator { - Terminator::Goto { target } if target.index() > DIVERGE_BLOCK.index() && - predecessor_map.num_predecessors(target) == 1 => { - changed = true; - let mut other_data = BasicBlockData { - statements: Vec::new(), - terminator: Terminator::Goto { target: target} - }; - mem::swap(&mut other_data, mir.basic_block_data_mut(target)); - - // target used to have 1 predecessor (bb), and still has only one (itself) - // All the successors of target have had target replaced by bb in their - // list of predecessors, keeping the number the same. - - let data = mir.basic_block_data_mut(bb); - data.statements.append(&mut other_data.statements); - mem::swap(&mut data.terminator, &mut other_data.terminator); + while let Terminator::Goto { target } = terminator { + if target.index() <= DIVERGE_BLOCK.index() || predecessor_map.num_predecessors(target) > 1 { + break; } - _ => mir.basic_block_data_mut(bb).terminator = terminator + + changed = true; + + let mut other_data = mem::replace(mir.basic_block_data_mut(target), BasicBlockData { + statements: Vec::new(), + terminator: Terminator::Goto { target: target } + }); + + // target used to have 1 predecessor (bb), and still has only one (itself) + // All the successors of target have had target replaced by bb in their + // list of predecessors, keeping the number the same. + + let data = mir.basic_block_data_mut(bb); + data.statements.append(&mut other_data.statements); + terminator = other_data.terminator; } + // Restore the terminator we swapped out for Diverge + mir.basic_block_data_mut(bb).terminator = terminator; + for succ in mir.basic_block_data(bb).terminator.successors() { if !seen[succ.index()] { seen[succ.index()] = true; From db8729e58b57dcedc32e4bff9809ef98f58dfe7a Mon Sep 17 00:00:00 2001 From: Jonathan S Date: Fri, 18 Dec 2015 21:58:54 -0600 Subject: [PATCH 5/6] Move the shortcutting of empty chains in merge_consecutive_blocks to after the main merging loop where it can do more good immediately. --- src/librustc_mir/transform/simplify_cfg.rs | 32 +++++++++++----------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/librustc_mir/transform/simplify_cfg.rs b/src/librustc_mir/transform/simplify_cfg.rs index 80b7b3412db63..99cca451a5f94 100644 --- a/src/librustc_mir/transform/simplify_cfg.rs +++ b/src/librustc_mir/transform/simplify_cfg.rs @@ -54,22 +54,6 @@ impl SimplifyCfg { let mut terminator = mem::replace(&mut mir.basic_block_data_mut(bb).terminator, Terminator::Diverge); - // Shortcut chains of empty blocks that just jump from one to the next - for target in terminator.successors_mut() { - let new_target = match final_target(mir, *target) { - Some(new_target) => new_target, - None if mir.basic_block_data(bb).statements.is_empty() => bb, - None => continue - }; - - if *target != new_target { - changed = true; - predecessor_map.remove_predecessor(*target); - predecessor_map.add_predecessor(new_target); - *target = new_target; - } - } - // See if we can merge the target block into this one while let Terminator::Goto { target } = terminator { if target.index() <= DIVERGE_BLOCK.index() || predecessor_map.num_predecessors(target) > 1 { @@ -92,6 +76,22 @@ impl SimplifyCfg { terminator = other_data.terminator; } + // Shortcut chains of empty blocks that just jump from one to the next + for target in terminator.successors_mut() { + let new_target = match final_target(mir, *target) { + Some(new_target) => new_target, + None if mir.basic_block_data(bb).statements.is_empty() => bb, + None => continue + }; + + if *target != new_target { + changed = true; + predecessor_map.remove_predecessor(*target); + predecessor_map.add_predecessor(new_target); + *target = new_target; + } + } + // Restore the terminator we swapped out for Diverge mir.basic_block_data_mut(bb).terminator = terminator; From 77f0d58e020071e6c42f4f68f494af036ca6f872 Mon Sep 17 00:00:00 2001 From: Jonathan S Date: Fri, 18 Dec 2015 23:40:17 -0600 Subject: [PATCH 6/6] Fix make tidy --- src/librustc_mir/transform/simplify_cfg.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/simplify_cfg.rs b/src/librustc_mir/transform/simplify_cfg.rs index 99cca451a5f94..b4b38506311cb 100644 --- a/src/librustc_mir/transform/simplify_cfg.rs +++ b/src/librustc_mir/transform/simplify_cfg.rs @@ -56,7 +56,8 @@ impl SimplifyCfg { // See if we can merge the target block into this one while let Terminator::Goto { target } = terminator { - if target.index() <= DIVERGE_BLOCK.index() || predecessor_map.num_predecessors(target) > 1 { + if target.index() <= DIVERGE_BLOCK.index() || + predecessor_map.num_predecessors(target) > 1 { break; }