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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In http://reviews.llvm.org/D11721#223084, @reames wrote:

> At a very high level, the parallel data structures being introduced here really bother me.  We already have a materialization mechanism for lazily loading functions into the module.  I know that we fairly eagerly materialize today, but have you considered a) materializing less eagerly and b) merging your index structures into the materialization mechanisms?


By parallel assume you mean the fact that this is outside the Module structure, and that the associated object file interfaces in http://reviews.llvm.org/D11723 are separate from the IRObjectFile data structures?

The reasons are discussed a bit in the RFC, but it is buried in there, so I copied the motivation here, which essentially boils down to the fact that the module's ThinLTO information is not needed when parsing the rest of the module and vice versa:

"In order to save time and memory in the plugin step, we don’t want to parse the entire bitcode for a module during the plugin step (which only needs to read the function index/summary for merging into a combined index/summary). We also don’t need to parse the ThinLTO information out of the module’s IR when constructing the Module object during the backend compile step (the ThinLTO importing step will read the combined index file, not the module’s own ThinLTO index).

Therefore any data structure created to encapsulate ThinLTO function index and summary information should be independent of LTOModule and IRObjectFile, as those structures are created when we read/parse the module’s normal (non-ThinLTO) bitcode, and result in the creation of a Module object. "

Let me know if I misinterpreted your concern.

> (Small comments inline, but by no means a complete review.)



================
Comment at: include/llvm/IR/ThinLTOInfo.h:1
@@ +1,2 @@
+//===-- llvm/ThinLTOInfo.h - ThinLTO Index and Summary ----------*- C++ -*-===//
+//
----------------
reames wrote:
> Meta comment: This shouldn't be ThinLTO specific.  If you want to propose adding an index to the IR, that's fine, but it needs to be extension used by everything, not something specific to ThinLTO.
I thought the information here was very ThinLTO specific. It isn't used when parsing the rest of the module, but is meant to hold information required for making ThinLTO importing decisions and doing the importing. Do you think some of this is useful in other cases?

================
Comment at: include/llvm/IR/ThinLTOInfo.h:25
@@ +24,3 @@
+// Function summary information to aid in importing decisions.
+class ThinLTOFunctionSummary {
+ private:
----------------
reames wrote:
> Until this is filled in, the patch isn't really worth reviewing...
I'll go ahead and add a field I was using in my prototype implementation, specifically the function's instruction count. This data structure will evolve as we tune the importing heuristics. 

================
Comment at: include/llvm/IR/ThinLTOInfo.h:42
@@ +41,3 @@
+  // True if we are still parsing into this instance.
+  bool Parsing;
+  // Used to flag functions that have local linkage types and need to
----------------
reames wrote:
> This shouldn't be a public flag...
None of these should be actually, looks like I accidentally left out the "private" at the top.

================
Comment at: include/llvm/IR/ThinLTOInfo.h:46
@@ +45,3 @@
+  // index, to disambiguiate from other functions with the same name.
+  bool IsLocalFunction;
+
----------------
reames wrote:
> Why isn't the function linkage enough here?
Because we don't want to parse all of the IR when building the combined index during the plugin step. See the ThinLTOFunctionSummaryIndex::mergeFrom() implementation below where this is used to do renaming of the functions in the combined index, to disambiguate local functions from different modules.


http://reviews.llvm.org/D11721