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

Mehdi AMINI via llvm-commits llvm-commits at lists.llvm.org
Tue Mar 8 11:24:33 PST 2016


joker.eph marked an inline comment as done.
joker.eph added a comment.

Thanks for all the comments! 
(Please see inline for the discussion)


================
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,
----------------
slarin wrote:
> Does this handles commons properly?
Can you be more specific? I'm not sure what is specific with common on this aspect?

================
Comment at: include/llvm/LTO/ThinLTOCodeGenerator.h:148
@@ +147,3 @@
+  void setCodeGenOptLevel(CodeGenOpt::Level CGOptLevel) {
+    TMBuilder.CGOptLevel = CGOptLevel;
+  }
----------------
slarin wrote:
> Can we theoretically have a mixture of opt levels? -O2/-Os? Should I be able to respect per library opt level?
Ideally I think we'd want this parameter to be recorded in the bitcode itself, what do you think?
Right now we don't even expose any linker flag for that.

Here it is about the *codegen* opt level though, it won't impact the optimizer.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:67
@@ +66,3 @@
+    ModuleOrErr = parseBitcodeFile(Buffer, Context);
+  }
+  if (std::error_code EC = ModuleOrErr.getError()) {
----------------
slarin wrote:
> 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.
slarin: how does it play with cross-module importing? If a function is defined in a module compiled with O3 but imported in a module compiled with O2?

================
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.
----------------
slarin wrote:
> Default to /tmp?
The way the client is enabling dumping temporaries is by providing a path.
Having a default would mean either always dumping temporaries (we don't want that...) or having a more complicated way of enabling dumping.


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:144
@@ +143,3 @@
+  PMB.VerifyInput = true;
+  PMB.VerifyOutput = false;
+
----------------
slarin wrote:
> So I assume I should be able to control all of those...
Yes, but we don't have an interface for these as of today. A serialization of the PMB options in the bitcode would help.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:184
@@ +183,3 @@
+  // Save temps: after IPO.
+  saveTempBitcode(TheModule, SaveTempsDir, count, ".1.IPO.bc");
+
----------------
slarin wrote:
> Sorry, I miss something - why is this unconditional?
As mentioned above, this is conditional in the `saveTempBitcode` function itself, which starts with 

```
if (SaveTempsDir.empty()) return;
```

makes sense?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:363
@@ +362,3 @@
+        // Parse module now
+        auto TheModule =
+            loadModuleFromBuffer(ModuleMap[ModuleBuffer.getBufferIdentifier()],
----------------
slarin wrote:
> 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 :)
We'll always verify the module once at the beginning of the optimizer pipeline, but I guess we could do it more frequently in assert builds.

================
Comment at: tools/llvm-lto/llvm-lto.cpp:82
@@ +81,3 @@
+                                         "cross-module importing (requires "
+                                         "-thinlto-index)."),
+        clEnumValN(THINOPT, "optimize", "Perform ThinLTO optimizations."),
----------------
tejohnson wrote:
> Actual option below is "functionindex". But per the ref graph patch, this is broader than a function index, so I am looking at changing references to function index to something else anyway. So I would suggest changing the actual option below to "thinlto-index".
Will do.

================
Comment at: tools/llvm-lto/llvm-lto.cpp:442
@@ +441,3 @@
+      ThinGenerator.optimize(*TheModule);
+
+      std::string OutputName = OutputFilename;
----------------
slarin wrote:
> Verify TheModule?
This is done in optimized()

(see line 143 in ThinLTOCodeGenerator.cpp   `PMB.VerifyInput = true;`)


http://reviews.llvm.org/D17066





More information about the llvm-commits mailing list