Skip to content

Commit

Permalink
Auto merge of #122225 - DianQK:nits-120268, r=cjgillot
Browse files Browse the repository at this point in the history
 Rename `UninhabitedEnumBranching` to `UnreachableEnumBranching`

Per [#120268](#120268 (comment)), I rename `UninhabitedEnumBranching` to `UnreachableEnumBranching` .

I solved some nits to add some comments.

I adjusted the workaround restrictions. This should be useful for `a <= b` and `if let Some/Ok(v)`. For enum with few variants, `early-tailduplication` should not cause compile time overhead.

r? RalfJung
  • Loading branch information
bors committed Apr 3, 2024
2 parents 8938f88 + ec313d1 commit 76cf07d
Show file tree
Hide file tree
Showing 33 changed files with 256 additions and 166 deletions.
7 changes: 4 additions & 3 deletions compiler/rustc_mir_transform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ pub mod simplify;
mod simplify_branches;
mod simplify_comparison_integral;
mod sroa;
mod uninhabited_enum_branching;
mod unreachable_enum_branching;
mod unreachable_prop;

use rustc_const_eval::transform::check_consts::{self, ConstCx};
Expand Down Expand Up @@ -580,9 +580,10 @@ fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
&remove_zsts::RemoveZsts,
&remove_unneeded_drops::RemoveUnneededDrops,
// Type instantiation may create uninhabited enums.
&uninhabited_enum_branching::UninhabitedEnumBranching,
// Also eliminates some unreachable branches based on variants of enums.
&unreachable_enum_branching::UnreachableEnumBranching,
&unreachable_prop::UnreachablePropagation,
&o1(simplify::SimplifyCfg::AfterUninhabitedEnumBranching),
&o1(simplify::SimplifyCfg::AfterUnreachableEnumBranching),
// Inlining may have introduced a lot of redundant code and a large move pattern.
// Now, we need to shrink the generated MIR.

Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_mir_transform/src/simplify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ pub enum SimplifyCfg {
PreOptimizations,
Final,
MakeShim,
AfterUninhabitedEnumBranching,
AfterUnreachableEnumBranching,
}

impl SimplifyCfg {
Expand All @@ -57,8 +57,8 @@ impl SimplifyCfg {
SimplifyCfg::PreOptimizations => "SimplifyCfg-pre-optimizations",
SimplifyCfg::Final => "SimplifyCfg-final",
SimplifyCfg::MakeShim => "SimplifyCfg-make_shim",
SimplifyCfg::AfterUninhabitedEnumBranching => {
"SimplifyCfg-after-uninhabited-enum-branching"
SimplifyCfg::AfterUnreachableEnumBranching => {
"SimplifyCfg-after-unreachable-enum-branching"
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//! A pass that eliminates branches on uninhabited enum variants.
//! A pass that eliminates branches on uninhabited or unreachable enum variants.
use crate::MirPass;
use rustc_data_structures::fx::FxHashSet;
Expand All @@ -11,7 +11,7 @@ use rustc_middle::ty::layout::TyAndLayout;
use rustc_middle::ty::{Ty, TyCtxt};
use rustc_target::abi::{Abi, Variants};

pub struct UninhabitedEnumBranching;
pub struct UnreachableEnumBranching;

fn get_discriminant_local(terminator: &TerminatorKind<'_>) -> Option<Local> {
if let TerminatorKind::SwitchInt { discr: Operand::Move(p), .. } = terminator {
Expand Down Expand Up @@ -71,13 +71,13 @@ fn variant_discriminants<'tcx>(
}
}

impl<'tcx> MirPass<'tcx> for UninhabitedEnumBranching {
impl<'tcx> MirPass<'tcx> for UnreachableEnumBranching {
fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
sess.mir_opt_level() > 0
}

fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
trace!("UninhabitedEnumBranching starting for {:?}", body.source);
trace!("UnreachableEnumBranching starting for {:?}", body.source);

let mut unreachable_targets = Vec::new();
let mut patch = MirPatch::new(body);
Expand All @@ -96,8 +96,10 @@ impl<'tcx> MirPass<'tcx> for UninhabitedEnumBranching {
);

let mut allowed_variants = if let Ok(layout) = layout {
// Find allowed variants based on uninhabited.
variant_discriminants(&layout, discriminant_ty, tcx)
} else if let Some(variant_range) = discriminant_ty.variant_range(tcx) {
// If there are some generics, we can still get the allowed variants.
variant_range
.map(|variant| {
discriminant_ty.discriminant_for_variant(tcx, variant).unwrap().val
Expand All @@ -121,9 +123,26 @@ impl<'tcx> MirPass<'tcx> for UninhabitedEnumBranching {
}
let otherwise_is_empty_unreachable =
body.basic_blocks[targets.otherwise()].is_empty_unreachable();
// After resolving https://github.com/llvm/llvm-project/issues/78578,
// we can remove the limit on the number of successors.
fn check_successors(basic_blocks: &BasicBlocks<'_>, bb: BasicBlock) -> bool {
// After resolving https://github.com/llvm/llvm-project/issues/78578,
// We can remove this check.
// The main issue here is that `early-tailduplication` causes compile time overhead
// and potential performance problems.
// Simply put, when encounter a switch (indirect branch) statement,
// `early-tailduplication` tries to duplicate the switch branch statement with BB
// into (each) predecessors. This makes CFG very complex.
// We can understand it as it transforms the following code
// ```rust
// match a { ... many cases };
// match b { ... many cases };
// ```
// into
// ```rust
// match a { ... many match b { goto BB cases } }
// ... BB cases
// ```
// Abandon this transformation when it is possible (the best effort)
// to encounter the problem.
let mut successors = basic_blocks[bb].terminator().successors();
let Some(first_successor) = successors.next() else { return true };
if successors.next().is_some() {
Expand All @@ -136,11 +155,32 @@ impl<'tcx> MirPass<'tcx> for UninhabitedEnumBranching {
};
true
}
// If and only if there is a variant that does not have a branch set,
// change the current of otherwise as the variant branch and set otherwise to unreachable.
// It transforms following code
// ```rust
// match c {
// Ordering::Less => 1,
// Ordering::Equal => 2,
// _ => 3,
// }
// ```
// to
// ```rust
// match c {
// Ordering::Less => 1,
// Ordering::Equal => 2,
// Ordering::Greater => 3,
// }
// ```
let otherwise_is_last_variant = !otherwise_is_empty_unreachable
&& allowed_variants.len() == 1
&& check_successors(&body.basic_blocks, targets.otherwise());
// Despite the LLVM issue, we hope that small enum can still be transformed.
// This is valuable for both `a <= b` and `if let Some/Ok(v)`.
&& (targets.all_targets().len() <= 3
|| check_successors(&body.basic_blocks, targets.otherwise()));
let replace_otherwise_to_unreachable = otherwise_is_last_variant
|| !otherwise_is_empty_unreachable && allowed_variants.is_empty();
|| (!otherwise_is_empty_unreachable && allowed_variants.is_empty());

if unreachable_targets.is_empty() && !replace_otherwise_to_unreachable {
continue;
Expand All @@ -150,6 +190,7 @@ impl<'tcx> MirPass<'tcx> for UninhabitedEnumBranching {
let mut targets = targets.clone();
if replace_otherwise_to_unreachable {
if otherwise_is_last_variant {
// We have checked that `allowed_variants` has only one element.
#[allow(rustc::potential_query_instability)]
let last_variant = *allowed_variants.iter().next().unwrap();
targets.add_target(last_variant, targets.otherwise());
Expand Down
24 changes: 0 additions & 24 deletions tests/codegen/enum/uninhabited_enum_default_branch.rs

This file was deleted.

43 changes: 43 additions & 0 deletions tests/codegen/enum/unreachable_enum_default_branch.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
//@ compile-flags: -O

#![crate_type = "lib"]

#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
pub struct Int(u32);

const A: Int = Int(201);
const B: Int = Int(270);
const C: Int = Int(153);

// The code is from https://github.com/rust-lang/rust/issues/119520.
// This code will basically turn into `matches!(x.partial_cmp(&A), Some(Greater | Equal))`.
// The otherwise branch must be `Less`.
// CHECK-LABEL: @implicit_match(
// CHECK-SAME: [[TMP0:%.*]])
// CHECK-NEXT: start:
// CHECK-NEXT: [[TMP1:%.*]] = add i32 [[TMP0]], -201
// CHECK-NEXT: icmp ult i32 [[TMP1]], 70
// CHECK-NEXT: icmp eq i32 [[TMP0]], 153
// CHECK-NEXT: [[SPEC_SELECT:%.*]] = or i1
// CHECK-NEXT: ret i1 [[SPEC_SELECT]]
#[no_mangle]
pub fn implicit_match(x: Int) -> bool {
(x >= A && x <= B)
|| x == C
}

// The code is from https://github.com/rust-lang/rust/issues/110097.
// We expect it to generate the same optimized code as a full match.
// CHECK-LABEL: @if_let(
// CHECK-NEXT: start:
// CHECK-NEXT: insertvalue
// CHECK-NEXT: insertvalue
// CHECK-NEXT: ret
#[no_mangle]
pub fn if_let(val: Result<i32, ()>) -> Result<i32, ()> {
if let Ok(x) = val {
Ok(x)
} else {
Err(())
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
- // MIR for `assert_nonzero_nonmax` before SimplifyCfg-after-unreachable-enum-branching
+ // MIR for `assert_nonzero_nonmax` after SimplifyCfg-after-unreachable-enum-branching

fn assert_nonzero_nonmax(_1: u8) -> u8 {
let mut _0: u8;

bb0: {
- switchInt(_1) -> [0: bb3, 1: bb2, 255: bb3, otherwise: bb4];
+ switchInt(_1) -> [0: bb2, 1: bb1, 255: bb2, otherwise: bb3];
}

bb1: {
- _0 = const 1_u8;
- return;
- }
-
- bb2: {
_0 = const 2_u8;
return;
}

- bb3: {
+ bb2: {
unreachable;
}

- bb4: {
+ bb3: {
_0 = _1;
return;
}
}

52 changes: 52 additions & 0 deletions tests/mir-opt/simplify_dead_blocks.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
//@ unit-test: SimplifyCfg-after-unreachable-enum-branching
#![feature(custom_mir, core_intrinsics)]
#![crate_type = "lib"]

use std::intrinsics::mir::*;

// Check that we correctly cleaned up the dead BB.
// EMIT_MIR simplify_dead_blocks.assert_nonzero_nonmax.SimplifyCfg-after-unreachable-enum-branching.diff
#[custom_mir(dialect = "runtime", phase = "post-cleanup")]
pub unsafe fn assert_nonzero_nonmax(x: u8) -> u8 {
// CHECK-LABEL: fn assert_nonzero_nonmax(
// CHECK: bb0: {
// CHECK-NEXT: switchInt({{.*}}) -> [0: [[unreachable:bb.*]], 1: [[retblock2:bb.*]], 255: [[unreachable:bb.*]], otherwise: [[retblock:bb.*]]];
// CHECK-NEXT: }
// CHECK-NOT: _0 = const 1_u8;
// CHECK: [[retblock2]]: {
// CHECK-NEXT: _0 = const 2_u8;
// CHECK-NEXT: return;
// CHECK-NEXT: }
// CHECK: [[unreachable]]: {
// CHECK-NEXT: unreachable;
// CHECK-NEXT: }
// CHECK: [[retblock]]: {
// CHECK-NEXT: _0 = _1;
// CHECK-NEXT: return;
// CHECK-NEXT: }
mir!(
{
match x {
0 => unreachable,
1 => retblock2,
u8::MAX => unreachable,
_ => retblock,
}
}
deadRetblock1 = {
RET = 1;
Return()
}
retblock2 = {
RET = 2;
Return()
}
unreachable = {
Unreachable()
}
retblock = {
RET = x;
Return()
}
)
}

This file was deleted.

31 changes: 0 additions & 31 deletions tests/mir-opt/simplify_duplicate_unreachable_blocks.rs

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
- // MIR for `eliminate_fallthrough` before UninhabitedEnumBranching
+ // MIR for `eliminate_fallthrough` after UninhabitedEnumBranching
- // MIR for `eliminate_fallthrough` before UnreachableEnumBranching
+ // MIR for `eliminate_fallthrough` after UnreachableEnumBranching

fn eliminate_fallthrough(_1: S) -> u32 {
debug s => _1;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
- // MIR for `keep_fallthrough` before UninhabitedEnumBranching
+ // MIR for `keep_fallthrough` after UninhabitedEnumBranching
- // MIR for `keep_fallthrough` before UnreachableEnumBranching
+ // MIR for `keep_fallthrough` after UnreachableEnumBranching

fn keep_fallthrough(_1: S) -> u32 {
debug s => _1;
Expand Down
Loading

0 comments on commit 76cf07d

Please sign in to comment.