Rollup merge of #136959 - nnethercote:simplify-SwitchSources, r=tmiasko

Simplify switch sources

`SwitchSources` and the code around it can be simplified.

r? `@tmiasko`

Author: matthiaskrgr

compiler/rustc_middle/src/mir/basic_blocks.rs

@@ -20,7 +20,22 @@ pub struct BasicBlocks<'tcx> {
 // Typically 95%+ of basic blocks have 4 or fewer predecessors.
 type Predecessors = IndexVec<BasicBlock, SmallVec<[BasicBlock; 4]>>;
 
-type SwitchSources = FxHashMap<(BasicBlock, BasicBlock), SmallVec<[Option<u128>; 1]>>;
+/// Each `(target, switch)` entry in the map contains a list of switch values
+/// that lead to a `target` block from a `switch` block.
+///
+/// Note: this type is currently never instantiated, because it's only used for
+/// `BasicBlocks::switch_sources`, which is only called by backwards analyses
+/// that do `SwitchInt` handling, and we don't have any of those, not even in
+/// tests. See #95120 and #94576.
+type SwitchSources = FxHashMap<(BasicBlock, BasicBlock), SmallVec<[SwitchTargetValue; 1]>>;
+
+#[derive(Debug, Clone, Copy)]
+pub enum SwitchTargetValue {
+    // A normal switch value.
+    Normal(u128),
+    // The final "otherwise" fallback value.
+    Otherwise,
+}
 
 #[derive(Clone, Default, Debug)]
 struct Cache {
@@ -70,8 +85,8 @@ impl<'tcx> BasicBlocks<'tcx> {
         })
     }
 
