[PATCH] D17066: libLTO: add a ThinLTOCodeGenerator on the model of LTOCodeGenerator.

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Did an initial pass through, some comments and questions below.


================
Comment at: include/llvm-c/lto.h:563
@@ +562,3 @@
+/**
+ * Type to wrap returned objects to the linker for ThinLTO.
+ *
----------------
Wraps a single returned object file? (Comment for this and following structure are currently identical)

================
Comment at: include/llvm-c/lto.h:586
@@ +585,3 @@
+ *
+ * \since prior to LTO_API_VERSION=18
+ */
----------------
Why "prior to" here and other places?

================
Comment at: include/llvm-c/lto.h:625
@@ +624,3 @@
+ * Optimize and codegen all the modules added to the codegenerator using
+ * ThinLTO.
+ *
----------------
Document return type?

================
Comment at: include/llvm/LTO/ThinLTOCodeGenerator.h:52
@@ +51,3 @@
+   * code. If a symbol is not listed there, it might be inlined into every usage
+   * and optimized away.
+   */
----------------
do you mean "If a symbol is not listed there, it will be optimized away if it is inlined into every usage"?

================
Comment at: include/llvm/LTO/ThinLTOCodeGenerator.h:82
@@ +81,3 @@
+  std::vector<MemoryBufferRef> Modules;
+  StringSet<> PreservedSymbols;
+  /// Cache for incremental build.
----------------
Doxygen comments on these and other members as well?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:64
@@ +63,3 @@
+            ModuleBuffer, diagnosticHandler,
+            false); // FIXME Should it be lazy?
+    if (std::error_code EC = ObjOrErr.getError()) {
----------------
No, should not be lazy. That is just for reading the combined index.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:175
@@ +174,3 @@
+    PMB.Inliner = createFunctionInliningPass();
+    //  PMB.OptLevel = OptLevel;
+    PMB.VerifyInput = false;
----------------
Why don't we want to set an opt level?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:248
@@ +247,3 @@
+  auto Linkage = GV.getLinkage();
+  if (Linkage == GlobalValue::AvailableExternallyLinkage) {
+    Keep.insert(GV.getName());
----------------
This case was already handled above.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:297
@@ +296,3 @@
+  Mang.getNameWithPrefix(Buffer, &GV, false);
+  if (PreservedSymbols.count(Buffer)) {
+    Keep.insert(GV.getName());
----------------
So the linker must place any external symbols with references from other modules in this set, right? I see that you added a comment that suggest that to the thinlto_codegen_add_must_preserve_symbol interface. Might want to add something to that effect here as well.

================
Comment at: tools/llvm-lto/llvm-lto.cpp:279
@@ +278,3 @@
+      //      InitTargetOptionsFromCodeGenFlags(), ErrorMsg));
+      // Can't do that, we want to leak the buffer instead, ld64 style!
+      ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr =
----------------
Is this temporary?

================
Comment at: tools/llvm-lto/llvm-lto.cpp:291
@@ +290,3 @@
+    }
+    //    CodeGen.linkProfile(ThinLTODestinationDirectory + "summary.bc");
+    //    CodeGen.optimize(ThinLTODestinationDirectory);
----------------
Why are all these commented out?

================
Comment at: tools/lto/lto.cpp:282
@@ +281,3 @@
+void lto_module_set_preserved(lto_module_t mod, const char *symbol) {
+  return; // FIXME  unwrap(mod)->addPreservedSymbols(symbol);
+}
----------------
Why the FIXME?


http://reviews.llvm.org/D17066