Skip to content
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

Deduplicate unreachable blocks, for real this time #110569

Merged
merged 2 commits into from
Apr 21, 2023
Merged
Show file tree
Hide file tree
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
21 changes: 3 additions & 18 deletions compiler/rustc_mir_transform/src/instcombine.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
//! Performs various peephole optimizations.

use crate::simplify::combine_duplicate_switch_targets;
use crate::MirPass;
use rustc_hir::Mutability;
use rustc_middle::mir::{
BinOp, Body, CastKind, Constant, ConstantKind, LocalDecls, Operand, Place, ProjectionElem,
Rvalue, SourceInfo, Statement, StatementKind, SwitchTargets, Terminator, TerminatorKind, UnOp,
};
use rustc_middle::mir::*;
use rustc_middle::ty::layout::ValidityRequirement;
use rustc_middle::ty::util::IntTypeExt;
use rustc_middle::ty::{self, ParamEnv, SubstsRef, Ty, TyCtxt};
Expand Down Expand Up @@ -46,7 +44,7 @@ impl<'tcx> MirPass<'tcx> for InstCombine {
&mut block.terminator.as_mut().unwrap(),
&mut block.statements,
);
ctx.combine_duplicate_switch_targets(&mut block.terminator.as_mut().unwrap());
combine_duplicate_switch_targets(block.terminator.as_mut().unwrap());
}
}
}
Expand Down Expand Up @@ -264,19 +262,6 @@ impl<'tcx> InstCombineContext<'tcx, '_> {
terminator.kind = TerminatorKind::Goto { target: destination_block };
}

fn combine_duplicate_switch_targets(&self, terminator: &mut Terminator<'tcx>) {
let TerminatorKind::SwitchInt { targets, .. } = &mut terminator.kind
else { return };

let otherwise = targets.otherwise();
if targets.iter().any(|t| t.1 == otherwise) {
*targets = SwitchTargets::new(
targets.iter().filter(|t| t.1 != otherwise),
targets.otherwise(),
);
}
}

fn combine_intrinsic_assert(
&self,
terminator: &mut Terminator<'tcx>,
Expand Down
14 changes: 14 additions & 0 deletions compiler/rustc_mir_transform/src/simplify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,18 @@ impl<'a, 'tcx> CfgSimplifier<'a, 'tcx> {
}
}

pub fn combine_duplicate_switch_targets(terminator: &mut Terminator<'_>) {
if let TerminatorKind::SwitchInt { targets, .. } = &mut terminator.kind {
let otherwise = targets.otherwise();
if targets.iter().any(|t| t.1 == otherwise) {
*targets = SwitchTargets::new(
targets.iter().filter(|t| t.1 != otherwise),
targets.otherwise(),
);
}
}
}

pub fn remove_duplicate_unreachable_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
struct OptApplier<'tcx> {
tcx: TyCtxt<'tcx>,
Expand All @@ -298,6 +310,8 @@ pub fn remove_duplicate_unreachable_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut B
}
}

combine_duplicate_switch_targets(terminator);

self.super_terminator(terminator, location);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
- // + literal: Const { ty: unsafe fn(Option<T>) -> T {Option::<T>::unwrap_unchecked}, val: Value(<ZST>) }
+ StorageLive(_3); // scope 0 at $DIR/unwrap_unchecked.rs:+1:9: +1:27
+ _4 = discriminant(_2); // scope 1 at $SRC_DIR/core/src/option.rs:LL:COL
+ switchInt(move _4) -> [0: bb1, 1: bb2, otherwise: bb1]; // scope 1 at $SRC_DIR/core/src/option.rs:LL:COL
+ switchInt(move _4) -> [1: bb2, otherwise: bb1]; // scope 1 at $SRC_DIR/core/src/option.rs:LL:COL
}

bb1: {
Expand Down
16 changes: 16 additions & 0 deletions tests/mir-opt/instcombine_duplicate_switch_targets_e2e.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// compile-flags: -Zmir-opt-level=2 -Zinline-mir
// ignore-debug: standard library debug assertions add a panic that breaks this optimization
Copy link
Contributor

Choose a reason for hiding this comment

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

Blessing the ignore-debug tests requires to recompile everything, which is tedious.
Can you use the intrinsic directly and cook your own unreachable_unchecked?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm really wary of doing things like that. Modifying the mir-opt test for the convenience of testing is what caused this regression in the first place, and this test is already somewhat removed from the code that was reported to not optimize.

#![crate_type = "lib"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated: should we make mir-opt tests "lib" by default, to avoid to list all the cases in a fn main everywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we can. The codegen tests could do with the same treatment; I pulled this style of testing from them. There's a #![crate_type = "lib"] in nearly every single codegen test file.


pub enum Thing {
A,
B,
}

// EMIT_MIR instcombine_duplicate_switch_targets_e2e.ub_if_b.PreCodegen.after.mir
pub unsafe fn ub_if_b(t: Thing) -> Thing {
match t {
Thing::A => t,
Thing::B => std::hint::unreachable_unchecked(),
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// MIR for `ub_if_b` after PreCodegen

fn ub_if_b(_1: Thing) -> Thing {
debug t => _1; // in scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+0:23: +0:24
let mut _0: Thing; // return place in scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+0:36: +0:41
let mut _2: isize; // in scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+2:9: +2:17
scope 1 (inlined unreachable_unchecked) { // at $DIR/instcombine_duplicate_switch_targets_e2e.rs:14:21: 14:55
scope 2 {
scope 3 (inlined unreachable_unchecked::runtime) { // at $SRC_DIR/core/src/intrinsics.rs:LL:COL
}
}
}

bb0: {
_2 = discriminant(_1); // scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+1:11: +1:12
switchInt(move _2) -> [0: bb2, otherwise: bb1]; // scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+1:5: +1:12
}

bb1: {
unreachable; // scope 2 at $SRC_DIR/core/src/intrinsics.rs:LL:COL
}

bb2: {
_0 = move _1; // scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+2:21: +2:22
return; // scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+5:2: +5:2
}
}