[LTO] Add the ability to specify a subset of passes to run

[davide]

Summary

This commits add a --lto-newpm-passes= option that can specify the set of passes that need to be run (similarly to what opt already does). This makes testing easier (e.g. tuning the LTO pipeline), among others.
runLTOPasses() is growing (maybe) too much. I can split in two separate functions, if you like that better.

[davide]

git-clang-formatted.
Also, this doesn't still allow to specify an AliasAnalysis pipeline (a-la opt). I'll introduce another option for that in a subsequent commit.
Ideally we wouldn't need two different options, but the current architecture doesn't allow to get away with a single option, to the best of my knowledge.

[chandlerc]

ELF/LTO.cpp

91–99 ↗

(On Diff #57279)

We should really refactor this into a helper in PassBuilder.

[davide]

Update after refactoring cross registration of Analysis managers in PassBuilder.

[chandlerc]

FWIW, this looks good to me, but you should get someone who works on LLD to give the final LGTM.

[ruiu]

ELF/LTO.cpp

69–70 ↗

(On Diff #57288)

Can you please split the function so that you can do

if (!Config->LtoNewPmPasses.empty())
  runNewLtoPasses(M, TM);
else
  runOldLtoPasses(M, TM);

97–98 ↗

(On Diff #57288)

You need to handle the error gracefully (i.e. don't call fatal which exits) as it is a user-supplied command line option. Instead, please call error() here.

Please add a test to verify that it prints out an error message for an unknown --lto-newpm-passes argument.

[davide]

ELF/LTO.cpp

69–70 ↗

(On Diff #57288)

I don't want to bikeshed, but.
The name runNewLtoPasses is misleading because it doesn't capture the fact that we're running a custom pipeline. runNewLtoCustomPasses would be more appropriate. Or whatever you want to suggest, I'll take.

[ruiu]

ELF/LTO.cpp

69–70 ↗

(On Diff #57288)

Maybe your name is better. :) My point was just to split the function.

[davide]

Addressed comments.

[ruiu]

LGTM with a few nits.

ELF/LTO.cpp

65 ↗

(On Diff #57303)

Fix indentation of this code.

90 ↗

(On Diff #57303)

error("unable to parse pass pipeline description: " + Config->LtoNewPmPasses);

116 ↗

(On Diff #57303)

Remove this line.

[davide]

Thanks for your review, Rui/Chandler. I'll let this bake in the tree for a bit and then implement --lto-newpm-aa-pipeline (as opt does).

lld/trunk/ELF/CMakeLists.txt

add_lld_library(lldELF
   BitReader
   BitWriter
   Codegen
   Core
   IPO
   Linker
   Object
   Option
+  Passes
   MC
   Support
   Target
   TransformUtils
 
   LINK_LIBS
   lldConfig
   ${PTHREAD_LIB}
   )
 
 add_dependencies(lldELF intrinsics_gen ELFOptionsTableGen)

lld/trunk/ELF/Config.h

 struct Configuration {
   Symbol *EntrySym = nullptr;
   InputFile *FirstElf = nullptr;
   llvm::StringRef DynamicLinker;
   llvm::StringRef Entry;
   llvm::StringRef Emulation;
   llvm::StringRef Fini;
   llvm::StringRef Init;
+  llvm::StringRef LtoNewPmPasses;
   llvm::StringRef OutputFile;
   llvm::StringRef SoName;
   llvm::StringRef Sysroot;
   std::string RPath;
   std::vector<llvm::StringRef> DynamicList;
   std::vector<llvm::StringRef> SearchPaths;
   std::vector<llvm::StringRef> Undefined;
   std::vector<llvm::StringRef> VersionScriptGlobals;

lld/trunk/ELF/Driver.cpp

void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->Trace = Args.hasArg(OPT_trace);
   Config->Verbose = Args.hasArg(OPT_verbose);
   Config->WarnCommon = Args.hasArg(OPT_warn_common);
 
   Config->DynamicLinker = getString(Args, OPT_dynamic_linker);
   Config->Entry = getString(Args, OPT_entry);
   Config->Fini = getString(Args, OPT_fini, "_fini");
   Config->Init = getString(Args, OPT_init, "_init");
+  Config->LtoNewPmPasses = getString(Args, OPT_lto_newpm_passes);
   Config->OutputFile = getString(Args, OPT_o);
   Config->SoName = getString(Args, OPT_soname);
   Config->Sysroot = getString(Args, OPT_sysroot);
 
   Config->Optimize = getInteger(Args, OPT_O, 1);
   Config->LtoO = getInteger(Args, OPT_lto_O, 2);
   if (Config->LtoO > 3)
     error("invalid optimization level for LTO: " + getString(Args, OPT_lto_O));
if (HasError)
     return; // There were duplicate symbols or incompatible files
 
   Symtab.scanUndefinedFlags();
   Symtab.scanShlibUndefined();
   Symtab.scanDynamicList();
   Symtab.scanVersionScript();
 
   Symtab.addCombinedLtoObject();
+  if (HasError)
+    return;
 
   for (auto *Arg : Args.filtered(OPT_wrap))
     Symtab.wrap(Arg->getValue());
 
   // Write the result to the file.
   if (Config->GcSections)
     markLive<ELFT>();
   if (Config->ICF)
     doIcf<ELFT>();
   writeResult<ELFT>(&Symtab);
 }

lld/trunk/ELF/LTO.cpp

 //===- LTO.cpp ------------------------------------------------------------===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "LTO.h"
 #include "Config.h"
 #include "Driver.h"
 #include "Error.h"
 #include "InputFiles.h"
 #include "Symbols.h"
+#include "llvm/Analysis/AliasAnalysis.h"
+#include "llvm/Analysis/CGSCCPassManager.h"
+#include "llvm/Analysis/LoopPassManager.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/Bitcode/ReaderWriter.h"
 #include "llvm/CodeGen/CommandFlags.h"
 #include "llvm/CodeGen/ParallelCG.h"
 #include "llvm/IR/AutoUpgrade.h"
 #include "llvm/IR/LegacyPassManager.h"
+#include "llvm/IR/PassManager.h"
 #include "llvm/Linker/IRMover.h"
+#include "llvm/Passes/PassBuilder.h"
 #include "llvm/Support/StringSaver.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
+#include <llvm/IR/Verifier.h>
 
 using namespace llvm;
 using namespace llvm::object;
 using namespace llvm::ELF;
 
 using namespace lld;
 using namespace lld::elf;
 

 static void saveBCFile(Module &M, StringRef Suffix) {
   std::error_code EC;
   raw_fd_ostream OS(Config->OutputFile.str() + Suffix.str(), EC,
                     sys::fs::OpenFlags::F_None);
   check(EC);
   WriteBitcodeToFile(&M, OS, /* ShouldPreserveUseListOrder */ true);
 }
 
-// Run LTO passes.
+static void runNewCustomLtoPasses(Module &M, TargetMachine &TM) {
+  PassBuilder PB(&TM);
+
+  AAManager AA;
+  LoopAnalysisManager LAM;
+  FunctionAnalysisManager FAM;
+  CGSCCAnalysisManager CGAM;
+  ModuleAnalysisManager MAM;
+
+  // Register the AA manager first so that our version is the one used.
+  FAM.registerPass([&] { return std::move(AA); });
+
+  // Register all the basic analyses with the managers.
+  PB.registerModuleAnalyses(MAM);
+  PB.registerCGSCCAnalyses(CGAM);
+  PB.registerFunctionAnalyses(FAM);
+  PB.registerLoopAnalyses(LAM);
+  PB.crossRegisterProxies(LAM, FAM, CGAM, MAM);
+
+  ModulePassManager MPM;
+  if (!Config->DisableVerify)
+    MPM.addPass(VerifierPass());
+
+  // Now, add all the passes we've been requested to.
+  if (!PB.parsePassPipeline(MPM, Config->LtoNewPmPasses)) {
+    error("unable to parse pass pipeline description: " +
+          Config->LtoNewPmPasses);
+    return;
+  }
+
+  if (!Config->DisableVerify)
+    MPM.addPass(VerifierPass());
+  MPM.run(M, MAM);
+}
+
+static void runOldLtoPasses(Module &M, TargetMachine &TM) {
   // Note that the gold plugin has a similar piece of code, so
   // it is probably better to move this code to a common place.
-static void runLTOPasses(Module &M, TargetMachine &TM) {
   legacy::PassManager LtoPasses;
   LtoPasses.add(createTargetTransformInfoWrapperPass(TM.getTargetIRAnalysis()));
   PassManagerBuilder PMB;
   PMB.LibraryInfo = new TargetLibraryInfoImpl(Triple(TM.getTargetTriple()));
   PMB.Inliner = createFunctionInliningPass();
   PMB.VerifyInput = PMB.VerifyOutput = !Config->DisableVerify;
   PMB.LoopVectorize = true;
   PMB.SLPVectorize = true;
   PMB.OptLevel = Config->LtoO;
   PMB.populateLTOPassManager(LtoPasses);
   LtoPasses.run(M);
+}
+
+static void runLTOPasses(Module &M, TargetMachine &TM) {
+  if (!Config->LtoNewPmPasses.empty()) {
+    // The user explicitly asked for a set of passes to be run.
+    // This needs the new PM to work as there's no clean way to
+    // pass a set of passes to run in the legacy PM.
+    runNewCustomLtoPasses(M, TM);
+    if (HasError)
+      return;
+  } else {
+    // Run the 'default' set of LTO passes. This code still uses
+    // the legacy PM as the new one is not the default.
+    runOldLtoPasses(M, TM);
+  }
 
   if (Config->SaveTemps)
     saveBCFile(M, ".lto.opt.bc");
 }
 
 static bool shouldInternalize(const SmallPtrSet<GlobalValue *, 8> &Used,
                               Symbol *S, GlobalValue *GV) {
   if (S->IsUsedInRegularObj)
std::vector<std::unique_ptr<InputFile>> BitcodeCompiler::compile() {
 
   auto CreateTargetMachine = [&]() {
     return std::unique_ptr<TargetMachine>(
         T->createTargetMachine(TheTriple, "", "", Options, R));
   };
 
   std::unique_ptr<TargetMachine> TM = CreateTargetMachine();
   runLTOPasses(*Combined, *TM);
+  if (HasError)
+    return {};
 
   return runSplitCodegen(CreateTargetMachine);
 }

lld/trunk/ELF/Options.td

 def alias_define_common_d : Flag<["-"], "d">, Alias<define_common>;
 def alias_define_common_dc : Flag<["-"], "dc">, Alias<define_common>;
 def alias_define_common_dp : Flag<["-"], "dp">, Alias<define_common>;
 def alias_version_script_version_script : Joined<["--"], "version-script=">, Alias<version_script>;
 
 // LTO-related options.
 def lto_jobs : Joined<["--"], "lto-jobs=">,
   HelpText<"Number of threads to run codegen">;
+def lto_newpm_passes : Joined<["--"], "lto-newpm-passes=">,
+  HelpText<"Passes to run during LTO">;
 def disable_verify : Flag<["-"], "disable-verify">;
 def mllvm : Separate<["-"], "mllvm">;
 def save_temps : Flag<["-"], "save-temps">;

lld/trunk/test/ELF/lto/ltopasses-custom.ll

+; REQUIRES: x86
+; RUN: llvm-as %s -o %t.o
+; RUN: ld.lld -m elf_x86_64 %t.o -o %t.so -save-temps --lto-newpm-passes=ipsccp -shared
+; RUN: ld.lld -m elf_x86_64 %t.o -o %t2.so -save-temps --lto-newpm-passes=loweratomic -shared
+; RUN: llvm-dis %t.so.lto.opt.bc -o - | FileCheck %s
+; RUN: llvm-dis %t2.so.lto.opt.bc -o - | FileCheck %s --check-prefix=ATOMIC
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @barrier() {
+  fence seq_cst
+  ret void
+}
+
+; IPSCCP won't remove the fence.
+; CHECK: define void @barrier() {
+; CHECK-NEXT: fence seq_cst
+; CHECK-NEXT: ret void
+
+; LowerAtomic will remove the fence.
+; ATOMIC: define void @barrier() {
+; ATOMIC-NEXT: ret void
+
+; Check that invalid passes are rejected gracefully.
+; RUN: not ld.lld -m elf_x86_64 %t.o -o %t2.so \
+; RUN:   --lto-newpm-passes=iamnotapass -shared 2>&1 | \
+; RUN:   FileCheck %s --check-prefix=INVALID
+; INVALID: unable to parse pass pipeline description: iamnotapass