[mlir] Fix dumping invalid ops

[sgrechanik]

Summary

This patch fixes the crash when printing some ops (like affine.for and
scf.for) when they are dumped in invalid state, e.g. during pattern
application. Now the AsmState constructor verifies the operation
first and switches to generic operation printing when the verification
fails. Also operations are now printed in generic form when emitting
diagnostics and the severity level is Error.

[rriddle]

We shouldn't be baking in verification checking into the custom printers. If anything we should have a dump/printer API that falls back to generic in the case of verification failure. It isn't feasible/maintainable to re-encode all of the verifier constraints, so we should keep custom printers clean from things they should be able to assume.

[sgrechanik]

We can call opInfo->verifyInvariants(op) in OperationPrinter::printOperation and switch to generic printing on failure, something along the lines:

if (auto opInfo = op->getRegisteredInfo()) {
  // First verify the operation. If the operation fails to verify then the
  // custom print function may fail and we have to use the generic one.
  {
    // Ignore errors emitted by the verifier.
    ScopedDiagnosticHandler diagHandler(
        op->getContext(), [](Diagnostic &) { return success(); });
    if (failed(opInfo->verifyInvariants(op))) {
      printGenericOp(op, /*printOpName=*/true);
      return;
    }
  }
  opInfo->printAssembly(op, *this, defaultDialectStack.back());
  return;
}

However, doing this exposes some bugs in other ops. I'll try to investigate.

[mehdi_amini]

I talked about this with River in the past: I'd call the verifier on the entry point of the printer (the top-level operation to print), which will verify recursively the IR down. And switch to generically printing for the whole IR and not on an op-by-op basis.

[sgrechanik]

Not sure at which level to put the verification, Operation::dump seems to be a good candidate, it will fix the original crash and won't affect other kinds of printing.

[rriddle]

mlir/test/Conversion/AffineToStandard/lower-affine.mlir

1–2 ↗

