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

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/IR/ThinLTOInfo.h:26
@@ +25,3 @@
+class ThinLTOFunctionSummary {
+ private:
+  // Number of instructions (ignoring debug instructions, e.g.) computed
----------------
Should also include body bitcode offset info, right?

================
Comment at: include/llvm/IR/ThinLTOInfo.h:47
@@ +46,3 @@
+  // Path of module containing function IR
+  StringRef ModulePath;
+  // Index of function within bitcode module block in its module.
----------------
tejohnson wrote:
> davidxl wrote:
> > This field 'ModulePath' belongs to ThinLTOFunctionSummaryIndex class (when used before combining).
> But it can't be on the ThinLTOFunctionSummaryIndex class in the combined index. I could have a ModulePath on the ThinLTOFunctionSummaryIndex class just for the case of the per-module index, but it seemed confusing to have it in two places (and which one is used depends on the context). Since StringRefs are small and fast to construct (just a pointer to a char * and a length), and we need one here anyway for the combined case, it seemed clearer to me to always keep it in the per-function info. For the per-module case this would be populated when we read in the per-module ThinLTO information from the IR.
ok.

================
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
----------------
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.

================
Comment at: include/llvm/IR/ThinLTOInfo.h:116
@@ +115,3 @@
+typedef StringMap<ThinLTOFunctionInfoList> ThinLTOFunctionMap;
+typedef StringMap<uint64_t> ThinLTOModulePathStringTable;
+
----------------
tejohnson wrote:
> davidxl wrote:
> > Why making a reverse map from stringPath to module id instead of a map from module id to module path?
> > 
> > In the combined summary index, ThinLTOFunctionInfo object should contain a module id field. From that module id, the module path can be obtained.
> The string table map is not queried directly, as described in the associated RFC it is just for owning the module strings, since the string is duplicated when inserting to the StringMap. We don't need to save the module ID and use it to look up in the string table since it is fast and fairly efficient just to save the StringRef directly.
> 
> I will update the comments in this file to reflect some of the info in the RFC and make it hopefully a lot clearer.
ok.

================
Comment at: include/llvm/IR/ThinLTOInfo.h:124
@@ +123,3 @@
+
+ private:
+  // Map from function name to list of function information instances
----------------
tejohnson wrote:
> davidxl wrote:
> > I think it is better to have a ModulePath field in this class -- this field will only be used for per-module SummaryIndex.
> In the combined index we need a module path in the function info, as noted earlier. I think it is confusing to have module paths in multiple places used in different contexts.
ok


http://reviews.llvm.org/D11721