[PATCH] D11721: [ThinLTO] Data structures for holding ThinLTO function index/summary

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

I am going to upload a new patch now that addresses the past few rounds of comments.

I'm also going to upload a new patch http://reviews.llvm.org/D11723 in a few minutes that uses the mergeFrom interface to create a combined index in the gold plugin.


================
Comment at: include/llvm/IR/ThinLTOInfo.h:26
@@ +25,3 @@
+class ThinLTOFunctionSummary {
+ private:
+  // Number of instructions (ignoring debug instructions, e.g.) computed
----------------
davidxl wrote:
> Should also include body bitcode offset info, right?
Yes, that is going to move here in the new patch I am about to upload here.

================
Comment at: include/llvm/IR/ThinLTOInfo.h:51
@@ +50,3 @@
+  // Function summary information used to help make importing decisions.
+  ThinLTOFunctionSummary *FunctionSummary;
+  // Used during bitcode parsing and writing to hold the offset of the
----------------
davidxl wrote:
> tejohnson wrote:
> > davidxl wrote:
> > > Is there a 1-1 mapping from ThinLTOFunctionInfo to FunctionSummary? If yes, can ThinLTOFunctionInfo inherite from FunctionSummary?
> > When creating the combined index it is not 1-1. Another reason why I have a pointer here instead of inheriting or combining the two is to be more efficient in the case where the function summary information is read lazily during importing. I.e. initially only the ThinLTO symtab and module paths are read, and each function's summary is only read when considering importing that function.
> > 
> > In fact, I should move the BitcodeIndex and ModulePath into the ThinLTOFunctionSummary object for the same reason - they are only populated when the function summary is read/parsed. Will make that change to the patch.
> Makes sense.
> 
> I think it is better to add more comments before the FunctionSummary declaration to indicate this enables lazy reading of summary from combined summary index file during importing so that it is more memory efficient.  Also document that ThinLTOFunctionInfo should only include minimal info required to locate the summary etc.
Added some comments in the new patch.


http://reviews.llvm.org/D11721