[mlir] Include anchor op when printing pass managers

[rkayaith]

Summary

Previously a pipeline nested on anchor-op would print as just
'pipeline', now it will print as 'anchor-op(pipeline)'. This ensures
the text form includes all information needed to reconstruct the pass
manager.

[rkayaith]

clang-format

[rkayaith]

update description

[mehdi_amini]

I'm not sure I understand the behavior right now, but the test changes look concerning at first look.

mlir/test/python/pass_manager.py

49

The behavior here is not clear to me: we're not really round-tripping anymore? Seems like some asymmetry on the parser/printer.

[rkayaith]

mlir/test/python/pass_manager.py

49

Yes this is one of the places that builtin.module is baked into the C/python api, when parsing a pass manager it always creates a pm anchored on builtin.module, parses the string as a pipeline, and appends it to the pm. Printing the anchor op just makes the implicit builtin.module apparent.

I think it should be updated to round-trip this new printed format properly, but that would break existing usage; I can look into that if @stellaraccident doesn't plan to already.

[mehdi_amini]

mlir/test/python/pass_manager.py

49

OK for Python but what about the others above?

[rkayaith]

mlir/test/python/pass_manager.py

49

the C api roundtripping is the same issue as python, and then the other one where -pass-pipelineis asymmetric with the printed form is because -pass-pipeline is really -append-pipeline-to-current-passmanager; it can be specified multiple times and interleaved with the --pass-name options, so including the anchor op in the string doesn't really make sense. It seemed better to me to leave that how it is, and instead add a new option which includes the anchor op (wip here: https://reviews.llvm.org/D134900), which would be symmetric with the printed form.

And then one other place I noticed was the reproducer pipeline string not including the anchor op, wip patch here: https://reviews.llvm.org/D134623

[rkayaith]

mlir/test/python/pass_manager.py

49

@mehdi_amini any remaining concerns here?

[mehdi_amini]

mlir/test/python/pass_manager.py

49

I guess I'd rather simplify the --pass-pipeline flow to be self-contained.

I don't see a reason to allow mix-and-match of the -pass-name form (which is a bit underspecified in itself and should be reserved to isolated test cases) and the --pass-pipeline form.

I'd look into making --pass-pipeline be the exact specification of the pipeline, without any extra implicit nesting, and forbid any other --pass-name option along with it. Ultimately we should have all this trivially symmetric.

[rkayaith]

mlir/test/python/pass_manager.py

49

Sure that's doable, I was mainly concerned about changing the behaviour of a commonly-used option. The new behaviour would also be pretty verbose in some cases, e.g. if you convert a pass from a FunctionOp pass to a FunctionOpInterface pass, running it goes from -my-pass to -pass-pipeline='builtin.module(func.func(my-pass))' because of the implicit module op.

Anyways, what do you think about merging this change and continuing discussion of -pass-pipeline on https://reviews.llvm.org/D134900?

[mehdi_amini]

mlir/test/python/pass_manager.py

49

I rather get some agreement on a plan first right now: these can stay independent changes after we get to a plan. Otherwise I'm slightly concerned about not converging and what kind of intermediate state we'll end-up in.

[rkayaith]

mlir/test/python/pass_manager.py

49

fair enough, I commented about this on D134900, please take a look

[rkayaith]

mlir/test/python/pass_manager.py

49

seems there's agreement on the end state, @mehdi_amini is this good to go in now?

[rkayaith]

rebase to try and fix patch application in stack

[mehdi_amini]

mlir/test/python/pass_manager.py

49

So all of these test changes will be reverted later at the end of your work?

[mehdi_amini]

mlir/test/python/pass_manager.py

49

I think I see in https://reviews.llvm.org/D134900 that you're adjusting for some things, but not all (like the python tests)

[rkayaith]

mlir/test/python/pass_manager.py

49

the test changes won't be reverted, instead the python and C api will need a change similar to D134900 that makes the anchor op explicit when parsing. i'll work on patches for that.

[rkayaith]

mlir/test/python/pass_manager.py

49

ah actually I see what you mean, yes some of this will end up reverted

[mehdi_amini]

mlir/test/python/pass_manager.py

49

LG

mlir/include/mlir/Pass/Pass.h

