Print custom assembly on pass failure by default

[mehdi_amini]

Summary

The printer is now resilient to invalid IR and will already automatically
fallback to the generic form on invalid IR. Using the generic printer on
pass failure was a conservative option before the printer was made
failsafe.

[mehdi_amini]

Improve testing

mlir/test/Pass/invalid-ir.mlir

2

If we want a flag to force the custom printer, it should address the two cases above.

[jpienaar]

Nice, good point about the change to dump for invalid IR, this seems to provide the functionality of the previous change without needing a flag/API change and producing the prettier output in more cases. Of course the discussion still ongoing but looks like general improvement.

mlir/test/Pass/invalid-ir.mlir

1

invalid-ir-print-after-failure? (The file name sounds like it could be testing a verifier).

mlir/test/lib/Pass/TestPassManager.cpp

166

s/as well// ? (This could be triggered independently)

[lattner]

Why is this safe? Does the printer already check that the IR passes verify() and falls back to the generic form automatically?

If so, this is awesome. Does Operation::dump() have the same behavior?

[mehdi_amini]

In D123915#3456079, @lattner wrote:

Why is this safe? Does the printer already check that the IR passes verify() and falls back to the generic form automatically?

If so, this is awesome. Does Operation::dump() have the same behavior?

Yes: this changed ~ last month I think.

That is what I was trying to explain in Uday's patch and why I was objecting to adding a flag.

[lattner]

nice! Very excited this "just works" instead of being another knob for experts

[rriddle]

mlir/test/lib/Pass/TestPassManager.cpp

11–14

Are all of these new includes necessary?

145

This shouldn't need to be called explicitly, options are implicitly copied.

161

You can use the builder to create the unknown loc.

[mehdi_amini]

Address River's comments

mlir/test/lib/Pass/TestPassManager.cpp

11–14

I think we get them transitively and VSCode added it for me as I was using APIs.

[mehdi_amini]

Address Jacques' comment (rename file)

[mehdi_amini]

Will wait one more day to make sure @bondhugula and @stellaraccident can chime in as well.

[bondhugula]

In D123915#3456042, @jpienaar wrote:

Nice, good point about the change to dump for invalid IR, this seems to provide the functionality of the previous change without needing a flag/API change and producing the prettier output in more cases. Of course the discussion still ongoing but looks like general improvement.

Not completely. It doesn't provide you with a custom assembly when the IR is invalid -- you *will* need an extra flag to say you need custom assembly even when the IR is invalid.

mlir/test/Pass/invalid-ir-print-after-failure.mlir

12 ↗

