[PATCH] D17212: [ThinLTO] Support for call graph in per-module and combined summary.

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

I skimmed through and it sounds good to me, please see a few comments below.


================
Comment at: include/llvm/Bitcode/LLVMBitCodes.h:190-202
@@ -189,4 +189,15 @@
   enum FunctionSummarySymtabCodes {
-    FS_CODE_PERMODULE_ENTRY = 1,  // FS_ENTRY: [valueid, linkage, instcount]
-    FS_CODE_COMBINED_ENTRY  = 2,  // FS_ENTRY: [modid, linkage, instcount]
+    // PERMODULE_NOCALLS: [valueid, linkage, instcount]
+    FS_PERMODULE_NOCALLS = 1,
+    // PERMODULE_CALLS: [valueid, linkage, instcount, n x valueid]
+    FS_PERMODULE_CALLS = 2,
+    // PERMODULE_CALLS_PROFILE: [valueid, linkage, instcount,
+    //                           n x (valueid, count)]
+    FS_PERMODULE_CALLS_PROFILE = 3,
+    // COMBINED_NOCALLS: [modid, linkage, instcount]
+    FS_COMBINED_NOCALLS  = 4,
+    // COMBINED_CALLS: [modid, linkage, instcount, n x valueid]
+    FS_COMBINED_CALLS  = 5,
+    // COMBINED_CALLS_PROFILE: [modid, linkage, instcount, n x (valueid, count)]
+    FS_COMBINED_CALLS_PROFILE  = 6,
   };
----------------
Side note: we will need to nail the format in the coming weeks/months somehow, or have a plan for backward compatibility (auto upgrade...).

================
Comment at: include/llvm/Bitcode/LLVMBitCodes.h:195
@@ +194,3 @@
+    // PERMODULE_CALLS_PROFILE: [valueid, linkage, instcount,
+    //                           n x (valueid, count)]
+    FS_PERMODULE_CALLS_PROFILE = 3,
----------------
It is a bit annoying to have to manually overload for every "optional" data. It may also lead to a combinatory explosion if there were many. Unfortunately the only alternative in the current bitcode format is to have function summary stored as block instead of record...

================
Comment at: include/llvm/IR/FunctionInfo.h:97
@@ +96,3 @@
+
+  /// Record a call graph edge from this function to the function identified
+  /// by \p CalleeGUID, with cumulative profile count (across all calls from
----------------
> Without profile data, it might be useful to record the number of static callsites from a function to the callee. This can help compiler backend to make better global decisions later (e.g. better inlining and enable more function GC).

Can you elaborate how would you use this information and how it would help inlining and GC? 

================
Comment at: include/llvm/IR/FunctionInfo.h:298
@@ +297,3 @@
+  /// Get ownership of the function summary unique_ptr.
+  std::unique_ptr<FunctionSummary> getFunctionSummary() {
+    return std::move(Summary);
----------------
I think we usually prefix `takeXXX` and not `getXXX` when ownership is transferred.


http://reviews.llvm.org/D17212