(On Diff #403069)

LG, except for these. I'm not sure we should be adding these to the tests. Realistically tests shouldn't be crashing with -debug, but we shouldn't need to add debug individually to all of the tests to verify that.

[mehdi_amini]

mlir/lib/IR/AsmPrinter.cpp

2989

I think this should belong to the printer and not just "dump()"

The behavior does not seem desirable to be coupled to the llvm::errs() in itself. We don't want to crash when printing in general.
We had to hardcode this in the python bindings, but it fits better the framework itself.

[sgrechanik]

Fixing this not only for dump uncovered a bug in verification of spirv::ConstantOp, which should be fixed first: https://reviews.llvm.org/D118939

[jpienaar]

mlir/lib/IR/AsmPrinter.cpp

2984

There is (I believe) still a change that an error can be emitted here in multi threaded environment, it would require a new handler to be set after this but before verify below. Not a high probability event though.

[sgrechanik]

• Moved the check to Operation::print and Block::print
• Removed error suppression: errors may be helpful.

mlir/lib/IR/AsmPrinter.cpp

2984

Hmm, yes, you are right, and I don't immediately see how to solve it.

But maybe actually we shouldn't try to suppress error messages here, because without error messages it's hard to tell why the printer switched to generic op printing.

2989

Moved it to Operation::print and Block::print

mlir/test/Conversion/AffineToStandard/lower-affine.mlir

1–2 ↗

(On Diff #403069)

Do you have a better idea how to tests this?

[mehdi_amini]

mlir/lib/IR/AsmPrinter.cpp

2973

Please run git clang-format on your commit (arc should do it for you if clang-format is in the path I think)

2984

I wouldn't expect diagnostic about verification failure in a place where I call "print" though.

River: is there a safe way to silence these in the context of the printer?

3000

Is this check needed/useful?

Also you checked in the Operation::print is the generic printer flag isn't already set before verifying, should you do the same here?

[rriddle]

mlir/lib/IR/AsmPrinter.cpp

2984

Generally if you only want diagnostics for this thread, you should check the result of llvm::get_threadid() within the handler against the one for the main thread. Something like:

auto parentThreadId = llvm::get_threadid();
auto handlerFn = [&](Diagnostic &diag) {
  return success(parentThreadId == llvm::get_threadid());
};

[sgrechanik]

• Error suppression is back
• fixed formatting errors

mlir/lib/IR/AsmPrinter.cpp

2984

Thanks, I'll use that.

3000

I inserted this check to avoid messing with diagnostic handling if the block is empty.

Here the flags are always the default ones. AsmState contains its own set of flags, but as far as I understand, they are not used for the actual printing. However, I wonder whether AsmState and OperationPrinter flags mismatch would cause any trouble. Maybe we shouldn't do the verification in functions taking AsmState, and verify in more higher level Operation::print(raw_ostream &os, const OpPrintingFlags &printerFlags) and Block::print(raw_ostream &os) instead?

[rriddle]

mlir/lib/IR/AsmPrinter.cpp

3000

It's generally too late to do verification if the AsmState already exists, the construction of that may call back into operations (and thus crash if invalid). I'd just verify in the non-AsmState functions.

Tangentially though, we should just expose the OpPrintingFlags held by the AsmState and use those (same applies for the other function).

[sgrechanik]

mlir/lib/IR/AsmPrinter.cpp

3000

Tangentially though, we should just expose the OpPrintingFlags held by the AsmState and use those (same applies for the other function).

Then it probably makes sense to do that and perform verification in the AsmState constructor. It doesn't seem to be a lot of code change.
However, I ran into an issue when a verifier tries to emit an error and print the op, and printing the op requires calling the verifier first, resulting in infinite recursion. I need to think how to solve it, something like temporary changing printOpOnDiagnostic, but there doesn't seem to be a thread-safe way to do it.

[sgrechanik]

Ok, let's do it gradually and expose printing flags in AsmState first: https://reviews.llvm.org/D119870

[sgrechanik]

Rebased and moved the verification to the AsmState constructor.

[stellaraccident]

At the python level, we have a flag to assume verified and not reverify. When using systematically, there are often cases where you know verification is successful and can skip doing it again.

We can add that in after this patch if needed, but I feel we should do so.

[mehdi_amini]

mlir/lib/IR/AsmPrinter.cpp

1274

Shouldn't the verifier failure be printing in generic mode already which should skip the verifier?
I'm not sure why we need this?

[sgrechanik]

mlir/lib/IR/AsmPrinter.cpp

1274

Not always. When the error is emitted like op1.emitError() << op2; then op1 will be printed in generic mode (or not at all), and op2 will be printed with default flags. This actually happens when verifying whether a block has a terminator in mlir/lib/IR/Verifier.cpp:154

Operation &terminator = block.back();
if (!terminator.mightHaveTrait<OpTrait::IsTerminator>())
  return block.back().emitError("block with no terminator, has ")
         << terminator;

And also printing an op may require verification of its parent op because AsmState may be constructed for the parent op.

[mehdi_amini]

mlir/lib/IR/AsmPrinter.cpp

1274

When the error is emitted like op1.emitError() << op2; then op1 will be printed in generic mode (or not at all), and op2 will be printed with default flags. This actually happens when verifying whether a block has a terminator in mlir/lib/IR/Verifier.cpp:154

But isn't this something we should fix in the verifier? Seems like unintended to me.

And also printing an op may require verification of its parent op because AsmState may be constructed for the parent op.

Similarly: if we print an op with the generic flag, then constructing the AsmState for the parent op could just inherit the flags again?

In general I'm wary of mutable global state when we can do differently: it's not clear to me still that this isn't just a workaround instead of something that is fundamentally needed if the system is consistent.

[mehdi_amini]

mlir/lib/IR/AsmPrinter.cpp

1274

I think we're always gonna hit custom verifier that may print op without propagating flags, I can't see a better option than what you have here right now, so LGTM for now.

mlir/test/Conversion/AffineToStandard/lower-affine.mlir

1–2 ↗

(On Diff #403069)

I agree with River: we should keep the testing focused.

what about adding a test with invalid IR and check that the printed op is in generic form?

[rriddle]

mlir/lib/IR/AsmPrinter.cpp

1274

We should avoid any kind of global or thread_local state. We should just switch how we handle printing operations in diagnostics, e.g. to just use generic form by default (i.e. change this https://github.com/llvm/llvm-project/blob/8e7995884a6525b44fe2f71318883ba2fec2d972/mlir/lib/IR/Diagnostics.cpp#L126).

[sgrechanik]

mlir/lib/IR/AsmPrinter.cpp

1274

We can switch to generic form by default but that will somewhat worsen user experience by making error messages less pretty. I'm ok with it though.

Alternatively we can implement per-thread error suppression in DiagnosticEngine, but that would be a much bigger change.

[sgrechanik]

• Switched to generic printing in diagnostics, removed the thread_local trick
• Added a flag to skip verification
• Implemented more explicit testing of invalid op printing

[mehdi_amini]

Please wait for River to have another look!

Thanks :)

[rriddle]

Nice.

mlir/test/Analysis/test-match-reduction.mlir

10 ↗

(On Diff #411513)

If we wanted, we could allow remarks to use the proper custom format. We would need to add some documentation somewhere about what types of diagnostics are expected from the verifier though.

[sgrechanik]

mlir/test/Analysis/test-match-reduction.mlir

10 ↗

(On Diff #411513)

Makes sense, may be worth doing it in this review to avoid changing tests back and forth.

[sgrechanik]

• Switch to generic form only when severity level is Error

[sgrechanik]

• Standard -> Func

[sgrechanik]

@rriddle Please take another look, if you have any concerns I can revert to the last accepted state and extract the change into a separate patch.

[rriddle]

Yeah, looks good. (Didn't mean to block, when I approved that meant I was okay with submitting/fixing the remaining comments before submitting).

[sgrechanik]

Thanks, I'll merge it on Monday.

mlir/docs/Diagnostics.md

 // Diagnostics can be composed via the streaming operators.
 op->emitError() << "Compose an interesting error: " << fooAttr << ", " << fooType
                 << ", (" << fooInts << ')';
 
 // This could generate something like (FuncAttr:@foo, IntegerType:i32, {0,1,2}):
 "Compose an interesting error: @foo, i32, (0, 1, 2)"
 ```
 
+Operations attached to a diagnostic will be printed in generic form if the
+severity level is `Error`, otherwise custom operation printers will be used.
+```c++
+// `anotherOp` will be printed in generic form,
+// e.g. %3 = "arith.addf"(%arg4, %2) : (f32, f32) -> f32
+op->emitError() << anotherOp;
+
+// `anotherOp` will be printed using the custom printer,
+// e.g. %3 = arith.addf %arg4, %2 : f32
+op->emitRemark() << anotherOp;
+```
+
 ### Attaching notes
 
 Unlike many other compiler frameworks, notes in MLIR cannot be emitted directly.
 They must be explicitly attached to another diagnostic non-note diagnostic. When
 emitting a diagnostic, notes can be directly attached via `attachNote`. When
 attaching a note, if the user does not provide an explicit source location the
 note will inherit the location of the parent diagnostic.
 

mlir/docs/OpDefinitions.md

1. Traits/Interfaces that have marked their verifier as `verifyRegionTrait` or
    operations in its regions.
 1. Custom verifier which is defined in the op and has marked
    `hasRegionVerifier=1`
 
 Note that the second phase will be run after the operations in the region are
 verified. Verifiers further down the order can rely on certain invariants being
 verified by a previous verifier and do not need to re-verify them.
 
+#### Emitting diagnostics in custom verifiers
+
+Custom verifiers should avoid printing operations using custom operation
+printers, because they require the printed operation (and sometimes its parent
+operation) to be verified first. In particular, when emitting diagnostics,
+custom verifiers should use the `Error` severity level, which prints operations
+in generic form by default, and avoid using lower severity levels (`Note`,
+`Remark`, `Warning`).
+
 ### Declarative Assembly Format
 
 The custom assembly form of the operation may be specified in a declarative
 string that matches the operations operands, attributes, etc. With the ability
 to express additional information that needs to be parsed to build the
 operation:
 
 ```tablegen

mlir/include/mlir/IR/OperationSupport.h

public:
   /// Enable printing of debug information. If 'prettyForm' is set to true,
   /// debug information is printed in a more readable 'pretty' form. Note: The
   /// IR generated with 'prettyForm' is not parsable.
   OpPrintingFlags &enableDebugInfo(bool prettyForm = false);
 
   /// Always print operations in the generic form.
   OpPrintingFlags &printGenericOpForm();
 
+  /// Do not verify the operation when using custom operation printers.
+  OpPrintingFlags &assumeVerified();
+
   /// Use local scope when printing the operation. This allows for using the
   /// printer in a more localized and thread-safe setting, but may not
   /// necessarily be identical to what the IR will look like when dumping
   /// the full module.
   OpPrintingFlags &useLocalScope();
 
   /// Return if the given ElementsAttr should be elided.
   bool shouldElideElementsAttr(ElementsAttr attr) const;
 
   /// Return the size limit for printing large ElementsAttr.
   Optional<int64_t> getLargeElementsAttrLimit() const;
 
   /// Return if debug information should be printed.
   bool shouldPrintDebugInfo() const;
 
   /// Return if debug information should be printed in the pretty form.
   bool shouldPrintDebugInfoPrettyForm() const;
 
   /// Return if operations should be printed in the generic form.
   bool shouldPrintGenericOpForm() const;
 
+  /// Return if operation verification should be skipped.
+  bool shouldAssumeVerified() const;
+
   /// Return if the printer should use local scope when dumping the IR.
   bool shouldUseLocalScope() const;
 
 private:
   /// Elide large elements attributes if the number of elements is larger than
   /// the upper limit.
   Optional<int64_t> elementsAttrElementLimit;
 
   /// Print debug information.
   bool printDebugInfoFlag : 1;
   bool printDebugInfoPrettyFormFlag : 1;
 
   /// Print operations in the generic form.
   bool printGenericOpFormFlag : 1;
 
+  /// Skip operation verification.
+  bool assumeVerifiedFlag : 1;
+
   /// Print operations with numberings local to the current operation.
   bool printLocalScope : 1;
 };
 
 //===----------------------------------------------------------------------===//
 // Operation Value-Iterators
 //===----------------------------------------------------------------------===//
 

mlir/include/mlir/IR/Value.h

 #include "llvm/Support/PointerLikeTypeTraits.h"
 
 namespace mlir {
 class AsmState;
 class Block;
 class BlockArgument;
 class Operation;
 class OpOperand;
+class OpPrintingFlags;
 class OpResult;
 class Region;
 class Value;
 
 //===----------------------------------------------------------------------===//
 // Value
 //===----------------------------------------------------------------------===//
 
public:
   user_iterator user_begin() const { return use_begin(); }
   user_iterator user_end() const { return use_end(); }
   user_range getUsers() const { return {user_begin(), user_end()}; }
 
   //===--------------------------------------------------------------------===//
   // Utilities
 
   void print(raw_ostream &os);
+  void print(raw_ostream &os, const OpPrintingFlags &flags);
   void print(raw_ostream &os, AsmState &state);
   void dump();
 
   /// Print this value as if it were an operand.
   void printAsOperand(raw_ostream &os, AsmState &state);
 
   /// Methods for supporting PointerLikeTypeTraits.
   void *getAsOpaquePointer() const { return impl; }

mlir/lib/IR/AsmPrinter.cpp

 #include "mlir/IR/BuiltinTypes.h"
 #include "mlir/IR/Dialect.h"
 #include "mlir/IR/DialectImplementation.h"
 #include "mlir/IR/IntegerSet.h"
 #include "mlir/IR/MLIRContext.h"
 #include "mlir/IR/OpImplementation.h"
 #include "mlir/IR/Operation.h"
 #include "mlir/IR/SubElementInterfaces.h"
+#include "mlir/IR/Verifier.h"
 #include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/ScopedHashTable.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/TypeSwitch.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/SaveAndRestore.h"
+#include "llvm/Support/Threading.h"
 
 #include <tuple>
 
 using namespace mlir;
 using namespace mlir::detail;
 
 void OperationName::print(raw_ostream &os) const { os << getStringRef(); }
 
llvm::cl::opt<bool> printPrettyDebugInfoOpt{
       llvm::cl::desc("Print pretty debug info in MLIR output")};
 
   // Use the generic op output form in the operation printer even if the custom
   // form is defined.
   llvm::cl::opt<bool> printGenericOpFormOpt{
       "mlir-print-op-generic", llvm::cl::init(false),
       llvm::cl::desc("Print the generic op form"), llvm::cl::Hidden};
 
+  llvm::cl::opt<bool> assumeVerifiedOpt{
+      "mlir-print-assume-verified", llvm::cl::init(false),
+      llvm::cl::desc("Skip op verification when using custom printers"),
+      llvm::cl::Hidden};
+
   llvm::cl::opt<bool> printLocalScopeOpt{
       "mlir-print-local-scope", llvm::cl::init(false),
       llvm::cl::desc("Print with local scope and inline information (eliding "
                      "aliases for attributes, types, and locations")};
 };
 } // namespace
 
 static llvm::ManagedStatic<AsmPrinterOptions> clOptions;
 
 /// Register a set of useful command-line options that can be used to configure
 /// various flags within the AsmPrinter.
 void mlir::registerAsmPrinterCLOptions() {
   // Make sure that the options struct has been initialized.
   *clOptions;
 }
 
 /// Initialize the printing flags with default supplied by the cl::opts above.
 OpPrintingFlags::OpPrintingFlags()
     : printDebugInfoFlag(false), printDebugInfoPrettyFormFlag(false),
-      printGenericOpFormFlag(false), printLocalScope(false) {
+      printGenericOpFormFlag(false), assumeVerifiedFlag(false),
+      printLocalScope(false) {
   // Initialize based upon command line options, if they are available.
   if (!clOptions.isConstructed())
     return;
   if (clOptions->elideElementsAttrIfLarger.getNumOccurrences())
     elementsAttrElementLimit = clOptions->elideElementsAttrIfLarger;
   printDebugInfoFlag = clOptions->printDebugInfoOpt;
   printDebugInfoPrettyFormFlag = clOptions->printPrettyDebugInfoOpt;
   printGenericOpFormFlag = clOptions->printGenericOpFormOpt;
+  assumeVerifiedFlag = clOptions->assumeVerifiedOpt;
   printLocalScope = clOptions->printLocalScopeOpt;
 }
 
 /// Enable the elision of large elements attributes, by printing a '...'
 /// instead of the element data, when the number of elements is greater than
 /// `largeElementLimit`. Note: The IR generated with this option is not
 /// parsable.
 OpPrintingFlags &

 }
 
 /// Always print operations in the generic form.
 OpPrintingFlags &OpPrintingFlags::printGenericOpForm() {
   printGenericOpFormFlag = true;
   return *this;
 }
 
+/// Do not verify the operation when using custom operation printers.
+OpPrintingFlags &OpPrintingFlags::assumeVerified() {
+  assumeVerifiedFlag = true;
+  return *this;
+}
+
 /// Use local scope when printing the operation. This allows for using the
 /// printer in a more localized and thread-safe setting, but may not necessarily
 /// be identical of what the IR will look like when dumping the full module.
 OpPrintingFlags &OpPrintingFlags::useLocalScope() {
   printLocalScope = true;
   return *this;
 }
 
bool OpPrintingFlags::shouldPrintDebugInfoPrettyForm() const {
   return printDebugInfoPrettyFormFlag;
 }
 
 /// Return if operations should be printed in the generic form.
 bool OpPrintingFlags::shouldPrintGenericOpForm() const {
   return printGenericOpFormFlag;
 }
 
+/// Return if operation verification should be skipped.
+bool OpPrintingFlags::shouldAssumeVerified() const {
+  return assumeVerifiedFlag;
+}
+
 /// Return if the printer should use local scope when dumping the IR.
 bool OpPrintingFlags::shouldUseLocalScope() const { return printLocalScope; }
 
 /// Returns true if an ElementsAttr with the given number of elements should be
 /// printed with hex.
 static bool shouldPrintElementsAttrWithHex(int64_t numElements) {
   // Check to see if a command line option was provided for the limit.
   if (clOptions.isConstructed()) {
private:
   OpPrintingFlags printerFlags;
 
   /// An optional location map to be populated.
   AsmState::LocationMap *locationMap;
 };
 } // namespace detail
 } // namespace mlir
 
+/// Verifies the operation and switches to generic op printing if verification
+/// fails. We need to do this because custom print functions may fail for
+/// invalid ops.
+static OpPrintingFlags verifyOpAndAdjustFlags(Operation *op,
+                                              OpPrintingFlags printerFlags) {
+  if (printerFlags.shouldPrintGenericOpForm() ||
+      printerFlags.shouldAssumeVerified())
+    return printerFlags;
+
+  // Ignore errors emitted by the verifier. We check the thread id to avoid
+  // consuming other threads' errors.
+  auto parentThreadId = llvm::get_threadid();
+  ScopedDiagnosticHandler diagHandler(op->getContext(), [&](Diagnostic &) {
+    return success(parentThreadId == llvm::get_threadid());
+  });
+  if (failed(verify(op)))
+    printerFlags.printGenericOpForm();
+
+  return printerFlags;
+}
+
 AsmState::AsmState(Operation *op, const OpPrintingFlags &printerFlags,
                    LocationMap *locationMap)
-    : impl(std::make_unique<AsmStateImpl>(op, printerFlags, locationMap)) {}
+    : impl(std::make_unique<AsmStateImpl>(
+          op, verifyOpAndAdjustFlags(op, printerFlags), locationMap)) {}
 AsmState::~AsmState() = default;
 
 const OpPrintingFlags &AsmState::getPrinterFlags() const {
   return impl->getPrinterFlags();
 }
 
 //===----------------------------------------------------------------------===//
 // AsmPrinter::Impl
void AffineMap::print(raw_ostream &os) const {
   }
   AsmPrinter::Impl(os).printAffineMap(*this);
 }
 
 void IntegerSet::print(raw_ostream &os) const {
   AsmPrinter::Impl(os).printIntegerSet(*this);
 }
 
-void Value::print(raw_ostream &os) {
+void Value::print(raw_ostream &os) { print(os, OpPrintingFlags()); }
+void Value::print(raw_ostream &os, const OpPrintingFlags &flags) {
   if (!impl) {
     os << "<<NULL VALUE>>";
     return;
   }
 
   if (auto *op = getDefiningOp())
-    return op->print(os);
+    return op->print(os, flags);
   // TODO: Improve BlockArgument print'ing.
   BlockArgument arg = this->cast<BlockArgument>();
   os << "<block argument> of type '" << arg.getType()
      << "' at index: " << arg.getArgNumber();
 }
 void Value::print(raw_ostream &os, AsmState &state) {
   if (!impl) {
     os << "<<NULL VALUE>>";
void Operation::print(raw_ostream &os, const OpPrintingFlags &printerFlags) {
 
   AsmState state(op, printerFlags);
   print(os, state);
 }
 void Operation::print(raw_ostream &os, AsmState &state) {
   OperationPrinter printer(os, state.getImpl());
   if (!getParent() && !state.getPrinterFlags().shouldUseLocalScope())
     printer.printTopLevelOperation(this);
   else
     printer.print(this);
 }
 
 void Operation::dump() {
   print(llvm::errs(), OpPrintingFlags().useLocalScope());
   llvm::errs() << "\n";
 }
 
 void Block::print(raw_ostream &os) {
   Operation *parentOp = getParentOp();
   if (!parentOp) {
     os << "<<UNLINKED BLOCK>>\n";
     return;
   }
   // Get the top-level op.
   while (auto *nextOp = parentOp->getParentOp())
     parentOp = nextOp;
 
   AsmState state(parentOp);
   print(os, state);
 }
 void Block::print(raw_ostream &os, AsmState &state) {
   OperationPrinter(os, state.getImpl()).print(this);
 }
 
 void Block::dump() { print(llvm::errs()); }
 
 /// Print out the name of the block without printing its body.
 void Block::printAsOperand(raw_ostream &os, bool printType) {
   Operation *parentOp = getParentOp();
   if (!parentOp) {
     os << "<<UNLINKED BLOCK>>\n";
     return;
   }
   AsmState state(parentOp);
   printAsOperand(os, state);
 }
 void Block::printAsOperand(raw_ostream &os, AsmState &state) {
   OperationPrinter printer(os, state.getImpl());
   printer.printBlockName(this);
 }

mlir/lib/IR/Diagnostics.cpp

 /// Stream in an OperationName.
 Diagnostic &Diagnostic::operator<<(OperationName val) {
   // An OperationName is stored in the context, so we don't need to worry about
   // the lifetime of its data.
   arguments.push_back(DiagnosticArgument(val.getStringRef()));
   return *this;
 }
 
+/// Adjusts operation printing flags used in diagnostics for the given severity
+/// level.
+static OpPrintingFlags adjustPrintingFlags(OpPrintingFlags flags,
+                                           DiagnosticSeverity severity) {
+  flags.useLocalScope();
+  flags.elideLargeElementsAttrs();
+  if (severity == DiagnosticSeverity::Error)
+    flags.printGenericOpForm();
+  return flags;
+}
+
 /// Stream in an Operation.
 Diagnostic &Diagnostic::operator<<(Operation &val) {
   return appendOp(val, OpPrintingFlags());
 }
 Diagnostic &Diagnostic::appendOp(Operation &val, const OpPrintingFlags &flags) {
   std::string str;
   llvm::raw_string_ostream os(str);
-  val.print(os,
-            OpPrintingFlags(flags).useLocalScope().elideLargeElementsAttrs());
+  val.print(os, adjustPrintingFlags(flags, severity));
   return *this << os.str();
 }
 
 /// Stream in a Value.
 Diagnostic &Diagnostic::operator<<(Value val) {
   std::string str;
   llvm::raw_string_ostream os(str);
-  val.print(os);
+  val.print(os, adjustPrintingFlags(OpPrintingFlags(), severity));
   return *this << os.str();
 }
 
 /// Outputs this diagnostic to a stream.
 void Diagnostic::print(raw_ostream &os) const {
   for (auto &arg : getArguments())
     arg.print(os);
 }

mlir/lib/IR/Operation.cpp

while (!pendingRegions.empty()) {
           // we don't assert here as this function is called from the verifier
           // and so could be called on invalid IR.
           if (!operand)
             return op.emitOpError("operation's operand is null");
 
           // Check that any value that is used by an operation is defined in the
           // same region as either an operation result.
           auto *operandRegion = operand.getParentRegion();
+          if (!operandRegion)
+            return op.emitError("operation's operand is unlinked");
           if (!region.isAncestor(operandRegion)) {
             return op.emitOpError("using value defined outside the region")
                        .attachNote(isolatedOp->getLoc())
                    << "required by region isolation constraints";
           }
         }
 
         // Schedule any regions in the operation for further checking.  Don't

mlir/test/IR/print-ir-invalid.mlir

+// # RUN: mlir-opt -test-print-invalid %s | FileCheck %s
+// # RUN: mlir-opt -test-print-invalid %s --mlir-print-assume-verified  | FileCheck %s --check-prefix=ASSUME-VERIFIED
+
+// The pass creates some ops and prints them to stdout, the input is just an
+// empty module.
+module {}
+
+// The operation is invalid because the body does not have a terminator, print
+// the generic form.
+// CHECK:      Invalid operation:
+// CHECK-NEXT: "builtin.func"() ({
+// CHECK-NEXT: ^bb0:
+// CHECK-NEXT: })
+// CHECK-SAME: sym_name = "test"
+
+// The operation is valid because the body has a terminator, print the custom
+// form.
+// CHECK:      Valid operation:
+// CHECK-NEXT: func @test() {
+// CHECK-NEXT:   return
+// CHECK-NEXT: }
+
+// With --mlir-print-assume-verified the custom form is printed in both cases.
+// This works in this particular case, but may crash in general.
+
+// ASSUME-VERIFIED:      Invalid operation:
+// ASSUME-VERIFIED-NEXT: func @test() {
+// ASSUME-VERIFIED-NEXT: }
+
+// ASSUME-VERIFIED:      Valid operation:
+// ASSUME-VERIFIED-NEXT: func @test() {
+// ASSUME-VERIFIED-NEXT:   return
+// ASSUME-VERIFIED-NEXT: }

mlir/test/lib/IR/CMakeLists.txt

 # Exclude tests from libMLIR.so
 add_mlir_library(MLIRTestIR
   TestBuiltinAttributeInterfaces.cpp
   TestDiagnostics.cpp
   TestDominance.cpp
   TestFunc.cpp
   TestInterfaces.cpp
   TestMatchers.cpp
   TestOpaqueLoc.cpp
   TestOperationEquals.cpp
   TestPrintDefUse.cpp
+  TestPrintInvalid.cpp
   TestPrintNesting.cpp
   TestSideEffects.cpp
   TestSlicing.cpp
   TestSymbolUses.cpp
   TestTypes.cpp
   TestVisitors.cpp
   TestVisitorsGeneric.cpp
 

mlir/test/lib/IR/TestPrintInvalid.cpp

+//===- TestPrintInvalid.cpp - Test printing invalid ops -------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+//
+// This pass creates and prints to the standard output an invalid operation and
+// a valid operation.
+//
+//===----------------------------------------------------------------------===//
+
+#include "mlir/Dialect/Func/IR/FuncOps.h"
+#include "mlir/Pass/Pass.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace mlir;
+
+namespace {
+struct TestPrintInvalidPass
+    : public PassWrapper<TestPrintInvalidPass, OperationPass<ModuleOp>> {
+  StringRef getArgument() const final { return "test-print-invalid"; }
+  StringRef getDescription() const final {
+    return "Test printing invalid ops.";
+  }
+  void getDependentDialects(DialectRegistry &registry) const {
+    registry.insert<func::FuncDialect>();
+  }
+
+  void runOnOperation() override {
+    Location loc = getOperation().getLoc();
+    OpBuilder builder(getOperation().body());
+    auto funcOp = builder.create<FuncOp>(
+        loc, "test", FunctionType::get(getOperation().getContext(), {}, {}));
+    funcOp.addEntryBlock();
+    // The created function is invalid because there is no return op.
+    llvm::outs() << "Invalid operation:\n" << funcOp << "\n";
+    builder.setInsertionPointToEnd(&funcOp.getBody().front());
+    builder.create<func::ReturnOp>(loc);
+    // Now this function is valid.
+    llvm::outs() << "Valid operation:\n" << funcOp << "\n";
+    funcOp.erase();
+  }
+};
+} // namespace
+
+namespace mlir {
+void registerTestPrintInvalidPass() {
+  PassRegistration<TestPrintInvalidPass>{};
+}
+} // namespace mlir

mlir/tools/mlir-opt/mlir-opt.cpp

 void registerTestAffineLoopUnswitchingPass();
 void registerTestAllReduceLoweringPass();
 void registerTestFunc();
 void registerTestGpuMemoryPromotionPass();
 void registerTestLoopPermutationPass();
 void registerTestMatchers();
 void registerTestOperationEqualPass();
 void registerTestPrintDefUsePass();
+void registerTestPrintInvalidPass();
 void registerTestPrintNestingPass();
 void registerTestReducer();
 void registerTestSpirvEntryPointABIPass();
 void registerTestSpirvGLSLCanonicalizationPass();
 void registerTestSpirvModuleCombinerPass();
 void registerTestTraitsPass();
 void registerTosaTestQuantUtilAPIPass();
 void registerVectorizerTestPass();
void registerTestPasses() {
   registerTestAffineLoopUnswitchingPass();
   registerTestAllReduceLoweringPass();
   registerTestFunc();
   registerTestGpuMemoryPromotionPass();
   registerTestLoopPermutationPass();
   registerTestMatchers();
   registerTestOperationEqualPass();
   registerTestPrintDefUsePass();
+  registerTestPrintInvalidPass();
   registerTestPrintNestingPass();
   registerTestReducer();
   registerTestSpirvEntryPointABIPass();
   registerTestSpirvGLSLCanonicalizationPass();
   registerTestSpirvModuleCombinerPass();
   registerTestTraitsPass();
   registerVectorizerTestPass();
   registerTosaTestQuantUtilAPIPass();