[NewPM] Attempt to run opt passes specified via -foo-pass under NPM

[aeubanks]

Summary

In order to enable mass testing of opt under NPM, specifically passes
specified via -foo-pass.

This is gated under a new opt flag -enable-new-pm. Currently
the pass flag parser looks for legacy PM passes with the name "foo" (for
opt arg "-foo") and creates a PassInfo for each one. Here we take the
(legacy PM) pass name and try to match it with one defined in (NPM)
PassRegistry.def. Ultimately if we want all tests to pass like this,
we'll need to port all passes to NPM and register them in
PassRegistry.def under the same name as they were reigstered in the
legacy PM.

Maybe at some point we'll migrate all -foo to --passes=foo, but that
would be after the NPM switch.

Flipping on the flag causes 2XXX failures under check-llvm. By far most
of them are passes either not ported to NPM or don't have the same name
in PassRegistry.def as their old name.

[ychen]

This is great! What are these failures look like? Do they look straightforward to fix?

llvm/tools/opt/opt.cpp

75

This is a bit verbose to me. Maybe enable-new-pm. It is fine either way.

742

PassPipeline.getNumOccurrences() > 0 is more intuitive?

[aeubanks]

Add test
Address review comments

[aeubanks]

Add trailing newline in test

[aeubanks]

In D82320#2107762, @ychen wrote:

This is great! What are these failures look like? Do they look straightforward to fix?

Updated the description.
Most of them are passes either not ported to NPM or don't have the same name in PassRegistry.def as their old name.
I was thinking about potentially creating some wrapper class around old PM passes but I don't think that would work.

llvm/tools/opt/opt.cpp

75

Done, matches another option in NewPMDriver.cpp

742

That's definitely better, done.

[ychen]

In D82320#2107835, @aeubanks wrote:

In D82320#2107762, @ychen wrote:

This is great! What are these failures look like? Do they look straightforward to fix?

Updated the description.
Most of them are passes either not ported to NPM

I would assume most of these are passes for codegen pipeline?

or don't have the same name in PassRegistry.def as their old name.

I was thinking about potentially creating some wrapper class around old PM passes but I don't think that would work.

Not for this patch, how about adding the old name in the PassRegistry.def as an alternative name?

[aeubanks]

In D82320#2107899, @ychen wrote:

In D82320#2107835, @aeubanks wrote:

In D82320#2107762, @ychen wrote:

This is great! What are these failures look like? Do they look straightforward to fix?

Updated the description.
Most of them are passes either not ported to NPM

I would assume most of these are passes for codegen pipeline?

