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

[Philip Reames via llvm-commits]

reames added a subscriber: reames.
reames added a comment.

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?

(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++ -*-===//
+//
----------------
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.

================
Comment at: include/llvm/IR/ThinLTOInfo.h:25
@@ +24,3 @@
+// Function summary information to aid in importing decisions.
+class ThinLTOFunctionSummary {
+ private:
----------------
Until this is filled in, the patch isn't really worth reviewing...

================
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
----------------
This shouldn't be a public flag...

================
Comment at: include/llvm/IR/ThinLTOInfo.h:46
@@ +45,3 @@
+  // index, to disambiguiate from other functions with the same name.
+  bool IsLocalFunction;
+
----------------
Why isn't the function linkage enough here?


http://reviews.llvm.org/D11721