Auto merge of #38944 - michaelwoerister:incr-generics-partitioning, r=nikomatsakis

trans: Treat generics like regular functions, not like #[inline] function, during CGU partitioning

This PR makes generics be treated just like regular functions during CGU partitioning:

+ the function instantiation is placed in a codegen unit based on the function's DefPath,
+ unless it is marked with `#[inline]`  -- which causes a private copy of the function to be placed in every referencing codegen unit.

This has the following effects:
+ Multi codegen unit builds will become faster because code for generic functions is duplicated less.
+ Multi codegen unit builds might have lower runtime performance, since generics are not available for inlining automatically any more.
+ Single codegen unit builds are not affected one way or the other.

This partitioning scheme is particularly good for incremental compilation as it drastically reduces the number of false positives during codegen unit invalidation.

I'd love to have a benchmark suite for estimating the effect on runtime performance for changes like this one.

r? @nikomatsakis

cc @rust-lang/compiler

15 files changed, 175 insertions, 187 deletions

diff --git a/src/librustc/middle/cstore.rs b/src/librustc/middle/cstore.rs
index 7151e5226ca..496a3d4a498 100644
--- a/src/librustc/middle/cstore.rs
+++ b/src/librustc/middle/cstore.rs
@@ -211,6 +211,7 @@ pub trait CrateStore<'tcx> {
     fn is_foreign_item(&self, did: DefId) -> bool;
     fn is_dllimport_foreign_item(&self, def: DefId) -> bool;
     fn is_statically_included_foreign_item(&self, def_id: DefId) -> bool;
+    fn is_exported_symbol(&self, def_id: DefId) -> bool;
 
     // crate metadata
     fn dylib_dependency_formats(&self, cnum: CrateNum)
@@ -258,11 +259,6 @@ pub trait CrateStore<'tcx> {
     fn get_item_mir<'a>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId) -> Mir<'tcx>;
     fn is_item_mir_available(&self, def: DefId) -> bool;
 
-    /// Take a look if we need to inline or monomorphize this. If so, we
-    /// will emit code for this item in the local crate, and thus
-    /// create a translation item for it.
-    fn can_have_local_instance<'a>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId) -> bool;
-
     // This is basically a 1-based range of ints, which is a little
     // silly - I may fix that.
     fn crates(&self) -> Vec<CrateNum>;
@@ -368,6 +364,7 @@ impl<'tcx> CrateStore<'tcx> for DummyCrateStore {
     fn is_foreign_item(&self, did: DefId) -> bool { bug!("is_foreign_item") }
     fn is_dllimport_foreign_item(&self, id: DefId) -> bool { false }
     fn is_statically_included_foreign_item(&self, def_id: DefId) -> bool { false }
+    fn is_exported_symbol(&self, def_id: DefId) -> bool { false }
 
     // crate metadata
     fn dylib_dependency_formats(&self, cnum: CrateNum)
@@ -436,9 +433,6 @@ impl<'tcx> CrateStore<'tcx> for DummyCrateStore {
     fn is_item_mir_available(&self, def: DefId) -> bool {
         bug!("is_item_mir_available")
     }
-    fn can_have_local_instance<'a>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId) -> bool {
-        bug!("can_have_local_instance")
-    }
 
     // This is basically a 1-based range of ints, which is a little
     // silly - I may fix that.
diff --git a/src/librustc_metadata/creader.rs b/src/librustc_metadata/creader.rs
index 724c164b3b4..8f7b9c24cbf 100644
--- a/src/librustc_metadata/creader.rs
+++ b/src/librustc_metadata/creader.rs
@@ -302,10 +302,13 @@ impl<'a> CrateLoader<'a> {
             crate_root.def_path_table.decode(&metadata)
         });
 
