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

[Teresa Johnson via Phabricator via llvm-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL291841: [ThinLTO] Import static functions from the same module as caller (authored by tejohnson).

Changed prior to commit:
  https://reviews.llvm.org/D28440?vs=84177&id=84178#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D28440

Files:
  llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
  llvm/trunk/test/ThinLTO/X86/local_name_conflict.ll


Index: llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
===================================================================
--- llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
+++ llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
@@ -124,7 +124,7 @@
 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) {
@@ -145,6 +145,21 @@
 
         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;
 
@@ -163,11 +178,13 @@
 /// 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 */,
@@ -202,7 +219,8 @@
     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;
Index: llvm/trunk/test/ThinLTO/X86/local_name_conflict.ll
===================================================================
--- llvm/trunk/test/ThinLTO/X86/local_name_conflict.ll
+++ llvm/trunk/test/ThinLTO/X86/local_name_conflict.ll
@@ -1,14 +1,25 @@
+; 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.
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D28440.84178.patch
Type: text/x-patch
Size: 5069 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170112/9799d93f/attachment.bin>