[PATCH] D15025: [ThinLTO] Option to invoke ThinLTO backend passes and importing

[Mehdi AMINI via cfe-commits]

joker.eph added inline comments.

================
Comment at: lib/CodeGen/BackendUtil.cpp:290
@@ +289,3 @@
+  // setup for LTO compiles invoked via the gold plugin and the llvm-lto tool.
+  if (!CodeGenOpts.ThinLTOIndexFile.empty() && !CodeGenOpts.DisableLLVMOpts) {
+    legacy::PassManager *MPM = getPerModulePasses();
----------------
The `!CodeGenOpts.DisableLLVMOpts` part is unclear to me. This may lead to unexpected user experience?

================
Comment at: lib/CodeGen/BackendUtil.cpp:308
@@ +307,3 @@
+    PMB.populateLTOPassManager(*MPM);
+    return;
+  }
----------------
This duplication of PMBuilder / CodeGenOpts setup is what triggered my initial comment.

================
Comment at: lib/CodeGen/BackendUtil.cpp:323
@@ -298,3 +322,3 @@
   PMBuilder.RerollLoops = CodeGenOpts.RerollLoops;
 
   PMBuilder.addExtension(PassManagerBuilder::EP_EarlyAsPossible,
----------------
Could you move your code here and reuse the same PBBuilder initialized here?
I expect that you don't need to do (almost) anything else than:

```
if (!CodeGenOpts.ThinLTOIndexFile.empty()) {
  legacy::PassManager *MPM = getPerModulePasses();
  PMB.populateLTOPassManager(MPM);
  return;
}
```

================
Comment at: lib/CodeGen/BackendUtil.cpp:390
@@ -365,3 +389,3 @@
   Triple TargetTriple(TheModule->getTargetTriple());
   PMBuilder.LibraryInfo = createTLII(TargetTriple, CodeGenOpts);
 
----------------
Could this be moved earlier so that it is before your code and you don't need to duplicate it?

================
Comment at: lib/CodeGen/BackendUtil.cpp:406-407
@@ -381,4 +405,4 @@
       PMBuilder.Inliner = createAlwaysInlinerPass();
     break;
   }
 
----------------
Could we move the inlining setup before your code so that you don't need to it?

================
Comment at: lib/CodeGen/CodeGenAction.cpp:796
@@ +795,3 @@
+    std::function<void(const llvm::DiagnosticInfo &)> DiagHandler =
+        std::bind(&CodeGenAction::diagnosticHandler, this, _1);
+
----------------
I think I didn't express what I meant by "name it", it can still be a lambda:

```
  auto DiagHandler = [&] (const DiagnosticInfo &DI) {
     linkerDiagnosticHandler(DI, TheModule.get(),
                          getCompilerInstance().getDiagnostics());
  };
```

(there not real reason to use std::bind() for anything now that we have lambda I think)

================
Comment at: lib/CodeGen/CodeGenAction.cpp:824
@@ +823,3 @@
+
+      TheModule = std::move(Combined);
+      // Stash on the module object so it can be used by the function
----------------
Side note: I found terrible that we can't do the renaming/linkage upgrade in-place :(



http://reviews.llvm.org/D15025