Auto merge of #58505 - schomatis:fix/nll/remove-live-var, r=matthewja…

…sper

[NLL] Remove `LiveVar`

The `LiveVar` type (and related) made it harder to reason about the code. It seemed as an abstraction that didn't bring any useful concept to the reader (when transitioning from the RFC theory to the actual implementation code).

It achieved a compactness in the vectors storing the def/use/drop information that was related only to the `LocalUseMap`. This PR went in the other direction and favored time over memory (but this decision can be easily reverted to the other side without reintroducing `LiveVar`).

What this PR aims at is to clarify that there's no significant transformation between the MIR `Local` and the `LiveVar` (now refactored as `live_locals: Vec<Local>`): we're just filtering (not mapping) the entire group of `Local`s into a meaningful subset that we should perform the liveness analysis on.

As a side note, there is no guarantee that the liveness analysis is performed only on (what the code calls) "live" variables, if the NLL facts are requested it will be performed on *any* variable so there can't be any assumptions on that regard. (Still, this PR didn't change the general naming convention to reduce the number of changes here and streamline the review process).

**Acceptance criteria:** This PR attempts to do only a minor refactoring and not to change the logic so it can't have any performance impact, particularly, it can't lose any of the significant performance improvement achieved in the great work done in #52115.

r? @nikomatsakis

src/librustc_mir/borrow_check/nll/mod.rs

@@ -2,7 +2,6 @@ use crate::borrow_check::borrow_set::BorrowSet;
 use crate::borrow_check::location::{LocationIndex, LocationTable};
 use crate::borrow_check::nll::facts::AllFactsExt;
 use crate::borrow_check::nll::type_check::{MirTypeckResults, MirTypeckRegionConstraints};
-use crate::borrow_check::nll::type_check::liveness::liveness_map::NllLivenessMap;
 use crate::borrow_check::nll::region_infer::values::RegionValueElements;
 use crate::dataflow::indexes::BorrowIndex;
 use crate::dataflow::move_paths::MoveData;

src/librustc_mir/borrow_check/nll/type_check/liveness/local_use_map.rs

@@ -1,6 +1,5 @@
 use crate::borrow_check::nll::region_infer::values::{PointIndex, RegionValueElements};
-use crate::borrow_check::nll::type_check::liveness::liveness_map::{LiveVar, NllLivenessMap};
-use crate::util::liveness::{categorize, DefUse, LiveVariableMap};
+use crate::util::liveness::{categorize, DefUse};
 use rustc::mir::visit::{PlaceContext, Visitor};
 use rustc::mir::{Local, Location, Mir};
 use rustc_data_structures::indexed_vec::{Idx, IndexVec};
@@ -9,26 +8,33 @@ use rustc_data_structures::vec_linked_list as vll;
 /// A map that cross references each local with the locations where it
 /// is defined (assigned), used, or dropped. Used during liveness
 /// computation.
-crate struct LocalUseMap<'me> {
-    liveness_map: &'me NllLivenessMap,
-
+///
+/// We keep track only of `Local`s we'll do the liveness analysis later,
+/// this means that our internal `IndexVec`s will only be sparsely populated.
+/// In the time-memory trade-off between keeping compact vectors with new
+/// indexes (and needing to continuously map the `Local` index to its compact
+/// counterpart) and having `IndexVec`s that we only use a fraction of, time
+/// (and code simplicity) was favored. The rationale is that we only keep
+/// a small number of `IndexVec`s throughout the entire analysis while, in
+/// contrast, we're accessing each `Local` *many* times.
+crate struct LocalUseMap {
     /// Head of a linked list of **definitions** of each variable --
     /// definition in this context means assignment, e.g., `x` is
     /// defined in `x = y` but not `y`; that first def is the head of
     /// a linked list that lets you enumerate all places the variable
     /// is assigned.
-    first_def_at: IndexVec<LiveVar, Option<AppearanceIndex>>,
+    first_def_at: IndexVec<Local, Option<AppearanceIndex>>,
 
     /// Head of a linked list of **uses** of each variable -- use in
     /// this context means that the existing value of the variable is
     /// read or modified. e.g., `y` is used in `x = y` but not `x`.
     /// Note that `DROP(x)` terminators are excluded from this list.
-    first_use_at: IndexVec<LiveVar, Option<AppearanceIndex>>,
+    first_use_at: IndexVec<Local, Option<AppearanceIndex>>,
 
     /// Head of a linked list of **drops** of each variable -- these
     /// are a special category of uses corresponding to the drop that
     /// we add for each local variable.
-    first_drop_at: IndexVec<LiveVar, Option<AppearanceIndex>>,
+    first_drop_at: IndexVec<Local, Option<AppearanceIndex>>,
 
     appearances: IndexVec<AppearanceIndex, Appearance>,
 }
