Disable implicit nesting on parsing textual pass pipeline

[mehdi_amini]

Summary

Previous the textual form of the pass pipeline would implicitly nest,
instead we opt for the explicit form here: this has less surprise.

This also avoids asserting in the bindings when passing a pass pipeline
with incorrect nesting.

[mehdi_amini]

Remove debugging braces

[rriddle]

mlir/include/mlir/Pass/PassManager.h

127

nit: I generally lean more towards a enableFoo(bool enable) style, which matches enableVerifier below.

mlir/lib/Pass/Pass.cpp

351

nit: Allowed -> Enabled

mlir/lib/Pass/PassRegistry.cpp

36

The result of the handler is being ignored here.

mlir/lib/Support/MlirOptMain.cpp

66

nit: The .str() shouldn't be necessary here.

[mehdi_amini]

mlir/include/mlir/Pass/PassManager.h

127

We have an enum, I'll go with setNesting(Implicit/Explicit)

mlir/lib/Support/MlirOptMain.cpp

66

error: no matching conversion for functional-style cast from 'llvm::Twine' to 'mlir::DiagnosticArgument'

[mehdi_amini]

Address River's comments

[stellaraccident]

Aside from nits that River is discussing, lgtm! Thanks for this!

[rriddle]

mlir/include/mlir/Pass/PassManager.h

122

nit: Can we just have a getNesting to match the setNesting?

127

nit: Missing a parameter name here.

mlir/lib/Support/MlirOptMain.cpp

66

