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

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).

Reviewed By: rriddle

Differential Revision: https://reviews.llvm.org/D135745

mlir/include/mlir/Pass/PassRegistry.h

@@ -231,7 +231,8 @@ struct PassPipelineCLParserImpl;
 /// 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.
@@ -254,6 +255,8 @@ public:
 
 private:
   std::unique_ptr<detail::PassPipelineCLParserImpl> impl;
+
+  llvm::cl::opt<std::string> passPipeline;
 };
 
 /// This class implements a command-line parser specifically for MLIR pass

mlir/lib/Pass/PassRegistry.cpp

@@ -12,6 +12,7 @@
 #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"
@@ -532,6 +533,13 @@ LogicalResult TextualPipeline::initialize(StringRef text,
 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);
 }
 
@@ -730,10 +738,6 @@ struct PassArgData {
   /// 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
 
@@ -775,9 +779,8 @@ struct PassNameParser : public llvm::cl::parser<PassArgData> {
              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 include pipeline entries or the options
+  /// for pass entries.
   bool passNamesOnly = false;
 };
 } // namespace
@@ -785,12 +788,6 @@ struct PassNameParser : public llvm::cl::parser<PassArgData> {
 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,
@@ -823,11 +820,6 @@ void PassNameParser::printOptionInfo(const llvm::cl::Option &opt,
     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;
@@ -865,11 +857,6 @@ size_t PassNameParser::getOptionWidth(const llvm::cl::Option &opt) const {
 
 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;
@@ -907,12 +894,16 @@ struct PassPipelineCLParserImpl {
 /// 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
@@ -925,19 +916,22 @@ bool PassPipelineCLParser::contains(const PassRegistryEntry *entry) const {
 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");
+    std::string errMsg;
+    llvm::raw_string_ostream os(errMsg);
+    if (failed(parsePassPipeline(passPipeline, pm, os)))
+      return errorHandler(errMsg);
+    return success();
+  }
+
   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();
-    }
+    if (failed(passIt.registryEntry->addToPipeline(pm, passIt.options,
+                                                   errorHandler)))
+      return failure();
   }
   return success();
 }

mlir/test/Pass/pipeline-parsing.mlir

@@ -13,6 +13,9 @@
 // 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
 }