From 13003100f803419307ada53620d00aa51ea678db Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 11 Feb 2025 16:33:17 +1100 Subject: [PATCH 1/5] Refactor `apply_effects_in_block`. Very minor changes that will make the next few commits easier to follow. --- .../src/framework/direction.rs | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/framework/direction.rs b/compiler/rustc_mir_dataflow/src/framework/direction.rs index 07517d7edab01..528debcaba345 100644 --- a/compiler/rustc_mir_dataflow/src/framework/direction.rs +++ b/compiler/rustc_mir_dataflow/src/framework/direction.rs @@ -112,14 +112,11 @@ 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); + let si_target = SwitchIntTarget { value, target: block }; + analysis.apply_switch_int_edge_effect(&mut data, &mut tmp, si_target); propagate(pred, &tmp); } } else { @@ -292,12 +289,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 si_target = SwitchIntTarget { value: Some(value), target }; + analysis.apply_switch_int_edge_effect(&mut data, &mut tmp, si_target); propagate(target, &tmp); } From 23dbff88f6ecf7b92129ec3cfc41971b5cd9579c Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 13 Feb 2025 10:46:34 +1100 Subject: [PATCH 2/5] Add a useful comment. --- compiler/rustc_middle/src/mir/basic_blocks.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_middle/src/mir/basic_blocks.rs b/compiler/rustc_middle/src/mir/basic_blocks.rs index c32cf5f825334..5c67083037609 100644 --- a/compiler/rustc_middle/src/mir/basic_blocks.rs +++ b/compiler/rustc_middle/src/mir/basic_blocks.rs @@ -20,6 +20,13 @@ pub struct BasicBlocks<'tcx> { // Typically 95%+ of basic blocks have 4 or fewer predecessors. type Predecessors = IndexVec>; +/// 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<[Option; 1]>>; #[derive(Clone, Default, Debug)] @@ -70,8 +77,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(|| { From 8403d39dce2b6875381974a3f413c0c610f2ca1a Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 14 Feb 2025 09:54:01 +1100 Subject: [PATCH 3/5] Add `SwitchTargetValue`. This is much clearer than `Option`. --- compiler/rustc_middle/src/mir/basic_blocks.rs | 20 ++++++++++++++++--- compiler/rustc_middle/src/mir/mod.rs | 2 +- .../src/framework/direction.rs | 9 ++++++--- .../rustc_mir_dataflow/src/framework/mod.rs | 6 ++++-- .../src/impls/initialized.rs | 8 +++++--- 5 files changed, 33 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_middle/src/mir/basic_blocks.rs b/compiler/rustc_middle/src/mir/basic_blocks.rs index 5c67083037609..107c3198525a1 100644 --- a/compiler/rustc_middle/src/mir/basic_blocks.rs +++ b/compiler/rustc_middle/src/mir/basic_blocks.rs @@ -27,7 +27,15 @@ type Predecessors = IndexVec>; /// `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<[Option; 1]>>; +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 { @@ -89,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 diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 795cfcef2d36d..cc22cabcb3e80 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/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}; diff --git a/compiler/rustc_mir_dataflow/src/framework/direction.rs b/compiler/rustc_mir_dataflow/src/framework/direction.rs index 528debcaba345..02b2bbb35076f 100644 --- a/compiler/rustc_mir_dataflow/src/framework/direction.rs +++ b/compiler/rustc_mir_dataflow/src/framework/direction.rs @@ -1,6 +1,8 @@ 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}; @@ -290,7 +292,8 @@ impl Direction for Forward { let mut tmp = analysis.bottom_value(body); for (value, target) in targets.iter() { tmp.clone_from(exit_state); - let si_target = SwitchIntTarget { value: Some(value), target }; + let si_target = + SwitchIntTarget { value: SwitchTargetValue::Normal(value), target }; analysis.apply_switch_int_edge_effect(&mut data, &mut tmp, si_target); propagate(target, &tmp); } @@ -302,7 +305,7 @@ impl Direction for Forward { analysis.apply_switch_int_edge_effect( &mut data, exit_state, - SwitchIntTarget { value: None, target: otherwise }, + SwitchIntTarget { value: SwitchTargetValue::Otherwise, target: otherwise }, ); propagate(otherwise, exit_state); } else { diff --git a/compiler/rustc_mir_dataflow/src/framework/mod.rs b/compiler/rustc_mir_dataflow/src/framework/mod.rs index 60c5cb0cae8ad..14630991cd6c8 100644 --- a/compiler/rustc_mir_dataflow/src/framework/mod.rs +++ b/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; @@ -431,7 +433,7 @@ impl EffectIndex { } pub struct SwitchIntTarget { - pub value: Option, + pub value: SwitchTargetValue, pub target: BasicBlock, } diff --git a/compiler/rustc_mir_dataflow/src/impls/initialized.rs b/compiler/rustc_mir_dataflow/src/impls/initialized.rs index 3be450a0b3f47..f2fbadaac09d5 100644 --- a/compiler/rustc_mir_dataflow/src/impls/initialized.rs +++ b/compiler/rustc_mir_dataflow/src/impls/initialized.rs @@ -4,7 +4,9 @@ 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}; @@ -424,7 +426,7 @@ impl<'tcx> Analysis<'tcx> for MaybeInitializedPlaces<'_, 'tcx> { state: &mut Self::Domain, edge: SwitchIntTarget, ) { - if let Some(value) = edge.value { + if let SwitchTargetValue::Normal(value) = edge.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( @@ -537,7 +539,7 @@ impl<'tcx> Analysis<'tcx> for MaybeUninitializedPlaces<'_, 'tcx> { state: &mut Self::Domain, edge: SwitchIntTarget, ) { - if let Some(value) = edge.value { + if let SwitchTargetValue::Normal(value) = edge.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( From db1ca60470d3d9bcb533ad1130e669dcd8c7ffd2 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 14 Feb 2025 10:57:06 +1100 Subject: [PATCH 4/5] Update and clarify the comment on `SwitchTargets`. --- compiler/rustc_middle/src/mir/syntax.rs | 30 ++++++++++++++++--------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs index 9cec8d832dd14..dfd40a9535ba1 100644 --- a/compiler/rustc_middle/src/mir/syntax.rs +++ b/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 // 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]>, } From 3b81d9d42d07d3e2c4f9f0c714fb16d2527758fe Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 14 Feb 2025 13:23:00 +1100 Subject: [PATCH 5/5] Remove `SwitchIntTarget`. It's only passed to `Analysis::apply_switch_int_edge_effect`, and the existing impls of that method only use the `value` field. So pass that instead. --- .../rustc_mir_dataflow/src/framework/direction.rs | 12 +++++------- compiler/rustc_mir_dataflow/src/framework/mod.rs | 7 +------ compiler/rustc_mir_dataflow/src/impls/initialized.rs | 9 ++++----- 3 files changed, 10 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/framework/direction.rs b/compiler/rustc_mir_dataflow/src/framework/direction.rs index 02b2bbb35076f..3d7f9e2d8e71b 100644 --- a/compiler/rustc_mir_dataflow/src/framework/direction.rs +++ b/compiler/rustc_mir_dataflow/src/framework/direction.rs @@ -5,7 +5,7 @@ use rustc_middle::mir::{ }; use super::visitor::ResultsVisitor; -use super::{Analysis, Effect, EffectIndex, Results, SwitchIntTarget}; +use super::{Analysis, Effect, EffectIndex, Results}; pub trait Direction { const IS_FORWARD: bool; @@ -117,8 +117,7 @@ impl Direction for Backward { let mut tmp = analysis.bottom_value(body); for &value in &body.basic_blocks.switch_sources()[&(block, pred)] { tmp.clone_from(exit_state); - let si_target = SwitchIntTarget { value, target: block }; - analysis.apply_switch_int_edge_effect(&mut data, &mut tmp, si_target); + analysis.apply_switch_int_edge_effect(&mut data, &mut tmp, value); propagate(pred, &tmp); } } else { @@ -292,9 +291,8 @@ impl Direction for Forward { let mut tmp = analysis.bottom_value(body); for (value, target) in targets.iter() { tmp.clone_from(exit_state); - let si_target = - SwitchIntTarget { value: SwitchTargetValue::Normal(value), target }; - analysis.apply_switch_int_edge_effect(&mut data, &mut tmp, si_target); + let value = SwitchTargetValue::Normal(value); + analysis.apply_switch_int_edge_effect(&mut data, &mut tmp, value); propagate(target, &tmp); } @@ -305,7 +303,7 @@ impl Direction for Forward { analysis.apply_switch_int_edge_effect( &mut data, exit_state, - SwitchIntTarget { value: SwitchTargetValue::Otherwise, target: otherwise }, + SwitchTargetValue::Otherwise, ); propagate(otherwise, exit_state); } else { diff --git a/compiler/rustc_mir_dataflow/src/framework/mod.rs b/compiler/rustc_mir_dataflow/src/framework/mod.rs index 14630991cd6c8..09f6cdb5c4a72 100644 --- a/compiler/rustc_mir_dataflow/src/framework/mod.rs +++ b/compiler/rustc_mir_dataflow/src/framework/mod.rs @@ -222,7 +222,7 @@ pub trait Analysis<'tcx> { &mut self, _data: &mut Self::SwitchIntData, _state: &mut Self::Domain, - _edge: SwitchIntTarget, + _value: SwitchTargetValue, ) { unreachable!(); } @@ -432,10 +432,5 @@ impl EffectIndex { } } -pub struct SwitchIntTarget { - pub value: SwitchTargetValue, - pub target: BasicBlock, -} - #[cfg(test)] mod tests; diff --git a/compiler/rustc_mir_dataflow/src/impls/initialized.rs b/compiler/rustc_mir_dataflow/src/impls/initialized.rs index f2fbadaac09d5..f5ffc42d52ab0 100644 --- a/compiler/rustc_mir_dataflow/src/impls/initialized.rs +++ b/compiler/rustc_mir_dataflow/src/impls/initialized.rs @@ -12,7 +12,6 @@ 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, @@ -424,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 SwitchTargetValue::Normal(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( @@ -537,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 SwitchTargetValue::Normal(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(