-    /// `switch_sources()[&(target, switch)]` returns a list of switch
-    /// values that lead to a `target` block from a `switch` block.
+    /// Returns info about switch values that lead from one block to another
+    /// block. See `SwitchSources`.
     #[inline]
     pub fn switch_sources(&self) -> &SwitchSources {
         self.cache.switch_sources.get_or_init(|| {
@@ -82,9 +97,15 @@ impl<'tcx> BasicBlocks<'tcx> {
                 }) = &data.terminator
                 {
                     for (value, target) in targets.iter() {
-                        switch_sources.entry((target, bb)).or_default().push(Some(value));
+                        switch_sources
+                            .entry((target, bb))
+                            .or_default()
+                            .push(SwitchTargetValue::Normal(value));
                     }
-                    switch_sources.entry((targets.otherwise(), bb)).or_default().push(None);
+                    switch_sources
+                        .entry((targets.otherwise(), bb))
+                        .or_default()
+                        .push(SwitchTargetValue::Otherwise);
                 }
             }
             switch_sources

compiler/rustc_middle/src/mir/mod.rs

@@ -7,7 +7,7 @@ use std::fmt::{self, Debug, Formatter};
 use std::ops::{Index, IndexMut};
 use std::{iter, mem};
 
-pub use basic_blocks::BasicBlocks;
+pub use basic_blocks::{BasicBlocks, SwitchTargetValue};
 use either::Either;
 use polonius_engine::Atom;
 use rustc_abi::{FieldIdx, VariantIdx};

compiler/rustc_middle/src/mir/syntax.rs

@@ -1015,22 +1015,30 @@ impl TerminatorKind<'_> {
 
 #[derive(Debug, Clone, TyEncodable, TyDecodable, Hash, HashStable, PartialEq)]
 pub struct SwitchTargets {
-    /// Possible values. The locations to branch to in each case
-    /// are found in the corresponding indices from the `targets` vector.
+    /// Possible values. For each value, the location to branch to is found in
+    /// the corresponding element in 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 targets. The last element of this vector is used for
+    /// the "otherwise" branch, so `targets.len() == values.len() + 1` always
+    /// holds.
     //
-    // This invariant is quite non-obvious and also could be improved.
-    // One way to make this invariant is to have something like this instead:
+    // Note: This invariant is non-obvious and easy to violate. This would be a
+    // more rigorous representation:
     //
-    // branches: Vec<(ConstInt, BasicBlock)>,
-    // otherwise: Option<BasicBlock> // exhaustive if None
+    //   normal: SmallVec<[(Pu128, BasicBlock); 1]>,
+    //   otherwise: BasicBlock,
     //
-    // 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.
+    // But it's important to have the targets in a sliceable type, because
+    // target slices show up elsewhere. E.g. `TerminatorKind::InlineAsm` has a
+    // boxed slice, and `TerminatorKind::FalseEdge` has a single target that
+    // can be converted to a slice with `slice::from_ref`.
+    //
+    // Why does this matter? In functions like `TerminatorKind::successors` we
+    // return `impl Iterator` and a non-slice-of-targets representation here
+    // causes problems because multiple different concrete iterator types would
+    // be involved and we would need a boxed trait object, which requires an
+    // allocation, which is expensive if done frequently.
     pub(super) targets: SmallVec<[BasicBlock; 2]>,
 }
 

compiler/rustc_mir_dataflow/src/framework/direction.rs

@@ -1,9 +1,11 @@
 use std::ops::RangeInclusive;
 
-use rustc_middle::mir::{self, BasicBlock, CallReturnPlaces, Location, TerminatorEdges};
+use rustc_middle::mir::{
+    self, BasicBlock, CallReturnPlaces, Location, SwitchTargetValue, TerminatorEdges,
+};
 
 use super::visitor::ResultsVisitor;
-use super::{Analysis, Effect, EffectIndex, Results, SwitchIntTarget};
+use super::{Analysis, Effect, EffectIndex, Results};
 
 pub trait Direction {
     const IS_FORWARD: bool;
@@ -112,14 +114,10 @@ impl Direction for Backward {
 
                 mir::TerminatorKind::SwitchInt { targets: _, ref discr } => {
                     if let Some(mut data) = analysis.get_switch_int_data(block, discr) {
-                        let values = &body.basic_blocks.switch_sources()[&(block, pred)];
-                        let targets =
-                            values.iter().map(|&value| SwitchIntTarget { value, target: block });
-
                         let mut tmp = analysis.bottom_value(body);
-                        for target in targets {
-                            tmp.clone_from(&exit_state);
-                            analysis.apply_switch_int_edge_effect(&mut data, &mut tmp, target);
+                        for &value in &body.basic_blocks.switch_sources()[&(block, pred)] {
+                            tmp.clone_from(exit_state);
+                            analysis.apply_switch_int_edge_effect(&mut data, &mut tmp, value);
                             propagate(pred, &tmp);
                         }
                     } else {
@@ -292,12 +290,9 @@ impl Direction for Forward {
                 if let Some(mut data) = analysis.get_switch_int_data(block, discr) {
                     let mut tmp = analysis.bottom_value(body);
                     for (value, target) in targets.iter() {
-                        tmp.clone_from(&exit_state);
-                        analysis.apply_switch_int_edge_effect(
-                            &mut data,
-                            &mut tmp,
-                            SwitchIntTarget { value: Some(value), target },
-                        );
+                        tmp.clone_from(exit_state);
+                        let value = SwitchTargetValue::Normal(value);
+                        analysis.apply_switch_int_edge_effect(&mut data, &mut tmp, value);
                         propagate(target, &tmp);
                     }
 
@@ -308,7 +303,7 @@ impl Direction for Forward {
                     analysis.apply_switch_int_edge_effect(
                         &mut data,
                         exit_state,
-                        SwitchIntTarget { value: None, target: otherwise },
+                        SwitchTargetValue::Otherwise,
                     );
                     propagate(otherwise, exit_state);
                 } else {

compiler/rustc_mir_dataflow/src/framework/mod.rs

@@ -38,7 +38,9 @@ use rustc_data_structures::work_queue::WorkQueue;
 use rustc_index::bit_set::{DenseBitSet, MixedBitSet};
 use rustc_index::{Idx, IndexVec};
 use rustc_middle::bug;
-use rustc_middle::mir::{self, BasicBlock, CallReturnPlaces, Location, TerminatorEdges, traversal};
+use rustc_middle::mir::{
+    self, BasicBlock, CallReturnPlaces, Location, SwitchTargetValue, TerminatorEdges, traversal,
+};
 use rustc_middle::ty::TyCtxt;
 use tracing::error;
 
@@ -220,7 +222,7 @@ pub trait Analysis<'tcx> {
         &mut self,
         _data: &mut Self::SwitchIntData,
         _state: &mut Self::Domain,
-        _edge: SwitchIntTarget,
+        _value: SwitchTargetValue,
     ) {
         unreachable!();
     }
@@ -430,10 +432,5 @@ impl EffectIndex {
     }
 }
 
-pub struct SwitchIntTarget {
-    pub value: Option<u128>,
-    pub target: BasicBlock,
-}
-
 #[cfg(test)]
 mod tests;

compiler/rustc_mir_dataflow/src/impls/initialized.rs

@@ -4,13 +4,14 @@ use rustc_abi::VariantIdx;
 use rustc_index::Idx;
 use rustc_index::bit_set::{DenseBitSet, MixedBitSet};
 use rustc_middle::bug;
-use rustc_middle::mir::{self, Body, CallReturnPlaces, Location, TerminatorEdges};
+use rustc_middle::mir::{
+    self, Body, CallReturnPlaces, Location, SwitchTargetValue, TerminatorEdges,
+};
 use rustc_middle::ty::util::Discr;
 use rustc_middle::ty::{self, TyCtxt};
 use tracing::{debug, instrument};
 
 use crate::drop_flag_effects::DropFlagState;
-use crate::framework::SwitchIntTarget;
 use crate::move_paths::{HasMoveData, InitIndex, InitKind, LookupResult, MoveData, MovePathIndex};
 use crate::{
     Analysis, GenKill, MaybeReachable, drop_flag_effects, drop_flag_effects_for_function_entry,
@@ -422,9 +423,9 @@ impl<'tcx> Analysis<'tcx> for MaybeInitializedPlaces<'_, 'tcx> {
         &mut self,
         data: &mut Self::SwitchIntData,
         state: &mut Self::Domain,
-        edge: SwitchIntTarget,
+        value: SwitchTargetValue,
     ) {
-        if let Some(value) = edge.value {
+        if let SwitchTargetValue::Normal(value) = value {
             // Kill all move paths that correspond to variants we know to be inactive along this
             // particular outgoing edge of a `SwitchInt`.
             drop_flag_effects::on_all_inactive_variants(
@@ -535,9 +536,9 @@ impl<'tcx> Analysis<'tcx> for MaybeUninitializedPlaces<'_, 'tcx> {
         &mut self,
         data: &mut Self::SwitchIntData,
         state: &mut Self::Domain,
-        edge: SwitchIntTarget,
+        value: SwitchTargetValue,
     ) {
-        if let Some(value) = edge.value {
+        if let SwitchTargetValue::Normal(value) = value {
             // Mark all move paths that correspond to variants other than this one as maybe
             // uninitialized (in reality, they are *definitely* uninitialized).
             drop_flag_effects::on_all_inactive_variants(