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

[Sergei Larin via llvm-commits]

slarin added a comment.

Sorry for the late entry... Some of my questions could have been already answered.


================
Comment at: include/llvm-c/lto.h:736
@@ +735,3 @@
+ */
+extern void thinlto_codegen_add_cross_referenced_symbol(thinlto_code_gen_t cg,
+                                                        const char *name,
----------------
Does this handles commons properly?

================
Comment at: include/llvm/LTO/ThinLTOCodeGenerator.h:148
@@ +147,3 @@
+  void setCodeGenOptLevel(CodeGenOpt::Level CGOptLevel) {
+    TMBuilder.CGOptLevel = CGOptLevel;
+  }
----------------
Can we theoretically have a mixture of opt levels? -O2/-Os? Should I be able to respect per library opt level?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:67
@@ +66,3 @@
+    ModuleOrErr = parseBitcodeFile(Buffer, Context);
+  }
+  if (std::error_code EC = ModuleOrErr.getError()) {
----------------
I do not know if it is relevant at this point, but for what it worth - my IR might have metadata that changes opt/codegen.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:81
@@ +80,3 @@
+  if (TempDir.empty())
+    return;
+  // User asked to save temps, let dump the bitcode file after import.
----------------
Default to /tmp?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:140
@@ +139,3 @@
+  // FIXME: should get it from the bitcode?
+  PMB.OptLevel = 3;
+  PMB.LoopVectorize = true;
----------------
Yes! ...and default should probably be O2...

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:144
@@ +143,3 @@
+  PMB.VerifyInput = true;
+  PMB.VerifyOutput = false;
+
----------------
So I assume I should be able to control all of those...

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:184
@@ +183,3 @@
+  // Save temps: after IPO.
+  saveTempBitcode(TheModule, SaveTempsDir, count, ".1.IPO.bc");
+
----------------
Sorry, I miss something - why is this unconditional?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:363
@@ +362,3 @@
+        // Parse module now
+        auto TheModule =
+            loadModuleFromBuffer(ModuleMap[ModuleBuffer.getBufferIdentifier()],
----------------
In general - don't you want to verify modules as they progress through the stages? I do it in regular LTO and it did help on more than one occasion :)

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:378
@@ +377,2 @@
+    llvm::PrintStatistics();
+}
----------------
joker.eph wrote:
> tejohnson wrote:
> > Unfortunately this means that ThinLTOCodeGenerator::run() is currently untested. Consider adding a mode to llvm-lto that does all thinlto steps (thin-action=all?) to test all the steps via this interface.
> OK.
+1 on Teresa's point about llvm-lto.

================
Comment at: tools/llvm-lto/llvm-lto.cpp:442
@@ +441,3 @@
+      ThinGenerator.optimize(*TheModule);
+
+      std::string OutputName = OutputFilename;
----------------
Verify TheModule?


http://reviews.llvm.org/D17066