Auto merge of #52991 - nikomatsakis:nll-escaping-into-return, r=pnkfelix

avoid computing liveness for locals that escape into statics

Fixes #52713

I poked at this on the plane and I think it's working -- but I want to do a bit more investigation and double check. The idea is to identify those local variables where the entire value will "escape" into the return -- for them, we don't need to compute liveness, since we know that the outlives relations from the return type will force those regions to be equal to free regions. This should help with html5ever in particular.

- [x] test performance
- [x] verify correctness
- [x] add comments

r? @pnkfelix
cc @lqd

src/librustc_mir/borrow_check/nll/escaping_locals.rs

+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+//! Identify those variables whose entire value will eventually be
+//! returned from the fn via the RETURN_PLACE. As an optimization, we
+//! can skip computing liveness results for those variables. The idea
+//! is that the return type of the fn only ever contains free
+//! regions. Therefore, the types of those variables are going to
+//! ultimately be contrained to outlive those free regions -- since
+//! free regions are always live for the entire body, this implies
+//! that the liveness results are not important for those regions.
+//! This is most important in the "fns" that we create to represent static
+//! values, since those are often really quite large, and all regions in them
+//! will ultimately be constrained to be `'static`. Two examples:
+//!
+//! ```
+//! fn foo() -> &'static [u32] { &[] }
+//! static FOO: &[u32] = &[];
+//! ```
+//!
+//! In both these cases, the return value will only have static lifetime.
+//!
+//! NB: The simple logic here relies on the fact that outlives
+//! relations in our analysis don't have locations. Otherwise, we
+//! would have to restrict ourselves to values that are
+//! *unconditionally* returned (which would still cover the "big
+//! static value" case).
+//!
+//! The way that this code works is to use union-find -- we iterate
+//! over the MIR and union together two variables X and Y if all
+//! regions in the value of Y are going to be stored into X -- that
+//! is, if `typeof(X): 'a` requires that `typeof(Y): 'a`. This means
+//! that e.g. we can union together `x` and `y` if we have something
+//! like `x = (y, 22)`, but not something like `x = y.f` (since there
+//! may be regions in the type of `y` that do not appear in the field
+//! `f`).
+
+use rustc::mir::visit::Visitor;
+use rustc::mir::*;
+
+use rustc_data_structures::indexed_vec::Idx;
+use rustc_data_structures::unify as ut;
+
+crate struct EscapingLocals {
+    unification_table: ut::UnificationTable<ut::InPlace<AssignedLocal>>,
+}
+
+impl EscapingLocals {
+    crate fn compute(mir: &Mir<'tcx>) -> Self {
+        let mut visitor = GatherAssignedLocalsVisitor::new();
+        visitor.visit_mir(mir);
+
+        EscapingLocals {
+            unification_table: visitor.unification_table,
+        }
+    }
+
+    /// True if `local` is known to escape into static
+    /// memory.
+    crate fn escapes_into_return(&mut self, local: Local) -> bool {
+        let return_place = AssignedLocal::from(RETURN_PLACE);
+        let other_place = AssignedLocal::from(local);
+        self.unification_table.unioned(return_place, other_place)
+    }
+}
+
+/// The MIR visitor gathering the union-find of the locals used in
+/// assignments.
+struct GatherAssignedLocalsVisitor {
+    unification_table: ut::UnificationTable<ut::InPlace<AssignedLocal>>,
+}
+
+#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)]
+struct AssignedLocal(u32);
+
+impl ut::UnifyKey for AssignedLocal {
+    type Value = ();
+
+    fn index(&self) -> u32 {
+        self.0
+    }
+
+    fn from_index(i: u32) -> AssignedLocal {
+        AssignedLocal(i)
+    }
+
+    fn tag() -> &'static str {
+        "AssignedLocal"
+    }
+}
+
+impl From<Local> for AssignedLocal {
+    fn from(item: Local) -> Self {
+        // newtype_indexes use usize but are u32s.
+        assert!(item.index() < ::std::u32::MAX as usize);
+        AssignedLocal(item.index() as u32)
+    }
+}
+
+impl GatherAssignedLocalsVisitor {
+    fn new() -> Self {
+        Self {
+            unification_table: ut::UnificationTable::new(),
+        }
+    }
+
+    fn union_locals_if_needed(&mut self, lvalue: Option<Local>, rvalue: Option<Local>) {
+        if let Some(lvalue) = lvalue {
+            if let Some(rvalue) = rvalue {
+                if lvalue != rvalue {
+                    debug!("EscapingLocals: union {:?} and {:?}", lvalue, rvalue);
+                    self.unification_table
+                        .union(AssignedLocal::from(lvalue), AssignedLocal::from(rvalue));
+                }
+            }
+        }
+    }
+}
+
+// Returns the potential `Local` associated to this `Place` or `PlaceProjection`
+fn find_local_in_place(place: &Place) -> Option<Local> {
+    match place {
+        Place::Local(local) => Some(*local),
+
+        // If you do e.g. `x = a.f` then only *part* of the type of
+        // `a` escapes into `x` (the part contained in `f`); if `a`'s
+        // type has regions that don't appear in `f`, those might not
+        // escape.
+        Place::Projection(..) => None,
+
+        Place::Static { .. } | Place::Promoted { .. } => None,
+    }
+}
+
+// Returns the potential `Local` in this `Operand`.
+fn find_local_in_operand(op: &Operand) -> Option<Local> {
+    // Conservatively check a subset of `Operand`s we know our
+    // benchmarks track, for example `html5ever`.
+    match op {
+        Operand::Copy(place) | Operand::Move(place) => find_local_in_place(place),
+        Operand::Constant(_) => None,
+    }
+}
+
+impl Visitor<'tcx> for GatherAssignedLocalsVisitor {
+    fn visit_mir(&mut self, mir: &Mir<'tcx>) {
+        // We need as many union-find keys as there are locals
+        for _ in 0..mir.local_decls.len() {
+            self.unification_table.new_key(());
+        }
+
+        self.super_mir(mir);
+    }
+
+    fn visit_assign(
+        &mut self,
+        block: BasicBlock,
+        place: &Place<'tcx>,
+        rvalue: &Rvalue<'tcx>,
+        location: Location,
+    ) {
+        let local = find_local_in_place(place);
+
+        // Find those cases where there is a `Place` consumed by
+        // `rvalue` and we know that all regions in its type will be
+        // incorporated into `place`, the `Place` we are assigning to.
+        match rvalue {
+            // `x = y` is the simplest possible case.
+            Rvalue::Use(op) => self.union_locals_if_needed(local, find_local_in_operand(op)),
+
+            // `X = &'r P` -- the type of `X` will be `&'r T_P`, where
+            // `T_P` is the type of `P`.
+            Rvalue::Ref(_, _, place) => {
+                // Special case: if you have `X = &*Y` (or `X = &**Y`
+                // etc), then the outlives relationships will ensure
+                // that all regions in `Y` are constrained by regions
+                // in `X` -- this is because the lifetimes of the
+                // references we deref through are required to outlive
+                // the borrow lifetime `'r` (which appears in `X`).
+                //
+                // (We don't actually need to check the type of `Y`:
+                // since `ProjectionElem::Deref` represents a built-in
+                // deref and not an overloaded deref, if the thing we
+                // deref through is not a reference, then it must be a
+                // `Box` or `*const`, in which case it contains no
+                // references.)
+                let mut place_ref = place;
+                while let Place::Projection(proj) = place_ref {
+                    if let ProjectionElem::Deref = proj.elem {
+                        place_ref = &proj.base;
+                    } else {
+                        break;
+                    }
+                }
+
+                self.union_locals_if_needed(local, find_local_in_place(place_ref))
+            }
+
+            Rvalue::Cast(kind, op, _) => match kind {
+                CastKind::Unsize => {
+                    // Casting a `&[T; N]` to `&[T]` or `&Foo` to `&Trait` --
+                    // in both cases, no regions are "lost".
+                    self.union_locals_if_needed(local, find_local_in_operand(op))
+                }
+                _ => (),
+            },
+
+            // Constructing an aggregate like `(x,)` or `Foo { x }`
+            // includes the full type of `x`.
+            Rvalue::Aggregate(_, ops) => {
+                for rvalue in ops.iter().map(find_local_in_operand) {
+                    self.union_locals_if_needed(local, rvalue);
+                }
+            }
+
+            // For other things, be conservative and do not union.
+            _ => (),
+        };
+
+        self.super_assign(block, place, rvalue, location);
+    }
+}

