Auto merge of rust-lang#124188 - scottmcm:implicit-switchint-unreachable, r=<try>

MIR: Stop needing an explicit BB for `otherwise:unreachable`

So many places to update :D

Zulip conversation: <https://rust-lang.zulipchat.com/#narrow/stream/189540-t-compiler.2Fwg-mir-opt/topic/Pondering.20the.20SwitchInt.20.26.20unreachable/near/432988457>

For this PR I tried to keep things doing essentially the same thing as before.  No new passes to try to use it more, no change to MIR building for exhaustive matches, etc.

That said, `UnreachableProp` still picks it up, and thus there's still a bunch of `otherwise` arms removed and `unreachable` blocks that no longer show.

r? ghost
cc `@JakobDegen` `@saethlin` `@cuviper` `@wesleywiser`

Author: bors

compiler/rustc_codegen_cranelift/src/base.rs

@@ -135,6 +135,7 @@ pub(crate) fn codegen_fn<'tcx>(
         bcx,
         block_map,
         local_map: IndexVec::with_capacity(mir.local_decls.len()),
+        shared_unreachable: None,
         caller_location: None, // set by `codegen_fn_prelude`
 
         clif_comments,
@@ -441,7 +442,7 @@ fn codegen_fn_body(fx: &mut FunctionCx<'_, '_, '_>, start_block: Block) {
                     assert_eq!(targets.iter().count(), 1);
                     let (then_value, then_block) = targets.iter().next().unwrap();
                     let then_block = fx.get_block(then_block);
-                    let else_block = fx.get_block(targets.otherwise());
+                    let else_block = fx.get_switch_block(targets.otherwise());
                     let test_zero = match then_value {
                         0 => true,
                         1 => false,
@@ -472,7 +473,7 @@ fn codegen_fn_body(fx: &mut FunctionCx<'_, '_, '_>, start_block: Block) {
                         let block = fx.get_block(block);
                         switch.set_entry(value, block);
                     }
-                    let otherwise_block = fx.get_block(targets.otherwise());
+                    let otherwise_block = fx.get_switch_block(targets.otherwise());
                     switch.emit(&mut fx.bcx, discr, otherwise_block);
                 }
             }
@@ -561,6 +562,11 @@ fn codegen_fn_body(fx: &mut FunctionCx<'_, '_, '_>, start_block: Block) {
             }
         };
     }
+
+    if let Some(unreachable) = fx.shared_unreachable {
+        fx.bcx.switch_to_block(unreachable);
+        fx.bcx.ins().trap(TrapCode::user(1 /* unreachable */).unwrap());
+    }
 }
 
 fn codegen_stmt<'tcx>(

compiler/rustc_codegen_cranelift/src/common.rs