(On Diff #423435)

Drop leading space.

mlir/test/Pass/ir-printing.mlir

7

Nit: You can just call this AFTER_FAILURE (since you have the other checks for valid/invalid in another file).

mlir/test/lib/Pass/TestPassManager.cpp

148

add -> adds

220

Nit: Sorted order here.

[mehdi_amini]

In D123915#3458280, @bondhugula wrote:

In D123915#3456042, @jpienaar wrote:

Nice, good point about the change to dump for invalid IR, this seems to provide the functionality of the previous change without needing a flag/API change and producing the prettier output in more cases. Of course the discussion still ongoing but looks like general improvement.

Not completely. It doesn't provide you with a custom assembly when the IR is invalid -- you *will* need an extra flag to say you need custom assembly even when the IR is invalid.

Your patch didn’t handle invalid IR, as I shown with the examples in one of my last answers there.

[bondhugula]

In D123915#3456084, @mehdi_amini wrote:

In D123915#3456079, @lattner wrote:

Why is this safe? Does the printer already check that the IR passes verify() and falls back to the generic form automatically?

If so, this is awesome. Does Operation::dump() have the same behavior?

Yes: this changed ~ last month I think.

That is what I was trying to explain in Uday's patch and why I was objecting to adding a flag.

Without an extra flag, how do you get custom assembly when the IR is invalid for trivial reasons (like in the example I provided -- a subscript being an i32 type instead of an index type for example for a memref.load/affine.load in a nested loop)? There are several such cases in which custom printers will not crash.

[bondhugula]

In D123915#3458296, @mehdi_amini wrote:

In D123915#3458280, @bondhugula wrote:

In D123915#3456042, @jpienaar wrote:

Nice, good point about the change to dump for invalid IR, this seems to provide the functionality of the previous change without needing a flag/API change and producing the prettier output in more cases. Of course the discussion still ongoing but looks like general improvement.

Not completely. It doesn't provide you with a custom assembly when the IR is invalid -- you *will* need an extra flag to say you need custom assembly even when the IR is invalid.

Your patch didn’t handle invalid IR, as I shown with the examples in one of my last answers there.

Right. (I went back and checked that.) We need something more -- to force the printer to use the custom form at the risk of crashing. Are you planning to handle this in this revision?

[mehdi_amini]

In D123915#3458297, @bondhugula wrote:

In D123915#3456084, @mehdi_amini wrote:

In D123915#3456079, @lattner wrote:

Why is this safe? Does the printer already check that the IR passes verify() and falls back to the generic form automatically?

If so, this is awesome. Does Operation::dump() have the same behavior?

Yes: this changed ~ last month I think.

That is what I was trying to explain in Uday's patch and why I was objecting to adding a flag.

Without an extra flag, how do you get custom assembly when the IR is invalid for trivial reasons (like in the example I provided -- a subscript being an i32 type instead of an index type for example for a memref.load/affine.load in a nested loop)? There are several such cases in which custom printers will not crash.

You need an extra flag, your patch didn’t handle this though, as I demonstrated with an example there.
This why I mentioned two separate cases: 1) for the pass failing but the ir valid, handled by your patch but subsumed here by changing the default, and 2) the invalid IR case which needs a flag, but neither this patch nor your patch handles

[mehdi_amini]

We need something more -- to force the printer to use the custom form at the risk of crashing. Are you planning to handle this in this revision?

No: I’ll land this as is and then see how the printer can be forced to assume the ir is verified

[mehdi_amini]

In D123915#3458332, @mehdi_amini wrote:

We need something more -- to force the printer to use the custom form at the risk of crashing. Are you planning to handle this in this revision?

No: I’ll land this as is and then see how the printer can be forced to assume the ir is verified

Seems like the existing set of flags is enough, I improved the test to demonstrate it here: https://github.com/llvm/llvm-project/commit/02eac667ed2dbf6eca4bc1195600eb0d3ecaff77

Let me know if anything is missing!

[stellaraccident]

Thanks Mehdi (just catching up and LGTM).

mlir/lib/Pass/IRPrinting.cpp

 void IRPrinterInstrumentation::runAfterPassFailed(Pass *pass, Operation *op) {
   if (isa<OpToOpPassAdaptor>(pass))
     return;
   if (config->shouldPrintAfterOnlyOnChange())
     beforePassFingerPrints.erase(pass);
 
   config->printAfterIfEnabled(pass, op, [&](raw_ostream &out) {
     out << formatv("// -----// IR Dump After {0} Failed", pass->getName());
-    printIR(op, config->shouldPrintAtModuleScope(), out,
-            OpPrintingFlags().printGenericOpForm());
+    printIR(op, config->shouldPrintAtModuleScope(), out, OpPrintingFlags());
     out << "\n\n";
   });
 }
 
 //===----------------------------------------------------------------------===//
 // IRPrinterConfig
 //===----------------------------------------------------------------------===//
 

mlir/test/Pass/invalid-ir.mlir