src/librustc_mir/borrow_check/nll/liveness_map.rs

@@ -16,9 +16,10 @@
 //! liveness code so that it only operates over variables with regions in their
 //! types, instead of all variables.
 
+use borrow_check::nll::escaping_locals::EscapingLocals;
+use rustc::mir::{Local, Mir};
 use rustc::ty::TypeFoldable;
 use rustc_data_structures::indexed_vec::IndexVec;
-use rustc::mir::{Mir, Local};
 use util::liveness::LiveVariableMap;
 
 use rustc_data_structures::indexed_vec::Idx;
@@ -29,14 +30,13 @@
 crate struct NllLivenessMap {
     /// For each local variable, contains either None (if the type has no regions)
     /// or Some(i) with a suitable index.
-    pub from_local: IndexVec<Local, Option<LocalWithRegion>>,
-    /// For each LocalWithRegion, maps back to the original Local index.
-    pub to_local: IndexVec<LocalWithRegion, Local>,
+    from_local: IndexVec<Local, Option<LocalWithRegion>>,
 
+    /// For each LocalWithRegion, maps back to the original Local index.
+    to_local: IndexVec<LocalWithRegion, Local>,
 }
 
 impl LiveVariableMap for NllLivenessMap {
-
     fn from_local(&self, local: Local) -> Option<Self::LiveVar> {
         self.from_local[local]
     }
@@ -55,21 +55,43 @@ fn num_variables(&self) -> usize {
 impl NllLivenessMap {
     /// Iterates over the variables in Mir and assigns each Local whose type contains
     /// regions a LocalWithRegion index. Returns a map for converting back and forth.
-    pub fn compute(mir: &Mir) -> Self {
+    crate fn compute(mir: &Mir<'tcx>) -> Self {
+        let mut escaping_locals = EscapingLocals::compute(mir);
+
         let mut to_local = IndexVec::default();
-        let from_local: IndexVec<Local,Option<_>> = mir
+        let mut escapes_into_return = 0;
+        let mut no_regions = 0;
+        let from_local: IndexVec<Local, Option<_>> = mir
             .local_decls
             .iter_enumerated()
             .map(|(local, local_decl)| {
-                if local_decl.ty.has_free_regions() {
-                    Some(to_local.push(local))
+                if escaping_locals.escapes_into_return(local) {
+                    // If the local escapes into the return value,
+                    // then the return value will force all of the
+                    // regions in its type to outlive free regions
+                    // (e.g., `'static`) and hence liveness is not
+                    // needed. This is particularly important for big
+                    // statics.
+                    escapes_into_return += 1;
+                    None
+                } else if local_decl.ty.has_free_regions() {
+                    let l = to_local.push(local);
+                    debug!("liveness_map: {:?} = {:?}", local, l);
+                    Some(l)
+                } else {
+                    no_regions += 1;
+                    None
                 }
-                    else {
-                        None
-                    }
             }).collect();
 
-        Self { from_local, to_local }
+        debug!("liveness_map: {} variables need liveness", to_local.len());
+        debug!("liveness_map: {} escapes into return", escapes_into_return);
+        debug!("liveness_map: {} no regions", no_regions);
+
+        Self {
+            from_local,
+            to_local,
+        }
     }
 }
 

src/librustc_mir/borrow_check/nll/mod.rs

@@ -39,7 +39,9 @@
 use util as mir_util;
 use util::pretty::{self, ALIGN};
 
+mod constraints;
 mod constraint_generation;
+mod escaping_locals;
 pub mod explain_borrow;
 mod facts;
 mod invalidation;
@@ -49,8 +51,6 @@
 mod universal_regions;
 crate mod liveness_map;
 
-mod constraints;
-
 use self::facts::AllFacts;
 use self::region_infer::RegionInferenceContext;
 use self::universal_regions::UniversalRegions;
@@ -120,6 +120,7 @@ pub(in borrow_check) fn compute_regions<'cx, 'gcx, 'tcx>(
         location_table,
         borrow_set,
         &liveness,
+        &liveness_map,
         &mut all_facts,
         flow_inits,
         move_data,
@@ -205,6 +206,7 @@ pub(in borrow_check) fn compute_regions<'cx, 'gcx, 'tcx>(
     dump_mir_results(
         infcx,
         &liveness,
+        &liveness_map,
         MirSource::item(def_id),
         &mir,
         &regioncx,
@@ -221,6 +223,7 @@ pub(in borrow_check) fn compute_regions<'cx, 'gcx, 'tcx>(
 fn dump_mir_results<'a, 'gcx, 'tcx>(
     infcx: &InferCtxt<'a, 'gcx, 'tcx>,
     liveness: &LivenessResults<LocalWithRegion>,
+    liveness_map: &NllLivenessMap,
     source: MirSource,
     mir: &Mir<'tcx>,
     regioncx: &RegionInferenceContext,
@@ -230,8 +233,6 @@ fn dump_mir_results<'a, 'gcx, 'tcx>(
         return;
     }
 
-    let map = &NllLivenessMap::compute(mir);
-
     let regular_liveness_per_location: FxHashMap<_, _> = mir
         .basic_blocks()
         .indices()
@@ -239,7 +240,7 @@ fn dump_mir_results<'a, 'gcx, 'tcx>(
             let mut results = vec![];
             liveness
                 .regular
-                .simulate_block(&mir, bb, map, |location, local_set| {
+                .simulate_block(&mir, bb, liveness_map, |location, local_set| {
                     results.push((location, local_set.clone()));
                 });
             results
@@ -253,7 +254,7 @@ fn dump_mir_results<'a, 'gcx, 'tcx>(
             let mut results = vec![];
             liveness
                 .drop
-                .simulate_block(&mir, bb, map, |location, local_set| {
+                .simulate_block(&mir, bb, liveness_map, |location, local_set| {
                     results.push((location, local_set.clone()));
                 });
             results

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

@@ -37,6 +37,7 @@ pub(super) fn generate<'gcx, 'tcx>(
     cx: &mut TypeChecker<'_, 'gcx, 'tcx>,
     mir: &Mir<'tcx>,
     liveness: &LivenessResults<LocalWithRegion>,
+    liveness_map: &NllLivenessMap,
     flow_inits: &mut FlowAtLocation<MaybeInitializedPlaces<'_, 'gcx, 'tcx>>,
     move_data: &MoveData<'tcx>,
 ) {
@@ -44,10 +45,10 @@ pub(super) fn generate<'gcx, 'tcx>(
         cx,
         mir,
         liveness,
+        liveness_map,
         flow_inits,
         move_data,
         drop_data: FxHashMap(),
-        map: &NllLivenessMap::compute(mir),
     };
 
     for bb in mir.basic_blocks().indices() {
@@ -65,10 +66,10 @@ struct TypeLivenessGenerator<'gen, 'typeck, 'flow, 'gcx, 'tcx>
     cx: &'gen mut TypeChecker<'typeck, 'gcx, 'tcx>,
     mir: &'gen Mir<'tcx>,
     liveness: &'gen LivenessResults<LocalWithRegion>,
+    liveness_map: &'gen NllLivenessMap,
     flow_inits: &'gen mut FlowAtLocation<MaybeInitializedPlaces<'flow, 'gcx, 'tcx>>,
     move_data: &'gen MoveData<'tcx>,
     drop_data: FxHashMap<Ty<'tcx>, DropData<'tcx>>,
-    map: &'gen NllLivenessMap,
 }
 
 struct DropData<'tcx> {
@@ -86,9 +87,9 @@ fn add_liveness_constraints(&mut self, bb: BasicBlock) {
 
         self.liveness
             .regular
-            .simulate_block(self.mir, bb, self.map, |location, live_locals| {
+            .simulate_block(self.mir, bb, self.liveness_map, |location, live_locals| {
                 for live_local in live_locals.iter() {
-                    let local = self.map.from_live_var(live_local);
+                    let local = self.liveness_map.from_live_var(live_local);
                     let live_local_ty = self.mir.local_decls[local].ty;
                     Self::push_type_live_constraint(&mut self.cx, live_local_ty, location);
                 }
@@ -97,7 +98,7 @@ fn add_liveness_constraints(&mut self, bb: BasicBlock) {
         let mut all_live_locals: Vec<(Location, Vec<LocalWithRegion>)> = vec![];
         self.liveness
             .drop
-            .simulate_block(self.mir, bb, self.map, |location, live_locals| {
+            .simulate_block(self.mir, bb, self.liveness_map, |location, live_locals| {
                 all_live_locals.push((location, live_locals.iter().collect()));
             });
         debug!(
@@ -124,7 +125,7 @@ fn add_liveness_constraints(&mut self, bb: BasicBlock) {
                     });
                 }
 
-                let local = self.map.from_live_var(live_local);
+                let local = self.liveness_map.from_live_var(live_local);
                 let mpi = self.move_data.rev_lookup.find_local(local);
                 if let Some(initialized_child) = self.flow_inits.has_any_child_of(mpi) {
                     debug!(
@@ -133,7 +134,7 @@ fn add_liveness_constraints(&mut self, bb: BasicBlock) {
                         self.move_data.move_paths[initialized_child]
                     );
 
-                    let local = self.map.from_live_var(live_local);
+                    let local = self.liveness_map.from_live_var(live_local);
                     let live_local_ty = self.mir.local_decls[local].ty;
                     self.add_drop_live_constraint(live_local, live_local_ty, location);
                 }

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

@@ -15,9 +15,12 @@
 use borrow_check::location::LocationTable;
 use borrow_check::nll::constraints::{ConstraintSet, OutlivesConstraint};
 use borrow_check::nll::facts::AllFacts;
-use borrow_check::nll::region_infer::values::{RegionValueElements, LivenessValues};
+use borrow_check::nll::liveness_map::NllLivenessMap;
+use borrow_check::nll::region_infer::values::{LivenessValues, RegionValueElements};
 use borrow_check::nll::region_infer::{ClosureRegionRequirementsExt, TypeTest};
-use borrow_check::nll::type_check::free_region_relations::{CreateResult, UniversalRegionRelations};
+use borrow_check::nll::type_check::free_region_relations::{
+    CreateResult, UniversalRegionRelations,
+};
 use borrow_check::nll::universal_regions::UniversalRegions;
 use borrow_check::nll::LocalWithRegion;
 use borrow_check::nll::ToRegionVid;
@@ -116,6 +119,7 @@ pub(crate) fn type_check<'gcx, 'tcx>(
     location_table: &LocationTable,
     borrow_set: &BorrowSet<'tcx>,
     liveness: &LivenessResults<LocalWithRegion>,
+    liveness_map: &NllLivenessMap,
     all_facts: &mut Option<AllFacts>,
     flow_inits: &mut FlowAtLocation<MaybeInitializedPlaces<'_, 'gcx, 'tcx>>,
     move_data: &MoveData<'tcx>,
@@ -166,7 +170,7 @@ pub(crate) fn type_check<'gcx, 'tcx>(
             Some(&mut borrowck_context),
             Some(errors_buffer),
             |cx| {
-                liveness::generate(cx, mir, liveness, flow_inits, move_data);
+                liveness::generate(cx, mir, liveness, liveness_map, flow_inits, move_data);
                 cx.equate_inputs_and_outputs(
                     mir,
                     mir_def_id,

src/test/ui/borrowck/promote-ref-mut-in-let-issue-46557.nll.stderr

 error[E0597]: borrowed value does not live long enough
   --> $DIR/promote-ref-mut-in-let-issue-46557.rs:15:21
    |
-LL |   fn gimme_static_mut_let() -> &'static mut u32 {
-   |  _______________________________________________-
-LL | |     let ref mut x = 1234543; //~ ERROR
-   | |                     ^^^^^^^ temporary value does not live long enough
-LL | |     x
-LL | | }
-   | | -
-   | | |
-   | |_temporary value only lives until here
-   |   borrow later used here
+LL |     let ref mut x = 1234543; //~ ERROR
+   |                     ^^^^^^^ temporary value does not live long enough
+LL |     x
+LL | }
+   | - temporary value only lives until here
+   |
+   = note: borrowed value must be valid for the static lifetime...
 
 error[E0597]: borrowed value does not live long enough
   --> $DIR/promote-ref-mut-in-let-issue-46557.rs:20:25
    |
-LL |   fn gimme_static_mut_let_nested() -> &'static mut u32 {
-   |  ______________________________________________________-
-LL | |     let (ref mut x, ) = (1234543, ); //~ ERROR
-   | |                         ^^^^^^^^^^^ temporary value does not live long enough
-LL | |     x
-LL | | }
-   | | -
-   | | |
-   | |_temporary value only lives until here
-   |   borrow later used here
+LL |     let (ref mut x, ) = (1234543, ); //~ ERROR
+   |                         ^^^^^^^^^^^ temporary value does not live long enough
+LL |     x
+LL | }
+   | - temporary value only lives until here
+   |
+   = note: borrowed value must be valid for the static lifetime...
 
 error[E0597]: borrowed value does not live long enough
   --> $DIR/promote-ref-mut-in-let-issue-46557.rs:25:11

src/test/ui/nll/get_default.nll.stderr

@@ -63,9 +63,18 @@ LL |         match map.get() {
 LL |             Some(v) => {
 LL |                 map.set(String::new()); // Both AST and MIR error here
    |                 ^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
-...
-LL |                 return v;
-   |                        - borrow later used here
+   |
+note: borrowed value must be valid for the anonymous lifetime #1 defined on the function body at 41:1...
+  --> $DIR/get_default.rs:41:1
+   |
+LL | / fn err(map: &mut Map) -> &String {
+LL | |     loop {
+LL | |         match map.get() {
+LL | |             Some(v) => {
+...  |
+LL | |     }
+LL | | }
+   | |_^
 
 error[E0502]: cannot borrow `*map` as mutable because it is also borrowed as immutable (Mir)
   --> $DIR/get_default.rs:51:17

src/test/ui/nll/get_default.stderr

@@ -63,9 +63,18 @@ LL |         match map.get() {
 LL |             Some(v) => {
 LL |                 map.set(String::new()); // Both AST and MIR error here
    |                 ^^^ mutable borrow occurs here
-...
-LL |                 return v;
-   |                        - borrow later used here
+   |
+note: borrowed value must be valid for the anonymous lifetime #1 defined on the function body at 41:1...
+  --> $DIR/get_default.rs:41:1
+   |
+LL | / fn err(map: &mut Map) -> &String {
+LL | |     loop {
+LL | |         match map.get() {
+LL | |             Some(v) => {
+...  |
+LL | |     }
+LL | | }
+   | |_^
 
 error[E0502]: cannot borrow `*map` as mutable because it is also borrowed as immutable (Mir)
   --> $DIR/get_default.rs:51:17

src/test/ui/nll/return-ref-mut-issue-46557.stderr

 error[E0597]: borrowed value does not live long enough
   --> $DIR/return-ref-mut-issue-46557.rs:17:21
    |
-LL |   fn gimme_static_mut() -> &'static mut u32 {
-   |  ___________________________________________-
-LL | |     let ref mut x = 1234543; //~ ERROR borrowed value does not live long enough [E0597]
-   | |                     ^^^^^^^ temporary value does not live long enough
-LL | |     x
-LL | | }
-   | | -
-   | | |
-   | |_temporary value only lives until here
-   |   borrow later used here
+LL |     let ref mut x = 1234543; //~ ERROR borrowed value does not live long enough [E0597]
+   |                     ^^^^^^^ temporary value does not live long enough
+LL |     x
+LL | }
+   | - temporary value only lives until here
+   |
+   = note: borrowed value must be valid for the static lifetime...
 
 error: aborting due to previous error