@@ -298,6 +298,7 @@ pub(crate) struct FunctionCx<'m, 'clif, 'tcx: 'm> {
     pub(crate) bcx: FunctionBuilder<'clif>,
     pub(crate) block_map: IndexVec<BasicBlock, Block>,
     pub(crate) local_map: IndexVec<Local, CPlace<'tcx>>,
+    pub(crate) shared_unreachable: Option<Block>,
 
     /// When `#[track_caller]` is used, the implicit caller location is stored in this variable.
     pub(crate) caller_location: Option<CValue<'tcx>>,
@@ -375,6 +376,17 @@ impl<'tcx> FunctionCx<'_, '_, 'tcx> {
         *self.block_map.get(bb).unwrap()
     }
 
+    pub(crate) fn get_switch_block(&mut self, sa: SwitchAction) -> Block {
+        match sa {
+            SwitchAction::Goto(bb) => self.get_block(bb),
+            SwitchAction::Unreachable => self.unreachable_block(),
+        }
+    }
+
+    pub(crate) fn unreachable_block(&mut self) -> Block {
+        *self.shared_unreachable.get_or_insert_with(|| self.bcx.create_block())
+    }
+
     pub(crate) fn get_local_place(&mut self, local: Local) -> CPlace<'tcx> {
         *self.local_map.get(local).unwrap_or_else(|| {
             panic!("Local {:?} doesn't exist", local);

compiler/rustc_codegen_ssa/src/mir/block.rs

@@ -5,7 +5,8 @@ use rustc_ast as ast;
 use rustc_ast::{InlineAsmOptions, InlineAsmTemplatePiece};
 use rustc_hir::lang_items::LangItem;
 use rustc_middle::mir::{
-    self, AssertKind, BasicBlock, InlineAsmMacro, SwitchTargets, UnwindTerminateReason,
+    self, AssertKind, BasicBlock, InlineAsmMacro, SwitchAction, SwitchTargets,
+    UnwindTerminateReason,
 };
 use rustc_middle::ty::layout::{HasTyCtxt, LayoutOf, ValidityRequirement};
 use rustc_middle::ty::print::{with_no_trimmed_paths, with_no_visible_paths};
@@ -96,6 +97,17 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
         }
     }
 
+    fn llbb_with_cleanup_from_switch_action<Bx: BuilderMethods<'a, 'tcx>>(
+        &self,
+        fx: &mut FunctionCx<'a, 'tcx, Bx>,
+        target: mir::SwitchAction,
+    ) -> Bx::BasicBlock {
+        match target {
+            mir::SwitchAction::Unreachable => fx.unreachable_block(),
+            mir::SwitchAction::Goto(bb) => self.llbb_with_cleanup(fx, bb),
+        }
+    }
+
     fn llbb_characteristics<Bx: BuilderMethods<'a, 'tcx>>(
         &self,
         fx: &mut FunctionCx<'a, 'tcx, Bx>,
@@ -368,7 +380,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
         // If our discriminant is a constant we can branch directly
         if let Some(const_discr) = bx.const_to_opt_u128(discr_value, false) {
             let target = targets.target_for_value(const_discr);
-            bx.br(helper.llbb_with_cleanup(self, target));
+            bx.br(helper.llbb_with_cleanup_from_switch_action(self, target));
             return;
         };
 
@@ -379,9 +391,12 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             let (test_value, target) = target_iter.next().unwrap();
             let otherwise = targets.otherwise();
             let lltarget = helper.llbb_with_cleanup(self, target);
-            let llotherwise = helper.llbb_with_cleanup(self, otherwise);
+            let llotherwise = helper.llbb_with_cleanup_from_switch_action(self, otherwise);
             let target_cold = self.cold_blocks[target];
-            let otherwise_cold = self.cold_blocks[otherwise];
+            let otherwise_cold = match otherwise {
+                SwitchAction::Goto(otherwise) => self.cold_blocks[otherwise],
+                SwitchAction::Unreachable => true,
+            };
             // If `target_cold == otherwise_cold`, the branches have the same weight
             // so there is no expectation. If they differ, the `target` branch is expected
             // when the `otherwise` branch is cold.
@@ -406,7 +421,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             }
         } else if self.cx.sess().opts.optimize == OptLevel::No
             && target_iter.len() == 2
-            && self.mir[targets.otherwise()].is_empty_unreachable()
+            && self.mir.basic_blocks.is_empty_unreachable(targets.otherwise())
         {
             // In unoptimized builds, if there are two normal targets and the `otherwise` target is
             // an unreachable BB, emit `br` instead of `switch`. This leaves behind the unreachable
@@ -431,7 +446,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
         } else {
             bx.switch(
                 discr_value,
-                helper.llbb_with_cleanup(self, targets.otherwise()),
+                helper.llbb_with_cleanup_from_switch_action(self, targets.otherwise()),
                 target_iter.map(|(value, target)| (value, helper.llbb_with_cleanup(self, target))),
             );
         }
@@ -1648,11 +1663,10 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
     }
 
     fn unreachable_block(&mut self) -> Bx::BasicBlock {
-        self.unreachable_block.unwrap_or_else(|| {
+        *self.unreachable_block.get_or_insert_with(|| {
             let llbb = Bx::append_block(self.cx, self.llfn, "unreachable");
             let mut bx = Bx::build(self.cx, llbb);
             bx.unreachable();
-            self.unreachable_block = Some(llbb);
             llbb
         })
     }

compiler/rustc_const_eval/src/interpret/step.rs

@@ -478,12 +478,15 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                         &ImmTy::from_uint(const_int, discr.layout),
                     )?;
                     if res.to_scalar().to_bool()? {
-                        target_block = target;
+                        target_block = mir::SwitchAction::Goto(target);
                         break;
                     }
                 }
 
-                self.go_to_block(target_block);
+                match target_block {
+                    mir::SwitchAction::Goto(target) => self.go_to_block(target),
+                    mir::SwitchAction::Unreachable => throw_ub!(Unreachable),
+                }
             }
 
             Call {

compiler/rustc_middle/src/mir/basic_blocks.rs

@@ -9,7 +9,9 @@ use rustc_serialize::{Decodable, Decoder, Encodable, Encoder};
 use smallvec::SmallVec;
 
 use crate::mir::traversal::Postorder;
-use crate::mir::{BasicBlock, BasicBlockData, START_BLOCK, Terminator, TerminatorKind};
+use crate::mir::{
+    BasicBlock, BasicBlockData, START_BLOCK, SwitchAction, Terminator, TerminatorKind,
+};
 
 #[derive(Clone, TyEncodable, TyDecodable, Debug, HashStable, TypeFoldable, TypeVisitable)]
 pub struct BasicBlocks<'tcx> {
@@ -84,7 +86,9 @@ impl<'tcx> BasicBlocks<'tcx> {
                     for (value, target) in targets.iter() {
                         switch_sources.entry((target, bb)).or_default().push(Some(value));
                     }
-                    switch_sources.entry((targets.otherwise(), bb)).or_default().push(None);
+                    if let SwitchAction::Goto(otherwise) = targets.otherwise() {
+                        switch_sources.entry((otherwise, bb)).or_default().push(None);
+                    }
                 }
             }
             switch_sources
