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

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
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
----------------
davidxl wrote:
> davidxl wrote:
> > 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.
> My suggestion regarding using module->function edge can not replace the aggregate function->function call edge here -- but it can be emitted as additional information to track static callsite info -- but that is independent of what this patch does.
Right, we still want the function->function edges for better precision. Will consider module->function edges at a later point.

I will need to do some measurements to see what the overhead is of not collapsing the edges within each function. But if we simply add a static callsite count (possibly both with and without PGO), I think that may be sufficient.

================
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
----------------
davidxl wrote:
> 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?
It passes ownership to the function index in memory when the corresponding VST entry is created. After all the VST entries are created, it uses the saved (non-owned) pointer to create the call graph edges in the function index in memory (need to wait until all the VST entries are parsed as we need to know all the value id and GUIDs). See FunctionIndexBitcodeReader::parseValueSymbolTable (note the function return is in the EndBlock case in the switch statement, not at the bottom of the routine).

The other alternative would be to keep a mapping there from the FunctionSummaryIOHelper object to the corresponding FunctionSummary pointer (created when we parse the corresponding VST entry and transfer the ownership of the unique_ptr to the function index). I think that is probably cleaner and more straightforward. Will make that change.

As an aside, I just noticed that the ValueIdToCallGraphGUIDMap is only used in this routine, I will move it to function local instead of being on the FunctionIndexBitcodeReader class.


http://reviews.llvm.org/D17212