Skip to content

Commit

Permalink
Remove SimplifyBranchSame MIR optimization
Browse files Browse the repository at this point in the history 
This optimization never had any positive impact on compile or runtime
performance. It is relatively complex and used to have bugs in the past.
Its implementation contains significantly more lines of code than the
number of blocks it happens to optimize out during rustc build process
(including all dependencies and the standard library).

Remove it.
  • Loading branch information
@tmiasko
tmiasko committed Oct 8, 2020
1 parent f1dab24 commit 9de6df6
Show file tree
Hide file tree
Showing 19 changed files with 253 additions and 758 deletions.
1 change: 0 additions & 1 deletion compiler/rustc_mir/src/transform/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,6 @@ fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
&early_otherwise_branch::EarlyOtherwiseBranch,
&simplify_comparison_integral::SimplifyComparisonIntegral,
&simplify_try::SimplifyArmIdentity,
&simplify_try::SimplifyBranchSame,
&dest_prop::DestinationPropagation,
&copy_prop::CopyPropagation,
&simplify_branches::SimplifyBranches::new("after-copy-prop"),
Expand Down
269 changes: 3 additions & 266 deletions compiler/rustc_mir/src/transform/simplify_try.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,13 @@
//!
//! into just `x`.

use crate::transform::{simplify, MirPass};
use itertools::Itertools as _;
use crate::transform::MirPass;
use rustc_index::{bit_set::BitSet, vec::IndexVec};
use rustc_middle::mir::visit::{NonUseContext, PlaceContext, Visitor};
use rustc_middle::mir::*;
use rustc_middle::ty::{self, List, Ty, TyCtxt};
use rustc_middle::ty::{List, Ty, TyCtxt};
use rustc_target::abi::VariantIdx;
use std::iter::{once, Enumerate, Peekable};
use std::iter::{Enumerate, Peekable};
use std::slice::Iter;

/// Simplifies arms of form `Variant(x) => Variant(x)` to just a move.
Expand Down Expand Up @@ -523,265 +522,3 @@ fn match_variant_field_place<'tcx>(place: Place<'tcx>) -> Option<(Local, VarFiel
_ => None,
}
}

/// Simplifies `SwitchInt(_) -> [targets]`,
/// where all the `targets` have the same form,
/// into `goto -> target_first`.
pub struct SimplifyBranchSame;

impl<'tcx> MirPass<'tcx> for SimplifyBranchSame {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
trace!("Running SimplifyBranchSame on {:?}", body.source);
let finder = SimplifyBranchSameOptimizationFinder { body, tcx };
let opts = finder.find();

let did_remove_blocks = opts.len() > 0;
for opt in opts.iter() {
trace!("SUCCESS: Applying optimization {:?}", opt);
// Replace `SwitchInt(..) -> [bb_first, ..];` with a `goto -> bb_first;`.
body.basic_blocks_mut()[opt.bb_to_opt_terminator].terminator_mut().kind =
TerminatorKind::Goto { target: opt.bb_to_goto };
}

if did_remove_blocks {
// We have dead blocks now, so remove those.
simplify::remove_dead_blocks(body);
}
}
}

#[derive(Debug)]
struct SimplifyBranchSameOptimization {
/// All basic blocks are equal so go to this one
bb_to_goto: BasicBlock,
/// Basic block where the terminator can be simplified to a goto
bb_to_opt_terminator: BasicBlock,
}

struct SwitchTargetAndValue {
target: BasicBlock,
// None in case of the `otherwise` case
value: Option<u128>,
}

struct SimplifyBranchSameOptimizationFinder<'a, 'tcx> {
body: &'a Body<'tcx>,
tcx: TyCtxt<'tcx>,
}

