[ThinLTO] Import static functions from the same module as caller

[tejohnson]

Summary

We can sometimes end up with multiple copies of a local function that
have the same GUID in the index. This happens when there are local
functions with the same name that are in different source files with the
same name (but in different directories), and they were compiled in
their own directory so had the same path at compile time.

In this case make sure we import the copy in the caller's module. While
it isn't a correctness problem (the renamed reference which is based on the
module IR hash will be unique since the module must have had an
externally visible function that was imported), importing the wrong copy
will result in lost performance opportunity since it won't be referenced
and inlined.

[mehdi_amini]

LGTM.

[tejohnson]

Slight update to enable importing a local from another module when
there is a single summary for that GUID - in that case this must be
due to indirect call profile metadata. This was flagged by
test/Transforms/PGOProfile/thinlto_indirect_call_promotion.ll.

Note if there are multiple matching locals and there is indirect call
profile data pointing to them, we wouldn't know which to import
anyway (the profile data uses the same GUID).

llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp

 /// - The one with the least outgoing edges (already well optimized).
 /// - One from a module already being imported from in order to reduce the
 ///   number of source modules parsed/linked.
 /// - One that has PGO data attached.
 /// - [insert you fancy metric here]
 static const GlobalValueSummary *
 selectCallee(const ModuleSummaryIndex &Index,
              const GlobalValueSummaryList &CalleeSummaryList,
-             unsigned Threshold) {
+             unsigned Threshold, StringRef CallerModulePath) {
   auto It = llvm::find_if(
       CalleeSummaryList,
       [&](const std::unique_ptr<GlobalValueSummary> &SummaryPtr) {
         auto *GVSummary = SummaryPtr.get();
         if (GlobalValue::isInterposableLinkage(GVSummary->linkage()))
           // There is no point in importing these, we can't inline them
           return false;
         if (auto *AS = dyn_cast<AliasSummary>(GVSummary)) {
           GVSummary = &AS->getAliasee();
           // Alias can't point to "available_externally". However when we import
           // linkOnceODR the linkage does not change. So we import the alias
           // and aliasee only in this case.
           // FIXME: we should import alias as available_externally *function*,
           // the destination module does need to know it is an alias.
           if (!GlobalValue::isLinkOnceODRLinkage(GVSummary->linkage()))
             return false;
         }
 
         auto *Summary = cast<FunctionSummary>(GVSummary);
 
+        // If this is a local function, make sure we import the copy
+        // in the caller's module. The only time a local function can
+        // share an entry in the index is if there is a local with the same name
+        // in another module that had the same source file name (in a different
+        // directory), where each was compiled in their own directory so there
+        // was not distinguishing path.
+        // However, do the import from another module if there is only one
+        // entry in the list - in that case this must be a reference due
+        // to indirect call profile data, since a function pointer can point to
+        // a local in another module.
+        if (GlobalValue::isLocalLinkage(Summary->linkage()) &&
+            CalleeSummaryList.size() > 1 &&
+            Summary->modulePath() != CallerModulePath)
+          return false;
+
         if (Summary->instCount() > Threshold)
           return false;
 
         if (Summary->notEligibleToImport())
           return false;
 
         return true;
       });
   if (It == CalleeSummaryList.end())
     return nullptr;
 
   return cast<GlobalValueSummary>(It->get());
 }
 
 /// Return the summary for the function \p GUID that fits the \p Threshold, or
 /// null if there's no match.
 static const GlobalValueSummary *selectCallee(GlobalValue::GUID GUID,
                                               unsigned Threshold,
-                                              const ModuleSummaryIndex &Index) {
+                                              const ModuleSummaryIndex &Index,
+                                              StringRef CallerModulePath) {
   auto CalleeSummaryList = Index.findGlobalValueSummaryList(GUID);
   if (CalleeSummaryList == Index.end())
     return nullptr; // This function does not have a summary
-  return selectCallee(Index, CalleeSummaryList->second, Threshold);
+  return selectCallee(Index, CalleeSummaryList->second, Threshold,
+                      CallerModulePath);
 }
 
 using EdgeInfo = std::tuple<const FunctionSummary *, unsigned /* Threshold */,
                             GlobalValue::GUID>;
 
 /// Compute the list of functions to import for a given caller. Mark these
 /// imported functions and the symbols they reference in their source module as
 /// exported from their source module.
auto GetBonusMultiplier = [](CalleeInfo::HotnessType Hotness) -> float {
       if (Hotness == CalleeInfo::HotnessType::Cold)
         return ImportColdMultiplier;
       return 1.0;
     };
 
     const auto NewThreshold =
         Threshold * GetBonusMultiplier(Edge.second.Hotness);
 
-    auto *CalleeSummary = selectCallee(GUID, NewThreshold, Index);
+    auto *CalleeSummary =
+        selectCallee(GUID, NewThreshold, Index, Summary.modulePath());
     if (!CalleeSummary) {
       DEBUG(dbgs() << "ignored! No qualifying callee with summary found.\n");
       continue;
     }
     // "Resolve" the summary, traversing alias,
     const FunctionSummary *ResolvedCalleeSummary;
     if (isa<AliasSummary>(CalleeSummary)) {
       ResolvedCalleeSummary = cast<FunctionSummary>(

llvm/trunk/test/ThinLTO/X86/local_name_conflict.ll

+; Test handling when two files with the same source file name contain
+; static functions with the same name (which will have the same GUID
+; in the combined index.
+
 ; Do setup work for all below tests: generate bitcode and combined index
 ; RUN: opt -module-summary -module-hash %s -o %t.bc
 ; RUN: opt -module-summary -module-hash %p/Inputs/local_name_conflict1.ll -o %t2.bc
 ; RUN: opt -module-summary -module-hash %p/Inputs/local_name_conflict2.ll -o %t3.bc
 ; RUN: llvm-lto -thinlto-action=thinlink -o %t4.bc %t.bc %t2.bc %t3.bc
 
-; Make sure foo is promoted and renamed without complaint in both
-; Inputs/local_name_conflict1.ll and Inputs/local_name_conflict2.ll
-; FIXME: Once the importer is fixed to import the correct copy of the
-; local, we should be able to verify that via an import action.
-; RUN: llvm-lto -thinlto-action=promote %t2.bc -thinlto-index=%t4.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=EXPORTSTATIC
+; This module will import b() which should cause the copy of foo from
+; that module (%t3.bc) to be imported. Check that the imported reference's
+; promoted name matches the imported copy.
+; RUN: llvm-lto -thinlto-action=import %t.bc -thinlto-index=%t4.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=IMPORT
+; IMPORT: call i32 @foo.llvm.[[HASH:[0-9A-F]+]]
+; IMPORT: define available_externally hidden i32 @foo.llvm.[[HASH]]()
+
+; The copy in %t2.bc should not be exported/promoted/renamed
+; RUN: llvm-lto -thinlto-action=promote %t2.bc -thinlto-index=%t4.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=NOEXPORTSTATIC
+; NOEXPORTSTATIC: define internal i32 @foo()
+
+; Make sure foo is promoted and renamed without complaint in %t3.bc.
 ; RUN: llvm-lto -thinlto-action=promote %t3.bc -thinlto-index=%t4.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=EXPORTSTATIC
 ; EXPORTSTATIC: define hidden i32 @foo.llvm.
 
 ; ModuleID = 'local_name_conflict_main.o'
 source_filename = "local_name_conflict_main.c"
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"