+        let exported_symbols = crate_root.exported_symbols.decode(&metadata).collect();
+
         let mut cmeta = cstore::CrateMetadata {
             name: name,
             extern_crate: Cell::new(None),
             def_path_table: def_path_table,
+            exported_symbols: exported_symbols,
             proc_macros: crate_root.macro_derive_registrar.map(|_| {
                 self.load_derive_macros(&crate_root, dylib.clone().map(|p| p.0), span)
             }),
diff --git a/src/librustc_metadata/cstore.rs b/src/librustc_metadata/cstore.rs
index efc19abb33e..761041ad719 100644
--- a/src/librustc_metadata/cstore.rs
+++ b/src/librustc_metadata/cstore.rs
@@ -80,6 +80,8 @@ pub struct CrateMetadata {
     /// compilation support.
     pub def_path_table: DefPathTable,
 
+    pub exported_symbols: FxHashSet<DefIndex>,
+
     pub dep_kind: Cell<DepKind>,
     pub source: CrateSource,
 
diff --git a/src/librustc_metadata/cstore_impl.rs b/src/librustc_metadata/cstore_impl.rs
index 3d025e984b0..7cd26df0246 100644
--- a/src/librustc_metadata/cstore_impl.rs
+++ b/src/librustc_metadata/cstore_impl.rs
@@ -226,6 +226,10 @@ impl<'tcx> CrateStore<'tcx> for cstore::CStore {
         self.do_is_statically_included_foreign_item(def_id)
     }
 
+    fn is_exported_symbol(&self, def_id: DefId) -> bool {
+        self.get_crate_data(def_id.krate).exported_symbols.contains(&def_id.index)
+    }
+
     fn is_dllimport_foreign_item(&self, def_id: DefId) -> bool {
         if def_id.krate == LOCAL_CRATE {
             self.dllimport_foreign_items.borrow().contains(&def_id.index)
@@ -466,11 +470,6 @@ impl<'tcx> CrateStore<'tcx> for cstore::CStore {
         self.get_crate_data(def.krate).is_item_mir_available(def.index)
     }
 
-    fn can_have_local_instance<'a>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId) -> bool {
-        self.dep_graph.read(DepNode::MetaData(def));
-        def.is_local() || self.get_crate_data(def.krate).can_have_local_instance(tcx, def.index)
-    }
-
     fn crates(&self) -> Vec<CrateNum>
     {
         let mut result = vec![];
diff --git a/src/librustc_metadata/decoder.rs b/src/librustc_metadata/decoder.rs
index f3a673898b2..4abdee345c2 100644
--- a/src/librustc_metadata/decoder.rs
+++ b/src/librustc_metadata/decoder.rs
@@ -445,14 +445,6 @@ impl<'tcx> EntryKind<'tcx> {
             EntryKind::Closure(_) => return None,
         })
     }
-    fn is_const_fn(&self, meta: &CrateMetadata) -> bool {
-        let constness = match *self {
-            EntryKind::Method(data) => data.decode(meta).fn_data.constness,
-            EntryKind::Fn(data) => data.decode(meta).constness,
-            _ => hir::Constness::NotConst,
-        };
-        constness == hir::Constness::Const
-    }
 }
 
 impl<'a, 'tcx> CrateMetadata {
@@ -804,29 +796,6 @@ impl<'a, 'tcx> CrateMetadata {
         self.maybe_entry(id).and_then(|item| item.decode(self).mir).is_some()
     }
 
-    pub fn can_have_local_instance(&self,
-                                   tcx: TyCtxt<'a, 'tcx, 'tcx>,
-                                   id: DefIndex) -> bool {
-        self.maybe_entry(id).map_or(false, |item| {
-            let item = item.decode(self);
-            // if we don't have a MIR, then this item was never meant to be locally instantiated
-            // or we have a bug in the metadata serialization
-            item.mir.is_some() && (
-                // items with generics always can have local instances if monomorphized
-                item.generics.map_or(false, |generics| {
-                    let generics = generics.decode((self, tcx));
-                    generics.parent_types != 0 || !generics.types.is_empty()
-                }) ||
-                match item.kind {
-                    EntryKind::Closure(_) => true,
-                    _ => false,
-                } ||
-                item.kind.is_const_fn(self) ||
-                attr::requests_inline(&self.get_attributes(&item))
-            )
-        })
-    }
-
     pub fn maybe_get_item_mir(&self,
                               tcx: TyCtxt<'a, 'tcx, 'tcx>,
                               id: DefIndex)
@@ -1031,7 +1000,7 @@ impl<'a, 'tcx> CrateMetadata {
     }
 
     pub fn get_exported_symbols(&self) -> Vec<DefId> {
-        self.root.exported_symbols.decode(self).map(|index| self.local_def_id(index)).collect()
+        self.exported_symbols.iter().map(|&index| self.local_def_id(index)).collect()
     }
 
     pub fn get_macro(&self, id: DefIndex) -> (ast::Name, MacroDef) {
@@ -1043,7 +1012,12 @@ impl<'a, 'tcx> CrateMetadata {
     }
 
     pub fn is_const_fn(&self, id: DefIndex) -> bool {
-        self.entry(id).kind.is_const_fn(self)
+        let constness = match self.entry(id).kind {
+            EntryKind::Method(data) => data.decode(self).fn_data.constness,
+            EntryKind::Fn(data) => data.decode(self).constness,
+            _ => hir::Constness::NotConst,
+        };
+        constness == hir::Constness::Const
     }
 
     pub fn is_foreign_item(&self, id: DefIndex) -> bool {
diff --git a/src/librustc_trans/back/symbol_names.rs b/src/librustc_trans/back/symbol_names.rs
index 938848054fe..8f8c48a06b2 100644
--- a/src/librustc_trans/back/symbol_names.rs
+++ b/src/librustc_trans/back/symbol_names.rs
@@ -152,6 +152,15 @@ fn get_symbol_hash<'a, 'tcx>(scx: &SharedCrateContext<'a, 'tcx>,
             assert!(!substs.has_erasable_regions());
             assert!(!substs.needs_subst());
             substs.visit_with(&mut hasher);
+
+            // If this is an instance of a generic function, we also hash in
+            // the ID of the instantiating crate. This avoids symbol conflicts
+            // in case the same instances is emitted in two crates of the same
+            // project.
+            if substs.types().next().is_some() {
+                hasher.hash(scx.tcx().crate_name.as_str());
+                hasher.hash(scx.sess().local_crate_disambiguator().as_str());
+            }
         }
     });
 
diff --git a/src/librustc_trans/collector.rs b/src/librustc_trans/collector.rs
index ff8a14474e5..5e409a2aa55 100644
--- a/src/librustc_trans/collector.rs
+++ b/src/librustc_trans/collector.rs
@@ -212,7 +212,7 @@ use glue::{self, DropGlueKind};
 use monomorphize::{self, Instance};
 use util::nodemap::{FxHashSet, FxHashMap, DefIdMap};
 
-use trans_item::{TransItem, DefPathBasedNames};
+use trans_item::{TransItem, DefPathBasedNames, InstantiationMode};
 
 use std::iter;
 
@@ -337,6 +337,10 @@ fn collect_items_rec<'a, 'tcx: 'a>(scx: &SharedCrateContext<'a, 'tcx>,
         }
         TransItem::Static(node_id) => {
             let def_id = scx.tcx().map.local_def_id(node_id);
+
+            // Sanity check whether this ended up being collected accidentally
+            debug_assert!(should_trans_locally(scx.tcx(), def_id));
+
             let ty = scx.tcx().item_type(def_id);
             let ty = glue::get_drop_glue_type(scx, ty);
             neighbors.push(TransItem::DropGlue(DropGlueKind::Ty(ty)));
@@ -346,6 +350,9 @@ fn collect_items_rec<'a, 'tcx: 'a>(scx: &SharedCrateContext<'a, 'tcx>,
             collect_neighbours(scx, Instance::mono(scx, def_id), &mut neighbors);
         }
         TransItem::Fn(instance) => {
+            // Sanity check whether this ended up being collected accidentally
+            debug_assert!(should_trans_locally(scx.tcx(), instance.def));
+
             // Keep track of the monomorphization recursion depth
             recursion_depth_reset = Some(check_recursion_limit(scx.tcx(),
                                                                instance,
@@ -374,7 +381,7 @@ fn record_inlining_canditates<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
                                         callees: &[TransItem<'tcx>],
                                         inlining_map: &mut InliningMap<'tcx>) {
     let is_inlining_candidate = |trans_item: &TransItem<'tcx>| {
-        trans_item.needs_local_copy(tcx)
+        trans_item.instantiation_mode(tcx) == InstantiationMode::LocalCopy
     };
 
     let inlining_candidates = callees.into_iter()
@@ -490,15 +497,16 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> {
                         .require(ExchangeMallocFnLangItem)
                         .unwrap_or_else(|e| self.scx.sess().fatal(&e));
 
-                assert!(can_have_local_instance(self.scx.tcx(), exchange_malloc_fn_def_id));
-                let empty_substs = self.scx.empty_substs_for_def_id(exchange_malloc_fn_def_id);
-                let exchange_malloc_fn_trans_item =
-                    create_fn_trans_item(self.scx,
-                                         exchange_malloc_fn_def_id,
-                                         empty_substs,
-                                         self.param_substs);
+                if should_trans_locally(self.scx.tcx(), exchange_malloc_fn_def_id) {
+                    let empty_substs = self.scx.empty_substs_for_def_id(exchange_malloc_fn_def_id);
+                    let exchange_malloc_fn_trans_item =
+                        create_fn_trans_item(self.scx,
+                                             exchange_malloc_fn_def_id,
+                                             empty_substs,
+                                             self.param_substs);
 
-                self.output.push(exchange_malloc_fn_trans_item);
+                    self.output.push(exchange_malloc_fn_trans_item);
+                }
             }
             _ => { /* not interesting */ }
         }
@@ -609,7 +617,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> {
             match tcx.item_type(def_id).sty {
                 ty::TyFnDef(def_id, _, f) => {
                     // Some constructors also have type TyFnDef but they are
-                    // always instantiated inline and don't result in
+                    // always instantiated inline and don't result in a
                     // translation item. Same for FFI functions.
                     if let Some(hir_map::NodeForeignItem(_)) = tcx.map.get_if_local(def_id) {
                         return false;
@@ -625,7 +633,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> {
                 _ => return false
             }
 
-            can_have_local_instance(tcx, def_id)
+            should_trans_locally(tcx, def_id)
         }
     }
 
@@ -675,10 +683,27 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> {
     }
 }
 
-fn can_have_local_instance<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
-                                     def_id: DefId)
-                                     -> bool {
-    tcx.sess.cstore.can_have_local_instance(tcx, def_id)
+// Returns true if we should translate an instance in the local crate.
+// Returns false if we can just link to the upstream crate and therefore don't
+// need a translation item.
+fn should_trans_locally<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
+                                  def_id: DefId)
+                                  -> bool {
+    if def_id.is_local() {
+        true
+    } else {
+        if tcx.sess.cstore.is_exported_symbol(def_id) ||
+           tcx.sess.cstore.is_foreign_item(def_id) {
+            // We can link to the item in question, no instance needed in this
+            // crate
+            false
+        } else {
+            if !tcx.sess.cstore.is_item_mir_available(def_id) {
+                bug!("Cannot create local trans-item for {:?}", def_id)
+            }
+            true
+        }
+    }
 }
 
 fn find_drop_glue_neighbors<'a, 'tcx>(scx: &SharedCrateContext<'a, 'tcx>,
@@ -698,14 +723,15 @@ fn find_drop_glue_neighbors<'a, 'tcx>(scx: &SharedCrateContext<'a, 'tcx>,
     // Make sure the BoxFreeFn lang-item gets translated if there is a boxed value.
     if let ty::TyBox(content_type) = ty.sty {
         let def_id = scx.tcx().require_lang_item(BoxFreeFnLangItem);
-        assert!(can_have_local_instance(scx.tcx(), def_id));
-        let box_free_fn_trans_item =
-            create_fn_trans_item(scx,
-                                 def_id,
-                                 scx.tcx().mk_substs(iter::once(Kind::from(content_type))),
-                                 scx.tcx().intern_substs(&[]));
-
-        output.push(box_free_fn_trans_item);
+
+        if should_trans_locally(scx.tcx(), def_id) {
+            let box_free_fn_trans_item =
+                create_fn_trans_item(scx,
+                                     def_id,
+                                     scx.tcx().mk_substs(iter::once(Kind::from(content_type))),
+                                     scx.tcx().intern_substs(&[]));
+            output.push(box_free_fn_trans_item);
+        }
     }
 
     // If the type implements Drop, also add a translation item for the
@@ -735,7 +761,7 @@ fn find_drop_glue_neighbors<'a, 'tcx>(scx: &SharedCrateContext<'a, 'tcx>,
             _ => bug!()
         };
 
-        if can_have_local_instance(scx.tcx(), destructor_did) {
+        if should_trans_locally(scx.tcx(), destructor_did) {
             let trans_item = create_fn_trans_item(scx,
                                                   destructor_did,
                                                   substs,
@@ -1080,7 +1106,7 @@ fn create_trans_items_for_vtable_methods<'a, 'tcx>(scx: &SharedCrateContext<'a,
                         None
                     }
                 })
-                .filter(|&(def_id, _)| can_have_local_instance(scx.tcx(), def_id))
+                .filter(|&(def_id, _)| should_trans_locally(scx.tcx(), def_id))
                 .map(|(def_id, substs)| create_fn_trans_item(scx, def_id, substs, param_substs));
             output.extend(methods);
         }
@@ -1255,7 +1281,7 @@ fn create_trans_items_for_default_impls<'a, 'tcx>(scx: &SharedCrateContext<'a, '
                         continue;
                     }
 
-                    if can_have_local_instance(tcx, method.def_id) {
+                    if should_trans_locally(tcx, method.def_id) {
                         let item = create_fn_trans_item(scx,
                                                         method.def_id,
                                                         callee_substs,
diff --git a/src/librustc_trans/partitioning.rs b/src/librustc_trans/partitioning.rs
index d93bbec7efa..d1d167306c1 100644
--- a/src/librustc_trans/partitioning.rs
+++ b/src/librustc_trans/partitioning.rs
@@ -53,8 +53,6 @@
 //! - One for "stable", that is non-generic, code
 //! - One for more "volatile" code, i.e. monomorphized instances of functions
 //!   defined in that module
-//! - Code for monomorphized instances of functions from external crates gets
-//!   placed into every codegen unit that uses that instance.
 //!
 //! In order to see why this heuristic makes sense, let's take a look at when a
 //! codegen unit can get invalidated:
@@ -82,17 +80,6 @@
 //! side-effect of references a little by at least not touching the non-generic
 //! code of the module.
 //!
-//! As another optimization, monomorphized functions from external crates get
-//! some special handling. Since we assume that the definition of such a
-//! function changes rather infrequently compared to local items, we can just
-//! instantiate external functions in every codegen unit where it is referenced
-//! -- without having to fear that doing this will cause a lot of unnecessary
-//! re-compilations. If such a reference is added or removed, the codegen unit
-//! has to be re-translated anyway.
-//! (Note that this only makes sense if external crates actually don't change
-//! frequently. For certain multi-crate projects this might not be a valid
-//! assumption).
-//!
 //! A Note on Inlining
 //! ------------------
 //! As briefly mentioned above, in order for LLVM to be able to inline a
@@ -107,10 +94,9 @@
 //!   inlined, so it can distribute function instantiations accordingly. Since
 //!   there is no way of knowing for sure which functions LLVM will decide to
 //!   inline in the end, we apply a heuristic here: Only functions marked with
-//!   #[inline] and (as stated above) functions from external crates are
-//!   considered for inlining by the partitioner. The current implementation
-//!   will not try to determine if a function is likely to be inlined by looking
-//!   at the functions definition.
+//!   #[inline] are considered for inlining by the partitioner. The current
+//!   implementation will not try to determine if a function is likely to be
+//!   inlined by looking at the functions definition.
 //!
 //! Note though that as a side-effect of creating a codegen units per
 //! source-level module, functions from the same module will be available for
@@ -133,7 +119,7 @@ use std::sync::Arc;
 use symbol_map::SymbolMap;
 use syntax::ast::NodeId;
 use syntax::symbol::{Symbol, InternedString};
-use trans_item::TransItem;
+use trans_item::{TransItem, InstantiationMode};
 use util::nodemap::{FxHashMap, FxHashSet};
 
 pub enum PartitioningStrategy {
@@ -326,13 +312,15 @@ fn place_root_translation_items<'a, 'tcx, I>(scx: &SharedCrateContext<'a, 'tcx>,
     let tcx = scx.tcx();
     let mut roots = FxHashSet();
     let mut codegen_units = FxHashMap();
+    let is_incremental_build = tcx.sess.opts.incremental.is_some();
 
     for trans_item in trans_items {
-        let is_root = !trans_item.is_instantiated_only_on_demand(tcx);
+        let is_root = trans_item.instantiation_mode(tcx) == InstantiationMode::GloballyShared;
 
         if is_root {
             let characteristic_def_id = characteristic_def_id_of_trans_item(scx, trans_item);
-            let is_volatile = trans_item.is_generic_fn();
+            let is_volatile = is_incremental_build &&
+                              trans_item.is_generic_fn();
 
             let codegen_unit_name = match characteristic_def_id {
                 Some(def_id) => compute_codegen_unit_name(tcx, def_id, is_volatile),
@@ -350,25 +338,9 @@ fn place_root_translation_items<'a, 'tcx, I>(scx: &SharedCrateContext<'a, 'tcx>,
                 Some(explicit_linkage) => explicit_linkage,
                 None => {
                     match trans_item {
+                        TransItem::Fn(..) |
                         TransItem::Static(..) => llvm::ExternalLinkage,
                         TransItem::DropGlue(..) => unreachable!(),
-                        // Is there any benefit to using ExternalLinkage?:
-                        TransItem::Fn(ref instance) => {
-                            if instance.substs.types().next().is_none() {
-                                // This is a non-generic functions, we always
-                                // make it visible externally on the chance that
-                                // it might be used in another codegen unit.
-                                // Later on base::internalize_symbols() will
-                                // assign "internal" linkage to those symbols
-                                // that are not referenced from other codegen
-                                // units (and are not publicly visible).
-                                llvm::ExternalLinkage
-                            } else {
-                                // In the current setup, generic functions cannot
-                                // be roots.
-                                unreachable!()
-                            }
-                        }
                     }
                 }
             };
@@ -448,29 +420,14 @@ fn place_inlined_translation_items<'tcx>(initial_partitioning: PreInliningPartit
             if let Some(linkage) = codegen_unit.items.get(&trans_item) {
                 // This is a root, just copy it over
                 new_codegen_unit.items.insert(trans_item, *linkage);
-            } else if initial_partitioning.roots.contains(&trans_item) {
-                // This item will be instantiated in some other codegen unit,
-                // so we just add it here with AvailableExternallyLinkage
-                // FIXME(mw): I have not seen it happening yet but having
-                //            available_externally here could potentially lead
-                //            to the same problem with exception handling tables
-                //            as in the case below.
-                new_codegen_unit.items.insert(trans_item,
-                                              llvm::AvailableExternallyLinkage);
-            } else if trans_item.is_from_extern_crate() && !trans_item.is_generic_fn() {
-                // FIXME(mw): It would be nice if we could mark these as
-                // `AvailableExternallyLinkage`, since they should have
-                // been instantiated in the extern crate. But this
-                // sometimes leads to crashes on Windows because LLVM
-                // does not handle exception handling table instantiation
-                // reliably in that case.
-                new_codegen_unit.items.insert(trans_item, llvm::InternalLinkage);
             } else {
-                // We can't be sure if this will also be instantiated
-                // somewhere else, so we add an instance here with
-                // InternalLinkage so we don't get any conflicts.
-                new_codegen_unit.items.insert(trans_item,
-                                              llvm::InternalLinkage);
+                if initial_partitioning.roots.contains(&trans_item) {
+                    bug!("GloballyShared trans-item inlined into other CGU: \
+                          {:?}", trans_item);
+                }
+
+                // This is a cgu-private copy
+                new_codegen_unit.items.insert(trans_item, llvm::InternalLinkage);
             }
         }
 
diff --git a/src/librustc_trans/trans_item.rs b/src/librustc_trans/trans_item.rs
index f6f91411225..816c3442543 100644
--- a/src/librustc_trans/trans_item.rs
+++ b/src/librustc_trans/trans_item.rs
@@ -45,6 +45,18 @@ pub enum TransItem<'tcx> {
     Static(NodeId)
 }
 
+/// Describes how a translation item will be instantiated in object files.
+#[derive(PartialEq, Eq, Clone, Copy, Debug, Hash)]
+pub enum InstantiationMode {
+    /// There will be exactly one instance of the given TransItem. It will have
+    /// external linkage so that it can be linked to from other codegen units.
+    GloballyShared,
+
+    /// Each codegen unit containing a reference to the given TransItem will
+    /// have its own private copy of the function (with internal linkage).
+    LocalCopy,
+}
+
 impl<'a, 'tcx> TransItem<'tcx> {
 
     pub fn define(&self, ccx: &CrateContext<'a, 'tcx>) {
@@ -231,22 +243,21 @@ impl<'a, 'tcx> TransItem<'tcx> {
         }
     }
 
-    /// True if the translation item should only be translated to LLVM IR if
-    /// it is referenced somewhere (like inline functions, for example).
-    pub fn is_instantiated_only_on_demand(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>) -> bool {
-        if self.explicit_linkage(tcx).is_some() {
-            return false;
-        }
-
+    pub fn instantiation_mode(&self,
+                              tcx: TyCtxt<'a, 'tcx, 'tcx>)
+                              -> InstantiationMode {
         match *self {
             TransItem::Fn(ref instance) => {
-                !instance.def.is_local() ||
-                instance.substs.types().next().is_some() ||
-                common::is_closure(tcx, instance.def) ||
-                attr::requests_inline(&tcx.get_attrs(instance.def)[..])
+                if self.explicit_linkage(tcx).is_none() &&
+                   (common::is_closure(tcx, instance.def) ||
+                    attr::requests_inline(&tcx.get_attrs(instance.def)[..])) {
+                    InstantiationMode::LocalCopy
+                } else {
+                    InstantiationMode::GloballyShared
+                }
             }
-            TransItem::DropGlue(..) => true,
-            TransItem::Static(..)   => false,
+            TransItem::DropGlue(..) => InstantiationMode::LocalCopy,
+            TransItem::Static(..) => InstantiationMode::GloballyShared,
         }
     }
 
@@ -260,18 +271,6 @@ impl<'a, 'tcx> TransItem<'tcx> {
         }
     }
 
-    /// Returns true if there has to be a local copy of this TransItem in every
-    /// codegen unit that references it (as with inlined functions, for example)
-    pub fn needs_local_copy(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>) -> bool {
-        // Currently everything that is instantiated only on demand is done so
-        // with "internal" linkage, so we need a copy to be present in every
-        // codegen unit.
-        // This is coincidental: We could also instantiate something only if it
-        // is referenced (e.g. a regular, private function) but place it in its
-        // own codegen unit with "external" linkage.
-        self.is_instantiated_only_on_demand(tcx)
-    }
-
     pub fn explicit_linkage(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>) -> Option<llvm::Linkage> {
         let def_id = match *self {
             TransItem::Fn(ref instance) => instance.def,
diff --git a/src/test/codegen-units/partitioning/extern-generic.rs b/src/test/codegen-units/partitioning/extern-generic.rs
index 58f904f48a1..db36b50702a 100644
--- a/src/test/codegen-units/partitioning/extern-generic.rs
+++ b/src/test/codegen-units/partitioning/extern-generic.rs
@@ -57,8 +57,8 @@ mod mod3 {
 }
 
 // Make sure the two generic functions from the extern crate get instantiated
-// privately in every module they are use in.
-//~ TRANS_ITEM fn cgu_generic_function::foo[0]<&str> @@ extern_generic[Internal] extern_generic-mod1[Internal] extern_generic-mod2[Internal] extern_generic-mod1-mod1[Internal]
-//~ TRANS_ITEM fn cgu_generic_function::bar[0]<&str> @@ extern_generic[Internal] extern_generic-mod1[Internal] extern_generic-mod2[Internal] extern_generic-mod1-mod1[Internal]
+// once for the current crate
+//~ TRANS_ITEM fn cgu_generic_function::foo[0]<&str> @@ cgu_generic_function.volatile[External]
+//~ TRANS_ITEM fn cgu_generic_function::bar[0]<&str> @@ cgu_generic_function.volatile[External]
 
 //~ TRANS_ITEM drop-glue i8
diff --git a/src/test/codegen-units/partitioning/local-generic.rs b/src/test/codegen-units/partitioning/local-generic.rs
index 2d744169d3f..70fc75c6970 100644
--- a/src/test/codegen-units/partitioning/local-generic.rs
+++ b/src/test/codegen-units/partitioning/local-generic.rs
@@ -16,10 +16,10 @@
 #![allow(dead_code)]
 #![crate_type="lib"]
 
-//~ TRANS_ITEM fn local_generic::generic[0]<u32> @@ local_generic[Internal]
-//~ TRANS_ITEM fn local_generic::generic[0]<u64> @@ local_generic-mod1[Internal]
-//~ TRANS_ITEM fn local_generic::generic[0]<char> @@ local_generic-mod1-mod1[Internal]
-//~ TRANS_ITEM fn local_generic::generic[0]<&str> @@ local_generic-mod2[Internal]
+//~ TRANS_ITEM fn local_generic::generic[0]<u32> @@ local_generic.volatile[External]
+//~ TRANS_ITEM fn local_generic::generic[0]<u64> @@ local_generic.volatile[External]
+//~ TRANS_ITEM fn local_generic::generic[0]<char> @@ local_generic.volatile[External]
+//~ TRANS_ITEM fn local_generic::generic[0]<&str> @@ local_generic.volatile[External]
 pub fn generic<T>(x: T) -> T { x }
 
 //~ TRANS_ITEM fn local_generic::user[0] @@ local_generic[External]
diff --git a/src/test/codegen-units/partitioning/vtable-through-const.rs b/src/test/codegen-units/partitioning/vtable-through-const.rs
index 0007eaae289..7a0217072f3 100644
--- a/src/test/codegen-units/partitioning/vtable-through-const.rs
+++ b/src/test/codegen-units/partitioning/vtable-through-const.rs
@@ -74,19 +74,19 @@ fn main() {
     // Since Trait1::do_something() is instantiated via its default implementation,
     // it is considered a generic and is instantiated here only because it is
     // referenced in this module.
-    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::Trait1[0]::do_something_else[0]<u32> @@ vtable_through_const[Internal]
+    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::Trait1[0]::do_something_else[0]<u32> @@ vtable_through_const-mod1.volatile[External]
 
     // Although it is never used, Trait1::do_something_else() has to be
     // instantiated locally here too, otherwise the <&u32 as &Trait1> vtable
     // could not be fully constructed.
-    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::Trait1[0]::do_something[0]<u32> @@ vtable_through_const[Internal]
+    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::Trait1[0]::do_something[0]<u32> @@ vtable_through_const-mod1.volatile[External]
     mod1::TRAIT1_REF.do_something();
 
     // Same as above
-    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::{{impl}}[1]::do_something[0]<u8> @@ vtable_through_const[Internal]
-    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::{{impl}}[1]::do_something_else[0]<u8> @@ vtable_through_const[Internal]
+    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::{{impl}}[1]::do_something[0]<u8> @@ vtable_through_const-mod1.volatile[External]
+    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::{{impl}}[1]::do_something_else[0]<u8> @@ vtable_through_const-mod1.volatile[External]
     mod1::TRAIT1_GEN_REF.do_something(0u8);
 
-    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::id[0]<char> @@ vtable_through_const[Internal]
+    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::id[0]<char> @@ vtable_through_const-mod1.volatile[External]
     mod1::ID_CHAR('x');
 }
diff --git a/src/test/run-make/stable-symbol-names/Makefile b/src/test/run-make/stable-symbol-names/Makefile
index c1b14440faa..3cbc5593ac0 100644
--- a/src/test/run-make/stable-symbol-names/Makefile
+++ b/src/test/run-make/stable-symbol-names/Makefile
@@ -9,14 +9,32 @@
 #  5. write the result into a file
 
 dump-symbols = nm "$(TMPDIR)/lib$(1).rlib" \
-             | grep -E "some_test_function|Bar|bar" \
+             | grep -E "$(2)" \
              | sed "s/.*\(_ZN.*E\).*/\1/" \
              | sort \
-             > "$(TMPDIR)/$(1).nm"
+             > "$(TMPDIR)/$(1)$(3).nm"
+
+# This test
+# - compiles each of the two crates 2 times and makes sure each time we get
+#   exactly the same symbol names
+# - makes sure that both crates agree on the same symbol names for monomorphic
+#   functions
 
 all:
 	$(RUSTC) stable-symbol-names1.rs
+	$(call dump-symbols,stable_symbol_names1,generic_|mono_,_v1)
+	rm $(TMPDIR)/libstable_symbol_names1.rlib
+	$(RUSTC) stable-symbol-names1.rs
+	$(call dump-symbols,stable_symbol_names1,generic_|mono_,_v2)
+	cmp "$(TMPDIR)/stable_symbol_names1_v1.nm" "$(TMPDIR)/stable_symbol_names1_v2.nm"
+
 	$(RUSTC) stable-symbol-names2.rs
-	$(call dump-symbols,stable_symbol_names1)
-	$(call dump-symbols,stable_symbol_names2)
-	cmp "$(TMPDIR)/stable_symbol_names1.nm" "$(TMPDIR)/stable_symbol_names2.nm"
+	$(call dump-symbols,stable_symbol_names2,generic_|mono_,_v1)
+	rm $(TMPDIR)/libstable_symbol_names2.rlib
+	$(RUSTC) stable-symbol-names2.rs
+	$(call dump-symbols,stable_symbol_names2,generic_|mono_,_v2)
+	cmp "$(TMPDIR)/stable_symbol_names2_v1.nm" "$(TMPDIR)/stable_symbol_names2_v2.nm"
+
+	$(call dump-symbols,stable_symbol_names1,mono_,_cross)
+	$(call dump-symbols,stable_symbol_names2,mono_,_cross)
+	cmp "$(TMPDIR)/stable_symbol_names1_cross.nm" "$(TMPDIR)/stable_symbol_names2_cross.nm"
diff --git a/src/test/run-make/stable-symbol-names/stable-symbol-names1.rs b/src/test/run-make/stable-symbol-names/stable-symbol-names1.rs
index 5c73ff0fab6..7344bdf49f6 100644
--- a/src/test/run-make/stable-symbol-names/stable-symbol-names1.rs
+++ b/src/test/run-make/stable-symbol-names/stable-symbol-names1.rs
@@ -11,26 +11,31 @@
 #![crate_type="rlib"]
 
 pub trait Foo {
-  fn foo<T>();
+  fn generic_method<T>();
 }
 
 pub struct Bar;
 
 impl Foo for Bar {
-  fn foo<T>() {}
+  fn generic_method<T>() {}
 }
 
-pub fn bar() {
-  Bar::foo::<Bar>();
+pub fn mono_function() {
+  Bar::generic_method::<Bar>();
 }
 
-pub fn some_test_function<T>(t: T) -> T {
+pub fn mono_function_lifetime<'a>(x: &'a u64) -> u64 {
+  *x
+}
+
+pub fn generic_function<T>(t: T) -> T {
   t
 }
 
 pub fn user() {
-  some_test_function(0u32);
-  some_test_function("abc");
+  generic_function(0u32);
+  generic_function("abc");
   let x = 2u64;
-  some_test_function(&x);
+  generic_function(&x);
+  let _ = mono_function_lifetime(&x);
 }
diff --git a/src/test/run-make/stable-symbol-names/stable-symbol-names2.rs b/src/test/run-make/stable-symbol-names/stable-symbol-names2.rs
index 097dce876af..eacba4ddb25 100644
--- a/src/test/run-make/stable-symbol-names/stable-symbol-names2.rs
+++ b/src/test/run-make/stable-symbol-names/stable-symbol-names2.rs
@@ -13,14 +13,16 @@
 extern crate stable_symbol_names1;
 
 pub fn user() {
-  stable_symbol_names1::some_test_function(1u32);
-  stable_symbol_names1::some_test_function("def");
+  stable_symbol_names1::generic_function(1u32);
+  stable_symbol_names1::generic_function("def");
   let x = 2u64;
-  stable_symbol_names1::some_test_function(&x);
+  stable_symbol_names1::generic_function(&x);
+  stable_symbol_names1::mono_function();
+  stable_symbol_names1::mono_function_lifetime(&0);
 }
 
 pub fn trait_impl_test_function() {
   use stable_symbol_names1::*;
-  Bar::foo::<Bar>();
-  bar();
+  Bar::generic_method::<Bar>();
 }
+