Where is that error coming from?
(https://github.com/llvm/llvm-project/blob/782a93cc9abeee9bd90f7be92db4eb3895e15384/mlir/include/mlir/IR/Diagnostics.h#L207)

[mehdi_amini]

Use const Twine& for the handler parameter

mlir/include/mlir/Pass/PassManager.h

public:
   /// Merge the pass statistics of this class into 'other'.
   void mergeStatisticsInto(OpPassManager &other);
 
   /// Register dependent dialects for the current pass manager.
   /// This is forwarding to every pass in this PassManager, see the
   /// documentation for the same method on the Pass class.
   void getDependentDialects(DialectRegistry &dialects) const;
 
+  /// Return true if implicit nesting is current allowed.
+  bool isImplicitNestingAllowed();
+
+  /// Enable the implicit nesting on this particular PassManager.
+  /// This will also apply to any newly nested PassManager built from this
+  /// instance.
+  void enableImplicitNesting();
+
+  /// Disable the implicit nesting on this particular PassManager.
+  /// This will also apply to any newly nested PassManager built from this
+  /// instance.
+  void disableImplicitNesting();
+
 private:
   /// A pointer to an internal implementation instance.
   std::unique_ptr<detail::OpPassManagerImpl> impl;
 
   /// Allow access to the constructor.
   friend class PassManager;
   friend class Pass;
 

mlir/include/mlir/Pass/PassRegistry.h

 class Pass;
 
 namespace detail {
 class PassOptions;
 } // end namespace detail
 
 /// A registry function that adds passes to the given pass manager. This should
 /// also parse options and return success() if parsing succeeded.
-using PassRegistryFunction =
-    std::function<LogicalResult(OpPassManager &, StringRef options)>;
+/// `errorHandler` is a functor used to emit errors during parsing.
+/// parameter corresponds to the raw location within the pipeline string. This
+/// should always return failure.
+using PassRegistryFunction = std::function<LogicalResult(
+    OpPassManager &, StringRef options,
+    function_ref<LogicalResult(Twine)> errorHandler)>;
 using PassAllocatorFunction = std::function<std::unique_ptr<Pass>()>;
 
 //===----------------------------------------------------------------------===//
 // PassRegistry
 //===----------------------------------------------------------------------===//
 
 /// Structure to group information about a passes and pass pipelines (argument
 /// to invoke via mlir-opt, description, pass pipeline builder).
 class PassRegistryEntry {
 public:
   /// Adds this pass registry entry to the given pass manager. `options` is
   /// an opaque string that will be parsed by the builder. The success of
   /// parsing will be returned.
-  LogicalResult addToPipeline(OpPassManager &pm, StringRef options) const {
+  LogicalResult
+  addToPipeline(OpPassManager &pm, StringRef options,
+                function_ref<LogicalResult(Twine)> errorHandler) const {
     assert(builder &&
            "cannot call addToPipeline on PassRegistryEntry without builder");
-    return builder(pm, options);
+    return builder(pm, options, errorHandler);
   }
 
   /// Returns the command line option that may be passed to 'mlir-opt' that will
   /// cause this pass to run or null if there is no such argument.
   StringRef getPassArgument() const { return arg; }
 
   /// Returns a description for the pass, this never returns null.
   StringRef getPassDescription() const { return description; }

 ///                                          pipelineBuilder);
 template <typename Options = EmptyPipelineOptions>
 struct PassPipelineRegistration {
   PassPipelineRegistration(
       StringRef arg, StringRef description,
       std::function<void(OpPassManager &, const Options &options)> builder) {
     registerPassPipeline(
         arg, description,
-        [builder](OpPassManager &pm, StringRef optionsStr) {
+        [builder](OpPassManager &pm, StringRef optionsStr,
+                  function_ref<LogicalResult(Twine)> errorHandler) {
           Options options;
           if (failed(options.parseFromString(optionsStr)))
             return failure();
           builder(pm, options);
           return success();
         },
         [](function_ref<void(const detail::PassOptions &)> optHandler) {
           optHandler(Options());
         });
   }
 };
 
 /// Convenience specialization of PassPipelineRegistration for EmptyPassOptions
 /// that does not pass an empty options struct to the pass builder function.
 template <> struct PassPipelineRegistration<EmptyPipelineOptions> {
   PassPipelineRegistration(StringRef arg, StringRef description,
                            std::function<void(OpPassManager &)> builder) {
     registerPassPipeline(
         arg, description,
-        [builder](OpPassManager &pm, StringRef optionsStr) {
+        [builder](OpPassManager &pm, StringRef optionsStr,
+                  function_ref<LogicalResult(Twine)> errorHandler) {
           if (!optionsStr.empty())
             return failure();
           builder(pm);
           return success();
         },
         [](function_ref<void(const detail::PassOptions &)>) {});
   }
 };
public:
 
   /// 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) const;
+  LogicalResult
+  addToPipeline(OpPassManager &pm,
+                function_ref<LogicalResult(Twine)> errorHandler) const;
 
 private:
   std::unique_ptr<detail::PassPipelineCLParserImpl> impl;
 };
 
 } // end namespace mlir
 
 #endif // MLIR_PASS_PASSREGISTRY_H_

mlir/lib/Pass/Pass.cpp

static void registerDialectsForPipeline(const OpPassManager &pm,
   for (const Pass &pass : pm.getPasses())
     pass.getDependentDialects(dialects);
 }
 
 void OpPassManager::getDependentDialects(DialectRegistry &dialects) const {
   registerDialectsForPipeline(*this, dialects);
 }
 
+bool OpPassManager::isImplicitNestingAllowed() {
+  return impl->nesting == Nesting::Implicit;
+}
+
+void OpPassManager::enableImplicitNesting() {
+  impl->nesting = Nesting::Implicit;
+}
+
+void OpPassManager::disableImplicitNesting() {
+  impl->nesting = Nesting::Explicit;
+}
+
 //===----------------------------------------------------------------------===//
 // OpToOpPassAdaptor
 //===----------------------------------------------------------------------===//
 
 LogicalResult OpToOpPassAdaptor::run(Pass *pass, Operation *op,
                                      AnalysisManager am, bool verifyPasses) {
   if (!op->getName().getAbstractOperation())
     return op->emitOpError()

mlir/lib/Pass/PassRegistry.cpp

 
 /// Static mapping of all of the registered pass pipelines.
 static llvm::ManagedStatic<llvm::StringMap<PassPipelineInfo>>
     passPipelineRegistry;
 
 /// Utility to create a default registry function from a pass instance.
 static PassRegistryFunction
 buildDefaultRegistryFn(const PassAllocatorFunction &allocator) {
-  return [=](OpPassManager &pm, StringRef options) {
+  return [=](OpPassManager &pm, StringRef options,
+             function_ref<LogicalResult(Twine)> errorHandler) {
     std::unique_ptr<Pass> pass = allocator();
     LogicalResult result = pass->initializeOptions(options);
+    if (!pm.isImplicitNestingAllowed() && pass->getOpName() &&
+        *pass->getOpName() != pm.getOpName()) {
+      errorHandler(llvm::Twine("Can't add pass '") + pass->getName() +
+                   "' restricted to '" + *pass->getOpName() +
+                   "' on a PassManager intended to run on '" + pm.getOpName() +
+                   "', did you intend to nest?");
+      return failure();
+    }
     pm.addPass(std::move(pass));
     return result;
   };
 }
 
 /// Utility to print the help string for a specific option.
 static void printOptionHelp(StringRef arg, StringRef desc, size_t indent,
                             size_t descIndent, bool isTopLevel) {

 /// This class represents a textual description of a pass pipeline.
 class TextualPipeline {
 public:
   /// Try to initialize this pipeline with the given pipeline text.
   /// `errorStream` is the output stream to emit errors to.
   LogicalResult initialize(StringRef text, raw_ostream &errorStream);
 
   /// Add the internal pipeline elements to the provided pass manager.
-  LogicalResult addToPipeline(OpPassManager &pm) const;
+  LogicalResult
+  addToPipeline(OpPassManager &pm,
+                function_ref<LogicalResult(Twine)> errorHandler) const;
 
 private:
   /// A functor used to emit errors found during pipeline handling. The first
   /// parameter corresponds to the raw location within the pipeline string. This
   /// should always return failure.
   using ErrorHandlerT = function_ref<LogicalResult(const char *, Twine)>;
 
   /// A struct to capture parsed pass pipeline names.
private:
   resolvePipelineElements(MutableArrayRef<PipelineElement> elements,
                           ErrorHandlerT errorHandler);
 
   /// Resolve a single element of the pipeline.
   LogicalResult resolvePipelineElement(PipelineElement &element,
                                        ErrorHandlerT errorHandler);
 
   /// Add the given pipeline elements to the provided pass manager.
-  LogicalResult addToPipeline(ArrayRef<PipelineElement> elements,
-                              OpPassManager &pm) const;
+  LogicalResult
+  addToPipeline(ArrayRef<PipelineElement> elements, OpPassManager &pm,
+                function_ref<LogicalResult(Twine)> errorHandler) const;
 
   std::vector<PipelineElement> pipeline;
 };
 
 } // end anonymous namespace
 
 /// Try to initialize this pipeline with the given pipeline text. An option is
 /// given to enable accurate error reporting.
LogicalResult TextualPipeline::initialize(StringRef text,
 
   // Parse the provided pipeline string.
   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) const {
-  return addToPipeline(pipeline, pm);
+LogicalResult TextualPipeline::addToPipeline(
+    OpPassManager &pm, function_ref<LogicalResult(Twine)> errorHandler) const {
+  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) {
   SmallVector<std::vector<PipelineElement> *, 4> pipelineStack = {&pipeline};

 }
 
 /// Resolve a single element of the pipeline.
 LogicalResult
 TextualPipeline::resolvePipelineElement(PipelineElement &element,
                                         ErrorHandlerT errorHandler) {
   // If the inner pipeline of this element is not empty, this is an operation
   // pipeline.
-  if (!element.innerPipeline.empty())
+  if (!element.innerPipeline.empty()) {
     return resolvePipelineElements(element.innerPipeline, errorHandler);
-
+  }
   // Otherwise, this must be a pass or pass pipeline.
   // Check to see if a pipeline was registered with this name.
   auto pipelineRegistryIt = passPipelineRegistry->find(element.name);
   if (pipelineRegistryIt != passPipelineRegistry->end()) {
     element.registryEntry = &pipelineRegistryIt->second;
     return success();
   }
 
   // If not, then this must be a specific pass name.
   for (auto &passIt : *passRegistry) {
     if (passIt.second.getPassArgument() == element.name) {
       element.registryEntry = &passIt.second;
       return success();
     }
   }
 
   // Emit an error for the unknown pass.
   auto *rawLoc = element.name.data();
   return errorHandler(rawLoc, "'" + element.name +
                                   "' does not refer to a "
                                   "registered pass or pass pipeline");
 }
 
 /// Add the given pipeline elements to the provided pass manager.
-LogicalResult TextualPipeline::addToPipeline(ArrayRef<PipelineElement> elements,
-                                             OpPassManager &pm) const {
+LogicalResult TextualPipeline::addToPipeline(
+    ArrayRef<PipelineElement> elements, OpPassManager &pm,
+    function_ref<LogicalResult(Twine)> errorHandler) const {
   for (auto &elt : elements) {
     if (elt.registryEntry) {
-      if (failed(elt.registryEntry->addToPipeline(pm, elt.options)))
+      if (failed(
+              elt.registryEntry->addToPipeline(pm, elt.options, errorHandler)))
         return failure();
-    } else if (failed(addToPipeline(elt.innerPipeline, pm.nest(elt.name)))) {
+    } else if (failed(addToPipeline(elt.innerPipeline, pm.nest(elt.name),
+                                    errorHandler))) {
       return failure();
     }
   }
   return success();
 }
 
 /// This function parses the textual representation of a pass pipeline, and adds
 /// the result to 'pm' on success. This function returns failure if the given
 /// pipeline was invalid. 'errorStream' is an optional parameter that, if
 /// non-null, will be used to emit errors found during parsing.
 LogicalResult mlir::parsePassPipeline(StringRef pipeline, OpPassManager &pm,
                                       raw_ostream &errorStream) {
   TextualPipeline pipelineParser;
   if (failed(pipelineParser.initialize(pipeline, errorStream)))
     return failure();
-  if (failed(pipelineParser.addToPipeline(pm)))
+  auto errorHandler = [&](Twine msg) {
+    errorStream << msg << "\n";
+    return failure();
+  };
+  if (failed(pipelineParser.addToPipeline(pm, errorHandler)))
     return failure();
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // PassNameParser
 //===----------------------------------------------------------------------===//
 

 /// top-level of the parser, i.e. not within an explicit textual pipeline.
 bool PassPipelineCLParser::contains(const PassRegistryEntry *entry) const {
   return llvm::any_of(impl->passList, [&](const PassArgData &data) {
     return data.registryEntry == entry;
   });
 }
 
 /// Adds the passes defined by this parser entry to the given pass manager.
-LogicalResult PassPipelineCLParser::addToPipeline(OpPassManager &pm) const {
+LogicalResult PassPipelineCLParser::addToPipeline(
+    OpPassManager &pm, function_ref<LogicalResult(Twine)> errorHandler) const {
   for (auto &passIt : impl->passList) {
     if (passIt.registryEntry) {
-      if (failed(passIt.registryEntry->addToPipeline(pm, passIt.options)))
+      if (failed(passIt.registryEntry->addToPipeline(pm, passIt.options,
+                                                     errorHandler)))
         return failure();
-    } else if (failed(passIt.pipeline.addToPipeline(pm))) {
+    } else {
+      bool implicitNestingAllowed = pm.isImplicitNestingAllowed();
+      pm.disableImplicitNesting();
+      LogicalResult status = passIt.pipeline.addToPipeline(pm, errorHandler);
+      if (implicitNestingAllowed)
+        pm.enableImplicitNesting();
+      if (failed(status))
         return failure();
     }
   }
   return success();
 }

mlir/lib/Support/MlirOptMain.cpp

static LogicalResult performActions(raw_ostream &os, bool verifyDiagnostics,
   if (!module)
     return failure();
 
   // Apply any pass manager command line options.
   PassManager pm(context, OpPassManager::Nesting::Implicit);
   pm.enableVerifier(verifyPasses);
   applyPassManagerCLOptions(pm);
 
+  auto errorHandler = [&](Twine msg) {
+    emitError(UnknownLoc::get(context)) << msg.str();
+    return failure();
+  };
+
   // Build the provided pipeline.
-  if (failed(passPipeline.addToPipeline(pm)))
+  if (failed(passPipeline.addToPipeline(pm, errorHandler)))
     return failure();
 
   // Run the pipeline.
   if (failed(pm.run(*module)))
     return failure();
 
   // Print the output.
   module->print(os);

mlir/test/Bindings/Python/pass_manager.py

with Context():
     except ValueError as e:
       # CHECK: ValueError exception: invalid pass pipeline 'unknown-pass'.
       log("ValueError exception:", e)
     else:
       log("Exception not produced")
 run(testParseFail)
 
 
+# Verify failure on incorrect level of nesting.
+# CHECK-LABEL: TEST: testInvalidNesting
+def testInvalidNesting():
+  with Context():
+    try:
+      pm = PassManager.parse("func(print-op-graph)")
+    except ValueError as e:
+      # CHECK: Can't add pass 'PrintOp' restricted to 'module' on a PassManager intended to run on 'func', did you intend to nest?
+      # CHECK: ValueError exception: invalid pass pipeline 'func(print-op-graph)'.
+      log("ValueError exception:", e)
+    else:
+      log("Exception not produced")
+run(testInvalidNesting)
+
+
 # Verify that a pass manager can execute on IR
 # CHECK-LABEL: TEST: testRun
 def testRunPipeline():
   with Context():
     pm = PassManager.parse("print-op-stats")
     module = Module.parse(r"""func @successfulParse() { return }""")
     pm.run(module)
 # CHECK: Operations encountered:
 # CHECK: func              , 1
 # CHECK: module            , 1
 # CHECK: module_terminator , 1
 # CHECK: std.return        , 1
 run(testRunPipeline)

mlir/test/Pass/pipeline-options-parsing.mlir

 // RUN: not mlir-opt %s -pass-pipeline='module(test-module-pass{)' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_1 %s
 // RUN: not mlir-opt %s -pass-pipeline='module(test-module-pass{test-option=3})' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_2 %s
-// RUN: not mlir-opt %s -pass-pipeline='module(test-options-pass{list=3}, test-module-pass{invalid-option=3})' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_3 %s
+// RUN: not mlir-opt %s -pass-pipeline='module(func(test-options-pass{list=3}), test-module-pass{invalid-option=3})' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_3 %s
 // RUN: not mlir-opt %s -pass-pipeline='test-options-pass{list=3 list=notaninteger}' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_4 %s
 // RUN: not mlir-opt %s -pass-pipeline='func(test-options-pass{list=1,2,3,4 list=5 string=value1 string=value2})' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_5 %s
 // RUN: mlir-opt %s -verify-each=false -pass-pipeline='func(test-options-pass{string-list=a list=1,2,3,4 string-list=b,c list=5 string-list=d string=some_value})' -test-dump-pipeline 2>&1 | FileCheck --check-prefix=CHECK_1 %s
 // RUN: mlir-opt %s -verify-each=false -test-options-pass-pipeline='list=1 string-list=a,b' -test-dump-pipeline 2>&1 | FileCheck --check-prefix=CHECK_2 %s
-// RUN: mlir-opt %s -verify-each=false -pass-pipeline='module(test-options-pass{list=3}, test-options-pass{list=1,2,3,4})' -test-dump-pipeline 2>&1 | FileCheck --check-prefix=CHECK_3 %s
+// RUN: mlir-opt %s -verify-each=false -pass-pipeline='module(func(test-options-pass{list=3}), func(test-options-pass{list=1,2,3,4}))' -test-dump-pipeline 2>&1 | FileCheck --check-prefix=CHECK_3 %s
 
 // CHECK_ERROR_1: missing closing '}' while processing pass options
 // CHECK_ERROR_2: no such option test-option
 // CHECK_ERROR_3: no such option invalid-option
 // CHECK_ERROR_4: 'notaninteger' value invalid for integer argument
 // CHECK_ERROR_5: string option: may only occur zero or one times
 
 // CHECK_1: test-options-pass{list=1,2,3,4,5 string=some_value string-list=a,b,c,d}
 // CHECK_2: test-options-pass{list=1 string= string-list=a,b}
 // CHECK_3: module(func(test-options-pass{list=3 string= string-list=}), func(test-options-pass{list=1,2,3,4 string= string-list=}))

mlir/test/Pass/pipeline-parsing.mlir

 // RUN: mlir-opt %s -pass-pipeline='module(test-module-pass,func(test-function-pass)),func(test-function-pass)' -pass-pipeline="func(cse,canonicalize)" -verify-each=false -pass-timing -pass-timing-display=pipeline 2>&1 | FileCheck %s
 // RUN: mlir-opt %s -test-textual-pm-nested-pipeline -verify-each=false -pass-timing -pass-timing-display=pipeline 2>&1 | FileCheck %s --check-prefix=TEXTUAL_CHECK
 // RUN: not mlir-opt %s -pass-pipeline='module(test-module-pass' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_1 %s
 // RUN: not mlir-opt %s -pass-pipeline='module(test-module-pass))' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_2 %s
 // RUN: not mlir-opt %s -pass-pipeline='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(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 'module' on a PassManager intended to run on 'func', did you intend to nest?
 func @foo() {
   return
 }
 
 module {
   func @foo() {
     return
   }