@@ -122,6 +126,13 @@ impl<'tcx> BasicBlocks<'tcx> {
     pub fn invalidate_cfg_cache(&mut self) {
         self.cache = Cache::default();
     }
+
+    pub fn is_empty_unreachable(&self, action: SwitchAction) -> bool {
+        match action {
+            SwitchAction::Goto(bb) => self[bb].is_empty_unreachable(),
+            SwitchAction::Unreachable => true,
+        }
+    }
 }
 
 impl<'tcx> std::ops::Deref for BasicBlocks<'tcx> {

compiler/rustc_middle/src/mir/pretty.rs

@@ -1015,12 +1015,14 @@ impl<'tcx> TerminatorKind<'tcx> {
             | Unreachable
             | CoroutineDrop => vec![],
             Goto { .. } => vec!["".into()],
-            SwitchInt { ref targets, .. } => targets
-                .values
-                .iter()
-                .map(|&u| Cow::Owned(u.to_string()))
-                .chain(iter::once("otherwise".into()))
-                .collect(),
+            SwitchInt { ref targets, .. } => {
+                let mut labels: Vec<_> =
+                    targets.values.iter().map(|&u| Cow::Owned(u.to_string())).collect();
+                if let SwitchAction::Goto(_) = targets.otherwise() {
+                    labels.push("otherwise".into());
+                }
+                labels
+            }
             Call { target: Some(_), unwind: UnwindAction::Cleanup(_), .. } => {
                 vec!["return".into(), "unwind".into()]
             }

compiler/rustc_middle/src/mir/syntax.rs

@@ -959,21 +959,51 @@ pub struct SwitchTargets {
     /// are found in the corresponding indices from the `targets` vector.
     pub(super) values: SmallVec<[Pu128; 1]>,
 
-    /// Possible branch sites. The last element of this vector is used
-    /// for the otherwise branch, so targets.len() == values.len() + 1
-    /// should hold.
+    /// Possible branch sites.
+    ///
+    /// If `targets.len() == values.len() + 1`, the last element of this vector
+    /// is used for the otherwise branch in [`SwitchAction::Goto`].
+    ///
+    /// If `targets.len() == values.len()`, the otherwise branch is
+    /// [`SwitchAction::Unreachable`].
+    ///
+    /// You must ensure that we're always in at one of those cases.
     //
     // This invariant is quite non-obvious and also could be improved.
     // One way to make this invariant is to have something like this instead:
     //
     // branches: Vec<(ConstInt, BasicBlock)>,
-    // otherwise: Option<BasicBlock> // exhaustive if None
+    // otherwise: SwitchAction,
     //
     // However we’ve decided to keep this as-is until we figure a case
     // where some other approach seems to be strictly better than other.
     pub(super) targets: SmallVec<[BasicBlock; 2]>,
 }
 
+/// The action to be taken for a particular input to the [`TerminatorKind::SwitchInt`]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, TyEncodable, TyDecodable, Hash, HashStable)]
+#[derive(TypeFoldable, TypeVisitable)]
+pub enum SwitchAction {
+    /// Jump to the specified block
+    Goto(BasicBlock),
+    /// Triggers undefined behavior
+    ///
+    /// This is equivalent to jumping to a block with [`TerminatorKind::Unreachable`],
+    /// but lets us not have to store such a block in the body.
+    /// It also makes it easier in analysis to know this action is unreachable
+    /// without needing to have all the basic blocks around to look at.
+    Unreachable,
+}
+
+impl SwitchAction {
+    pub fn into_terminator<'tcx>(self) -> TerminatorKind<'tcx> {
+        match self {
+            SwitchAction::Goto(bb) => TerminatorKind::Goto { target: bb },
+            SwitchAction::Unreachable => TerminatorKind::Unreachable,
+        }
+    }
+}
+
 /// Action to be taken when a stack unwind happens.
 #[derive(Copy, Clone, Debug, PartialEq, Eq, TyEncodable, TyDecodable, Hash, HashStable)]
 #[derive(TypeFoldable, TypeVisitable)]

compiler/rustc_middle/src/mir/terminator.rs

@@ -14,10 +14,12 @@ impl SwitchTargets {
     ///
     /// The iterator may be empty, in which case the `SwitchInt` instruction is equivalent to
     /// `goto otherwise;`.
-    pub fn new(targets: impl Iterator<Item = (u128, BasicBlock)>, otherwise: BasicBlock) -> Self {
+    pub fn new(targets: impl Iterator<Item = (u128, BasicBlock)>, otherwise: SwitchAction) -> Self {
         let (values, mut targets): (SmallVec<_>, SmallVec<_>) =
             targets.map(|(v, t)| (Pu128(v), t)).unzip();
-        targets.push(otherwise);
+        if let SwitchAction::Goto(otherwise) = otherwise {
+            targets.push(otherwise);
+        }
         Self { values, targets }
     }
 
@@ -39,10 +41,17 @@ impl SwitchTargets {
         }
     }
 
-    /// Returns the fallback target that is jumped to when none of the values match the operand.
+    /// Returns the fallback action when none of the values match the operand.
     #[inline]
-    pub fn otherwise(&self) -> BasicBlock {
-        *self.targets.last().unwrap()
+    pub fn otherwise(&self) -> SwitchAction {
+        debug_assert!(
+            self.targets.len() == self.values.len() || self.targets.len() == self.values.len() + 1
+        );
+        if self.targets.len() > self.values.len() {
+            SwitchAction::Goto(*self.targets.last().unwrap())
+        } else {
+            SwitchAction::Unreachable
+        }
     }
 
     /// Returns an iterator over the switch targets.
@@ -82,8 +91,9 @@ impl SwitchTargets {
     /// specific value. This cannot fail, as it'll return the `otherwise`
     /// branch if there's not a specific match for the value.
     #[inline]
-    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())
+    pub fn target_for_value(&self, value: u128) -> SwitchAction {
+        let specific = self.iter().find_map(|(v, t)| (v == value).then_some(t));
+        if let Some(specific) = specific { SwitchAction::Goto(specific) } else { self.otherwise() }
     }
 
     /// Adds a new target to the switch. But You cannot add an already present value.
@@ -93,8 +103,12 @@ impl SwitchTargets {
         if self.values.contains(&value) {
             bug!("target value {:?} already present", value);
         }
+
+        // There may or may not be a fallback in `targets`, so make sure to
+        // insert the new target at the same index as its value.
+        let insert_point = self.values.len();
         self.values.push(value);
-        self.targets.insert(self.targets.len() - 1, bb);
+        self.targets.insert(insert_point, bb);
     }
 
     /// Returns true if all targets (including the fallback target) are distinct.
@@ -431,7 +445,11 @@ mod helper {
         #[inline]
         pub fn successors_for_value(&self, value: u128) -> Successors<'_> {
             let target = self.target_for_value(value);
-            (&[]).into_iter().copied().chain(Some(target))
+            (&[]).into_iter().copied().chain(if let SwitchAction::Goto(otherwise) = target {
+                Some(otherwise)
+            } else {
+                None
+            })
         }
     }
 

compiler/rustc_mir_build/src/builder/custom/parse/instruction.rs

@@ -163,7 +163,7 @@ impl<'a, 'tcx> ParseCtxt<'a, 'tcx> {
             targets.push(self.parse_block(arm.body)?);
         }
 
-        Ok(SwitchTargets::new(values.into_iter().zip(targets), otherwise))
+        Ok(SwitchTargets::new(values.into_iter().zip(targets), SwitchAction::Goto(otherwise)))
     }
 
     fn parse_call(&self, args: &[ExprId]) -> PResult<TerminatorKind<'tcx>> {

compiler/rustc_mir_build/src/builder/matches/test.rs

@@ -92,7 +92,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                             None
                         }
                     }),