@@ -50,52 +56,68 @@ impl vll::LinkElem for Appearance {
     }
 }
 
-impl LocalUseMap<'me> {
+impl LocalUseMap {
     crate fn build(
-        liveness_map: &'me NllLivenessMap,
+        live_locals: &Vec<Local>,
         elements: &RegionValueElements,
         mir: &Mir<'_>,
     ) -> Self {
-        let nones = IndexVec::from_elem_n(None, liveness_map.num_variables());
+        let nones = IndexVec::from_elem_n(None, mir.local_decls.len());
         let mut local_use_map = LocalUseMap {
-            liveness_map,
             first_def_at: nones.clone(),
             first_use_at: nones.clone(),
             first_drop_at: nones,
             appearances: IndexVec::new(),
         };
 
+        let mut locals_with_use_data: IndexVec<Local, bool> =
+            IndexVec::from_elem_n(false, mir.local_decls.len());
+        live_locals
+            .iter()
+            .for_each(|&local| locals_with_use_data[local] = true);
+
         LocalUseMapBuild {
             local_use_map: &mut local_use_map,
             elements,
-        }.visit_mir(mir);
+            locals_with_use_data,
+        }
+        .visit_mir(mir);
 
         local_use_map
     }
 
-    crate fn defs(&self, local: LiveVar) -> impl Iterator<Item = PointIndex> + '_ {
+    crate fn defs(&self, local: Local) -> impl Iterator<Item = PointIndex> + '_ {
         vll::iter(self.first_def_at[local], &self.appearances)
             .map(move |aa| self.appearances[aa].point_index)
     }
 
-    crate fn uses(&self, local: LiveVar) -> impl Iterator<Item = PointIndex> + '_ {
+    crate fn uses(&self, local: Local) -> impl Iterator<Item = PointIndex> + '_ {
         vll::iter(self.first_use_at[local], &self.appearances)
             .map(move |aa| self.appearances[aa].point_index)
     }
 
-    crate fn drops(&self, local: LiveVar) -> impl Iterator<Item = PointIndex> + '_ {
+    crate fn drops(&self, local: Local) -> impl Iterator<Item = PointIndex> + '_ {
         vll::iter(self.first_drop_at[local], &self.appearances)
             .map(move |aa| self.appearances[aa].point_index)
     }
 }
 
-struct LocalUseMapBuild<'me, 'map: 'me> {
-    local_use_map: &'me mut LocalUseMap<'map>,
+struct LocalUseMapBuild<'me> {
+    local_use_map: &'me mut LocalUseMap,
     elements: &'me RegionValueElements,
+
+    // Vector used in `visit_local` to signal which `Local`s do we need
+    // def/use/drop information on, constructed from `live_locals` (that
+    // contains the variables we'll do the liveness analysis for).
+    // This vector serves optimization purposes only: we could have
+    // obtained the same information from `live_locals` but we want to
+    // avoid repeatedly calling `Vec::contains()` (see `LocalUseMap` for
+    // the rationale on the time-memory trade-off we're favoring here).
+    locals_with_use_data: IndexVec<Local, bool>,
 }
 
