Skip to content

Replace a recursive algorithm with an iterative one and a stack. #74983

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 1, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 43 additions & 26 deletions src/librustc_mir/transform/simplify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ use rustc_index::vec::{Idx, IndexVec};
use rustc_middle::mir::visit::{MutVisitor, MutatingUseContext, PlaceContext, Visitor};
use rustc_middle::mir::*;
use rustc_middle::ty::TyCtxt;
use smallvec::SmallVec;
use std::borrow::Cow;

pub struct SimplifyCfg {
Expand Down Expand Up @@ -172,9 +173,12 @@ impl<'a, 'tcx> CfgSimplifier<'a, 'tcx> {
}
}

// Collapse a goto chain starting from `start`
fn collapse_goto_chain(&mut self, start: &mut BasicBlock, changed: &mut bool) {
let mut terminator = match self.basic_blocks[*start] {
/// This function will return `None` if
/// * the block has statements
/// * the block has a terminator other than `goto`
/// * the block has no terminator (meaning some other part of the current optimization stole it)
fn take_terminator_if_simple_goto(&mut self, bb: BasicBlock) -> Option<Terminator<'tcx>> {
match self.basic_blocks[bb] {
BasicBlockData {
ref statements,
terminator:
Expand All @@ -183,32 +187,45 @@ impl<'a, 'tcx> CfgSimplifier<'a, 'tcx> {
} if statements.is_empty() => terminator.take(),
// if `terminator` is None, this means we are in a loop. In that
// case, let all the loop collapse to its entry.
_ => return,
};

let target = match terminator {
Some(Terminator { kind: TerminatorKind::Goto { ref mut target }, .. }) => {
self.collapse_goto_chain(target, changed);
*target
}
_ => unreachable!(),
};
self.basic_blocks[*start].terminator = terminator;

debug!("collapsing goto chain from {:?} to {:?}", *start, target);

*changed |= *start != target;
_ => None,
}
}

if self.pred_count[*start] == 1 {
// This is the last reference to *start, so the pred-count to
// to target is moved into the current block.
self.pred_count[*start] = 0;
} else {
self.pred_count[target] += 1;
self.pred_count[*start] -= 1;
/// Collapse a goto chain starting from `start`
fn collapse_goto_chain(&mut self, start: &mut BasicBlock, changed: &mut bool) {
// Using `SmallVec` here, because in some logs on libcore oli-obk saw many single-element
// goto chains. We should probably benchmark different sizes.
let mut terminators: SmallVec<[_; 1]> = Default::default();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the SmallVec here? In the common case, start will not refer to a Goto with no statements, so terminators will be empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anecdotal evidence showed me many single-step goto chains (I had logging on during libcore builds). But we can also just go ahead and benchmark Vec vs SmallVec<[_; 1]> and maybe some other default sizes.

Copy link
Contributor

@ecstatic-morse ecstatic-morse Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm satisfied with that. It shouldn't matter much either way, maybe leave a comment?

let mut current = *start;
while let Some(terminator) = self.take_terminator_if_simple_goto(current) {
let target = match terminator {
Terminator { kind: TerminatorKind::Goto { target }, .. } => target,
_ => unreachable!(),
};
terminators.push((current, terminator));
current = target;
}
let last = current;
*start = last;
while let Some((current, mut terminator)) = terminators.pop() {
let target = match terminator {
Terminator { kind: TerminatorKind::Goto { ref mut target }, .. } => target,
_ => unreachable!(),
};
*target = last;
debug!("collapsing goto chain from {:?} to {:?}", current, target);

*start = target;
if self.pred_count[current] == 1 {
// This is the last reference to current, so the pred-count to
// to target is moved into the current block.
self.pred_count[current] = 0;
} else {
self.pred_count[*target] += 1;
self.pred_count[current] -= 1;
}
*changed = true;
self.basic_blocks[current].terminator = Some(terminator);
}
}

// merge a block with 1 `goto` predecessor to its parent
Expand Down