[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: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:
> > > 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.
> Right -- if the IO helper class only conditionally pass the ownership to functionIndex in some cases, the unique pointer is needed. However if it is intended to be always passed to another object, the helper just need to keep a naked pointer to the summary as it does not really own it.
In the BitcodeWriter it stays owned by the helper. In the BitcodeReader ownership is always transferred to the in-memory function index. Even in the latter case, I think creating a unique_ptr immediately, rather than creating a naked pointer and only creating a unique_ptr when the VST is read, is cleaner as it makes the current ownership clear and avoids malloc.


http://reviews.llvm.org/D17212