public:
 
   /// Attempt to initialize the options of this pass from the given string.
   /// Derived classes may override this method to hook into the point at which
   /// options are initialized, but should generally always invoke this base
   /// class variant.
   virtual LogicalResult initializeOptions(StringRef options);
 
   /// Prints out the pass in the textual representation of pipelines. If this is
-  /// an adaptor pass, print with the op_name(sub_pass,...) format.
+  /// an adaptor pass, print its pass managers.
   void printAsTextualPipeline(raw_ostream &os);
 
   //===--------------------------------------------------------------------===//
   // Statistics
   //===--------------------------------------------------------------------===//
 
   /// This class represents a single pass statistic. This statistic functions
   /// similarly to an unsigned integer value, and may be updated and incremented

mlir/lib/Pass/Pass.cpp

 
 /// Copy the option values from 'other', which is another instance of this
 /// pass.
 void Pass::copyOptionValuesFrom(const Pass *other) {
   passOptions.copyOptionValuesFrom(other->passOptions);
 }
 
 /// Prints out the pass in the textual representation of pipelines. If this is
-/// an adaptor pass, print with the op_name(sub_pass,...) format.
+/// an adaptor pass, print its pass managers.
 void Pass::printAsTextualPipeline(raw_ostream &os) {
-  // Special case for adaptors to use the 'op_name(sub_passes)' format.
+  // Special case for adaptors to print its pass managers.
   if (auto *adaptor = dyn_cast<OpToOpPassAdaptor>(this)) {
     llvm::interleave(
         adaptor->getPassManagers(),
-        [&](OpPassManager &pm) {
-          os << pm.getOpAnchorName() << "(";
-          pm.printAsTextualPipeline(os);
-          os << ")";
-        },
+        [&](OpPassManager &pm) { pm.printAsTextualPipeline(os); },
         [&] { os << ","; });
     return;
   }
   // Otherwise, print the pass argument followed by its options. If the pass
   // doesn't have an argument, print the name of the pass to give some indicator
   // of what pass was run.
   StringRef argument = getArgument();
   if (!argument.empty())

 Optional<OperationName> OpPassManager::getOpName(MLIRContext &context) const {
   return impl->getOpName(context);
 }
 
 StringRef OpPassManager::getOpAnchorName() const {
   return impl->getOpAnchorName();
 }
 
-/// Prints out the given passes as the textual representation of a pipeline.
-static void printAsTextualPipeline(ArrayRef<std::unique_ptr<Pass>> passes,
-                                   raw_ostream &os) {
+/// Prints out the passes of the pass manager as the textual representation
+/// of pipelines.
+void OpPassManager::printAsTextualPipeline(raw_ostream &os) const {
+  os << getOpAnchorName() << "(";
   llvm::interleave(
-      passes,
+      impl->passes,
       [&](const std::unique_ptr<Pass> &pass) {
         pass->printAsTextualPipeline(os);
       },
-      [&] { os << ","; });
-}
-
-/// Prints out the passes of the pass manager as the textual representation
-/// of pipelines.
-void OpPassManager::printAsTextualPipeline(raw_ostream &os) const {
-  ::printAsTextualPipeline(impl->passes, os);
+      [&]() { os << ","; });
+  os << ")";
 }
 
 void OpPassManager::dump() {
   llvm::errs() << "Pass Manager with " << impl->passes.size() << " passes: ";
-  ::printAsTextualPipeline(impl->passes, llvm::errs());
+  printAsTextualPipeline(llvm::errs());
   llvm::errs() << "\n";
 }
 
 static void registerDialectsForPipeline(const OpPassManager &pm,
                                         DialectRegistry &dialects) {
   for (const Pass &pass : pm.getPasses())
     pass.getDependentDialects(dialects);
 }

mlir/test/CAPI/pass.c

void testPrintPassPipeline() {
   MlirOpPassManager nestedModulePm = mlirPassManagerGetNestedUnder(
       pm, mlirStringRefCreateFromCString("builtin.module"));
   MlirOpPassManager nestedFuncPm = mlirOpPassManagerGetNestedUnder(
       nestedModulePm, mlirStringRefCreateFromCString("func.func"));
   MlirPass printOpStatPass = mlirCreateTransformsPrintOpStats();
   mlirOpPassManagerAddOwnedPass(nestedFuncPm, printOpStatPass);
 
   // Print the top level pass manager
-  // CHECK: Top-level: builtin.module(func.func(print-op-stats{json=false}))
+  //      CHECK: Top-level: builtin.module(
+  // CHECK-SAME:   builtin.module(func.func(print-op-stats{json=false}))
+  // CHECK-SAME: )
   fprintf(stderr, "Top-level: ");
   mlirPrintPassPipeline(mlirPassManagerGetAsOpPassManager(pm), printToStderr,
                         NULL);
   fprintf(stderr, "\n");
 
   // Print the pipeline nested one level down
-  // CHECK: Nested Module: func.func(print-op-stats{json=false})
+  // CHECK: Nested Module: builtin.module(func.func(print-op-stats{json=false}))
   fprintf(stderr, "Nested Module: ");
   mlirPrintPassPipeline(nestedModulePm, printToStderr, NULL);
   fprintf(stderr, "\n");
 
   // Print the pipeline nested two levels down
-  // CHECK: Nested Module>Func: print-op-stats
+  // CHECK: Nested Module>Func: func.func(print-op-stats{json=false})
   fprintf(stderr, "Nested Module>Func: ");
   mlirPrintPassPipeline(nestedFuncPm, printToStderr, NULL);
   fprintf(stderr, "\n");
 
   mlirPassManagerDestroy(pm);
   mlirContextDestroy(ctx);
 }
 
status = mlirParsePassPipeline(
           " func.func(print-op-stats{json=false}))"));
   // Expect a failure, we haven't registered the print-op-stats pass yet.
   if (mlirLogicalResultIsFailure(status)) {
     fprintf(stderr,
             "Unexpected failure parsing pipeline after registering the pass\n");
     exit(EXIT_FAILURE);
   }
 
