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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits