Add 'thin_lto_imported' metadata to imported function

[eraman]

lib/Transforms/IPO/FunctionImport.cpp

597

It will be more useful to name this something like thin_lto_src_module and attach the source module's name.

[tejohnson]

lib/Transforms/IPO/FunctionImport.cpp

597

I like the module name idea as well. We'd want to ensure that the module name MDString is uniqued though.

614

Is the GV->GA name change in this block needed? Ah I see from the patch description that this was done for readability - can you make the renaming here and in the above loop a separate earlier NFC patch?

[Prazek]

lib/Transforms/IPO/FunctionImport.cpp

597

tl;dr I don't think there is problem with that.

do we really need to ensure that? In the worst case we will merge 2 the same nodes into one and we will not be able to distinguish one from another, but is there any way to make a unique name, that is readable to human?

There are also 'distinct' nodes, but I am not sure if we should use that.

614

What is NFC? Internet tells me "no fat chicks", but then my context analysis fails.
But I guess you mean to just make it separate patch. Can I get lgtm on this rename, so I won't have to post it in phabricator.

[tejohnson]

lib/Transforms/IPO/FunctionImport.cpp

597

I just meant that we don't want to have N copies of a full module name if we have N imports from that module. So hopefully it could be done in a way that each unique module name was an MDString and its metadata id would be referenced by the thin_lto_src_module metadata on the N imported functions.

614

It means No Functional Change. Since it is a simple renaming that would apply and you don't need to submit a patch for review in phab. See other commits on the llvm-commits mailing list with "(NFC)" on the subject line for reference.

[Prazek]

lib/Transforms/IPO/FunctionImport.cpp

597

oh yes, of course.

[tejohnson]

Prefer "thinlto_src_module" (s/thin_lto/thinlto) for consistency with other ThinLTO references in llvm.

LGTM with that change.

lib/Transforms/IPO/FunctionImport.cpp

