[PATCH] D20268: [wip] Resolution-based LTO API.

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Thanks, at first glance this looks like a clearer set of interfaces. I'll need some more time to get through it all in detail, just a few comments/questions below after getting through the first bit now.


================
Comment at: include/llvm/CodeGen/LTOBackend.h:85
@@ +84,3 @@
+
+  /// A combined index hook is called after all module indices have been
+  /// combined. It can be used to implement -save-temps for the combined index.
----------------
Add your "(ThinLTO-specific)" tag here?

================
Comment at: include/llvm/CodeGen/LTOBackend.h:114
@@ +113,3 @@
+
+void upgradeLinkage(GlobalValue *GV);
+
----------------
doxygen comment

================
Comment at: include/llvm/LTO/LTO.h:34
@@ +33,3 @@
+
+class InputFile {
+  friend LTO;
----------------
Needs description

================
Comment at: include/llvm/LTO/LTO.h:181
@@ +180,3 @@
+
+/// This ThinBackend writes individual module indexes to files, instead of
+/// running the individual backend jobs. To find the path to write the index to
----------------
Indicate somewhere that this is for distributed builds where separate processes will invoke the real backends?

================
Comment at: include/llvm/LTO/LTO.h:214
@@ +213,3 @@
+  /// this constructor.
+  /// FIXME: We do currently require the DiagHandler field to be set in Conf.
+  LTO(Config Conf = {}, ThinBackend Backend = nullptr,
----------------
Make the Config argument non-optional in the meantime?

================
Comment at: include/llvm/LTO/LTO.h:242
@@ +241,3 @@
+
+  ModuleSummaryIndex CombinedIndex;
+  StringMap<MemoryBufferRef> ModuleMap;
----------------
Add comments to the members to indicate which are for regular vs thin LTO?

================
Comment at: include/llvm/LTO/LTO.h:259
@@ +258,3 @@
+    /// Partitions generally have a one-to-one correspondence with tasks, except
+    /// that partitions are not split during parallel LTO code generation.
+    size_t Partition = Unknown;
----------------
should this be "partitions are split during parallel LTO cod generation"? (i.e. remove the "not")

================
Comment at: include/llvm/LTO/LTO.h:276
@@ +275,3 @@
+  // IRMover and all possible cross-partition references have been seen.
+  StringMap<GlobalResolution> GlobalResolutions;
+
----------------
Will this be used for ThinLTO as well? In that case the comment about linking all modules via IRMover doesn't apply. Maybe "cannot be done until all possible cross-partition references have been seen. For regular LTO this is after all modules have been linked by the IRMover and for ThinLTO it is after all summaries have been linked into the combined index." 


http://reviews.llvm.org/D20268