From 848610fddc127b8fa44c5c40d336bc10ee34e563 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 22 Nov 2024 06:20:53 +1100 Subject: [PATCH 1/5] Add a comment to `MaybeInitializedPlaces::apply_terminator_effect`. I tried reordering this method to more closely match `MaybeUninitializedPlaces::apply_terminator_effect`, but doing so breaks tests. --- compiler/rustc_mir_dataflow/src/impls/initialized.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compiler/rustc_mir_dataflow/src/impls/initialized.rs b/compiler/rustc_mir_dataflow/src/impls/initialized.rs index fb02408e17dfa..9fdbbb1578e4f 100644 --- a/compiler/rustc_mir_dataflow/src/impls/initialized.rs +++ b/compiler/rustc_mir_dataflow/src/impls/initialized.rs @@ -293,6 +293,8 @@ impl<'tcx> Analysis<'tcx> for MaybeInitializedPlaces<'_, 'tcx> { terminator: &'mir mir::Terminator<'tcx>, location: Location, ) -> TerminatorEdges<'mir, 'tcx> { + // Note: `edges` must be computed first because `drop_flag_effects_for_location` can change + // the result of `is_unwind_dead`. let mut edges = terminator.edges(); if self.skip_unreachable_unwind && let mir::TerminatorKind::Drop { target, unwind, place, replace: _ } = terminator.kind From 4d8316f4d40cc9fb431b9cab5825c32fac43a19a Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 21 Nov 2024 10:20:30 +1100 Subject: [PATCH 2/5] Simplify dataflow `SwitchInt` handling. Current `SwitchInt` handling has complicated control flow. - The dataflow engine calls `Analysis::apply_switch_int_edge_effects`, passing in an "applier" that impls `SwitchIntEdgeEffects`. - `apply_switch_int_edge_effects` possibly calls `apply` on the applier, passing it a closure. - The `apply` method calls the closure on each `SwitchInt` edge. - The closure operates on the edge. I.e. control flow goes from the engine, to the analysis, to the applier (which came from the engine), to the closure (which came from the analysis). It took me a while to work this out. This commit changes to a simpler structure that maintains the important characteristics. - The dataflow engine calls `Analysis::get_switch_int_data`. - `get_switch_int_data` returns an `Option` value. - If that returned value was `Some`, the dataflow engine calls `Analysis::apply_switch_int_edge_effect` on each edge, passing the `Self::SwitchIntData` value. - `Analysis::apply_switch_int_edge_effect` operates on the edge. I.e. control flow goes from the engine, to the analysis, to the engine, to the analysis. Added: - The `Analysis::SwitchIntData` assoc type and the `Analysis::get_switch_int_data` method. Both only need to be defined by analyses that look at `SwitchInt` terminators. - The `MaybePlacesSwitchIntData` struct, which has three fields. Changes: - `Analysis::apply_switch_int_edge_effects` becomes `Analysis::apply_switch_int_edge_effect`, which is a little simpler because it's dealing with a single edge instead of all edges. Removed: - The `SwitchIntEdgeEffects` trait, and its two impls: `BackwardSwitchIntEdgeEffectsApplier` (which has six fields) and `ForwardSwitchIntEdgeEffectsApplier` structs (which has four fields). - The closure. The new structure is more concise and simpler. --- compiler/rustc_borrowck/src/dataflow.rs | 12 +- .../src/framework/direction.rs | 130 +++++------------ .../rustc_mir_dataflow/src/framework/mod.rs | 39 +++-- .../src/impls/initialized.rs | 137 ++++++++++-------- compiler/rustc_mir_dataflow/src/lib.rs | 4 +- 5 files changed, 139 insertions(+), 183 deletions(-) diff --git a/compiler/rustc_borrowck/src/dataflow.rs b/compiler/rustc_borrowck/src/dataflow.rs index dc4eab766c932..abe4d4f20ecc5 100644 --- a/compiler/rustc_borrowck/src/dataflow.rs +++ b/compiler/rustc_borrowck/src/dataflow.rs @@ -12,7 +12,7 @@ use rustc_mir_dataflow::impls::{ EverInitializedPlaces, EverInitializedPlacesDomain, MaybeUninitializedPlaces, MaybeUninitializedPlacesDomain, }; -use rustc_mir_dataflow::{Analysis, GenKill, JoinSemiLattice, SwitchIntEdgeEffects}; +use rustc_mir_dataflow::{Analysis, GenKill, JoinSemiLattice}; use tracing::debug; use crate::{BorrowSet, PlaceConflictBias, PlaceExt, RegionInferenceContext, places_conflict}; @@ -101,16 +101,6 @@ impl<'a, 'tcx> Analysis<'tcx> for Borrowck<'a, 'tcx> { // This is only reachable from `iterate_to_fixpoint`, which this analysis doesn't use. unreachable!(); } - - fn apply_switch_int_edge_effects( - &mut self, - _block: BasicBlock, - _discr: &mir::Operand<'tcx>, - _apply_edge_effects: &mut impl SwitchIntEdgeEffects, - ) { - // This is only reachable from `iterate_to_fixpoint`, which this analysis doesn't use. - unreachable!(); - } } impl JoinSemiLattice for BorrowckDomain { diff --git a/compiler/rustc_mir_dataflow/src/framework/direction.rs b/compiler/rustc_mir_dataflow/src/framework/direction.rs index 9d943ebe327bc..45b1023e79529 100644 --- a/compiler/rustc_mir_dataflow/src/framework/direction.rs +++ b/compiler/rustc_mir_dataflow/src/framework/direction.rs @@ -1,8 +1,6 @@ use std::ops::RangeInclusive; -use rustc_middle::mir::{ - self, BasicBlock, CallReturnPlaces, Location, SwitchTargets, TerminatorEdges, -}; +use rustc_middle::mir::{self, BasicBlock, CallReturnPlaces, Location, TerminatorEdges}; use super::visitor::ResultsVisitor; use super::{Analysis, Effect, EffectIndex, Results, SwitchIntTarget}; @@ -115,18 +113,18 @@ impl Direction for Backward { } mir::TerminatorKind::SwitchInt { targets: _, ref discr } => { - let mut applier = BackwardSwitchIntEdgeEffectsApplier { - body, - pred, - exit_state, - block, - propagate: &mut propagate, - effects_applied: false, - }; - - analysis.apply_switch_int_edge_effects(pred, discr, &mut applier); - - if !applier.effects_applied { + 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 = None; + for target in targets { + let tmp = opt_clone_from_or_clone(&mut tmp, exit_state); + analysis.apply_switch_int_edge_effect(&mut data, tmp, target); + propagate(pred, tmp); + } + } else { propagate(pred, exit_state) } } @@ -245,37 +243,6 @@ impl Direction for Backward { } } -struct BackwardSwitchIntEdgeEffectsApplier<'mir, 'tcx, D, F> { - body: &'mir mir::Body<'tcx>, - pred: BasicBlock, - exit_state: &'mir mut D, - block: BasicBlock, - propagate: &'mir mut F, - effects_applied: bool, -} - -impl super::SwitchIntEdgeEffects for BackwardSwitchIntEdgeEffectsApplier<'_, '_, D, F> -where - D: Clone, - F: FnMut(BasicBlock, &D), -{ - fn apply(&mut self, mut apply_edge_effect: impl FnMut(&mut D, SwitchIntTarget)) { - assert!(!self.effects_applied); - - let values = &self.body.basic_blocks.switch_sources()[&(self.block, self.pred)]; - let targets = values.iter().map(|&value| SwitchIntTarget { value, target: self.block }); - - let mut tmp = None; - for target in targets { - let tmp = opt_clone_from_or_clone(&mut tmp, self.exit_state); - apply_edge_effect(tmp, target); - (self.propagate)(self.pred, tmp); - } - - self.effects_applied = true; - } -} - /// Dataflow that runs from the entry of a block (the first statement), to its exit (terminator). pub struct Forward; @@ -324,23 +291,27 @@ impl Direction for Forward { } } TerminatorEdges::SwitchInt { targets, discr } => { - let mut applier = ForwardSwitchIntEdgeEffectsApplier { - exit_state, - targets, - propagate, - effects_applied: false, - }; - - analysis.apply_switch_int_edge_effects(block, discr, &mut applier); - - let ForwardSwitchIntEdgeEffectsApplier { - exit_state, - mut propagate, - effects_applied, - .. - } = applier; - - if !effects_applied { + if let Some(mut data) = analysis.get_switch_int_data(block, discr) { + let mut tmp = None; + for (value, target) in targets.iter() { + let tmp = opt_clone_from_or_clone(&mut tmp, exit_state); + analysis.apply_switch_int_edge_effect(&mut data, tmp, SwitchIntTarget { + value: Some(value), + target, + }); + propagate(target, tmp); + } + + // Once we get to the final, "otherwise" branch, there is no need to preserve + // `exit_state`, so pass it directly to `apply_switch_int_edge_effect` to save + // a clone of the dataflow state. + let otherwise = targets.otherwise(); + analysis.apply_switch_int_edge_effect(&mut data, exit_state, SwitchIntTarget { + value: None, + target: otherwise, + }); + propagate(otherwise, exit_state); + } else { for target in targets.all_targets() { propagate(*target, exit_state); } @@ -455,39 +426,6 @@ impl Direction for Forward { } } -struct ForwardSwitchIntEdgeEffectsApplier<'mir, D, F> { - exit_state: &'mir mut D, - targets: &'mir SwitchTargets, - propagate: F, - - effects_applied: bool, -} - -impl super::SwitchIntEdgeEffects for ForwardSwitchIntEdgeEffectsApplier<'_, D, F> -where - D: Clone, - F: FnMut(BasicBlock, &D), -{ - fn apply(&mut self, mut apply_edge_effect: impl FnMut(&mut D, SwitchIntTarget)) { - assert!(!self.effects_applied); - - let mut tmp = None; - for (value, target) in self.targets.iter() { - let tmp = opt_clone_from_or_clone(&mut tmp, self.exit_state); - apply_edge_effect(tmp, SwitchIntTarget { value: Some(value), target }); - (self.propagate)(target, tmp); - } - - // Once we get to the final, "otherwise" branch, there is no need to preserve `exit_state`, - // so pass it directly to `apply_edge_effect` to save a clone of the dataflow state. - let otherwise = self.targets.otherwise(); - apply_edge_effect(self.exit_state, SwitchIntTarget { value: None, target: otherwise }); - (self.propagate)(otherwise, self.exit_state); - - self.effects_applied = true; - } -} - /// An analogue of `Option::get_or_insert_with` that stores a clone of `val` into `opt`, but uses /// the more efficient `clone_from` if `opt` was `Some`. /// diff --git a/compiler/rustc_mir_dataflow/src/framework/mod.rs b/compiler/rustc_mir_dataflow/src/framework/mod.rs index 41df5fae0deaa..3de2c6e3f47c1 100644 --- a/compiler/rustc_mir_dataflow/src/framework/mod.rs +++ b/compiler/rustc_mir_dataflow/src/framework/mod.rs @@ -103,6 +103,9 @@ pub trait Analysis<'tcx> { /// The direction of this analysis. Either `Forward` or `Backward`. type Direction: Direction = Forward; + /// Auxiliary data used for analyzing `SwitchInt` terminators, if necessary. + type SwitchIntData = !; + /// A descriptive name for this analysis. Used only for debugging. /// /// This name should be brief and contain no spaces, periods or other characters that are not @@ -190,25 +193,36 @@ pub trait Analysis<'tcx> { ) { } - /// Updates the current dataflow state with the effect of taking a particular branch in a - /// `SwitchInt` terminator. + /// Used to update the current dataflow state with the effect of taking a particular branch in + /// a `SwitchInt` terminator. /// /// Unlike the other edge-specific effects, which are allowed to mutate `Self::Domain` - /// directly, overriders of this method must pass a callback to - /// `SwitchIntEdgeEffects::apply`. The callback will be run once for each outgoing edge and - /// will have access to the dataflow state that will be propagated along that edge. + /// directly, overriders of this method must return a `Self::SwitchIntData` value (wrapped in + /// `Some`). The `apply_switch_int_edge_effect` method will then be called once for each + /// outgoing edge and will have access to the dataflow state that will be propagated along that + /// edge, and also the `Self::SwitchIntData` value. /// /// This interface is somewhat more complex than the other visitor-like "effect" methods. /// However, it is both more ergonomic—callers don't need to recompute or cache information /// about a given `SwitchInt` terminator for each one of its edges—and more efficient—the /// engine doesn't need to clone the exit state for a block unless - /// `SwitchIntEdgeEffects::apply` is actually called. - fn apply_switch_int_edge_effects( + /// `get_switch_int_data` is actually called. + fn get_switch_int_data( &mut self, - _block: BasicBlock, + _block: mir::BasicBlock, _discr: &mir::Operand<'tcx>, - _apply_edge_effects: &mut impl SwitchIntEdgeEffects, + ) -> Option { + None + } + + /// See comments on `get_switch_int_data`. + fn apply_switch_int_edge_effect( + &mut self, + _data: &mut Self::SwitchIntData, + _state: &mut Self::Domain, + _edge: SwitchIntTarget, ) { + unreachable!(); } /* Extension methods */ @@ -421,12 +435,5 @@ pub struct SwitchIntTarget { pub target: BasicBlock, } -/// A type that records the edge-specific effects for a `SwitchInt` terminator. -pub trait SwitchIntEdgeEffects { - /// Calls `apply_edge_effect` for each outgoing edge from a `SwitchInt` terminator and - /// records the results. - fn apply(&mut self, apply_edge_effect: impl FnMut(&mut D, SwitchIntTarget)); -} - #[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 9fdbbb1578e4f..d5982b9cba2e1 100644 --- a/compiler/rustc_mir_dataflow/src/impls/initialized.rs +++ b/compiler/rustc_mir_dataflow/src/impls/initialized.rs @@ -1,20 +1,45 @@ use std::assert_matches::assert_matches; +use rustc_abi::VariantIdx; use rustc_index::Idx; use rustc_index::bit_set::{BitSet, MixedBitSet}; use rustc_middle::bug; use rustc_middle::mir::{self, Body, CallReturnPlaces, Location, TerminatorEdges}; +use rustc_middle::ty::util::Discr; use rustc_middle::ty::{self, TyCtxt}; use tracing::{debug, instrument}; use crate::elaborate_drops::DropFlagState; -use crate::framework::SwitchIntEdgeEffects; +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, drop_flag_effects_for_location, on_all_children_bits, on_lookup_result_bits, }; +// Used by both `MaybeInitializedPlaces` and `MaybeUninitializedPlaces`. +pub struct MaybePlacesSwitchIntData<'tcx> { + enum_place: mir::Place<'tcx>, + discriminants: Vec<(VariantIdx, Discr<'tcx>)>, + index: usize, +} + +impl<'tcx> MaybePlacesSwitchIntData<'tcx> { + // The discriminant order in the `SwitchInt` targets should match the order yielded by + // `AdtDef::discriminants`. We rely on this to match each discriminant in the targets to its + // corresponding variant in linear time. + fn next_discr(&mut self, value: u128) -> VariantIdx { + // An out-of-bounds abort will occur if the discriminant ordering isn't as described above. + loop { + let (variant, discr) = self.discriminants[self.index]; + self.index += 1; + if discr.val == value { + return variant; + } + } + } +} + /// `MaybeInitializedPlaces` tracks all places that might be /// initialized upon reaching a particular point in the control flow /// for a function. @@ -247,6 +272,8 @@ impl<'tcx> Analysis<'tcx> for MaybeInitializedPlaces<'_, 'tcx> { /// We use a mixed bitset to avoid paying too high a memory footprint. type Domain = MaybeReachable>; + type SwitchIntData = MaybePlacesSwitchIntData<'tcx>; + const NAME: &'static str = "maybe_init"; fn bottom_value(&self, _: &mir::Body<'tcx>) -> Self::Domain { @@ -328,46 +355,42 @@ impl<'tcx> Analysis<'tcx> for MaybeInitializedPlaces<'_, 'tcx> { }); } - fn apply_switch_int_edge_effects( + fn get_switch_int_data( &mut self, block: mir::BasicBlock, discr: &mir::Operand<'tcx>, - edge_effects: &mut impl SwitchIntEdgeEffects, - ) { + ) -> Option { if !self.tcx.sess.opts.unstable_opts.precise_enum_drop_elaboration { - return; + return None; } - let enum_ = discr.place().and_then(|discr| { - switch_on_enum_discriminant(self.tcx, self.body, &self.body[block], discr) - }); - - let Some((enum_place, enum_def)) = enum_ else { - return; - }; - - let mut discriminants = enum_def.discriminants(self.tcx); - edge_effects.apply(|state, edge| { - let Some(value) = edge.value else { - return; - }; - - // MIR building adds discriminants to the `values` array in the same order as they - // are yielded by `AdtDef::discriminants`. We rely on this to match each - // discriminant in `values` to its corresponding variant in linear time. - let (variant, _) = discriminants - .find(|&(_, discr)| discr.val == value) - .expect("Order of `AdtDef::discriminants` differed from `SwitchInt::values`"); + discr.place().and_then(|discr| { + switch_on_enum_discriminant(self.tcx, self.body, &self.body[block], discr).map( + |(enum_place, enum_def)| MaybePlacesSwitchIntData { + enum_place, + discriminants: enum_def.discriminants(self.tcx).collect(), + index: 0, + }, + ) + }) + } + fn apply_switch_int_edge_effect( + &mut self, + data: &mut Self::SwitchIntData, + state: &mut Self::Domain, + edge: SwitchIntTarget, + ) { + if let Some(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( - self.move_data(), - enum_place, - variant, + self.move_data, + data.enum_place, + data.next_discr(value), |mpi| state.kill(mpi), ); - }); + } } } @@ -378,6 +401,8 @@ pub type MaybeUninitializedPlacesDomain = MixedBitSet; impl<'tcx> Analysis<'tcx> for MaybeUninitializedPlaces<'_, 'tcx> { type Domain = MaybeUninitializedPlacesDomain; + type SwitchIntData = MaybePlacesSwitchIntData<'tcx>; + const NAME: &'static str = "maybe_uninit"; fn bottom_value(&self, _: &mir::Body<'tcx>) -> Self::Domain { @@ -447,50 +472,46 @@ impl<'tcx> Analysis<'tcx> for MaybeUninitializedPlaces<'_, 'tcx> { }); } - fn apply_switch_int_edge_effects( + fn get_switch_int_data( &mut self, block: mir::BasicBlock, discr: &mir::Operand<'tcx>, - edge_effects: &mut impl SwitchIntEdgeEffects, - ) { + ) -> Option { if !self.tcx.sess.opts.unstable_opts.precise_enum_drop_elaboration { - return; + return None; } if !self.mark_inactive_variants_as_uninit { - return; + return None; } - let enum_ = discr.place().and_then(|discr| { - switch_on_enum_discriminant(self.tcx, self.body, &self.body[block], discr) - }); - - let Some((enum_place, enum_def)) = enum_ else { - return; - }; - - let mut discriminants = enum_def.discriminants(self.tcx); - edge_effects.apply(|state, edge| { - let Some(value) = edge.value else { - return; - }; - - // MIR building adds discriminants to the `values` array in the same order as they - // are yielded by `AdtDef::discriminants`. We rely on this to match each - // discriminant in `values` to its corresponding variant in linear time. - let (variant, _) = discriminants - .find(|&(_, discr)| discr.val == value) - .expect("Order of `AdtDef::discriminants` differed from `SwitchInt::values`"); + discr.place().and_then(|discr| { + switch_on_enum_discriminant(self.tcx, self.body, &self.body[block], discr).map( + |(enum_place, enum_def)| MaybePlacesSwitchIntData { + enum_place, + discriminants: enum_def.discriminants(self.tcx).collect(), + index: 0, + }, + ) + }) + } + fn apply_switch_int_edge_effect( + &mut self, + data: &mut Self::SwitchIntData, + state: &mut Self::Domain, + edge: SwitchIntTarget, + ) { + if let Some(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( - self.move_data(), - enum_place, - variant, + self.move_data, + data.enum_place, + data.next_discr(value), |mpi| state.gen_(mpi), ); - }); + } } } diff --git a/compiler/rustc_mir_dataflow/src/lib.rs b/compiler/rustc_mir_dataflow/src/lib.rs index 85255db5d9a61..0cc79b0c93903 100644 --- a/compiler/rustc_mir_dataflow/src/lib.rs +++ b/compiler/rustc_mir_dataflow/src/lib.rs @@ -5,6 +5,7 @@ #![feature(exact_size_is_empty)] #![feature(file_buffered)] #![feature(let_chains)] +#![feature(never_type)] #![feature(try_blocks)] #![warn(unreachable_pub)] // tidy-alphabetical-end @@ -19,8 +20,7 @@ pub use self::drop_flag_effects::{ }; pub use self::framework::{ Analysis, Backward, Direction, EntryStates, Forward, GenKill, JoinSemiLattice, MaybeReachable, - Results, ResultsCursor, ResultsVisitor, SwitchIntEdgeEffects, fmt, graphviz, lattice, - visit_results, + Results, ResultsCursor, ResultsVisitor, fmt, graphviz, lattice, visit_results, }; use self::move_paths::MoveData; From c8c50f4351e629da2dde6bb6b23f4a2f93bd84b8 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 22 Nov 2024 08:39:34 +1100 Subject: [PATCH 3/5] Factor out some shared code from `Maybe{In,Unin}itializedPlaces`. --- .../src/impls/initialized.rs | 113 ++++++++---------- 1 file changed, 53 insertions(+), 60 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/initialized.rs b/compiler/rustc_mir_dataflow/src/impls/initialized.rs index d5982b9cba2e1..769f9c7cfc3eb 100644 --- a/compiler/rustc_mir_dataflow/src/impls/initialized.rs +++ b/compiler/rustc_mir_dataflow/src/impls/initialized.rs @@ -40,6 +40,57 @@ impl<'tcx> MaybePlacesSwitchIntData<'tcx> { } } +impl<'tcx> MaybePlacesSwitchIntData<'tcx> { + fn new( + tcx: TyCtxt<'tcx>, + body: &Body<'tcx>, + block: mir::BasicBlock, + discr: &mir::Operand<'tcx>, + ) -> Option { + let Some(discr) = discr.place() else { return None }; + + // Inspect a `SwitchInt`-terminated basic block to see if the condition of that `SwitchInt` + // is an enum discriminant. + // + // We expect such blocks to have a call to `discriminant` as their last statement like so: + // ```text + // ... + // _42 = discriminant(_1) + // SwitchInt(_42, ..) + // ``` + // If the basic block matches this pattern, this function gathers the place corresponding + // to the enum (`_1` in the example above) as well as the discriminants. + let block_data = &body[block]; + for statement in block_data.statements.iter().rev() { + match statement.kind { + mir::StatementKind::Assign(box (lhs, mir::Rvalue::Discriminant(enum_place))) + if lhs == discr => + { + match enum_place.ty(body, tcx).ty.kind() { + ty::Adt(enum_def, _) => { + return Some(MaybePlacesSwitchIntData { + enum_place, + discriminants: enum_def.discriminants(tcx).collect(), + index: 0, + }); + } + + // `Rvalue::Discriminant` is also used to get the active yield point for a + // coroutine, but we do not need edge-specific effects in that case. This + // may change in the future. + ty::Coroutine(..) => break, + + t => bug!("`discriminant` called on unexpected type {:?}", t), + } + } + mir::StatementKind::Coverage(_) => continue, + _ => break, + } + } + None + } +} + /// `MaybeInitializedPlaces` tracks all places that might be /// initialized upon reaching a particular point in the control flow /// for a function. @@ -364,15 +415,7 @@ impl<'tcx> Analysis<'tcx> for MaybeInitializedPlaces<'_, 'tcx> { return None; } - discr.place().and_then(|discr| { - switch_on_enum_discriminant(self.tcx, self.body, &self.body[block], discr).map( - |(enum_place, enum_def)| MaybePlacesSwitchIntData { - enum_place, - discriminants: enum_def.discriminants(self.tcx).collect(), - index: 0, - }, - ) - }) + MaybePlacesSwitchIntData::new(self.tcx, self.body, block, discr) } fn apply_switch_int_edge_effect( @@ -485,15 +528,7 @@ impl<'tcx> Analysis<'tcx> for MaybeUninitializedPlaces<'_, 'tcx> { return None; } - discr.place().and_then(|discr| { - switch_on_enum_discriminant(self.tcx, self.body, &self.body[block], discr).map( - |(enum_place, enum_def)| MaybePlacesSwitchIntData { - enum_place, - discriminants: enum_def.discriminants(self.tcx).collect(), - index: 0, - }, - ) - }) + MaybePlacesSwitchIntData::new(self.tcx, self.body, block, discr) } fn apply_switch_int_edge_effect( @@ -601,45 +636,3 @@ impl<'tcx> Analysis<'tcx> for EverInitializedPlaces<'_, 'tcx> { } } } - -/// Inspect a `SwitchInt`-terminated basic block to see if the condition of that `SwitchInt` is -/// an enum discriminant. -/// -/// We expect such blocks to have a call to `discriminant` as their last statement like so: -/// -/// ```text -/// ... -/// _42 = discriminant(_1) -/// SwitchInt(_42, ..) -/// ``` -/// -/// If the basic block matches this pattern, this function returns the place corresponding to the -/// enum (`_1` in the example above) as well as the `AdtDef` of that enum. -fn switch_on_enum_discriminant<'mir, 'tcx>( - tcx: TyCtxt<'tcx>, - body: &'mir mir::Body<'tcx>, - block: &'mir mir::BasicBlockData<'tcx>, - switch_on: mir::Place<'tcx>, -) -> Option<(mir::Place<'tcx>, ty::AdtDef<'tcx>)> { - for statement in block.statements.iter().rev() { - match &statement.kind { - mir::StatementKind::Assign(box (lhs, mir::Rvalue::Discriminant(discriminated))) - if *lhs == switch_on => - { - match discriminated.ty(body, tcx).ty.kind() { - ty::Adt(def, _) => return Some((*discriminated, *def)), - - // `Rvalue::Discriminant` is also used to get the active yield point for a - // coroutine, but we do not need edge-specific effects in that case. This may - // change in the future. - ty::Coroutine(..) => return None, - - t => bug!("`discriminant` called on unexpected type {:?}", t), - } - } - mir::StatementKind::Coverage(_) => continue, - _ => return None, - } - } - None -} From 83f3d204cf396502a15b3edf50b561b47c9f0884 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 6 Dec 2024 15:47:03 +1100 Subject: [PATCH 4/5] Remove `opt_clone_from_or_clone`. Switches to the idiom used elsewhere of calling `Analysis::bottom_value` to initialize a `state` value outside a loop, and then using `clone_from` to update it within the loop. This is simpler and has no impact on performance. --- .../src/framework/direction.rs | 43 ++++++------------- 1 file changed, 13 insertions(+), 30 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/framework/direction.rs b/compiler/rustc_mir_dataflow/src/framework/direction.rs index 45b1023e79529..f51d2a3da3a71 100644 --- a/compiler/rustc_mir_dataflow/src/framework/direction.rs +++ b/compiler/rustc_mir_dataflow/src/framework/direction.rs @@ -118,11 +118,11 @@ impl Direction for Backward { let targets = values.iter().map(|&value| SwitchIntTarget { value, target: block }); - let mut tmp = None; + let mut tmp = analysis.bottom_value(body); for target in targets { - let tmp = opt_clone_from_or_clone(&mut tmp, exit_state); - analysis.apply_switch_int_edge_effect(&mut data, tmp, target); - propagate(pred, tmp); + tmp.clone_from(&exit_state); + analysis.apply_switch_int_edge_effect(&mut data, &mut tmp, target); + propagate(pred, &tmp); } } else { propagate(pred, exit_state) @@ -251,7 +251,7 @@ impl Direction for Forward { fn apply_effects_in_block<'mir, 'tcx, A>( analysis: &mut A, - _body: &mir::Body<'tcx>, + body: &mir::Body<'tcx>, state: &mut A::Domain, block: BasicBlock, block_data: &'mir mir::BasicBlockData<'tcx>, @@ -292,14 +292,15 @@ impl Direction for Forward { } TerminatorEdges::SwitchInt { targets, discr } => { if let Some(mut data) = analysis.get_switch_int_data(block, discr) { - let mut tmp = None; + let mut tmp = analysis.bottom_value(body); for (value, target) in targets.iter() { - let tmp = opt_clone_from_or_clone(&mut tmp, exit_state); - analysis.apply_switch_int_edge_effect(&mut data, tmp, SwitchIntTarget { - value: Some(value), - target, - }); - propagate(target, tmp); + tmp.clone_from(&exit_state); + analysis.apply_switch_int_edge_effect( + &mut data, + &mut tmp, + SwitchIntTarget { value: Some(value), target }, + ); + propagate(target, &tmp); } // Once we get to the final, "otherwise" branch, there is no need to preserve @@ -425,21 +426,3 @@ impl Direction for Forward { vis.visit_block_end(state); } } - -/// An analogue of `Option::get_or_insert_with` that stores a clone of `val` into `opt`, but uses -/// the more efficient `clone_from` if `opt` was `Some`. -/// -/// Returns a mutable reference to the new clone that resides in `opt`. -// -// FIXME: Figure out how to express this using `Option::clone_from`, or maybe lift it into the -// standard library? -fn opt_clone_from_or_clone<'a, T: Clone>(opt: &'a mut Option, val: &T) -> &'a mut T { - if opt.is_some() { - let ret = opt.as_mut().unwrap(); - ret.clone_from(val); - ret - } else { - *opt = Some(val.clone()); - opt.as_mut().unwrap() - } -} From 5f40942f9c462576b4b7976e0a4c855ee350a9f9 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 6 Dec 2024 17:36:08 +1100 Subject: [PATCH 5/5] Remove a `FIXME` comment. It is possible to avoid the clone as suggested in the comment. It would require introducing an enum with two variants `CloneBeforeModifying(&Domain)` and `Modifiable(&mut Domain)`. But it's not worth the effort, because this code path just isn't very hot. E.g. when compiling a large benchmark like `cargo-0.60.0` it's only hit a few thousand times. --- compiler/rustc_mir_dataflow/src/framework/direction.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/framework/direction.rs b/compiler/rustc_mir_dataflow/src/framework/direction.rs index f51d2a3da3a71..6427fd0fd295b 100644 --- a/compiler/rustc_mir_dataflow/src/framework/direction.rs +++ b/compiler/rustc_mir_dataflow/src/framework/direction.rs @@ -76,8 +76,6 @@ impl Direction for Backward { for pred in body.basic_blocks.predecessors()[block].iter().copied() { match body[pred].terminator().kind { // Apply terminator-specific edge effects. - // - // FIXME(ecstaticmorse): Avoid cloning the exit state unconditionally. mir::TerminatorKind::Call { destination, target: Some(dest), .. } if dest == block => {