for (auto &Name : ModuleNameOrderedList) {
     // If modules were created with lazy metadata loading, materialize it
     // now, before linking it (otherwise this will be a noop).
     SrcModule->materializeMetadata();
     UpgradeDebugInfo(*SrcModule);
 
     auto &ImportGUIDs = FunctionsToImportPerModule->second;
     // Find the globals to import
     DenseSet<const GlobalValue *> GlobalsToImport;
-    for (auto &GV : *SrcModule) {
-      if (!GV.hasName())
+    for (Function &F : *SrcModule) {
+      if (!F.hasName())
         continue;
-      auto GUID = GV.getGUID();
+      auto GUID = F.getGUID();
       auto Import = ImportGUIDs.count(GUID);
       DEBUG(dbgs() << (Import ? "Is" : "Not") << " importing function " << GUID
-                   << " " << GV.getName() << " from "
+                   << " " << F.getName() << " from "
                    << SrcModule->getSourceFileName() << "\n");
       if (Import) {
-        GV.materialize();
-        GlobalsToImport.insert(&GV);
+        F.materialize();
+        // Add 'thin_lto_imported' metadata for statistics and debugging.
+        F.setMetadata("thin_lto_imported",
+                      llvm::MDNode::get(DestModule.getContext(),
+                                        llvm::ArrayRef<llvm::Metadata *>()));
+        GlobalsToImport.insert(&F);
       }
     }
-    for (auto &GV : SrcModule->globals()) {
+    for (GlobalVariable &GV : SrcModule->globals()) {
       if (!GV.hasName())
         continue;
       auto GUID = GV.getGUID();
       auto Import = ImportGUIDs.count(GUID);
       DEBUG(dbgs() << (Import ? "Is" : "Not") << " importing global " << GUID
                    << " " << GV.getName() << " from "
                    << SrcModule->getSourceFileName() << "\n");
       if (Import) {
         GV.materialize();
         GlobalsToImport.insert(&GV);
       }
     }
-    for (auto &GV : SrcModule->aliases()) {
-      if (!GV.hasName())
+    for (GlobalAlias &GA : SrcModule->aliases()) {
+      if (!GA.hasName())
         continue;
-      auto GUID = GV.getGUID();
+      auto GUID = GA.getGUID();
       auto Import = ImportGUIDs.count(GUID);
       DEBUG(dbgs() << (Import ? "Is" : "Not") << " importing alias " << GUID
-                   << " " << GV.getName() << " from "
+                   << " " << GA.getName() << " from "
                    << SrcModule->getSourceFileName() << "\n");
       if (Import) {
         // 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. This has been handled by
         // computeImportForFunction()
-        GlobalObject *GO = GV.getBaseObject();
+        GlobalObject *GO = GA.getBaseObject();
         assert(GO->hasLinkOnceODRLinkage() &&
                "Unexpected alias to a non-linkonceODR in import list");
 #ifndef NDEBUG
         if (!GlobalsToImport.count(GO))
           DEBUG(dbgs() << " alias triggers importing aliasee " << GO->getGUID()
                        << " " << GO->getName() << " from "
                        << SrcModule->getSourceFileName() << "\n");
 #endif
         GO->materialize();
         GlobalsToImport.insert(GO);
-        GV.materialize();
-        GlobalsToImport.insert(&GV);
+        GA.materialize();
+        GlobalsToImport.insert(&GA);
       }
     }
 
     // Link in the specified functions.
     if (renameModuleForThinLTO(*SrcModule, Index, &GlobalsToImport))
       return true;
 
     if (PrintImports) {

test/Transforms/FunctionImport/funcimport.ll

 ; Do setup work for all below tests: generate bitcode and combined index
 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: opt -module-summary %p/Inputs/funcimport.ll -o %t2.bc
 ; RUN: llvm-lto -thinlto -print-summary-global-ids -o %t3 %t.bc %t2.bc 2>&1 | FileCheck %s --check-prefix=GUID
 
 ; Do the import now
 ; RUN: opt -disable-force-link-odr -function-import -stats -print-imports -summary-file %t3.thinlto.bc %t.bc -S 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=INSTLIMDEF
 ; "-stats" requires +Asserts.
 ; REQUIRES: asserts
 
 ; Test import with smaller instruction limit
 ; RUN: opt -disable-force-link-odr -function-import -summary-file %t3.thinlto.bc %t.bc -import-instr-limit=5 -S | FileCheck %s --check-prefix=CHECK --check-prefix=INSTLIM5
 ; INSTLIM5-NOT: @staticfunc.llvm.
 
 ; Test import with smaller instruction limit and without the -disable-force-link-odr
 ; RUN: opt -function-import -summary-file %t3.thinlto.bc %t.bc -import-instr-limit=5 -S | FileCheck %s --check-prefix=INSTLIM5ODR
-; INSTLIM5ODR: define linkonce_odr void @linkonceodr()
+; INSTLIM5ODR: define linkonce_odr void @linkonceodr() {
 
 
 define i32 @main() #0 {
 entry:
   call void (...) @weakalias()
   call void (...) @analias()
   call void (...) @linkoncealias()
   %call = call i32 (...) @referencestatics()

 declare void @analias(...) #1
 
 ; FIXME: Add this checking back when follow on fix to add alias summary
 ; records is committed.
 ; Aliases import the aliasee function
 declare void @linkoncealias(...) #1
 
 ; INSTLIMDEF-DAG: Import referencestatics
-; INSTLIMDEF-DAG: define available_externally i32 @referencestatics(i32 %i)
+; INSTLIMDEF-DAG: define available_externally i32 @referencestatics(i32 %i) !thin_lto_imported !0 {
 ; INSTLIM5-DAG: declare i32 @referencestatics(...)
 declare i32 @referencestatics(...) #1
 
 ; The import of referencestatics will expose call to staticfunc that
 ; should in turn be imported as a promoted/renamed and hidden function.
 ; Ensure that the call is to the properly-renamed function.
 ; INSTLIMDEF-DAG: Import staticfunc
 ; INSTLIMDEF-DAG: %call = call i32 @staticfunc.llvm.
-; INSTLIMDEF-DAG: define available_externally hidden i32 @staticfunc.llvm.
+; INSTLIMDEF-DAG: define available_externally hidden i32 @staticfunc.llvm.{{.*}} !thin_lto_imported !0 {
 
 ; INSTLIMDEF-DAG: Import referenceglobals
-; CHECK-DAG: define available_externally i32 @referenceglobals(i32 %i)
+; CHECK-DAG: define available_externally i32 @referenceglobals(i32 %i) !thin_lto_imported !0 {
 declare i32 @referenceglobals(...) #1
 
 ; The import of referenceglobals will expose call to globalfunc1 that
 ; should in turn be imported.
 ; INSTLIMDEF-DAG: Import globalfunc1
-; CHECK-DAG: define available_externally void @globalfunc1()
+; CHECK-DAG: define available_externally void @globalfunc1() !thin_lto_imported !0
 
 ; INSTLIMDEF-DAG: Import referencecommon
-; CHECK-DAG: define available_externally i32 @referencecommon(i32 %i)
+; CHECK-DAG: define available_externally i32 @referencecommon(i32 %i) !thin_lto_imported !0 {
 declare i32 @referencecommon(...) #1
 
 ; INSTLIMDEF-DAG: Import setfuncptr
-; CHECK-DAG: define available_externally void @setfuncptr()
+; CHECK-DAG: define available_externally void @setfuncptr() !thin_lto_imported !0 {
 declare void @setfuncptr(...) #1
 
 ; INSTLIMDEF-DAG: Import callfuncptr
-; CHECK-DAG: define available_externally void @callfuncptr()
+; CHECK-DAG: define available_externally void @callfuncptr() !thin_lto_imported !0 {
 declare void @callfuncptr(...) #1
 
 ; Ensure that all uses of local variable @P which has used in setfuncptr
 ; and callfuncptr are to the same promoted/renamed global.
 ; CHECK-DAG: @P.llvm.{{.*}} = external hidden global void ()*
 ; CHECK-DAG: %0 = load void ()*, void ()** @P.llvm.
 ; CHECK-DAG: store void ()* @staticfunc2.llvm.{{.*}}, void ()** @P.llvm.
 
 ; Ensure that @referencelargelinkonce definition is pulled in, but later we
 ; also check that the linkonceodr function is not.
-; CHECK-DAG: define available_externally void @referencelargelinkonce()
+; CHECK-DAG: define available_externally void @referencelargelinkonce() !thin_lto_imported !0 {
 ; INSTLIM5-DAG: declare void @linkonceodr()
 declare void @referencelargelinkonce(...)
 
 ; Won't import weak func
 ; CHECK-DAG: declare void @weakfunc(...)
 declare void @weakfunc(...) #1
 
 ; Won't import linkonce func
 ; CHECK-DAG: declare void @linkoncefunc2(...)
 declare void @linkoncefunc2(...) #1
 
 ; INSTLIMDEF-DAG: Import funcwithpersonality
-; INSTLIMDEF-DAG: define available_externally hidden void @funcwithpersonality.llvm.{{.*}}() personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+; INSTLIMDEF-DAG: define available_externally hidden void @funcwithpersonality.llvm.{{.*}}() personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) !thin_lto_imported !0 {
 ; INSTLIM5-DAG: declare hidden void @funcwithpersonality.llvm.{{.*}}()
 
 ; INSTLIMDEF-DAG: Import globalfunc2
 ; INSTLIMDEF-DAG: 13 function-import - Number of functions imported
 
 ; The actual GUID values will depend on path to test.
 ; GUID-DAG: GUID {{.*}} is weakalias
 ; GUID-DAG: GUID {{.*}} is referenceglobals