[PATCH] D23144: Add instruction_count MD to imported functions

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D23144#505373, @Prazek wrote:

> In https://reviews.llvm.org/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.


================
Comment at: include/llvm/Transforms/IPO/FunctionImport.h:32
@@ -31,2 +31,3 @@
 public:
+  using ImportThresholdTy = unsigned;
   /// Set of functions to import from a source module. Each entry is a map
----------------
Prazek wrote:
> eraman wrote:
> > It seems unnecessary to me, but if you do this change you should use ImportThresholdTy everywhere Threshold is used.
> 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)
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.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:604
@@ -603,1 +603,3 @@
         F.materialize();
+        GlobalsToImport.insert(&F);
+
----------------
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?

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:620
@@ +619,3 @@
+          F.setMetadata(
+              "instructions_count",
+              llvm::MDNode::get(
----------------
"thinlto_inst_count" would make it clearer.

================
Comment at: test/Transforms/FunctionImport/funcimport.ll:70
@@ -69,3 +69,3 @@
 ; 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]]
 
----------------
Check for !instructions_count here and elsewhere?


https://reviews.llvm.org/D23144