Add 'thin_lto_imported' metadata to imported function

[eraman]

lib/Transforms/IPO/FunctionImport.cpp

597 ↗

(On Diff #62557)

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 ↗

(On Diff #62557)

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

614 ↗

(On Diff #62557)

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 ↗

(On Diff #62557)

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 ↗

(On Diff #62557)

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 ↗

(On Diff #62557)

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 ↗

(On Diff #62557)

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 ↗

(On Diff #62557)

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.

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

for (Function &F : *SrcModule) {
         continue;
       auto GUID = F.getGUID();
       auto Import = ImportGUIDs.count(GUID);
       DEBUG(dbgs() << (Import ? "Is" : "Not") << " importing function " << GUID
                    << " " << F.getName() << " from "
                    << SrcModule->getSourceFileName() << "\n");
       if (Import) {
         F.materialize();
+        // Add 'thinlto_src_module' metadata for statistics and debugging.
+        F.setMetadata("thinlto_src_module",
+                      llvm::MDNode::get(DestModule.getContext(),
+                                        {llvm::MDString::get(
+                                            DestModule.getContext(),
+                                            SrcModule->getSourceFileName())}));
         GlobalsToImport.insert(&F);
       }
     }
     for (GlobalVariable &GV : SrcModule->globals()) {
       if (!GV.hasName())
         continue;
       auto GUID = GV.getGUID();
       auto Import = ImportGUIDs.count(GUID);

llvm/trunk/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) !thinlto_src_module !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.{{.*}} !thinlto_src_module !0 {
 
 ; INSTLIMDEF-DAG: Import referenceglobals
-; CHECK-DAG: define available_externally i32 @referenceglobals(i32 %i)
+; CHECK-DAG: define available_externally i32 @referenceglobals(i32 %i) !thinlto_src_module !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() !thinlto_src_module !0
 
 ; INSTLIMDEF-DAG: Import referencecommon
-; CHECK-DAG: define available_externally i32 @referencecommon(i32 %i)
+; CHECK-DAG: define available_externally i32 @referencecommon(i32 %i) !thinlto_src_module !0 {
 declare i32 @referencecommon(...) #1
 
 ; INSTLIMDEF-DAG: Import setfuncptr
-; CHECK-DAG: define available_externally void @setfuncptr()
+; CHECK-DAG: define available_externally void @setfuncptr() !thinlto_src_module !0 {
 declare void @setfuncptr(...) #1
 
 ; INSTLIMDEF-DAG: Import callfuncptr
-; CHECK-DAG: define available_externally void @callfuncptr()
+; CHECK-DAG: define available_externally void @callfuncptr() !thinlto_src_module !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() !thinlto_src_module !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*) !thinlto_src_module !0 {
 ; INSTLIM5-DAG: declare hidden void @funcwithpersonality.llvm.{{.*}}()
 
 ; INSTLIMDEF-DAG: Import globalfunc2
 ; INSTLIMDEF-DAG: 13 function-import - Number of functions imported
+; CHECK-DAG: !0 = !{!"{{.*}}/Inputs/funcimport.ll"}
 
 ; The actual GUID values will depend on path to test.
 ; GUID-DAG: GUID {{.*}} is weakalias
 ; GUID-DAG: GUID {{.*}} is referenceglobals
 ; GUID-DAG: GUID {{.*}} is weakfunc
 ; GUID-DAG: GUID {{.*}} is main
 ; GUID-DAG: GUID {{.*}} is referencecommon
 ; GUID-DAG: GUID {{.*}} is analias