[mlir][Pass] Disallow mixing -pass-pipeline with other pass options

[rkayaith]

Summary

Currently -pass-pipeline can be specified multiple times and mixed
with the individual -pass-name options. Removing this feature will
allow for including the pipeline anchor as part of the option
argument (see D134900).

[rkayaith]

clang-format

[rriddle]

mlir/include/mlir/Pass/PassRegistry.h

259

Can you move this to the impl?

mlir/lib/Pass/PassRegistry.cpp

781

typo: does not does not

923–924

Can you just fix this now? i.e. you could have a temporary std::string+raw_string_ostream to capture error messages on failure.

[rkayaith]

address comments

mlir/include/mlir/Pass/PassRegistry.h

259

PassPipelineCLParserImpl is used by PassNameCLParser as well, so moving the option into there causes it to be registered twice.

mlir/lib/Pass/PassRegistry.cpp

923–924

fixed, but since the messages from parsePassPipeline don't have a consistent format some cases end up looking odd, e.g.

$ mlir-opt -pass-pipeline='unbalanced('
MLIR Textual PassPipeline Parser:1:12: error: encountered unbalanced parentheses while parsing pipeline
unbalanced(
           ^

becomes:

$ mlir-opt -pass-pipeline='unbalanced('
<unknown>:0: error: MLIR Textual PassPipeline Parser:1:12: error: encountered unbalanced parentheses while parsing pipeline
unbalanced(
           ^

(the <location>: error: part is being printed twice)

[rkayaith]

friendly ping

[rkayaith]

@rriddle could you take another look at this? it's the last patch in the stack that needs review

[rriddle]

LGTM. Apologies for the delay, I've been a little swamped lately.

mlir/lib/Pass/PassRegistry.cpp

923–924

Hmmm, can you file an issue for that? It'd be good to have a nice experience here.

[rkayaith]

mlir/lib/Pass/PassRegistry.cpp

923–924

filed here: https://github.com/llvm/llvm-project/issues/58771

mlir/include/mlir/Pass/PassRegistry.h

 struct PassPipelineCLParserImpl;
 } // namespace detail
 
 /// This class implements a command-line parser for MLIR passes. It registers a
 /// cl option with a given argument and description. This parser will register
 /// options for each of the passes and pipelines that have been registered with
 /// the pass registry; Meaning that `-cse` will refer to the CSE pass in MLIR.
 /// It also registers an argument, `pass-pipeline`, that supports parsing a
-/// textual description of a pipeline.
+/// textual description of a pipeline. This option is mutually exclusive with
+/// the individual pass options.
 class PassPipelineCLParser {
 public:
   /// Construct a pass pipeline parser with the given command line description.
   PassPipelineCLParser(StringRef arg, StringRef description);
   ~PassPipelineCLParser();
 
   /// Returns true if this parser contains any valid options to add.
   bool hasAnyOccurrences() const;
 
   /// Returns true if the given pass registry entry was registered at the
   /// top-level of the parser, i.e. not within an explicit textual pipeline.
   bool contains(const PassRegistryEntry *entry) const;
 
   /// Adds the passes defined by this parser entry to the given pass manager.
   /// Returns failure() if the pass could not be properly constructed due
   /// to options parsing.
   LogicalResult
   addToPipeline(OpPassManager &pm,
                 function_ref<LogicalResult(const Twine &)> errorHandler) const;
 
 private:
   std::unique_ptr<detail::PassPipelineCLParserImpl> impl;
+
+  llvm::cl::opt<std::string> passPipeline;
 };
 
 /// This class implements a command-line parser specifically for MLIR pass
 /// names. It registers a cl option with a given argument and description that
 /// accepts a comma delimited list of pass names.
 class PassNameCLParser {
 public:
   /// Construct a parser with the given command line description.

mlir/lib/Pass/PassRegistry.cpp

 //===- PassRegistry.cpp - Pass Registration Utilities ---------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <utility>
 
 #include "mlir/Pass/Pass.h"
 #include "mlir/Pass/PassManager.h"
 #include "mlir/Pass/PassRegistry.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/SourceMgr.h"
 
 using namespace mlir;
 using namespace detail;
 
if (failed(parsePipelineText(text, errorHandler)))
     return failure();
   return resolvePipelineElements(pipeline, errorHandler);
 }
 
 /// Add the internal pipeline elements to the provided pass manager.
 LogicalResult TextualPipeline::addToPipeline(
     OpPassManager &pm,
     function_ref<LogicalResult(const Twine &)> errorHandler) const {
+  // Temporarily disable implicit nesting while we append to the pipeline. We
+  // want the created pipeline to exactly match the parsed text pipeline, so
+  // it's preferrable to just error out if implicit nesting would be required.
+  OpPassManager::Nesting nesting = pm.getNesting();
+  pm.setNesting(OpPassManager::Nesting::Explicit);
+  auto restore = llvm::make_scope_exit([&]() { pm.setNesting(nesting); });
+
   return addToPipeline(pipeline, pm, errorHandler);
 }
 
 /// Parse the given pipeline text into the internal pipeline vector. This
 /// function only parses the structure of the pipeline, and does not resolve
 /// its elements.
 LogicalResult TextualPipeline::parsePipelineText(StringRef text,
                                                  ErrorHandlerT errorHandler) {
struct PassArgData {
 
   /// This field is used when the parsed option corresponds to a registered pass
   /// or pass pipeline.
   const PassRegistryEntry *registryEntry{nullptr};
 
   /// This field is set when instance specific pass options have been provided
   /// on the command line.
   StringRef options;
-
-  /// This field is used when the parsed option corresponds to an explicit
-  /// pipeline.
-  TextualPipeline pipeline;
 };
 } // namespace
 
 namespace llvm {
 namespace cl {
 /// Define a valid OptionValue for the command line pass argument.
 template <>
 struct OptionValue<PassArgData> final
struct PassNameParser : public llvm::cl::parser<PassArgData> {
   void initialize();
   void printOptionInfo(const llvm::cl::Option &opt,
                        size_t globalWidth) const override;
   size_t getOptionWidth(const llvm::cl::Option &opt) const override;
   bool parse(llvm::cl::Option &opt, StringRef argName, StringRef arg,
              PassArgData &value);
 
   /// If true, this parser only parses entries that correspond to a concrete
-  /// pass registry entry, and does not add a `pass-pipeline` argument, does not
-  /// include the options for pass entries, and does not include pass pipelines
-  /// entries.
+  /// pass registry entry, and does not does not include the options for pass
+  /// entries, and does not include pass pipelines entries.
   bool passNamesOnly = false;
 };
 } // namespace
 
 void PassNameParser::initialize() {
   llvm::cl::parser<PassArgData>::initialize();
 
-  /// Add an entry for the textual pass pipeline option.
-  if (!passNamesOnly) {
-    addLiteralOption(passPipelineArg, PassArgData(),
-                     "A textual description of a pass pipeline to run");
-  }
-
   /// Add the pass entries.
   for (const auto &kv : *passRegistry) {
     addLiteralOption(kv.second.getPassArgument(), &kv.second,
                      kv.second.getPassDescription());
   }
   /// Add the pass pipeline entries.
   if (!passNamesOnly) {
     for (const auto &kv : *passPipelineRegistry) {
void PassNameParser::printOptionInfo(const llvm::cl::Option &opt,
   // Print the information for the top-level option.
   if (opt.hasArgStr()) {
     llvm::outs() << "  --" << opt.ArgStr;
     opt.printHelpStr(opt.HelpStr, globalWidth, opt.ArgStr.size() + 7);
   } else {
     llvm::outs() << "  " << opt.HelpStr << '\n';
   }
 
-  // Print the top-level pipeline argument.
-  printOptionHelp(passPipelineArg,
-                  "A textual description of a pass pipeline to run",
-                  /*indent=*/4, globalWidth, /*isTopLevel=*/!opt.hasArgStr());
-
   // Functor used to print the ordered entries of a registration map.
   auto printOrderedEntries = [&](StringRef header, auto &map) {
     llvm::SmallVector<PassRegistryEntry *, 32> orderedEntries;
     for (auto &kv : map)
       orderedEntries.push_back(&kv.second);
     llvm::array_pod_sort(
         orderedEntries.begin(), orderedEntries.end(),
         [](PassRegistryEntry *const *lhs, PassRegistryEntry *const *rhs) {
for (auto &entry : *passRegistry)
     maxWidth = std::max(maxWidth, entry.second.getOptionWidth() + 4);
   for (auto &entry : *passPipelineRegistry)
     maxWidth = std::max(maxWidth, entry.second.getOptionWidth() + 4);
   return maxWidth;
 }
 
 bool PassNameParser::parse(llvm::cl::Option &opt, StringRef argName,
                            StringRef arg, PassArgData &value) {
-  // Handle the pipeline option explicitly.
-  if (argName == passPipelineArg)
-    return failed(value.pipeline.initialize(arg, llvm::errs()));
-
-  // Otherwise, default to the base for handling.
   if (llvm::cl::parser<PassArgData>::parse(opt, argName, arg, value))
     return true;
   value.options = arg;
   return false;
 }
 
 //===----------------------------------------------------------------------===//
 // PassPipelineCLParser
struct PassPipelineCLParserImpl {
   llvm::cl::list<PassArgData, bool, PassNameParser> passList;
 };
 } // namespace detail
 } // namespace mlir
 
 /// Construct a pass pipeline parser with the given command line description.
 PassPipelineCLParser::PassPipelineCLParser(StringRef arg, StringRef description)
     : impl(std::make_unique<detail::PassPipelineCLParserImpl>(
-          arg, description, /*passNamesOnly=*/false)) {}
+          arg, description, /*passNamesOnly=*/false)),
+      passPipeline(
+          StringRef(passPipelineArg),
+          llvm::cl::desc("Textual description of the pass pipeline to run")) {}
 PassPipelineCLParser::~PassPipelineCLParser() = default;
 
 /// Returns true if this parser contains any valid options to add.
 bool PassPipelineCLParser::hasAnyOccurrences() const {
-  return impl->passList.getNumOccurrences() != 0;
+  return passPipeline.getNumOccurrences() != 0 ||
+         impl->passList.getNumOccurrences() != 0;
 }
 
 /// Returns true if the given pass registry entry was registered at the
 /// top-level of the parser, i.e. not within an explicit textual pipeline.
 bool PassPipelineCLParser::contains(const PassRegistryEntry *entry) const {
   return impl->contains(entry);
 }
 
 /// Adds the passes defined by this parser entry to the given pass manager.
 LogicalResult PassPipelineCLParser::addToPipeline(
     OpPassManager &pm,
     function_ref<LogicalResult(const Twine &)> errorHandler) const {
+  if (passPipeline.getNumOccurrences()) {
+    if (impl->passList.getNumOccurrences())
+      return errorHandler(
+          "'-" + passPipelineArg +
+          "' option can't be used with individual pass options");
+    // FIXME: Report errors to `errorHandler` instead of stderr
+    return parsePassPipeline(passPipeline, pm, llvm::errs());
+  }
+
   for (auto &passIt : impl->passList) {
-    if (passIt.registryEntry) {
-      if (failed(passIt.registryEntry->addToPipeline(pm, passIt.options,
-                                                     errorHandler)))
-        return failure();
-    } else {
-      OpPassManager::Nesting nesting = pm.getNesting();
-      pm.setNesting(OpPassManager::Nesting::Explicit);
-      LogicalResult status = passIt.pipeline.addToPipeline(pm, errorHandler);
-      pm.setNesting(nesting);
-      if (failed(status))
-        return failure();
-    }
   }
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // PassNameCLParser
 
 /// Construct a pass pipeline parser with the given command line description.

mlir/test/Pass/pipeline-parsing.mlir

 // RUN: mlir-opt %s -mlir-disable-threading -pass-pipeline='builtin.module(test-module-pass,func.func(test-function-pass)),func.func(test-function-pass),func.func(cse,canonicalize)' -verify-each=false -mlir-timing -mlir-timing-display=tree 2>&1 | FileCheck %s
 // RUN: mlir-opt %s -mlir-disable-threading -test-textual-pm-nested-pipeline -verify-each=false -mlir-timing -mlir-timing-display=tree 2>&1 | FileCheck %s --check-prefix=TEXTUAL_CHECK
 // RUN: mlir-opt %s -mlir-disable-threading -pass-pipeline='builtin.module(test-module-pass),any(test-interface-pass),any(test-interface-pass),func.func(test-function-pass),any(canonicalize),func.func(cse)' -verify-each=false -mlir-timing -mlir-timing-display=tree 2>&1 | FileCheck %s --check-prefix=GENERIC_MERGE_CHECK
 // RUN: not mlir-opt %s -pass-pipeline='builtin.module(test-module-pass' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_1 %s
 // RUN: not mlir-opt %s -pass-pipeline='builtin.module(test-module-pass))' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_2 %s
 // RUN: not mlir-opt %s -pass-pipeline='builtin.module()(' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_3 %s
 // RUN: not mlir-opt %s -pass-pipeline=',' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_4 %s
 // RUN: not mlir-opt %s -pass-pipeline='func.func(test-module-pass)' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_5 %s
 
 // CHECK_ERROR_1: encountered unbalanced parentheses while parsing pipeline
 // CHECK_ERROR_2: encountered extra closing ')' creating unbalanced parentheses while parsing pipeline
 // CHECK_ERROR_3: expected ',' after parsing pipeline
 // CHECK_ERROR_4: does not refer to a registered pass or pass pipeline
 // CHECK_ERROR_5:  Can't add pass '{{.*}}TestModulePass' restricted to 'builtin.module' on a PassManager intended to run on 'func.func', did you intend to nest?
 
+// RUN: not mlir-opt %s -pass-pipeline='' -cse 2>&1 | FileCheck --check-prefix=CHECK_ERROR_6 %s
+// CHECK_ERROR_6: '-pass-pipeline' option can't be used with individual pass options
+
 func.func @foo() {
   return
 }
 
 module {
   func.func @foo() {
     return
   }