impl<'a, 'tcx> SimplifyBranchSameOptimizationFinder<'a, 'tcx> {
fn find(&self) -> Vec<SimplifyBranchSameOptimization> {
self.body
.basic_blocks()
.iter_enumerated()
.filter_map(|(bb_idx, bb)| {
let (discr_switched_on, targets_and_values) = match &bb.terminator().kind {
TerminatorKind::SwitchInt { targets, discr, values, .. } => {
// if values.len() == targets.len() - 1, we need to include None where no value is present
// such that the zip does not throw away targets. If no `otherwise` case is in targets, the zip will simply throw away the added None
let values_extended = values.iter().map(|x|Some(*x)).chain(once(None));
let targets_and_values:Vec<_> = targets.iter().zip(values_extended)
.map(|(target, value)| SwitchTargetAndValue{target:*target, value})
.collect();
assert_eq!(targets.len(), targets_and_values.len());
(discr, targets_and_values)},
_ => return None,
};

// find the adt that has its discriminant read
// assuming this must be the last statement of the block
let adt_matched_on = match &bb.statements.last()?.kind {
StatementKind::Assign(box (place, rhs))
if Some(*place) == discr_switched_on.place() =>
{
match rhs {
Rvalue::Discriminant(adt_place) if adt_place.ty(self.body, self.tcx).ty.is_enum() => adt_place,
_ => {
trace!("NO: expected a discriminant read of an enum instead of: {:?}", rhs);
return None;
}
}
}
other => {
trace!("NO: expected an assignment of a discriminant read to a place. Found: {:?}", other);
return None
},
};

let mut iter_bbs_reachable = targets_and_values
.iter()
.map(|target_and_value| (target_and_value, &self.body.basic_blocks()[target_and_value.target]))
.filter(|(_, bb)| {
// Reaching `unreachable` is UB so assume it doesn't happen.
bb.terminator().kind != TerminatorKind::Unreachable
// But `asm!(...)` could abort the program,
// so we cannot assume that the `unreachable` terminator itself is reachable.
// FIXME(Centril): use a normalization pass instead of a check.
|| bb.statements.iter().any(|stmt| match stmt.kind {
StatementKind::LlvmInlineAsm(..) => true,
_ => false,
})
})
.peekable();

let bb_first = iter_bbs_reachable.peek().map(|(idx, _)| *idx).unwrap_or(&targets_and_values[0]);
let mut all_successors_equivalent = StatementEquality::TrivialEqual;

// All successor basic blocks must be equal or contain statements that are pairwise considered equal.
for ((target_and_value_l,bb_l), (target_and_value_r,bb_r)) in iter_bbs_reachable.tuple_windows() {
let trivial_checks = bb_l.is_cleanup == bb_r.is_cleanup
&& bb_l.terminator().kind == bb_r.terminator().kind
&& bb_l.statements.len() == bb_r.statements.len();
let statement_check = || {
bb_l.statements.iter().zip(&bb_r.statements).try_fold(StatementEquality::TrivialEqual, |acc,(l,r)| {
let stmt_equality = self.statement_equality(*adt_matched_on, &l, target_and_value_l, &r, target_and_value_r);
if matches!(stmt_equality, StatementEquality::NotEqual) {
// short circuit
None
} else {
Some(acc.combine(&stmt_equality))
}
})
.unwrap_or(StatementEquality::NotEqual)
};
if !trivial_checks {
all_successors_equivalent = StatementEquality::NotEqual;
break;
}
all_successors_equivalent = all_successors_equivalent.combine(&statement_check());
};

match all_successors_equivalent{
StatementEquality::TrivialEqual => {
// statements are trivially equal, so just take first
trace!("Statements are trivially equal");
Some(SimplifyBranchSameOptimization {
bb_to_goto: bb_first.target,
bb_to_opt_terminator: bb_idx,
})
}
StatementEquality::ConsideredEqual(bb_to_choose) => {
trace!("Statements are considered equal");
Some(SimplifyBranchSameOptimization {
bb_to_goto: bb_to_choose,
bb_to_opt_terminator: bb_idx,
})
}
StatementEquality::NotEqual => {
trace!("NO: not all successors of basic block {:?} were equivalent", bb_idx);
None
}
}
})
.collect()
}

/// Tests if two statements can be considered equal
///
/// Statements can be trivially equal if the kinds match.
/// But they can also be considered equal in the following case A:
/// ```
/// discriminant(_0) = 0; // bb1
/// _0 = move _1; // bb2
/// ```
/// In this case the two statements are equal iff
/// 1: _0 is an enum where the variant index 0 is fieldless, and
/// 2: bb1 was targeted by a switch where the discriminant of _1 was switched on
fn statement_equality(
&self,
adt_matched_on: Place<'tcx>,
x: &Statement<'tcx>,
x_target_and_value: &SwitchTargetAndValue,
y: &Statement<'tcx>,
y_target_and_value: &SwitchTargetAndValue,
) -> StatementEquality {
let helper = |rhs: &Rvalue<'tcx>,
place: &Place<'tcx>,
variant_index: &VariantIdx,
side_to_choose| {
let place_type = place.ty(self.body, self.tcx).ty;
let adt = match *place_type.kind() {
ty::Adt(adt, _) if adt.is_enum() => adt,
_ => return StatementEquality::NotEqual,
};
let variant_is_fieldless = adt.variants[*variant_index].fields.is_empty();
if !variant_is_fieldless {
trace!("NO: variant {:?} was not fieldless", variant_index);
return StatementEquality::NotEqual;
}

match rhs {
Rvalue::Use(operand) if operand.place() == Some(adt_matched_on) => {
StatementEquality::ConsideredEqual(side_to_choose)
}
_ => {
trace!(
"NO: RHS of assignment was {:?}, but expected it to match the adt being matched on in the switch, which is {:?}",
rhs,
adt_matched_on
);
StatementEquality::NotEqual
}
}
};
match (&x.kind, &y.kind) {
// trivial case
(x, y) if x == y => StatementEquality::TrivialEqual,

// check for case A
(
StatementKind::Assign(box (_, rhs)),
StatementKind::SetDiscriminant { place, variant_index },
)
// we need to make sure that the switch value that targets the bb with SetDiscriminant (y), is the same as the variant index
if Some(variant_index.index() as u128) == y_target_and_value.value => {
// choose basic block of x, as that has the assign
helper(rhs, place, variant_index, x_target_and_value.target)
}
(
StatementKind::SetDiscriminant { place, variant_index },
StatementKind::Assign(box (_, rhs)),
)
// we need to make sure that the switch value that targets the bb with SetDiscriminant (x), is the same as the variant index
if Some(variant_index.index() as u128) == x_target_and_value.value => {
// choose basic block of y, as that has the assign
helper(rhs, place, variant_index, y_target_and_value.target)
}
_ => {
trace!("NO: statements `{:?}` and `{:?}` not considered equal", x, y);
StatementEquality::NotEqual
}
}
}
}