No, codegen hasn't really been updated to work with NPM at all. For example we don't even have some sort of MachineFunctionPassManager yet (I found your older https://reviews.llvm.org/D67687 which hasn't been submitted yet).
Under clang if NPM is enabled, it runs the pre-codegen passes under NPM, but then just runs the codegen passes separately under legacy PM.
For tests, almost all codegen passes use llc instead of opt. The one odd pass I've run into was CodeGenPrepare, which is pre-codegen, but requires some codegen analysis (MachineModuleAnalysis I believe), which was part of https://reviews.llvm.org/D67687.

or don't have the same name in PassRegistry.def as their old name.

I was thinking about potentially creating some wrapper class around old PM passes but I don't think that would work.

Not for this patch, how about adding the old name in the PassRegistry.def as an alternative name?

Yes that works for some cases where the pass has been ported to NPM.

[ychen]

In D82320#2107956, @aeubanks wrote:

In D82320#2107899, @ychen wrote:

In D82320#2107835, @aeubanks wrote:

In D82320#2107762, @ychen wrote:

This is great! What are these failures look like? Do they look straightforward to fix?

Updated the description.
Most of them are passes either not ported to NPM

I would assume most of these are passes for codegen pipeline?

No, codegen hasn't really been updated to work with NPM at all. For example we don't even have some sort of MachineFunctionPassManager yet (I found your older https://reviews.llvm.org/D67687 which hasn't been submitted yet).
Under clang if NPM is enabled, it runs the pre-codegen passes under NPM, but then just runs the codegen passes separately under legacy PM.
For tests, almost all codegen passes use llc instead of opt. The one odd pass I've run into was CodeGenPrepare, which is pre-codegen, but requires some codegen analysis (MachineModuleAnalysis I believe), which was part of https://reviews.llvm.org/D67687.

Thanks. I'm asking because I have vague memory that there are some codegen IR passes that are driven by opt instead of llc. But as you said, they are not. I guess @leonardchan got a better idea about our situation on passes (ported/not ported).

or don't have the same name in PassRegistry.def as their old name.

I was thinking about potentially creating some wrapper class around old PM passes but I don't think that would work.

Not for this patch, how about adding the old name in the PassRegistry.def as an alternative name?

Yes that works for some cases where the pass has been ported to NPM.

[hans]

Cool!

Maybe expand the commit description to be more explicit about the motivation. I assume this is to enable mass testing of current tests with the new pm?

[aeubanks]

In D82320#2108754, @hans wrote:

Cool!

Maybe expand the commit description to be more explicit about the motivation. I assume this is to enable mass testing of current tests with the new pm?

Yup, done.

[asbirlea]

llvm/tools/opt/NewPMDriver.cpp

340

Add an assert that Passes is empty? This should match the callsite assert from opt.cpp

llvm/test/Other/opt-npm.ll

+; RUN: opt -dce -enable-new-pm -disable-output -debug-pass-manager %s 2>&1 | FileCheck %s
+
+; CHECK: DCEPass
+define void @foo() {
+    ret void
+}

llvm/tools/opt/NewPMDriver.h

 /// file. Eventually all of the routines here will get folded back into
 /// opt.cpp.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_OPT_NEWPMDRIVER_H
 #define LLVM_TOOLS_OPT_NEWPMDRIVER_H
 
+#include "llvm/ADT/ArrayRef.h"
+
 namespace llvm {
 class StringRef;
 class Module;
 class TargetMachine;
 class ToolOutputFile;
 
 namespace opt_tool {
 enum OutputKind {

 /// file. It's interface is consequentially somewhat ad-hoc, but will go away
 /// 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,
                      ToolOutputFile *Out, ToolOutputFile *ThinLinkOut,
                      ToolOutputFile *OptRemarkFile, StringRef PassPipeline,
-                     opt_tool::OutputKind OK, opt_tool::VerifierKind VK,
+                     ArrayRef<StringRef> PassInfos, opt_tool::OutputKind OK,
+                     opt_tool::VerifierKind VK,
                      bool ShouldPreserveAssemblyUseListOrder,
                      bool ShouldPreserveBitcodeUseListOrder,
                      bool EmitSummaryIndex, bool EmitModuleHash,
                      bool EnableDebugify, bool Coroutines);
 } // namespace llvm
 
 #endif

llvm/tools/opt/NewPMDriver.cpp

 
 #define HANDLE_EXTENSION(Ext)                                                  \
   llvm::PassPluginLibraryInfo get##Ext##PluginInfo();
 #include "llvm/Support/Extension.def"
 
 bool llvm::runPassPipeline(StringRef Arg0, Module &M, TargetMachine *TM,
                            ToolOutputFile *Out, ToolOutputFile *ThinLTOLinkOut,
                            ToolOutputFile *OptRemarkFile,
-                           StringRef PassPipeline, OutputKind OK,
-                           VerifierKind VK,
+                           StringRef PassPipeline, ArrayRef<StringRef> Passes,
+                           OutputKind OK, VerifierKind VK,
                            bool ShouldPreserveAssemblyUseListOrder,
                            bool ShouldPreserveBitcodeUseListOrder,
                            bool EmitSummaryIndex, bool EmitModuleHash,
                            bool EnableDebugify, bool Coroutines) {
   bool VerifyEachPass = VK == VK_VerifyEachPass;
 
   Optional<PGOOptions> P;
   switch (PGOKindFlag) {
#include "llvm/Support/Extension.def"
   PB.crossRegisterProxies(LAM, FAM, CGAM, MAM);
 
   ModulePassManager MPM(DebugPM);
   if (VK > VK_NoVerifier)
     MPM.addPass(VerifierPass());
   if (EnableDebugify)
     MPM.addPass(NewPMDebugifyPass());
 
+  if (!PassPipeline.empty()) {
     if (auto Err =
             PB.parsePassPipeline(MPM, PassPipeline, VerifyEachPass, DebugPM)) {
       errs() << Arg0 << ": " << toString(std::move(Err)) << "\n";
       return false;
     }
+  }
+  for (auto PassName : Passes) {
+    if (auto Err =
+            PB.parsePassPipeline(MPM, PassName, VerifyEachPass, DebugPM)) {
+      errs() << Arg0 << ": " << toString(std::move(Err)) << "\n";
+      return false;
+    }
+  }
 
   if (VK > VK_NoVerifier)
     MPM.addPass(VerifierPass());
   if (EnableDebugify)
     MPM.addPass(NewPMCheckDebugifyPass());
 
   // Add any relevant output pass at the end of the pipeline.
   switch (OK) {

llvm/tools/opt/opt.cpp

 static codegen::RegisterCodeGenFlags CFG;
 
 // The OptimizationList is automatically populated with registered Passes by the
 // PassNameParser.
 //
 static cl::list<const PassInfo*, bool, PassNameParser>
 PassList(cl::desc("Optimizations available:"));
 
+static cl::opt<bool>
+    EnableNewPassManager("enable-new-pm",
+                         cl::desc("Enable the new pass manager"));
+
 // This flag specifies a textual description of the optimization pass pipeline
 // to run over the module. This flag switches opt to use the new pass manager
 // infrastructure, completely disabling all of the flags specific to the old
 // pass management.
 static cl::opt<std::string> PassPipeline(
     "passes",
     cl::desc("A textual description of the pass pipeline for optimizing"),
     cl::Hidden);
#endif
   // impress anyone by spewing tons of binary goo to a terminal.
   if (!Force && !NoOutput && !AnalyzeOnly && !OutputAssembly)
     if (CheckBitcodeOutputToConsole(Out->os(), !Quiet))
       NoOutput = true;
 
   if (OutputThinLTOBC)
     M->addModuleFlag(Module::Error, "EnableSplitLTOUnit", SplitLTOUnit);
 
-  if (PassPipeline.getNumOccurrences() > 0) {
-    if (PassList.size()) {
+  if (EnableNewPassManager || PassPipeline.getNumOccurrences() > 0) {
+    if (PassPipeline.getNumOccurrences() > 0 && PassList.size() > 0) {
       errs()
           << "Cannot specify passes via both -foo-pass and --passes=foo-pass";
       return 1;
     }
+    SmallVector<StringRef, 4> Passes;
+    for (const auto &P : PassList) {
+      Passes.push_back(P->getPassArgument());
+    }
     OutputKind OK = OK_NoOutput;
     if (!NoOutput)
       OK = OutputAssembly
                ? OK_OutputAssembly
                : (OutputThinLTOBC ? OK_OutputThinLTOBitcode : OK_OutputBitcode);
 
     VerifierKind VK = VK_VerifyInAndOut;
     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(), Out.get(), ThinLinkOut.get(),
-                           RemarksFile.get(), PassPipeline, OK, VK,
+                           RemarksFile.get(), PassPipeline, Passes, OK, VK,
                            PreserveAssemblyUseListOrder,
                            PreserveBitcodeUseListOrder, EmitSummaryIndex,
                            EmitModuleHash, EnableDebugify, Coroutines)
                ? 0
                : 1;
   }
 
   // Create a PassManager to hold and optimize the collection of passes we are