+// RUN: not mlir-opt %s -pass-pipeline='func.func(test-pass-create-invalid-ir{emit-invalid-ir=true signal-pass-failure=true})' -mlir-print-ir-after-failure 2>&1 | FileCheck %s --check-prefix=CHECK-INVALID
+// RUN: not mlir-opt %s -pass-pipeline='func.func(test-pass-create-invalid-ir{emit-invalid-ir=true signal-pass-failure=false})' -mlir-print-ir-after-failure 2>&1 | FileCheck %s --check-prefix=CHECK-INVALID
+// RUN: not mlir-opt %s -pass-pipeline='func.func(test-pass-create-invalid-ir{emit-invalid-ir=false signal-pass-failure=true})' -mlir-print-ir-after-failure 2>&1  | FileCheck %s --check-prefix=CHECK-VALID
+// RUN: mlir-opt %s -pass-pipeline='func.func(test-pass-create-invalid-ir{emit-invalid-ir=false signal-pass-failure=false})' -mlir-print-ir-after-failure 2>&1  | FileCheck %s --check-prefix=CHECK-VALID
+
+// Test whether we print generically or not on pass failure, depending on whether there is invalid IR or not.
+
+// CHECK-VALID: func @TestCreateInvalidCallInPass
+// CHECK-INVALID: "func.func"
+func @TestCreateInvalidCallInPass() {
+  return
+ }

mlir/test/Pass/ir-printing.mlir

 // RUN: mlir-opt %s -mlir-disable-threading=true -pass-pipeline='func.func(cse,canonicalize)' -mlir-print-ir-before=cse  -o /dev/null 2>&1 | FileCheck -check-prefix=BEFORE %s
 // RUN: mlir-opt %s -mlir-disable-threading=true -pass-pipeline='func.func(cse,canonicalize)' -mlir-print-ir-before-all -o /dev/null 2>&1 | FileCheck -check-prefix=BEFORE_ALL %s
 // RUN: mlir-opt %s -mlir-disable-threading=true -pass-pipeline='func.func(cse,canonicalize)' -mlir-print-ir-after=cse -o /dev/null 2>&1 | FileCheck -check-prefix=AFTER %s
 // RUN: mlir-opt %s -mlir-disable-threading=true -pass-pipeline='func.func(cse,canonicalize)' -mlir-print-ir-after-all -o /dev/null 2>&1 | FileCheck -check-prefix=AFTER_ALL %s
 // RUN: mlir-opt %s -mlir-disable-threading=true -pass-pipeline='func.func(cse,canonicalize)' -mlir-print-ir-before=cse -mlir-print-ir-module-scope -o /dev/null 2>&1 | FileCheck -check-prefix=BEFORE_MODULE %s
 // RUN: mlir-opt %s -mlir-disable-threading=true -pass-pipeline='func.func(cse,cse)' -mlir-print-ir-after-all -mlir-print-ir-after-change -o /dev/null 2>&1 | FileCheck -check-prefix=AFTER_ALL_CHANGE %s
