Load pass plugins during option processing, so that plugin options are registered and live.

[mehdi_amini]

llvm/tools/opt/opt.cpp

602

Should we sanitize that the new PM is enabled if PassPlugins is provided and error otherwise?

[w2yehia]

Issue an error when -load-pass-plugin is used with legacy PM.

[w2yehia]

llvm/tools/opt/opt.cpp

602

Thanks. Done.

[w2yehia]

I got many failures in poly.
Seems it uses both -load and -load-pass-plugin with legacy PM passes?

Here's one:

BUILD FAILED: failed 1054 unexpected failures (failure)

Step 6 (check_polly) failure: 1054 unexpected failures (failure)
******************** TEST 'Polly :: ScheduleOptimizer/pattern-matching-based-opts_14.ll' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/bin/opt -load /home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/lib/LLVMPolly.so -load-pass-plugin /home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/lib/LLVMPolly.so -polly-process-unprofitable  -polly-remarks-minimal  -polly-use-llvm-names  -polly-import-jscop-dir=/home/worker/src/llvm-project/polly/test/ScheduleOptimizer  -polly-codegen-verify  -polly-import-jscop -polly-opt-isl   -polly-target-throughput-vector-fma=1  -polly-target-latency-vector-fma=8  -polly-target-1st-cache-level-associativity=8  -polly-target-2nd-cache-level-associativity=8  -polly-target-1st-cache-level-size=32768  -polly-target-vector-register-bitwidth=256  -polly-target-2nd-cache-level-size=262144  -polly-import-jscop-postfix=transformed -polly-codegen -S < /home/worker/src/llvm-project/polly/test/ScheduleOptimizer/pattern-matching-based-opts_14.ll  | /home/worker/buildbot-workers/polly-x86_64-g
 ce1/rundir/llvm.obj/bin/FileCheck /home/worker/src/llvm-project/polly/test/ScheduleOptimizer/pattern-matching-based-opts_14.ll
--
Exit Code: 2

Command Output (stderr):
--
/home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/bin/opt: load-pass-plugin specified with legacy PM.
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/bin/FileCheck /home/worker/src/llvm-project/polly/test/ScheduleOptimizer/pattern-matching-based-opts_14.ll

[w2yehia]

@Meinersbur Hi Michael. I see this in polly's lit config file:

config.substitutions.append(('%loadPolly', '-load '
                             + config.polly_lib_dir + '/LLVMPolly@LLVM_SHLIBEXT@'
                             + ' -load-pass-plugin '
                             + config.polly_lib_dir + '/LLVMPolly@LLVM_SHLIBEXT@'

The -load-pass-plugin was added in D45484, but the follow up change D45493 never landed.
From my understanding, -load and -load-pass-plugin are disjoint (one is for legacy PM and one for NPM) by design.
Since you cannot run both legacy and new PM, the only reason I think you need both -load and -load-pass-plugin is to be able to register polly's options before they're processed.
Which is the problem this PR addresses.
Should we remove -load-pass-plugin from %loadPolly for now, until polly switches to using NPM (i'm guessing it's using legacy PM because the new diagnostic i'm adding is being triggered by polly's tests) ?

[Meinersbur]

Seems it uses both -load and -load-pass-plugin with legacy PM passes?

It was written with the assumption that -load and load-pass-plugin means "make the passes from the .so register in the PM". As such, they (should have) no effect until actually used (-load can do other things than register pass plugins). Only allowing the option corresponding to the PM actually used may help reduce user confusion, but if consequent it should also give an error if no pass from the loaded plugin was used.

Moving Polly completely to the NPM will take a while (some aspects of the NPM are still not compatible with pass plugins, or work differently needing to redesign a lot of regression tests). D45493 only takes account for the few Polly that had equivalent NPM passes by then.

If you really want that error, I suggest to introduce a new %loadPolly_NPM (or similar name) that loads the NPM plugin, and %loadPolly to only load the legacy PM plugin.

llvm/test/Feature/load_extension.ll

21–23

Can you move all the RUN lines together at the beginning of the file? It is surprising to find another RUN line after e.g. having adapted the others.

[w2yehia]

but if consequent it should also give an error if no pass from the loaded plugin was used.

Agreed. However, how can I tell if this occurs? Is there a simple way to check that?

If you really want that error, I suggest to introduce a new %loadPolly_NPM (or similar name) that loads the NPM plugin, and %loadPolly to only load the legacy PM plugin.

%loadPolly is currently used in all polly testcases. Does this require touching every polly testcase that uses %loadPolly? Is there an easy way to tell (by inspection) if a testcase is calling the NPM passes of polly?

[Meinersbur]

In D121566#3386778, @w2yehia wrote:

If you really want that error, I suggest to introduce a new %loadPolly_NPM (or similar name) that loads the NPM plugin, and %loadPolly to only load the legacy PM plugin.

%loadPolly is currently used in all polly testcases. Does this require touching every polly testcase that uses %loadPolly? Is there an easy way to tell (by inspection) if a testcase is calling the NPM passes of polly?

%loadPolly would only need to be replaced with %loadPolly_NPM in test cases using the NPM. There are not that many yet. You get a list of test cases using the NPM by removing -load-pass-plugin from %loadPolly and run ninja check-polly-tests (using LLVM_POLLY_LINK_INTO_TOOLS=OFF). Those are also the RUN-lines that would need %loadPolly_NPM instead of %loadPolly. Alternatively, grep for RUN-lines with -passes= option.

[w2yehia]

Fixed failing polly tests by adding a NPM macro %loadNPMPolly.
@Meinersbur Thanks for the review. Please let me know if the cfg changes are ok before I commit the changes.

[Meinersbur]

Could you rebase the patch? The premerge-check's patch application failed: https://buildkite.com/llvm-project/diff-checks/builds/94599

[Meinersbur]

polly/test/lit.site.cfg.in

29–33 ↗

(On Diff #416182)

Parens are unnecessary here

49–50 ↗

(On Diff #416182)

Preprending empty string does nothing

[w2yehia]

In D121566#3389537, @Meinersbur wrote:

Could you rebase the patch? The premerge-check's patch application failed: https://buildkite.com/llvm-project/diff-checks/builds/94599

I did rebase it (on 12a2f7494e74) before posting. The premerge-check error seems something bogus?:

$ trap 'kill -- $$' INT TERM QUIT; pip install -q -r scripts/requirements.txt
scripts/apply_patch.sh
INFO    repository exist, will reuse
INFO    working dir /var/lib/buildkite-agent/builds/llvm-project-fork
INFO    Syncing local, origin and upstream...
INFO    Planning to apply in order:
INFO    https://reviews.llvm.org/D121566?id=416182
INFO    D121566#416182 commit 12a2f7494e745eb4c90133ea17cadac3a8eb8d07 exists
INFO    creating branch phab-diff-416182 at 12a2f7494e745eb4c90133ea17cadac3a8eb8d07
INFO    Base branch revision is 12a2f7494e745eb4c90133ea17cadac3a8eb8d07
INFO    Applying 416182 for revision 121566...
INFO    uploaded 1_416182.patch to https://buildkite.com/organizations/llvm-project/pipelines/premerge-checks/builds/94599/jobs/ee057fd3-a5f6-469f-8ebd-dd087982b0f1/artifacts/b22020f8-e0ae-4eef-a4f7-b5be718dc8ee
ERROR   exception: Cmd('git') failed due to: exit code(1)
  cmdline: git push --force origin phab-diff-416182
  stderr: 'remote: Internal Server Error
To ssh://github.com/llvm-premerge-tests/llvm-project
 ! [remote rejected]           phab-diff-416182 -> phab-diff-416182 (Internal Server Error)
error: failed to push some refs to 'ssh://github.com/llvm-premerge-tests/llvm-project''
Reporting results to Phabricator build https://reviews.llvm.org/harbormaster/build/230024
 
report issue
 
failed
🚨 Error: The command exited with status 1
user command error: exit status 1

[Meinersbur]

Mmmh, indeed. "Internal server error" 🤔

Giving my LGTM assuming you are still doing the Python code cleanup. Please observe the Polly buildbots after committing.

[w2yehia]

Giving my LGTM assuming you are still doing the Python code cleanup. Please observe the Polly buildbots after committing.

Thanks, will keep an eye for sure.

llvm/test/Feature/load_extension.ll

 ; REQUIRES: x86-registered-target
 ; RUN: opt %s %loadbye -goodbye -wave-goodbye -disable-output -enable-new-pm=0 2>&1 | FileCheck %s
 ; RUN: opt %s %loadnewpmbye %loadbye -passes="goodbye" -wave-goodbye -disable-output 2>&1 | FileCheck %s
+; RUN: opt %s %loadnewpmbye -passes="goodbye" -wave-goodbye -disable-output 2>&1 | FileCheck %s
 ; RUN: opt -module-summary %s -o %t.o
 ; RUN: llvm-lto2 run %t.o %loadbye -wave-goodbye -use-new-pm=0 -o %t -r %t.o,somefunk,plx -r %t.o,junk,plx 2>&1 | FileCheck %s
 ; RUN: llvm-lto2 run %t.o %loadbye %loadnewpmbye -wave-goodbye -use-new-pm -o %t -r %t.o,somefunk,plx -r %t.o,junk,plx 2>&1 | FileCheck %s
 ; RUN: llvm-lto2 run %t.o %loadbye %loadnewpmbye -opt-pipeline="goodbye" -wave-goodbye -use-new-pm -o %t -r %t.o,somefunk,plx -r %t.o,junk,plx 2>&1 | FileCheck %s
 ; REQUIRES: plugins, examples
 ; UNSUPPORTED: windows
 ; CHECK: Bye
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 @junk = global i32 0
 
 define i32* @somefunk() {
   ret i32* @junk
 }
 
+; Specifying a new PM pass plugin with the old PM is an error.
+; RUN: ! opt %s %loadnewpmbye -goodbye -wave-goodbye -disable-output -enable-new-pm=0 2>&1 | FileCheck %s --check-prefix=ERROR
+; ERROR: load-pass-plugin specified with legacy PM.

llvm/tools/opt/NewPMDriver.h

 #define LLVM_TOOLS_OPT_NEWPMDRIVER_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/CommandLine.h"
 
 namespace llvm {
 class StringRef;
 class Module;
+class PassPlugin;
 class TargetMachine;
 class ToolOutputFile;
 class TargetLibraryInfoImpl;
 
 extern cl::opt<bool> DebugifyEach;
 extern cl::opt<std::string> DebugifyExport;
 
 namespace opt_tool {

 /// when the transition finishes.
 ///
 /// ThinLTOLinkOut is only used when OK is OK_OutputThinLTOBitcode, and can be
 /// nullptr.
 bool runPassPipeline(StringRef Arg0, Module &M, TargetMachine *TM,
                      TargetLibraryInfoImpl *TLII, ToolOutputFile *Out,
                      ToolOutputFile *ThinLinkOut, ToolOutputFile *OptRemarkFile,
                      StringRef PassPipeline, ArrayRef<StringRef> PassInfos,
-                     opt_tool::OutputKind OK, opt_tool::VerifierKind VK,
+                     ArrayRef<PassPlugin> PassPlugins, opt_tool::OutputKind OK,
+                     opt_tool::VerifierKind VK,
                      bool ShouldPreserveAssemblyUseListOrder,
                      bool ShouldPreserveBitcodeUseListOrder,
                      bool EmitSummaryIndex, bool EmitModuleHash,
                      bool EnableDebugify);
 } // namespace llvm
 
 #endif

llvm/tools/opt/NewPMDriver.cpp

static cl::opt<DebugLogging> DebugPM(
     cl::values(
         clEnumValN(DebugLogging::Normal, "", ""),
         clEnumValN(DebugLogging::Quiet, "quiet",
                    "Skip printing info about analyses"),
         clEnumValN(
             DebugLogging::Verbose, "verbose",
             "Print extra information about adaptors and pass managers")));
 
-static cl::list<std::string>
-    PassPlugins("load-pass-plugin",
-                cl::desc("Load passes from plugin library"));
-
 // This flag specifies a textual description of the alias analysis pipeline to
 // use when querying for aliasing information. It only works in concert with
 // the "passes" flag above.
 static cl::opt<std::string>
     AAPipeline("aa-pipeline",
                cl::desc("A textual description of the alias analysis "
                         "pipeline for handling managed aliasing queries"),
                cl::Hidden, cl::init("default"));
PB.registerFullLinkTimeOptimizationLastEPCallback(
           Err(PB.parsePassPipeline(PM, FullLinkTimeOptimizationLastEPPipeline));
         });
 }
 
 #define HANDLE_EXTENSION(Ext)                                                  \
   llvm::PassPluginLibraryInfo get##Ext##PluginInfo();
 #include "llvm/Support/Extension.def"
 
 bool llvm::runPassPipeline(StringRef Arg0, Module &M, TargetMachine *TM,
                            TargetLibraryInfoImpl *TLII, ToolOutputFile *Out,
                            ToolOutputFile *ThinLTOLinkOut,
                            ToolOutputFile *OptRemarkFile,
                            StringRef PassPipeline, ArrayRef<StringRef> Passes,
+                           ArrayRef<PassPlugin> PassPlugins,
                            OutputKind OK, VerifierKind VK,
                            bool ShouldPreserveAssemblyUseListOrder,
                            bool ShouldPreserveBitcodeUseListOrder,
                            bool EmitSummaryIndex, bool EmitModuleHash,
                            bool EnableDebugify) {
   bool VerifyEachPass = VK == VK_VerifyEachPass;
 
   Optional<PGOOptions> P;
bool llvm::runPassPipeline(StringRef Arg0, Module &M, TargetMachine *TM,
   PipelineTuningOptions PTO;
   // LoopUnrolling defaults on to true and DisableLoopUnrolling is initialized
   // to false above so we shouldn't necessarily need to check whether or not the
   // option has been enabled.
   PTO.LoopUnrolling = !DisableLoopUnrolling;
   PassBuilder PB(TM, PTO, P, &PIC);
   registerEPCallbacks(PB);
 
-  // Load requested pass plugins and let them register pass builder callbacks
-  for (auto &PluginFN : PassPlugins) {
-    auto PassPlugin = PassPlugin::Load(PluginFN);
-    if (!PassPlugin) {
-      errs() << "Failed to load passes from '" << PluginFN
-             << "'. Request ignored.\n";
-      continue;
-    }
-
-    PassPlugin->registerPassBuilderCallbacks(PB);
-  }
+  // For any loaded plugins, let them register pass builder callbacks.
+  for (auto &PassPlugin : PassPlugins)
+    PassPlugin.registerPassBuilderCallbacks(PB);
 
   PB.registerPipelineParsingCallback(
       [](StringRef Name, ModulePassManager &MPM,
          ArrayRef<PassBuilder::PipelineElement>) {
         AddressSanitizerOptions Opts;
         if (Name == "asan-pipeline") {
           MPM.addPass(
               RequireAnalysisPass<ASanGlobalsMetadataAnalysis, Module>());

llvm/tools/opt/opt.cpp

 #include "llvm/IR/Module.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/IRReader/IRReader.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/LinkAllIR.h"
 #include "llvm/LinkAllPasses.h"
 #include "llvm/MC/SubtargetFeature.h"
 #include "llvm/MC/TargetRegistry.h"
+#include "llvm/Passes/PassPlugin.h"
 #include "llvm/Remarks/HotnessThresholdParser.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/PluginLoader.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/SystemUtils.h"
RemarksPasses("pass-remarks-filter",
                            "names match the given regular expression"),
                   cl::value_desc("regex"));
 
 static cl::opt<std::string> RemarksFormat(
     "pass-remarks-format",
     cl::desc("The format used for serializing remarks (default: YAML)"),
     cl::value_desc("format"), cl::init("yaml"));
 
+static cl::list<std::string>
+    PassPlugins("load-pass-plugin",
+                cl::desc("Load passes from plugin library"));
+
 namespace llvm {
 cl::opt<PGOKind>
     PGOKindFlag("pgo-kind", cl::init(NoPGO), cl::Hidden,
                 cl::desc("The kind of profile guided optimization"),
                 cl::values(clEnumValN(NoPGO, "nopgo", "Do not use PGO."),
                            clEnumValN(InstrGen, "pgo-instr-gen-pipeline",
                                       "Instrument the IR to generate profile."),
                            clEnumValN(InstrUse, "pgo-instr-use-pipeline",
int main(int argc, char **argv) {
   initializeTypePromotionPass(Registry);
   initializeReplaceWithVeclibLegacyPass(Registry);
   initializeJMCInstrumenterPass(Registry);
 
 #ifdef BUILD_EXAMPLES
   initializeExampleIRTransforms(Registry);
 #endif
 
+  SmallVector<PassPlugin, 1> PluginList;
+  PassPlugins.setCallback([&](const std::string &PluginPath) {
+    auto Plugin = PassPlugin::Load(PluginPath);
+    if (!Plugin) {
+      errs() << "Failed to load passes from '" << PluginPath
+             << "'. Request ignored.\n";
+      return;
+    }
+    PluginList.emplace_back(Plugin.get());
+  });
+
   cl::ParseCommandLineOptions(argc, argv,
     "llvm .bc -> .bc modular optimizer and analysis printer\n");
 
   LLVMContext Context;
 
   if (AnalyzeOnly && NoOutput) {
     errs() << argv[0] << ": analyze mode conflicts with no-output mode.\n";
     return 1;
   }
 
+  // If `-passes=` is specified, use NPM.
+  // If `-enable-new-pm` is specified and there are no codegen passes, use NPM.
+  // e.g. `-enable-new-pm -sroa` will use NPM.
+  // but `-enable-new-pm -codegenprepare` will still revert to legacy PM.
+  const bool UseNPM = (EnableNewPassManager && !shouldForceLegacyPM()) ||
+                      PassPipeline.getNumOccurrences() > 0;
+
+  if (!UseNPM && PluginList.size()) {
+    errs() << argv[0] << ": " << PassPlugins.ArgStr
+           << " specified with legacy PM.\n";
+    return 1;
+  }
+
   // FIXME: once the legacy PM code is deleted, move runPassPipeline() here and
   // construct the PassBuilder before parsing IR so we can reuse the same
   // PassBuilder for print passes.
   if (PrintPasses) {
     printPasses(outs());
     return 0;
   }
 
for (auto &FuncName : DisableBuiltins)
         TLII.setUnavailable(F);
       else {
         errs() << argv[0] << ": cannot disable nonexistent builtin function "
                << FuncName << '\n';
         return 1;
       }
   }
 
-  // If `-passes=` is specified, use NPM.
-  // If `-enable-new-pm` is specified and there are no codegen passes, use NPM.
-  // e.g. `-enable-new-pm -sroa` will use NPM.
-  // but `-enable-new-pm -codegenprepare` will still revert to legacy PM.
-  if ((EnableNewPassManager && !shouldForceLegacyPM()) ||
-      PassPipeline.getNumOccurrences() > 0) {
+  if (UseNPM) {
     if (AnalyzeOnly) {
       errs() << "Cannot specify -analyze under new pass manager, either "
                 "specify '-enable-new-pm=0', or use the corresponding new pass "
                 "manager pass, e.g. '-passes=print<scalar-evolution>'. For a "
                 "full list of passes, see the '--print-passes' flag.\n";
       return 1;
     }
     if (legacy::debugPassSpecified()) {
if (UseNPM) {
     if (NoVerify)
       VK = VK_NoVerifier;
     else if (VerifyEach)
       VK = VK_VerifyEachPass;
 
     // The user has asked to use the new pass manager and provided a pipeline
     // string. Hand off the rest of the functionality to the new code for that
     // layer.
     return runPassPipeline(argv[0], *M, TM.get(), &TLII, Out.get(),
                            ThinLinkOut.get(), RemarksFile.get(), Pipeline,
-                           Passes, OK, VK, PreserveAssemblyUseListOrder,
+                           Passes, PluginList, OK, VK, PreserveAssemblyUseListOrder,
                            PreserveBitcodeUseListOrder, EmitSummaryIndex,
                            EmitModuleHash, EnableDebugify)
                ? 0
                : 1;
   }
 
   // Create a PassManager to hold and optimize the collection of passes we are
   // about to build. If the -debugify-each option is set, wrap each pass with