#[derive(Copy, Clone, Eq, PartialEq)]
enum StatementEquality {
/// The two statements are trivially equal; same kind
TrivialEqual,
/// The two statements are considered equal, but may be of different kinds. The BasicBlock field is the basic block to jump to when performing the branch-same optimization.
/// For example, `_0 = _1` and `discriminant(_0) = discriminant(0)` are considered equal if 0 is a fieldless variant of an enum. But we don't want to jump to the basic block with the SetDiscriminant, as that is not legal if _1 is not the 0 variant index
ConsideredEqual(BasicBlock),
/// The two statements are not equal
NotEqual,
}

impl StatementEquality {
fn combine(&self, other: &StatementEquality) -> StatementEquality {
use StatementEquality::*;
match (self, other) {
(TrivialEqual, TrivialEqual) => TrivialEqual,
(TrivialEqual, ConsideredEqual(b)) | (ConsideredEqual(b), TrivialEqual) => {
ConsideredEqual(*b)
}
(ConsideredEqual(b1), ConsideredEqual(b2)) => {
if b1 == b2 {
ConsideredEqual(*b1)
} else {
NotEqual
}
}
(_, NotEqual) | (NotEqual, _) => NotEqual,
}
}
}
17 changes: 0 additions & 17 deletions src/test/codegen/try_identity.rs
View file

This file was deleted.

28 changes: 0 additions & 28 deletions src/test/mir-opt/76803_regression.encode.SimplifyBranchSame.diff
View file

This file was deleted.

19 changes: 0 additions & 19 deletions src/test/mir-opt/76803_regression.rs
View file

This file was deleted.

3 changes: 0 additions & 3 deletions src/test/mir-opt/simplify-arm.rs
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
// compile-flags: -Z mir-opt-level=2 -Zunsound-mir-opts
// EMIT_MIR simplify_arm.id.SimplifyArmIdentity.diff
// EMIT_MIR simplify_arm.id.SimplifyBranchSame.diff
// EMIT_MIR simplify_arm.id_result.SimplifyArmIdentity.diff
// EMIT_MIR simplify_arm.id_result.SimplifyBranchSame.diff
// EMIT_MIR simplify_arm.id_try.SimplifyArmIdentity.diff
// EMIT_MIR simplify_arm.id_try.SimplifyBranchSame.diff

fn id(o: Option<u8>) -> Option<u8> {
match o {
Expand Down
Loading

0 comments on commit 9de6df6

Please sign in to comment.