Skip to content

Commit

Permalink
Rollup merge of rust-lang#120268 - DianQK:otherwise_is_last_variant_s…
Browse files Browse the repository at this point in the history
…witchs, r=oli-obk

Replace the default branch with an unreachable branch If it is the last variant

Fixes rust-lang#119520.

LLVM currently has limited ability to eliminate dead branches in switches, even with the patch of llvm/llvm-project#73446.

The main reasons are as follows:

- Additional costs are required to calculate the range of values, and there exist many scenarios that cannot be analyzed accurately.
- Matching values by bitwise calculation cannot handle odd branches, nor can it handle values like `-1, 0, 1`. See [SimplifyCFG.cpp#L5424](https://github.com/llvm/llvm-project/blob/llvmorg-17.0.6/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L5424) and https://llvm.godbolt.org/z/qYMqhvMa8
- The current range information is continuous, even if the metadata for the range is submitted. See [ConstantRange.cpp#L1869-L1870](https://github.com/llvm/llvm-project/blob/llvmorg-17.0.6/llvm/lib/IR/ConstantRange.cpp#L1869-L1870).
- The metadata of the range may be lost in passes such as SROA. See https://rust.godbolt.org/z/e7f87vKMK.

Although we can make improvements, I think it would be more appropriate to put this issue to rustc first. After all, we can easily know the possible values.

Note that we've currently found a slow compilation problem in the presence of unreachable branches. See
llvm/llvm-project#78578.

r? compiler
  • Loading branch information
Nadrieril authored Jan 27, 2024
2 parents 696cec3 + a3341df commit 35c1914
Show file tree
Hide file tree
Showing 32 changed files with 1,316 additions and 112 deletions.
11 changes: 11 additions & 0 deletions compiler/rustc_middle/src/mir/terminator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,17 @@ impl SwitchTargets {
pub fn target_for_value(&self, value: u128) -> BasicBlock {
self.iter().find_map(|(v, t)| (v == value).then_some(t)).unwrap_or_else(|| self.otherwise())
}

/// Adds a new target to the switch. But You cannot add an already present value.
#[inline]
pub fn add_target(&mut self, value: u128, bb: BasicBlock) {
let value = Pu128(value);
if self.values.contains(&value) {
bug!("target value {:?} already present", value);
}
self.values.push(value);
self.targets.insert(self.targets.len() - 1, bb);
}
}

pub struct SwitchTargetsIter<'a> {
Expand Down
28 changes: 22 additions & 6 deletions compiler/rustc_mir_transform/src/uninhabited_enum_branching.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ impl<'tcx> MirPass<'tcx> for UninhabitedEnumBranching {
trace!("UninhabitedEnumBranching starting for {:?}", body.source);

let mut removable_switchs = Vec::new();
let mut otherwise_is_last_variant_switchs = Vec::new();

for (bb, bb_data) in body.basic_blocks.iter_enumerated() {
trace!("processing block {:?}", bb);
Expand All @@ -92,8 +93,14 @@ impl<'tcx> MirPass<'tcx> for UninhabitedEnumBranching {
tcx.param_env_reveal_all_normalized(body.source.def_id()).and(discriminant_ty),
);

let allowed_variants = if let Ok(layout) = layout {
let mut allowed_variants = if let Ok(layout) = layout {
variant_discriminants(&layout, discriminant_ty, tcx)
} else if let Some(variant_range) = discriminant_ty.variant_range(tcx) {
variant_range
.map(|variant| {
discriminant_ty.discriminant_for_variant(tcx, variant).unwrap().val
})
.collect()
} else {
continue;
};
Expand All @@ -103,20 +110,29 @@ impl<'tcx> MirPass<'tcx> for UninhabitedEnumBranching {
let terminator = bb_data.terminator();
let TerminatorKind::SwitchInt { targets, .. } = &terminator.kind else { bug!() };

let mut reachable_count = 0;
for (index, (val, _)) in targets.iter().enumerate() {
if allowed_variants.contains(&val) {
reachable_count += 1;
} else {
if !allowed_variants.remove(&val) {
removable_switchs.push((bb, index));
}
}

if reachable_count == allowed_variants.len() {
if allowed_variants.is_empty() {
removable_switchs.push((bb, targets.iter().count()));
} else if allowed_variants.len() == 1 {
#[allow(rustc::potential_query_instability)]
let last_variant = *allowed_variants.iter().next().unwrap();
otherwise_is_last_variant_switchs.push((bb, last_variant));
}
}

for (bb, last_variant) in otherwise_is_last_variant_switchs {
let bb_data = &mut body.basic_blocks.as_mut()[bb];
let terminator = bb_data.terminator_mut();
let TerminatorKind::SwitchInt { targets, .. } = &mut terminator.kind else { bug!() };
targets.add_target(last_variant, targets.otherwise());
removable_switchs.push((bb, targets.iter().count()));
}

if removable_switchs.is_empty() {
return;
}
Expand Down
24 changes: 24 additions & 0 deletions tests/codegen/enum/uninhabited_enum_default_branch.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// 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);

// CHECK-LABEL: @foo
// CHECK-SAME: [[TMP0:%.*]])
// CHECK-NEXT: start:
// CHECK-NEXT: [[TMP1:%.*]] = add i32 [[TMP0]], -201
// CHECK-NEXT: [[OR_COND:%.*]] = icmp ult i32 [[TMP1]], 70
// CHECK-NEXT: [[TMP2:%.*]] = icmp eq i32 [[TMP0]], 153
// CHECK-NEXT: [[SPEC_SELECT:%.*]] = or i1 [[OR_COND]], [[TMP2]]
// CHECK-NEXT: ret i1 [[SPEC_SELECT]]
#[no_mangle]
pub fn foo(x: Int) -> bool {
(x >= A && x <= B)
|| x == C
}
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
StorageLive(_6);
_6 = ((*_1).4: std::option::Option<usize>);
_7 = discriminant(_6);
switchInt(move _7) -> [1: bb4, otherwise: bb6];
switchInt(move _7) -> [1: bb4, 0: bb6, otherwise: bb9];
}

bb4: {
Expand Down Expand Up @@ -135,5 +135,9 @@
StorageDead(_6);
return;
}

bb9: {
unreachable;
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
StorageLive(_6);
_6 = ((*_1).4: std::option::Option<usize>);
_7 = discriminant(_6);
switchInt(move _7) -> [1: bb4, otherwise: bb6];
switchInt(move _7) -> [1: bb4, 0: bb6, otherwise: bb9];
}

bb4: {
Expand Down Expand Up @@ -135,5 +135,9 @@
StorageDead(_6);
return;
}

bb9: {
unreachable;
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
fn num_to_digit(_1: char) -> u32 {
debug num => _1;
let mut _0: u32;
let mut _5: std::option::Option<u32>;
let mut _5: bool;
let mut _6: std::option::Option<u32>;
scope 1 (inlined char::methods::<impl char>::is_digit) {
debug self => _1;
debug radix => const 8_u32;
Expand All @@ -15,15 +16,16 @@ fn num_to_digit(_1: char) -> u32 {
}
}
scope 3 (inlined #[track_caller] Option::<u32>::unwrap) {
debug self => _5;
let mut _6: isize;
let mut _7: !;
debug self => _6;
let mut _7: isize;
let mut _8: !;
scope 4 {
debug val => _0;
}
}

bb0: {
StorageLive(_5);
StorageLive(_3);
StorageLive(_2);
_2 = char::methods::<impl char>::to_digit(_1, const 8_u32) -> [return: bb1, unwind unreachable];
Expand All @@ -33,45 +35,59 @@ fn num_to_digit(_1: char) -> u32 {
_3 = &_2;
StorageLive(_4);
_4 = discriminant(_2);
StorageDead(_3);
StorageDead(_2);
switchInt(move _4) -> [1: bb2, otherwise: bb7];
switchInt(move _4) -> [1: bb2, 0: bb3, otherwise: bb11];
}

bb2: {
StorageDead(_4);
StorageLive(_5);
_5 = char::methods::<impl char>::to_digit(move _1, const 8_u32) -> [return: bb3, unwind unreachable];
_5 = const true;
goto -> bb4;
}

bb3: {
StorageLive(_6);
_6 = discriminant(_5);
switchInt(move _6) -> [0: bb4, 1: bb5, otherwise: bb6];
_5 = const false;
goto -> bb4;
}

bb4: {
_7 = option::unwrap_failed() -> unwind unreachable;
StorageDead(_4);
StorageDead(_3);
StorageDead(_2);
switchInt(move _5) -> [0: bb5, otherwise: bb6];
}

bb5: {
_0 = move ((_5 as Some).0: u32);
StorageDead(_6);
StorageDead(_5);
goto -> bb8;
_0 = const 0_u32;
goto -> bb10;
}

bb6: {
unreachable;
StorageLive(_6);
_6 = char::methods::<impl char>::to_digit(move _1, const 8_u32) -> [return: bb7, unwind unreachable];
}

bb7: {
StorageDead(_4);
_0 = const 0_u32;
goto -> bb8;
StorageLive(_7);
_7 = discriminant(_6);
switchInt(move _7) -> [0: bb8, 1: bb9, otherwise: bb11];
}

bb8: {
_8 = option::unwrap_failed() -> unwind unreachable;
}

bb9: {
_0 = move ((_6 as Some).0: u32);
StorageDead(_7);
StorageDead(_6);
goto -> bb10;
}

bb10: {
StorageDead(_5);
return;
}

bb11: {
unreachable;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
fn num_to_digit(_1: char) -> u32 {
debug num => _1;
let mut _0: u32;
let mut _5: std::option::Option<u32>;
let mut _5: bool;
let mut _6: std::option::Option<u32>;
scope 1 (inlined char::methods::<impl char>::is_digit) {
debug self => _1;
debug radix => const 8_u32;
Expand All @@ -15,15 +16,16 @@ fn num_to_digit(_1: char) -> u32 {
}
}
scope 3 (inlined #[track_caller] Option::<u32>::unwrap) {
debug self => _5;
let mut _6: isize;
let mut _7: !;
debug self => _6;
let mut _7: isize;
let mut _8: !;
scope 4 {
debug val => _0;
}
}

bb0: {
StorageLive(_5);
StorageLive(_3);
StorageLive(_2);
_2 = char::methods::<impl char>::to_digit(_1, const 8_u32) -> [return: bb1, unwind continue];
Expand All @@ -33,45 +35,59 @@ fn num_to_digit(_1: char) -> u32 {
_3 = &_2;
StorageLive(_4);
_4 = discriminant(_2);
StorageDead(_3);
StorageDead(_2);
switchInt(move _4) -> [1: bb2, otherwise: bb7];
switchInt(move _4) -> [1: bb2, 0: bb3, otherwise: bb11];
}

bb2: {
StorageDead(_4);
StorageLive(_5);
_5 = char::methods::<impl char>::to_digit(move _1, const 8_u32) -> [return: bb3, unwind continue];
_5 = const true;
goto -> bb4;
}

bb3: {
StorageLive(_6);
_6 = discriminant(_5);
switchInt(move _6) -> [0: bb4, 1: bb5, otherwise: bb6];
_5 = const false;
goto -> bb4;
}

bb4: {
_7 = option::unwrap_failed() -> unwind continue;
StorageDead(_4);
StorageDead(_3);
StorageDead(_2);
switchInt(move _5) -> [0: bb5, otherwise: bb6];
}

bb5: {
_0 = move ((_5 as Some).0: u32);
StorageDead(_6);
StorageDead(_5);
goto -> bb8;
_0 = const 0_u32;
goto -> bb10;
}

bb6: {
unreachable;
StorageLive(_6);
_6 = char::methods::<impl char>::to_digit(move _1, const 8_u32) -> [return: bb7, unwind continue];
}

bb7: {
StorageDead(_4);
_0 = const 0_u32;
goto -> bb8;
StorageLive(_7);
_7 = discriminant(_6);
switchInt(move _7) -> [0: bb8, 1: bb9, otherwise: bb11];
}

bb8: {
_8 = option::unwrap_failed() -> unwind continue;
}

bb9: {
_0 = move ((_6 as Some).0: u32);
StorageDead(_7);
StorageDead(_6);
goto -> bb10;
}

bb10: {
StorageDead(_5);
return;
}

bb11: {
unreachable;
}
}
Loading

0 comments on commit 35c1914

Please sign in to comment.