-impl LocalUseMapBuild<'_, '_> {
-    fn insert_def(&mut self, local: LiveVar, location: Location) {
+impl LocalUseMapBuild<'_> {
+    fn insert_def(&mut self, local: Local, location: Location) {
         Self::insert(
             self.elements,
             &mut self.local_use_map.first_def_at[local],
@@ -104,7 +126,7 @@ impl LocalUseMapBuild<'_, '_> {
         );
     }
 
-    fn insert_use(&mut self, local: LiveVar, location: Location) {
+    fn insert_use(&mut self, local: Local, location: Location) {
         Self::insert(
             self.elements,
             &mut self.local_use_map.first_use_at[local],
@@ -113,7 +135,7 @@ impl LocalUseMapBuild<'_, '_> {
         );
     }
 
-    fn insert_drop(&mut self, local: LiveVar, location: Location) {
+    fn insert_drop(&mut self, local: Local, location: Location) {
         Self::insert(
             self.elements,
             &mut self.local_use_map.first_drop_at[local],
@@ -137,13 +159,13 @@ impl LocalUseMapBuild<'_, '_> {
     }
 }
 
-impl Visitor<'tcx> for LocalUseMapBuild<'_, '_> {
+impl Visitor<'tcx> for LocalUseMapBuild<'_> {
     fn visit_local(&mut self, &local: &Local, context: PlaceContext<'tcx>, location: Location) {
-        if let Some(local_with_region) = self.local_use_map.liveness_map.from_local(local) {
+        if self.locals_with_use_data[local] {
             match categorize(context) {
-                Some(DefUse::Def) => self.insert_def(local_with_region, location),
-                Some(DefUse::Use) => self.insert_use(local_with_region, location),
-                Some(DefUse::Drop) => self.insert_drop(local_with_region, location),
+                Some(DefUse::Def) => self.insert_def(local, location),
+                Some(DefUse::Use) => self.insert_use(local, location),
+                Some(DefUse::Drop) => self.insert_drop(local, location),
                 _ => (),
             }
         }

src/librustc_mir/borrow_check/nll/type_check/liveness/mod.rs

@@ -1,19 +1,19 @@
 use crate::borrow_check::location::LocationTable;
-use crate::borrow_check::nll::region_infer::values::RegionValueElements;
 use crate::borrow_check::nll::constraints::ConstraintSet;
-use crate::borrow_check::nll::NllLivenessMap;
+use crate::borrow_check::nll::facts::{AllFacts, AllFactsExt};
+use crate::borrow_check::nll::region_infer::values::RegionValueElements;
 use crate::borrow_check::nll::universal_regions::UniversalRegions;
+use crate::borrow_check::nll::ToRegionVid;
 use crate::dataflow::move_paths::MoveData;
-use crate::dataflow::MaybeInitializedPlaces;
 use crate::dataflow::FlowAtLocation;
-use rustc::mir::Mir;
-use rustc::ty::RegionVid;
+use crate::dataflow::MaybeInitializedPlaces;
+use rustc::mir::{Local, Mir};
+use rustc::ty::{RegionVid, TyCtxt};
 use rustc_data_structures::fx::FxHashSet;
 use std::rc::Rc;
 
 use super::TypeChecker;
 
-crate mod liveness_map;
 mod local_use_map;
 mod trace;
 
@@ -34,16 +34,71 @@ pub(super) fn generate<'gcx, 'tcx>(
     location_table: &LocationTable,
 ) {
     debug!("liveness::generate");
-    let free_regions = {
-        let borrowck_context = typeck.borrowck_context.as_ref().unwrap();
-        regions_that_outlive_free_regions(
-            typeck.infcx.num_region_vars(),
-            &borrowck_context.universal_regions,
-            &borrowck_context.constraints.outlives_constraints,
-        )
+
+    let live_locals: Vec<Local> = if AllFacts::enabled(typeck.tcx()) {
+        // If "dump facts from NLL analysis" was requested perform
+        // the liveness analysis for all `Local`s. This case opens
+        // the possibility of the variables being analyzed in `trace`
+        // to be *any* `Local`, not just the "live" ones, so we can't
+        // make any assumptions past this point as to the characteristics
+        // of the `live_locals`.
+        // FIXME: Review "live" terminology past this point, we should
+        // not be naming the `Local`s as live.
+        mir.local_decls.indices().collect()
+    } else {
+        let free_regions = {
+            let borrowck_context = typeck.borrowck_context.as_ref().unwrap();
+            regions_that_outlive_free_regions(
+                typeck.infcx.num_region_vars(),
+                &borrowck_context.universal_regions,
+                &borrowck_context.constraints.outlives_constraints,
+            )
+        };
+        compute_live_locals(typeck.tcx(), &free_regions, mir)
     };
-    let liveness_map = NllLivenessMap::compute(typeck.tcx(), &free_regions, mir);
-    trace::trace(typeck, mir, elements, flow_inits, move_data, &liveness_map, location_table);
+
+    if !live_locals.is_empty() {
+        trace::trace(
+            typeck,
+            mir,
+            elements,
+            flow_inits,
+            move_data,
+            live_locals,
+            location_table,
+        );
+    }
+}
+
+// The purpose of `compute_live_locals` is to define the subset of `Local`
+// variables for which we need to do a liveness computation. We only need
+// to compute whether a variable `X` is live if that variable contains
+// some region `R` in its type where `R` is not known to outlive a free
+// region (i.e., where `R` may be valid for just a subset of the fn body).
+fn compute_live_locals(
+    tcx: TyCtxt<'_, '_, 'tcx>,
+    free_regions: &FxHashSet<RegionVid>,
+    mir: &Mir<'tcx>,
+) -> Vec<Local> {
+    let live_locals: Vec<Local> = mir
+        .local_decls
+        .iter_enumerated()
+        .filter_map(|(local, local_decl)| {
+            if tcx.all_free_regions_meet(&local_decl.ty, |r| {
+                free_regions.contains(&r.to_region_vid())
+            }) {
+                None
+            } else {
+                Some(local)
+            }
+        })
+        .collect();
+
+    debug!("{} total variables", mir.local_decls.len());
+    debug!("{} variables need liveness", live_locals.len());
+    debug!("{} regions outlive free regions", free_regions.len());
+
+    live_locals
 }
 
 /// Computes all regions that are (currently) known to outlive free