Auto merge of #68414 - michaelwoerister:share-drop-glue, r=<try>

Also share drop-glue when compiling with -Zshare-generics (i.e. at opt-level=0)

This PR adds drop-glue to the set of monomorphizations that can be shared across crates via `-Zshare-generics`.

This version of the PR might have detrimental effects on performance as it makes lots of stuff dependent on a single query results (`upstream_monomorphizations_for(def_id_of_drop_in_place)`). That should be fixable but let's do a perf run first.

Potentially fixes issue #64140. (cc @alexcrichton)
The changes here are related to @matthewjasper's #67332 but should be mostly orthogonal.

r? @ghost

Author: bors

src/librustc/middle/exported_symbols.rs

@@ -3,7 +3,6 @@ use crate::ty::subst::SubstsRef;
 use crate::ty::{self, TyCtxt};
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
 use rustc_hir::def_id::{DefId, LOCAL_CRATE};
-use std::cmp;
 use std::mem;
 
 /// The SymbolExportLevel of a symbols specifies from which kinds of crates
@@ -43,43 +42,6 @@ impl<'tcx> ExportedSymbol<'tcx> {
             ExportedSymbol::NoDefId(symbol_name) => symbol_name,
         }
     }
-
-    pub fn compare_stable(&self, tcx: TyCtxt<'tcx>, other: &ExportedSymbol<'tcx>) -> cmp::Ordering {
-        match *self {
-            ExportedSymbol::NonGeneric(self_def_id) => match *other {
-                ExportedSymbol::NonGeneric(other_def_id) => {
-                    tcx.def_path_hash(self_def_id).cmp(&tcx.def_path_hash(other_def_id))
-                }
-                ExportedSymbol::Generic(..) | ExportedSymbol::NoDefId(_) => cmp::Ordering::Less,
-            },
-            ExportedSymbol::Generic(self_def_id, self_substs) => match *other {
-                ExportedSymbol::NonGeneric(_) => cmp::Ordering::Greater,
-                ExportedSymbol::Generic(other_def_id, other_substs) => {
-                    // We compare the symbol names because they are cached as query
-                    // results which makes them relatively cheap to access repeatedly.
-                    //
-                    // It might be even faster to build a local cache of stable IDs
-                    // for sorting. Exported symbols are really only sorted once
-                    // in order to make the `exported_symbols` query result stable.
-                    let self_symbol_name =
-                        tcx.symbol_name(ty::Instance::new(self_def_id, self_substs));
-                    let other_symbol_name =
-                        tcx.symbol_name(ty::Instance::new(other_def_id, other_substs));
-
-                    self_symbol_name.cmp(&other_symbol_name)
-                }
-                ExportedSymbol::NoDefId(_) => cmp::Ordering::Less,
-            },
-            ExportedSymbol::NoDefId(self_symbol_name) => match *other {
-                ExportedSymbol::NonGeneric(_) | ExportedSymbol::Generic(..) => {
-                    cmp::Ordering::Greater
-                }
-                ExportedSymbol::NoDefId(ref other_symbol_name) => {
-                    self_symbol_name.cmp(other_symbol_name)
-                }
-            },
-        }
-    }
 }
 
 pub fn metadata_symbol_name(tcx: TyCtxt<'_>) -> String {

src/librustc/mir/mono.rs

@@ -79,7 +79,7 @@ impl<'tcx> MonoItem<'tcx> {
     }
 
     pub fn instantiation_mode(&self, tcx: TyCtxt<'tcx>) -> InstantiationMode {
-        let inline_in_all_cgus = tcx
+        let generate_cgu_internal_copies = tcx
             .sess
             .opts
             .debugging_opts
@@ -93,7 +93,7 @@ impl<'tcx> MonoItem<'tcx> {
                 // If this function isn't inlined or otherwise has explicit
                 // linkage, then we'll be creating a globally shared version.
                 if self.explicit_linkage(tcx).is_some()
-                    || !instance.def.requires_local(tcx)
+                    || !instance.def.generates_cgu_internal_copy(tcx)
                     || Some(instance.def_id()) == entry_def_id
                 {
                     return InstantiationMode::GloballyShared { may_conflict: false };
@@ -102,7 +102,7 @@ impl<'tcx> MonoItem<'tcx> {
                 // At this point we don't have explicit linkage and we're an
                 // inlined function. If we're inlining into all CGUs then we'll
                 // be creating a local copy per CGU
-                if inline_in_all_cgus {
+                if generate_cgu_internal_copies {
                     return InstantiationMode::LocalCopy;
                 }
 

src/librustc/ty/instance.rs

@@ -114,7 +114,12 @@ impl<'tcx> InstanceDef<'tcx> {
         tcx.get_attrs(self.def_id())
     }
 
-    pub fn is_inline(&self, tcx: TyCtxt<'tcx>) -> bool {
+    /// Returns `true` if the LLVM version of this instance is unconditionally
+    /// marked with `inline`. This implies that a copy of this instance is
+    /// generated in every codegen unit.
+    /// Note that this is only a hint. See the documentation for
+    /// `generates_cgu_internal_copy` for more information.
+    pub fn requires_inline(&self, tcx: TyCtxt<'tcx>) -> bool {
         use crate::hir::map::DefPathData;
         let def_id = match *self {
             ty::InstanceDef::Item(def_id) => def_id,
@@ -127,8 +132,15 @@ impl<'tcx> InstanceDef<'tcx> {
         }
     }
 
-    pub fn requires_local(&self, tcx: TyCtxt<'tcx>) -> bool {
-        if self.is_inline(tcx) {
+    /// Returns `true` if the machine code for this instance is instantiated in
+    /// each codegen unit that references it.
+    /// Note that this is only a hint! The compiler can globally decide to *not*
+    /// do this in order to speed up compilation. CGU-internal copies are
+    /// only exist to enable inlining. If inlining is not performed (e.g. at
+    /// `-Copt-level=0`) then the time for generating them is wasted and it's
+    /// better to create a single copy with external linkage.
+    pub fn generates_cgu_internal_copy(&self, tcx: TyCtxt<'tcx>) -> bool {
+        if self.requires_inline(tcx) {
             return true;
         }
         if let ty::InstanceDef::DropGlue(..) = *self {

src/librustc_codegen_llvm/attributes.rs

@@ -246,7 +246,7 @@ pub fn from_fn_attrs(
     }
 
     // FIXME(eddyb) consolidate these two `inline` calls (and avoid overwrites).
-    if instance.def.is_inline(cx.tcx) {
+    if instance.def.requires_inline(cx.tcx) {
         inline(cx, llfn, attributes::InlineAttr::Hint);
     }
 

src/librustc_codegen_ssa/back/symbol_export.rs

@@ -5,7 +5,7 @@ use rustc::middle::codegen_fn_attrs::CodegenFnAttrFlags;
 use rustc::middle::exported_symbols::{metadata_symbol_name, ExportedSymbol, SymbolExportLevel};
 use rustc::session::config;
 use rustc::ty::query::Providers;
-use rustc::ty::subst::SubstsRef;
+use rustc::ty::subst::{GenericArgKind, SubstsRef};
 use rustc::ty::Instance;
 use rustc::ty::{SymbolName, TyCtxt};
 use rustc_codegen_utils::symbol_names;
@@ -17,8 +17,6 @@ use rustc_hir::Node;
 use rustc_index::vec::IndexVec;
 use syntax::expand::allocator::ALLOCATOR_METHODS;
 
-pub type ExportedSymbols = FxHashMap<CrateNum, Arc<Vec<(String, SymbolExportLevel)>>>;
-
 pub fn threshold(tcx: TyCtxt<'_>) -> SymbolExportLevel {
     crates_export_threshold(&tcx.sess.crate_types.borrow())
 }
@@ -96,7 +94,7 @@ fn reachable_non_generics_provider(
                     if !generics.requires_monomorphization(tcx) &&
                         // Functions marked with #[inline] are only ever codegened
                         // with "internal" linkage and are never exported.
-                        !Instance::mono(tcx, def_id).def.requires_local(tcx)
+                        !Instance::mono(tcx, def_id).def.generates_cgu_internal_copy(tcx)
                     {
                         Some(def_id)
                     } else {
@@ -240,19 +238,31 @@ fn exported_symbols_provider_local(
                 continue;
             }
 
-            if let &MonoItem::Fn(Instance { def: InstanceDef::Item(def_id), substs }) = mono_item {
-                if substs.non_erasable_generics().next().is_some() {
-                    symbols
-                        .push((ExportedSymbol::Generic(def_id, substs), SymbolExportLevel::Rust));
+            match *mono_item {
+                MonoItem::Fn(Instance { def: InstanceDef::Item(def_id), substs }) => {
+                    if substs.non_erasable_generics().next().is_some() {
+                        let symbol = ExportedSymbol::Generic(def_id, substs);
+                        symbols.push((symbol, SymbolExportLevel::Rust));
+                    }
+                }
+                MonoItem::Fn(Instance { def: InstanceDef::DropGlue(def_id, Some(ty)), substs }) => {
+                    // A little sanity-check
+                    debug_assert_eq!(
+                        substs.non_erasable_generics().next(),
+                        Some(GenericArgKind::Type(ty))
+                    );
+                    let symbol = ExportedSymbol::Generic(def_id, substs);
+                    symbols.push((symbol, SymbolExportLevel::Rust));
+                }
+                _ => {
+                    // Any other symbols don't qualify for sharing
                 }
             }
         }
     }
 
     // Sort so we get a stable incr. comp. hash.
-    symbols.sort_unstable_by(|&(ref symbol1, ..), &(ref symbol2, ..)| {
-        symbol1.compare_stable(tcx, symbol2)
-    });
+    symbols.sort_by_cached_key(|s| s.0.symbol_name_for_local_instance(tcx));
 
     Arc::new(symbols)
 }

src/librustc_codegen_ssa/back/write.rs

@@ -2,7 +2,8 @@ use super::command::Command;
 use super::link::{self, get_linker, remove};
 use super::linker::LinkerInfo;
 use super::lto::{self, SerializedModule};
-use super::symbol_export::{symbol_name_for_instance_in_crate, ExportedSymbols};
+use super::symbol_export::symbol_name_for_instance_in_crate;
+
 use crate::{
     CachedModuleCodegen, CodegenResults, CompiledModule, CrateInfo, ModuleCodegen, ModuleKind,
     RLIB_BYTECODE_EXTENSION,
@@ -12,6 +13,7 @@ use crate::traits::*;
 use jobserver::{Acquired, Client};
 use rustc::dep_graph::{WorkProduct, WorkProductFileKind, WorkProductId};
 use rustc::middle::cstore::EncodedMetadata;
+use rustc::middle::exported_symbols::SymbolExportLevel;
 use rustc::session::config::{
     self, Lto, OutputFilenames, OutputType, Passes, Sanitizer, SwitchWithOptPath,
 };
@@ -205,6 +207,8 @@ impl<B: WriteBackendMethods> Clone for TargetMachineFactory<B> {
     }
 }
 
+pub type ExportedSymbols = FxHashMap<CrateNum, Arc<Vec<(String, SymbolExportLevel)>>>;
+
 /// Additional resources used by optimize_and_codegen (not module specific)
 #[derive(Clone)]
 pub struct CodegenContext<B: WriteBackendMethods> {

src/librustc_mir/monomorphize/collector.rs

@@ -713,7 +713,8 @@ fn visit_instance_use<'tcx>(
 // need a mono item.
 fn should_monomorphize_locally<'tcx>(tcx: TyCtxt<'tcx>, instance: &Instance<'tcx>) -> bool {
     let def_id = match instance.def {
-        ty::InstanceDef::Item(def_id) => def_id,
+        ty::InstanceDef::Item(def_id) | ty::InstanceDef::DropGlue(def_id, Some(_)) => def_id,
+
         ty::InstanceDef::VtableShim(..)
         | ty::InstanceDef::ReifyShim(..)
         | ty::InstanceDef::ClosureOnceShim { .. }
@@ -725,12 +726,14 @@ fn should_monomorphize_locally<'tcx>(tcx: TyCtxt<'tcx>, instance: &Instance<'tcx
     };
 
     if tcx.is_foreign_item(def_id) {
-        // We can always link to foreign items.
+        // Foreign items are always linked against, there's no way of
+        // instantiating them.
         return false;
     }
 
     if def_id.is_local() {
-        // Local items cannot be referred to locally without monomorphizing them locally.
+        // Local items cannot be referred to locally without
+        // monomorphizing them locally.
         return true;
     }
 
@@ -745,6 +748,7 @@ fn should_monomorphize_locally<'tcx>(tcx: TyCtxt<'tcx>, instance: &Instance<'tcx
     if !tcx.is_mir_available(def_id) {
         bug!("cannot create local mono-item for {:?}", def_id)
     }
+
     return true;
 
     fn is_available_upstream_generic<'tcx>(

src/librustc_mir/monomorphize/partitioning.rs

@@ -324,7 +324,7 @@ fn mono_item_visibility(
     };
 
     let def_id = match instance.def {
-        InstanceDef::Item(def_id) => def_id,
+        InstanceDef::Item(def_id) | InstanceDef::DropGlue(def_id, Some(_)) => def_id,
 
         // These are all compiler glue and such, never exported, always hidden.
         InstanceDef::VtableShim(..)

src/test/codegen-units/partitioning/auxiliary/shared_generics_aux.rs

@@ -1,4 +1,6 @@
-// compile-flags:-Zshare-generics=yes
+// NOTE: We always compile this test with -Copt-level=0 because higher opt-levels
+//       prevent drop-glue from participating in share-generics.
+// compile-flags:-Zshare-generics=yes -Copt-level=0
 // no-prefer-dynamic
 
 #![crate_type="rlib"]
@@ -8,5 +10,17 @@ pub fn generic_fn<T>(x: T, y: T) -> (T, T) {
 }
 
 pub fn use_generic_fn_f32() -> (f32, f32) {
+    // This line causes drop glue for Foo to be instantiated. We want to make
+    // sure that this crate exports an instance to be re-used by share-generics.
+    let _ = Foo(0);
+
     generic_fn(0.0f32, 1.0f32)
 }
+
+pub struct Foo(pub u32);
+
+impl Drop for Foo {
+    fn drop(&mut self) {
+        println!("foo");
+    }
+}

src/test/codegen-units/partitioning/shared-generics.rs

@@ -1,6 +1,8 @@
 // ignore-tidy-linelength
 // no-prefer-dynamic
-// compile-flags:-Zprint-mono-items=eager -Zshare-generics=yes -Zincremental=tmp/partitioning-tests/shared-generics-exe
+// NOTE: We always compile this test with -Copt-level=0 because higher opt-levels
+//       prevent drop-glue from participating in share-generics.
+// compile-flags:-Zprint-mono-items=eager -Zshare-generics=yes -Zincremental=tmp/partitioning-tests/shared-generics-exe -Copt-level=0
 
 #![crate_type="rlib"]
 
@@ -16,6 +18,10 @@ pub fn foo() {
     // This should not generate a monomorphization because it's already
     // available in `shared_generics_aux`.
     let _ = shared_generics_aux::generic_fn(0.0f32, 3.0f32);
-}
 
-// MONO_ITEM drop-glue i8
+    // The following line will drop an instance of `Foo`, generating a call to
+    // Foo's drop-glue function. However, share-generics should take care of
+    // reusing the drop-glue from the upstream crate, so we do not expect a
+    // mono item for the drop-glue
+    let _ = shared_generics_aux::Foo(1);
+}