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

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

Thanks for the review, I'll fix all of what you found :)


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

================
Comment at: include/llvm-c/lto.h:586
@@ +585,3 @@
+ *
+ * \since prior to LTO_API_VERSION=18
+ */
----------------
tejohnson wrote:
> Why "prior to" here and other places?
"prior to" is an unfortunate copy/paste, I'll remove.

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

================
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.
+   */
----------------
tejohnson wrote:
> do you mean "If a symbol is not listed there, it will be optimized away if it is inlined into every usage"?
Basically I expect the linker to tell me the list of symbols for which there is a reference from *outside* the ThinLTO modules.

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

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

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:175
@@ +174,3 @@
+    PMB.Inliner = createFunctionInliningPass();
+    //  PMB.OptLevel = OptLevel;
+    PMB.VerifyInput = false;
----------------
tejohnson wrote:
> Why don't we want to set an opt level?
Not sure, we don't have a linker flag to do that, with current full LTO there is none either I think. Maybe passing -mllvm -O3 can have an impact? (I think the -mllvm are supposed to be "debug" flags though)

This is something we'd like to figure out but not completely sure what is the best way (the front-end could encode this information in the bitcode for example).

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

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:297
@@ +296,3 @@
+  Mang.getNameWithPrefix(Buffer, &GV, false);
+  if (PreservedSymbols.count(Buffer)) {
+    Keep.insert(GV.getName());
----------------
tejohnson wrote:
> 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.
Any symbols with reference from other *non thinlto* modules to be exact.
I'll add a comment here.

================
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 =
----------------
tejohnson wrote:
> Is this temporary?
Yes! Will clean up

================
Comment at: tools/llvm-lto/llvm-lto.cpp:291
@@ +290,3 @@
+    }
+    //    CodeGen.linkProfile(ThinLTODestinationDirectory + "summary.bc");
+    //    CodeGen.optimize(ThinLTODestinationDirectory);
----------------
tejohnson wrote:
> Why are all these commented out?
Some debug code left over, I'll clean it up.

================
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);
+}
----------------
tejohnson wrote:
> Why the FIXME?
Oh this API should just be removed. 
(Early stage of prototyping I was expecting the linker to tell me the exported/preserved symbols *for each individual files*, but I could see any benefit from it later so I reverted to a global list).


http://reviews.llvm.org/D17066