-// RUN: not mlir-opt %s -mlir-disable-threading=true -pass-pipeline='func.func(cse,test-pass-failure)' -mlir-print-ir-after-failure -o /dev/null 2>&1 | FileCheck -check-prefix=AFTER_FAILURE %s
+// RUN: not mlir-opt %s -mlir-disable-threading=true -pass-pipeline='func.func(cse,test-pass-failure)' -mlir-print-ir-after-failure -o /dev/null 2>&1 | FileCheck -check-prefix=AFTER_FAILURE_CUSTOM %s
 
 func @foo() {
   %0 = arith.constant 0 : i32
   return
 }
 
 func @bar() {
   return

 // BEFORE_MODULE: func @bar()
 
 // AFTER_ALL_CHANGE: // -----// IR Dump After{{.*}}CSE //----- //
 // AFTER_ALL_CHANGE-NEXT: func @foo()
 // AFTER_ALL_CHANGE-NOT: // -----// IR Dump After{{.*}}CSE //----- //
 // We expect that only 'foo' changed during CSE, and the second run of CSE did
 // nothing.
 
-// AFTER_FAILURE-NOT: // -----// IR Dump After{{.*}}CSE
-// AFTER_FAILURE: // -----// IR Dump After{{.*}}TestFailurePass Failed //----- //
+// AFTER_FAILURE_CUSTOM-NOT: // -----// IR Dump After{{.*}}CSE
+// AFTER_FAILURE_CUSTOM: // -----// IR Dump After{{.*}}TestFailurePass Failed //----- //
+// AFTER_FAILURE_CUSTOM: func @foo()

mlir/test/lib/Pass/TestPassManager.cpp

 //===- TestPassManager.cpp - Test pass manager functionality --------------===//
 //
 // 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 "TestDialect.h"
 #include "mlir/Dialect/Func/IR/FuncOps.h"
 #include "mlir/IR/BuiltinOps.h"
+#include "mlir/IR/Location.h"
+#include "mlir/IR/OperationSupport.h"
 #include "mlir/Pass/Pass.h"
 #include "mlir/Pass/PassManager.h"
+#include "mlir/Pass/PassRegistry.h"
 
 using namespace mlir;
 
 namespace {
 struct TestModulePass
     : public PassWrapper<TestModulePass, OperationPass<ModuleOp>> {
   MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestModulePass)
 
struct TestInvalidParentPass
   void runOnOperation() final {
     FunctionOpInterface op = getOperation();
     OpBuilder b(getOperation().getBody());
     b.create<test::TestCallOp>(op.getLoc(), TypeRange(), "some_unknown_func",
                                ValueRange());
   }
 };
 
+/// A test pass that creates an invalid operation in a function body.
+struct TestInvalidIRPass
+    : public PassWrapper<TestInvalidIRPass,
+                         InterfacePass<FunctionOpInterface>> {
+  MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestInvalidIRPass)
+
+  TestInvalidIRPass() = default;
+  TestInvalidIRPass(const TestInvalidIRPass &other) {
+    signalFailure = other.signalFailure;
+  }
+
+  StringRef getArgument() const final { return "test-pass-create-invalid-ir"; }
+  StringRef getDescription() const final {
+    return "Test pass that add an invalid operation in a function body";
+  }
+  void getDependentDialects(DialectRegistry &registry) const final {
+    registry.insert<test::TestDialect>();
+  }
+  void runOnOperation() final {
+    if (signalFailure)
+      signalPassFailure();
+    if (!emitInvalidIR)
+      return;
+    OpBuilder b(getOperation().getBody());
+    OperationState state(UnknownLoc::get(&getContext()),
+                         "test.any_attr_of_i32_str");
+    b.create(state);
+  }
+  Option<bool> signalFailure{*this, "signal-pass-failure",
+                             llvm::cl::desc("Trigger a pass failure as well")};
+  Option<bool> emitInvalidIR{*this, "emit-invalid-ir", llvm::cl::init(true),
+                             llvm::cl::desc("Emit invalid IR")};
+};
+
 /// A test pass that contains a statistic.
 struct TestStatisticPass
     : public PassWrapper<TestStatisticPass, OperationPass<>> {
   MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestStatisticPass)
 
   TestStatisticPass() = default;
   TestStatisticPass(const TestStatisticPass &) {}
   StringRef getArgument() const final { return "test-stats-pass"; }
void registerPassManagerTestPass() {
 
   PassRegistration<TestFunctionPass>();
 
   PassRegistration<TestInterfacePass>();
 
   PassRegistration<TestCrashRecoveryPass>();
   PassRegistration<TestFailurePass>();
   PassRegistration<TestInvalidParentPass>();
+  PassRegistration<TestInvalidIRPass>();
 
   PassRegistration<TestStatisticPass>();
 
   PassPipelineRegistration<>("test-pm-nested-pipeline",
                              "Test a nested pipeline in the pass manager",
                              testNestedPipeline);
   PassPipelineRegistration<>("test-textual-pm-nested-pipeline",
                              "Test a nested pipeline in the pass manager",