-  // CHECK: Round-trip: builtin.module(func.func(print-op-stats{json=false}),
-  // func.func(print-op-stats{json=false}))
+  //      CHECK: Round-trip: builtin.module(builtin.module(
+  // CHECK-SAME:   func.func(print-op-stats{json=false}),
+  // CHECK-SAME:   func.func(print-op-stats{json=false})
+  // CHECK-SAME: ))
   fprintf(stderr, "Round-trip: ");
   mlirPrintPassPipeline(mlirPassManagerGetAsOpPassManager(pm), printToStderr,
                         NULL);
   fprintf(stderr, "\n");
   mlirPassManagerDestroy(pm);
   mlirContextDestroy(ctx);
 }
 

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

 // 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{test-option=3})' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_2 %s
 // RUN: not mlir-opt %s -pass-pipeline='builtin.module(func.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: mlir-opt %s -pass-pipeline='func.func(test-options-pass{list=1,2,3,4 list=5 string=value1 string=value2})'
 // RUN: mlir-opt %s -verify-each=false -pass-pipeline='func.func(test-options-pass{string-list=a list=1,2,3,4 string-list=b,c list=5 string-list=d string=nested_pipeline{arg1=10 arg2=" {} " arg3=true}})' -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='builtin.module(func.func(test-options-pass{list=3}), func.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_1: test-options-pass{list=1,2,3,4,5 string=nested_pipeline{arg1=10 arg2=" {} " arg3=true} string-list=a,b,c,d}
 // CHECK_2: test-options-pass{list=1 string= string-list=a,b}
-// CHECK_3: builtin.module(func.func(test-options-pass{list=3 string= }),func.func(test-options-pass{list=1,2,3,4 string= }))
+// CHECK_3: builtin.module(builtin.module(func.func(test-options-pass{list=3 string= }),func.func(test-options-pass{list=1,2,3,4 string= })))

mlir/test/python/pass_manager.py

with Context():
     except ValueError as e:
       # CHECK: ValueError exception: invalid pass pipeline 'builtin.module(func.func(not-existing-pass{json=false}))'.
       log("ValueError exception:", e)
     else:
       log("Exception not produced")
 
     # A registered pass should parse successfully.
     pm = PassManager.parse("builtin.module(func.func(print-op-stats{json=false}))")
-    # CHECK: Roundtrip: builtin.module(func.func(print-op-stats{json=false}))
+    # CHECK: Roundtrip: builtin.module(builtin.module(func.func(print-op-stats{json=false})))
     log("Roundtrip: ", pm)
 run(testParseSuccess)
 
 # Verify failure on unregistered pass.
 # CHECK-LABEL: TEST: testParseFail
 def testParseFail():
   with Context():
     try: