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

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/Bitcode/LLVMBitCodes.h:190
@@ -189,3 +189,3 @@
   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,
----------------
tejohnson wrote:
> davidxl wrote:
> > A side note -- it might be useful to reserve some bits for general function attributes such as 'address-taken' etc.
> What is the advantage of saving bits now? The format is still highly in flux anyway, and once it is nailed down eventually we would need to use some kind of version id to specify the change regardless of whether it was using saved bits or not.
ok.


================
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
----------------
tejohnson wrote:
> davidxl wrote:
> > 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).
> I could do that by overloading the count field to hold the static number of callsites. Alternatively, for statically-generated profile information is it useful to record the block frequency sums or something like that?
Regarding static # of callsites -- another way is to build a multi-graph -- basically do not collapse all edges from one caller to the same callee. How much space do we save there?  

If we want to save space, there is a better way to do this -- we can build a graph with edges from module to the callee.  Such a module->function edge contains the following information :
{ Aggregate call count,  max call count, static # of sites }

It is probably not useful to record static block frequency.

================
Comment at: include/llvm/IR/FunctionInfo.h:286
@@ +285,3 @@
+  /// or built during the per-module bitcode write process.
+  std::unique_ptr<FunctionSummary> Summary;
+  /// Hold a pointer to the above summary, so that we can still access it after
----------------
tejohnson wrote:
> davidxl wrote:
> > Is this member really needed? The helper is not intended to own the summary.
> The helper does own the function summary temporarily. We create it when the function summary section is read, then later transfer ownership to the index when the VST is read. The reason why I need to keep a non-owning pointer to the summary is that later on after all VST entries are read from the combined index I need to set up the call graph edges in the combined index, and for that I need to keep the association here between the function summary and the CallGraphEdgeValueIdList.
owns it temporarily in what sense? Does it have a chance to de-allocate it?


http://reviews.llvm.org/D17212