Skip to content

Remove SimplifyBranchSame MIR optimization #77706

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
+253 −758
Closed

Remove SimplifyBranchSame MIR optimization #77706

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
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
@@ -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"),
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
@@ -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.
@@ -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 {
Loading