From 274a49132b7728fa7254fa4b5bd0575bdffa8b56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jannis=20Christopher=20K=C3=B6hl?= Date: Wed, 19 Oct 2022 15:56:58 +0200 Subject: [PATCH] Improve documentation, plus some small changes --- .../rustc_mir_dataflow/src/value_analysis.rs | 236 +++++++++++------- .../src/dataflow_const_prop.rs | 24 +- .../checked.main.DataflowConstProp.diff | 3 +- ...implification.hello.DataflowConstProp.diff | 3 +- 4 files changed, 160 insertions(+), 106 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/value_analysis.rs b/compiler/rustc_mir_dataflow/src/value_analysis.rs index 116a8b7bd408..4f092d36103f 100644 --- a/compiler/rustc_mir_dataflow/src/value_analysis.rs +++ b/compiler/rustc_mir_dataflow/src/value_analysis.rs @@ -192,13 +192,27 @@ pub trait ValueAnalysis<'tcx> { .map(ValueOrPlaceOrRef::Ref) .unwrap_or(ValueOrPlaceOrRef::top()), Rvalue::Ref(_, _, place) | Rvalue::AddressOf(_, place) => { + // This is not a `&x` reference and could be used for modification. state.flood(place.as_ref(), self.map()); ValueOrPlaceOrRef::top() } Rvalue::CopyForDeref(place) => { self.handle_operand(&Operand::Copy(*place), state).into() } - _ => ValueOrPlaceOrRef::top(), + Rvalue::Repeat(..) + | Rvalue::ThreadLocalRef(..) + | Rvalue::Len(..) + | Rvalue::Cast(..) + | Rvalue::BinaryOp(..) + | Rvalue::CheckedBinaryOp(..) + | Rvalue::NullaryOp(..) + | Rvalue::UnaryOp(..) + | Rvalue::Discriminant(..) + | Rvalue::Aggregate(..) + | Rvalue::ShallowInitBox(..) => { + // No modification is possible through these r-values. + ValueOrPlaceOrRef::top() + } } } @@ -419,7 +433,7 @@ enum StateData { #[derive(PartialEq, Eq, Clone, Debug)] pub struct State(StateData); -impl State { +impl State { pub fn is_reachable(&self) -> bool { matches!(&self.0, StateData::Reachable(_)) } @@ -460,6 +474,10 @@ impl State { self.flood_idx_with(place, map, V::top()) } + /// Copies `source` to `target`, including all tracked places beneath. + /// + /// If `target` contains a place that is not contained in `source`, it will be overwritten with + /// Top. Also, because this will copy all entries one after another, it may only be pub fn assign_place_idx(&mut self, target: PlaceIndex, source: PlaceIndex, map: &Map) { // We use (A3) and copy all entries one after another. let StateData::Reachable(values) = &mut self.0 else { return }; @@ -514,7 +532,7 @@ impl State { // track *what* it points to (as in, what do we know about the target). For an // assignment `x = &y`, we thus copy the info we have for `y` to `*x`. This is // sound because we only track places that are `Freeze`, and (A4). - if let Some(target_deref) = map.apply_elem(target, ProjElem::Deref) { + if let Some(target_deref) = map.apply(target, TrackElem::Deref) { self.assign_place_idx(target_deref, source, map); } } @@ -530,7 +548,10 @@ impl State { StateData::Reachable(values) => { map.places[place].value_index.map(|v| values[v].clone()).unwrap_or(V::top()) } - StateData::Unreachable => V::top(), + StateData::Unreachable => { + // Because this is unreachable, we can return any value we want. + V::bottom() + } } } } @@ -548,10 +569,15 @@ impl JoinSemiLattice for State { } } +/// A partial mapping from `Place` to `PlaceIndex`. +/// +/// Some additioanl bookkeeping is done to speed up traversal: +/// - For iteration, every [`PlaceInfo`] contains an intrusive linked list of its children. +/// - To directly get the child for a specific projection, there is `projections` map. #[derive(Debug)] pub struct Map { locals: IndexVec>, - projections: FxHashMap<(PlaceIndex, ProjElem), PlaceIndex>, + projections: FxHashMap<(PlaceIndex, TrackElem), PlaceIndex>, places: IndexVec, value_count: usize, } @@ -566,7 +592,10 @@ impl Map { } } - /// Register all suitable places with matching types (up to a certain depth). + /// Returns a map that only tracks places whose type passes the filter. + /// + /// This is currently the only way to create a [`Map`]. The way in which the tracked places are + /// chosen is an implementation detail an may not be relied upon. pub fn from_filter<'tcx>( tcx: TyCtxt<'tcx>, body: &Body<'tcx>, @@ -579,7 +608,9 @@ impl Map { // the correctness relies on an aliasing model similar to Stacked Borrows (which is // not yet guaranteed). if tcx.sess.opts.unstable_opts.unsound_mir_opts { - map.register_with_filter(tcx, body, 3, filter, &[]); + // We might want to add additional limitations. If a struct has 10 boxed fields of + // itself, will currently be `10.pow(max_derefs)` tracked places. + map.register_with_filter(tcx, body, 2, filter, &[]); } else { map.register_with_filter(tcx, body, 0, filter, &escaped_places(body)); } @@ -587,6 +618,7 @@ impl Map { map } + /// Register all non-excluded places that pass the filter, up to a certain dereference depth. fn register_with_filter<'tcx>( &mut self, tcx: TyCtxt<'tcx>, @@ -595,7 +627,9 @@ impl Map { mut filter: impl FnMut(Ty<'tcx>) -> bool, exclude: &[Place<'tcx>], ) { + // This is used to tell whether a type is `!Freeze`. let param_env = tcx.param_env_reveal_all_normalized(body.source.def_id()); + let mut projection = Vec::new(); for (local, decl) in body.local_decls.iter_enumerated() { self.register_with_filter_rec( @@ -622,7 +656,7 @@ impl Map { param_env: ty::ParamEnv<'tcx>, exclude: &[Place<'tcx>], ) { - // This check could be improved. + // This currently does a linear scan, could be improved. if exclude.contains(&Place { local, projection: tcx.intern_place_elems(projection) }) { return; } @@ -664,6 +698,9 @@ impl Map { }); } + /// Tries to add the place to the map, without allocating a value slot. + /// + /// Can fail if the projection contains non-trackable elements. fn make_place<'tcx>( &mut self, local: Local, @@ -675,10 +712,6 @@ impl Map { // Apply the projection. for &elem in projection { - match elem { - PlaceElem::Downcast(..) => return Err(()), - _ => (), - } let elem = elem.try_into()?; index = *self.projections.entry((index, elem)).or_insert_with(|| { // Prepend new child to the linked list. @@ -713,6 +746,7 @@ impl Map { self.register_with_ty(local, projection, place_ty.ty) } + /// Tries to track the given place. Fails if type is non-scalar or projection is not trackable. fn register_with_ty<'tcx>( &mut self, local: Local, @@ -736,7 +770,7 @@ impl Map { Ok(()) } - pub fn apply_elem(&self, place: PlaceIndex, elem: ProjElem) -> Option { + pub fn apply(&self, place: PlaceIndex, elem: TrackElem) -> Option { self.projections.get(&(place, elem)).copied() } @@ -744,12 +778,13 @@ impl Map { let mut index = *self.locals.get(place.local)?.as_ref()?; for &elem in place.projection { - index = self.apply_elem(index, elem.try_into().ok()?)?; + index = self.apply(index, elem.try_into().ok()?)?; } Some(index) } + /// Iterate over all direct children. pub fn children(&self, parent: PlaceIndex) -> impl Iterator + '_ { Children::new(self, parent) } @@ -762,40 +797,31 @@ impl Map { } } +/// This is the information tracked for every [`PlaceIndex`] and is stored by [`Map`]. +/// +/// Together, `first_child` and `next_sibling` form an intrusive linked list, which is used to +/// model a tree structure (a replacement for a member like `children: Vec`). #[derive(Debug)] struct PlaceInfo { - next_sibling: Option, - first_child: Option, - /// The projection used to go from parent to this node (only None for root). - proj_elem: Option, + /// We store a [`ValueIndex`] if and only if the placed is tracked by the analysis. value_index: Option, + + /// The projection used to go from parent to this node (only None for root). + proj_elem: Option, + + /// The left-most child. + first_child: Option, + + /// Index of the sibling to the right of this node. + next_sibling: Option, } impl PlaceInfo { - fn new(proj_elem: Option) -> Self { + fn new(proj_elem: Option) -> Self { Self { next_sibling: None, first_child: None, proj_elem, value_index: None } } } -/// Returns all places, that have their reference or address taken. -fn escaped_places<'tcx>(body: &Body<'tcx>) -> Vec> { - struct Collector<'tcx> { - result: Vec>, - } - - impl<'tcx> Visitor<'tcx> for Collector<'tcx> { - fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, _location: Location) { - if context.is_borrow() || context.is_address_of() { - self.result.push(*place); - } - } - } - - let mut collector = Collector { result: Vec::new() }; - collector.visit_body(body); - collector.result -} - struct Children<'a> { map: &'a Map, next: Option, @@ -820,25 +846,28 @@ impl<'a> Iterator for Children<'a> { } } } + +/// Used as the result of an operand. pub enum ValueOrPlace { Value(V), Place(PlaceIndex), } -impl ValueOrPlace { - pub fn top() -> Self { +impl HasTop for ValueOrPlace { + fn top() -> Self { ValueOrPlace::Value(V::top()) } } +/// Used as the result of an r-value. pub enum ValueOrPlaceOrRef { Value(V), Place(PlaceIndex), Ref(PlaceIndex), } -impl ValueOrPlaceOrRef { - pub fn top() -> Self { +impl HasTop for ValueOrPlaceOrRef { + fn top() -> Self { ValueOrPlaceOrRef::Value(V::top()) } } @@ -872,27 +901,28 @@ impl HasTop for FlatSet { } } -/// Currently, we only track places through deref and field projections. +/// The set of projection elements that can be used by a tracked place. /// /// For now, downcast is not allowed due to aliasing between variants (see #101168). #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] -pub enum ProjElem { +pub enum TrackElem { Deref, Field(Field), } -impl TryFrom> for ProjElem { +impl TryFrom> for TrackElem { type Error = (); fn try_from(value: ProjectionElem) -> Result { match value { - ProjectionElem::Deref => Ok(ProjElem::Deref), - ProjectionElem::Field(field, _) => Ok(ProjElem::Field(field)), + ProjectionElem::Deref => Ok(TrackElem::Deref), + ProjectionElem::Field(field, _) => Ok(TrackElem::Field(field)), _ => Err(()), } } } +/// Invokes `f` on all direct fields of `ty`. fn iter_fields<'tcx>( ty: Ty<'tcx>, tcx: TyCtxt<'tcx>, @@ -922,58 +952,26 @@ fn iter_fields<'tcx>( } } -fn debug_with_context_rec( - place: PlaceIndex, - place_str: &str, - new: &IndexVec, - old: Option<&IndexVec>, - map: &Map, - f: &mut Formatter<'_>, -) -> std::fmt::Result { - if let Some(value) = map.places[place].value_index { - match old { - None => writeln!(f, "{}: {:?}", place_str, new[value])?, - Some(old) => { - if new[value] != old[value] { - writeln!(f, "\u{001f}-{}: {:?}", place_str, old[value])?; - writeln!(f, "\u{001f}+{}: {:?}", place_str, new[value])?; - } +/// Returns all places, that have their reference or address taken. +fn escaped_places<'tcx>(body: &Body<'tcx>) -> Vec> { + struct Collector<'tcx> { + result: Vec>, + } + + impl<'tcx> Visitor<'tcx> for Collector<'tcx> { + fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, _location: Location) { + if context.is_borrow() || context.is_address_of() { + self.result.push(*place); } } } - for child in map.children(place) { - let info_elem = map.places[child].proj_elem.unwrap(); - let child_place_str = match info_elem { - ProjElem::Deref => format!("*{}", place_str), - ProjElem::Field(field) => { - if place_str.starts_with("*") { - format!("({}).{}", place_str, field.index()) - } else { - format!("{}.{}", place_str, field.index()) - } - } - }; - debug_with_context_rec(child, &child_place_str, new, old, map, f)?; - } - - Ok(()) -} - -fn debug_with_context( - new: &IndexVec, - old: Option<&IndexVec>, - map: &Map, - f: &mut Formatter<'_>, -) -> std::fmt::Result { - for (local, place) in map.locals.iter_enumerated() { - if let Some(place) = place { - debug_with_context_rec(*place, &format!("{:?}", local), new, old, map, f)?; - } - } - Ok(()) + let mut collector = Collector { result: Vec::new() }; + collector.visit_body(body); + collector.result } +/// This is used to visualize the dataflow analysis. impl<'tcx, T> DebugWithContext> for State where T: ValueAnalysis<'tcx>, @@ -1000,3 +998,55 @@ where } } } + +fn debug_with_context_rec( + place: PlaceIndex, + place_str: &str, + new: &IndexVec, + old: Option<&IndexVec>, + map: &Map, + f: &mut Formatter<'_>, +) -> std::fmt::Result { + if let Some(value) = map.places[place].value_index { + match old { + None => writeln!(f, "{}: {:?}", place_str, new[value])?, + Some(old) => { + if new[value] != old[value] { + writeln!(f, "\u{001f}-{}: {:?}", place_str, old[value])?; + writeln!(f, "\u{001f}+{}: {:?}", place_str, new[value])?; + } + } + } + } + + for child in map.children(place) { + let info_elem = map.places[child].proj_elem.unwrap(); + let child_place_str = match info_elem { + TrackElem::Deref => format!("*{}", place_str), + TrackElem::Field(field) => { + if place_str.starts_with("*") { + format!("({}).{}", place_str, field.index()) + } else { + format!("{}.{}", place_str, field.index()) + } + } + }; + debug_with_context_rec(child, &child_place_str, new, old, map, f)?; + } + + Ok(()) +} + +fn debug_with_context( + new: &IndexVec, + old: Option<&IndexVec>, + map: &Map, + f: &mut Formatter<'_>, +) -> std::fmt::Result { + for (local, place) in map.locals.iter_enumerated() { + if let Some(place) = place { + debug_with_context_rec(*place, &format!("{:?}", local), new, old, map, f)?; + } + } + Ok(()) +} diff --git a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs index d4111fa313aa..75d9ea16bd60 100644 --- a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs +++ b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs @@ -1,10 +1,14 @@ +//! A constant propagation optimization pass based on dataflow analysis. +//! +//! Tracks places that have a scalar type. + use rustc_const_eval::interpret::{ConstValue, ImmTy, Immediate, InterpCx, Scalar}; use rustc_data_structures::fx::FxHashMap; use rustc_middle::mir::visit::{MutVisitor, Visitor}; use rustc_middle::mir::*; use rustc_middle::ty::{self, Ty, TyCtxt}; use rustc_mir_dataflow::value_analysis::{ - Map, ProjElem, State, ValueAnalysis, ValueOrPlace, ValueOrPlaceOrRef, + HasTop, Map, State, TrackElem, ValueAnalysis, ValueOrPlace, ValueOrPlaceOrRef, }; use rustc_mir_dataflow::{lattice::FlatSet, Analysis, ResultsVisitor, SwitchIntEdgeEffects}; use rustc_span::{sym, DUMMY_SP}; @@ -20,7 +24,7 @@ impl<'tcx> MirPass<'tcx> for DataflowConstProp { fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { // Decide which places to track during the analysis. - let map = Map::from_filter(tcx, body, |ty| ty.is_scalar() && !ty.is_unsafe_ptr()); + let map = Map::from_filter(tcx, body, Ty::is_scalar); // Perform the actual dataflow analysis. let analysis = ConstAnalysis::new(tcx, body, map); @@ -65,12 +69,10 @@ impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'tcx> { state.flood_idx(target, self.map()); } - let value_target = target.and_then(|target| { - self.map().apply_elem(target, ProjElem::Field(0_u32.into())) - }); - let overflow_target = target.and_then(|target| { - self.map().apply_elem(target, ProjElem::Field(1_u32.into())) - }); + let value_target = target + .and_then(|target| self.map().apply(target, TrackElem::Field(0_u32.into()))); + let overflow_target = target + .and_then(|target| self.map().apply(target, TrackElem::Field(1_u32.into()))); if value_target.is_some() || overflow_target.is_some() { let (val, overflow) = self.binary_op(state, *op, left, right); @@ -211,6 +213,7 @@ struct ScalarTy<'tcx>(Scalar, Ty<'tcx>); impl<'tcx> std::fmt::Debug for ScalarTy<'tcx> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + // This is used for dataflow visualization, so we return something more concise. std::fmt::Display::fmt(&ConstantKind::Val(ConstValue::Scalar(self.0), self.1), f) } } @@ -341,13 +344,16 @@ impl<'mir, 'tcx, 'map> ResultsVisitor<'mir, 'tcx> for CollectAndPatch<'tcx, 'map location: Location, ) { match statement.kind { + StatementKind::Assign(box (_, Rvalue::Use(Operand::Constant(_)))) => { + // Don't overwrite the assignment if it already uses a constant (to keep the span). + } StatementKind::Assign(box (place, _)) => match state.get(place.as_ref(), self.map) { FlatSet::Top => (), FlatSet::Elem(value) => { self.assignments.insert(location, value); } FlatSet::Bottom => { - // This statement is not reachable. Do nothing, it will (hopefully) be removed. + // This assignment is either unreachable, or an uninitialized value is assigned. } }, _ => (), diff --git a/src/test/mir-opt/dataflow-const-prop/checked.main.DataflowConstProp.diff b/src/test/mir-opt/dataflow-const-prop/checked.main.DataflowConstProp.diff index fbc95cea7cd9..944afed8f465 100644 --- a/src/test/mir-opt/dataflow-const-prop/checked.main.DataflowConstProp.diff +++ b/src/test/mir-opt/dataflow-const-prop/checked.main.DataflowConstProp.diff @@ -53,8 +53,7 @@ StorageDead(_5); // scope 2 at $DIR/checked.rs:+3:17: +3:18 StorageDead(_4); // scope 2 at $DIR/checked.rs:+3:17: +3:18 StorageLive(_7); // scope 3 at $DIR/checked.rs:+5:9: +5:10 -- _7 = const _; // scope 3 at $DIR/checked.rs:+5:13: +5:21 -+ _7 = const i32::MAX; // scope 3 at $DIR/checked.rs:+5:13: +5:21 + _7 = const _; // scope 3 at $DIR/checked.rs:+5:13: +5:21 StorageLive(_8); // scope 4 at $DIR/checked.rs:+6:9: +6:10 StorageLive(_9); // scope 4 at $DIR/checked.rs:+6:13: +6:14 - _9 = _7; // scope 4 at $DIR/checked.rs:+6:13: +6:14 diff --git a/src/test/mir-opt/dataflow-const-prop/previous/control_flow_simplification.hello.DataflowConstProp.diff b/src/test/mir-opt/dataflow-const-prop/previous/control_flow_simplification.hello.DataflowConstProp.diff index f13ca4b62e6f..d990c3b07e51 100644 --- a/src/test/mir-opt/dataflow-const-prop/previous/control_flow_simplification.hello.DataflowConstProp.diff +++ b/src/test/mir-opt/dataflow-const-prop/previous/control_flow_simplification.hello.DataflowConstProp.diff @@ -8,8 +8,7 @@ bb0: { StorageLive(_1); // scope 0 at $DIR/control-flow-simplification.rs:+1:8: +1:21 -- _1 = const _; // scope 0 at $DIR/control-flow-simplification.rs:+1:8: +1:21 -+ _1 = const false; // scope 0 at $DIR/control-flow-simplification.rs:+1:8: +1:21 + _1 = const _; // scope 0 at $DIR/control-flow-simplification.rs:+1:8: +1:21 switchInt(const false) -> [false: bb2, otherwise: bb1]; // scope 0 at $DIR/control-flow-simplification.rs:+1:8: +1:21 }