Add instruction_count MD to imported functions

[Prazek]

Summary

To check what kind of functions that get imported are inlined
I need the function size, counted while importing.

[Prazek]

include/llvm/Transforms/IPO/FunctionImport.h

32

This one is small change, that I don't know if you like or not, so I didn't want to push it without review.

include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h

80 ↗

(On Diff #66738)

This one is should not be part of patch. I will remove it.

[Prazek]

removed some line that I will post in text patch

[eraman]

I can imaging you wanting other summary information (callsite count, for example) too. Why not create a broader summary metadata and put everything there?

include/llvm/Transforms/IPO/FunctionImport.h

32

It seems unnecessary to me, but if you do this change you should use ImportThresholdTy everywhere Threshold is used.

[Prazek]

In D23144#505361, @eraman wrote:

I can imaging you wanting other summary information (callsite count, for example) too. Why not create a broader summary metadata and put everything there?

I don't have plans right now of using such information. Do you have idea how helpful it could be? If not then YAGNI

[Prazek]

include/llvm/Transforms/IPO/FunctionImport.h

32

ok maybe I will look for such places. I just hate to see code that is trying to say something using comments instead of just using code (like types)

[eraman]

In D23144#505373, @Prazek wrote:

In D23144#505361, @eraman wrote:

I can imaging you wanting other summary information (callsite count, for example) too. Why not create a broader summary metadata and put everything there?

I don't have plans right now of using such information. Do you have idea how helpful it could be? If not then YAGNI

Ok, fair enough, given that the importing heuristic only uses inst count currently. If and when CallGraphEdgeList is used (particularly some of callee profile counts), I would expect that it'll be useful to have that data but can be added then.

[tejohnson]

In D23144#505373, @Prazek wrote:

In D23144#505361, @eraman wrote:

I can imaging you wanting other summary information (callsite count, for example) too. Why not create a broader summary metadata and put everything there?

I don't have plans right now of using such information. Do you have idea how helpful it could be? If not then YAGNI

Streaming out the module summary to LLVM assembly has been on my TODO for awhile and would probably make all of this unnecessary. But that is a larger task. I would say just add what you need for now, since we may rip it out if/when the summary streaming is done.

BTW, the description would be clearer if you change "counted while importing" to "recorded from the summary during importing". I misunderstood what the patch was doing from the description, until I looked at the changes.

include/llvm/Transforms/IPO/FunctionImport.h

32

This is unrelated to the rest of the change and should be committed as a separate patch, if at all, but I don't think it is really necessary.

lib/Transforms/IPO/FunctionImport.cpp

604

Why did this move? I guess it makes more sense to have it before the optional EnableImportMetadata block, but could you commit that separately as an NFC patch?

620

"thinlto_inst_count" would make it clearer.

test/Transforms/FunctionImport/funcimport.ll

70

Check for !instructions_count here and elsewhere?

[Prazek]

test/Transforms/FunctionImport/funcimport.ll

70

Can I don't match for the count?

[Prazek]

Update

[tejohnson]

test/Transforms/FunctionImport/funcimport.ll

70

Yeah that's fine

[tejohnson]

Let's discuss Mehdi's point about simply using the summary to get this information during inlining first.

include/llvm/Transforms/IPO/FunctionImport.h

 class LLVMContext;
 class GlobalValueSummary;
 class Module;
 
 /// The function importer is automatically importing function from other modules
 /// based on the provided summary informations.
 class FunctionImporter {
 public:
+  using ImportThresholdTy = unsigned;
   /// Set of functions to import from a source module. Each entry is a map
   /// containing all the functions to import for a source module.
   /// The keys is the GUID identifying a function to import, and the value
   /// is the threshold applied when deciding to import it.
-  typedef std::map<GlobalValue::GUID, unsigned> FunctionsToImportTy;
+  typedef std::map<GlobalValue::GUID, ImportThresholdTy> FunctionsToImportTy;
 
   /// The map contains an entry for every module to import from, the key being
   /// the module identifier to pass to the ModuleLoader. The value is the set of
   /// functions to import.
   typedef StringMap<FunctionsToImportTy> ImportMapTy;
 
   /// The set contains an entry for every global value the module exports.
   typedef std::unordered_set<GlobalValue::GUID> ExportSetTy;

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();
+        GlobalsToImport.insert(&F);
+
         if (EnableImportMetadata) {
           // 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())}));
+
+          const auto InstructionCount =
+              cast<FunctionSummary>(
+                  Index.getGlobalValueSummary(F.getGUID(), false))
+                  ->instCount();
+          F.setMetadata(
+              "instructions_count",
+              llvm::MDNode::get(
+                  DestModule.getContext(),
+                  {llvm::ConstantAsMetadata::get(ConstantInt::getIntegerValue(
+                      Type::getInt32Ty(DestModule.getContext()),
+                      APInt(32, InstructionCount)))}));
         }
-        GlobalsToImport.insert(&F);
       }
     }
     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

test/Transforms/FunctionImport/funcimport.ll

 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) !thinlto_src_module !0 {
+; INSTLIMDEF-DAG: define available_externally i32 @referencestatics(i32 %i) !thinlto_src_module ![[MOD:.*]] !instructions_count
 ; 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.{{.*}} !thinlto_src_module !0 {
+; INSTLIMDEF-DAG: define available_externally hidden i32 @staticfunc.llvm.{{.*}} !thinlto_src_module ![[MOD]]
 
 ; INSTLIMDEF-DAG: Import referenceglobals
-; CHECK-DAG: define available_externally i32 @referenceglobals(i32 %i) !thinlto_src_module !0 {
+; CHECK-DAG: define available_externally i32 @referenceglobals(i32 %i) !thinlto_src_module ![[MOD:.*]] !instructions_count ![[COUNT:.*]] {
 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() !thinlto_src_module !0
+; CHECK-DAG: define available_externally void @globalfunc1() !thinlto_src_module ![[MOD]]
 
 ; INSTLIMDEF-DAG: Import referencecommon
-; CHECK-DAG: define available_externally i32 @referencecommon(i32 %i) !thinlto_src_module !0 {
+; CHECK-DAG: define available_externally i32 @referencecommon(i32 %i) !thinlto_src_module ![[MOD]]
 declare i32 @referencecommon(...) #1
 
 ; INSTLIMDEF-DAG: Import setfuncptr
-; CHECK-DAG: define available_externally void @setfuncptr() !thinlto_src_module !0 {
+; CHECK-DAG: define available_externally void @setfuncptr() !thinlto_src_module ![[MOD]]
 declare void @setfuncptr(...) #1
 
 ; INSTLIMDEF-DAG: Import callfuncptr
-; CHECK-DAG: define available_externally void @callfuncptr() !thinlto_src_module !0 {
+; CHECK-DAG: define available_externally void @callfuncptr() !thinlto_src_module ![[MOD]]
 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() !thinlto_src_module !0 {
+; CHECK-DAG: define available_externally void @referencelargelinkonce() !thinlto_src_module ![[MOD]]
 ; 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*) !thinlto_src_module !0 {
+; 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"}
+
+; CHECK-DAG: !{{.*}} = !{!"{{.*}}/Inputs/funcimport.ll"}
+; CHECK-DAG: ![[COUNT]] = !{i32 5}
 
 ; 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