-                    otherwise_block,
+                    SwitchAction::Goto(otherwise_block),
                 );
                 debug!("num_enum_variants: {}", adt_def.variants().len());
                 let discr_ty = adt_def.repr().discr_type().to_ty(self.tcx);
@@ -124,7 +124,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                             None
                         }
                     }),
-                    otherwise_block,
+                    SwitchAction::Goto(otherwise_block),
                 );
                 let terminator = TerminatorKind::SwitchInt {
                     discr: Operand::Copy(place),

compiler/rustc_mir_dataflow/src/elaborate_drops.rs

@@ -591,6 +591,10 @@ where
         succ: BasicBlock,
         unwind: Unwind,
     ) -> BasicBlock {
+        // FIXME: the arguments here are still using the manual-unreachable;
+        // consider changing them to allow `SwitchAction::Unreachable` somehow.
+        debug_assert_eq!(blocks.len(), values.len() + 1);
+
         // If there are multiple variants, then if something
         // is present within the enum the discriminant, tracked
         // by the rest path, must be initialized.
@@ -609,7 +613,7 @@ where
                     discr: Operand::Move(discr),
                     targets: SwitchTargets::new(
                         values.iter().copied().zip(blocks.iter().copied()),
-                        *blocks.last().unwrap(),
+                        SwitchAction::Goto(*blocks.last().unwrap()),
                     ),
                 },
             }),