Improve diagnostic when emitting operations with regions

[bondhugula]

Summary

This has a broad impact on diagnostics that attach an operation. Ops
with one or more regions will now be printed on a new line. It was
confusing and hard to read with a trailing first line for ops
with regions.

Before:

<unknown>:0: note: see current operation: affine.for %arg3 = 0 to 8192 {
  affine.for %arg4 = 0 to 8192 step 512 {
    affine.for %arg5 = 0 to 8192 step 128 {

    ...

After:

<unknown>:0: note: see current operation:
affine.for %arg3 = 0 to 8192 {
  affine.for %arg4 = 0 to 8192 step 512 {
    affine.for %arg5 = 0 to 8192 step 128 {
      ...

[bondhugula]

I'm not 100% sure about this -- whether we should be consistent in the format and do this for all ops or for none (current approach). But this chnage is a readability improvement for ops with regions.

[mehdi_amini]

That seems fine with me, but I'm curious what others have to say about it?

[lattner]

The goal of this makes sense to me. That said, would it be possible to key this off of "whether the printed form of an op has \n's in it"?

I think that is the actual predicate that matters here. It is possible to have ops with regions that have custom printing hooks (e.g. if the region has an op to reduce and prints the op inline when it is simple and singular), and it is possible to have ops with embedded \n's in their printed form. Keying off "contains a \n" seems more stable/correct to me.

[rriddle]

In D132645#3749286, @lattner wrote:

The goal of this makes sense to me. That said, would it be possible to key this off of "whether the printed form of an op has \n's in it"?

I think that is the actual predicate that matters here. It is possible to have ops with regions that have custom printing hooks (e.g. if the region has an op to reduce and prints the op inline when it is simple and singular), and it is possible to have ops with embedded \n's in their printed form. Keying off "contains a \n" seems more stable/correct to me.

+1 here. I would key off of the generated output, instead of assuming based on the number of regions.

[bondhugula]

In D132645#3750379, @rriddle wrote:

In D132645#3749286, @lattner wrote:

The goal of this makes sense to me. That said, would it be possible to key this off of "whether the printed form of an op has \n's in it"?

I think that is the actual predicate that matters here. It is possible to have ops with regions that have custom printing hooks (e.g. if the region has an op to reduce and prints the op inline when it is simple and singular), and it is possible to have ops with embedded \n's in their printed form. Keying off "contains a \n" seems more stable/correct to me.

+1 here. I would key off of the generated output, instead of assuming based on the number of regions.

Wouldn't checking the printed form for a new line (for a region holding op in particular) be undesirable (too much unnecessary work for too little)? Is it possible to do this cleanly?

I used regions as a proxy here -- that every op with a region will almost always have a new line and that every op without a region will almost always be printed on a single line. I haven't myself seen a single case deviating from this in the tree or in an open-source downstream project. In fact, printing on a single line for region-holding ops hurts readability (you are likely to mix the external attribute list with the region since braces are at play in both), and printing on multiple lines for a non-region op is similarly confusing.

The change is really addressing the overwhelmingly common case with near-zero overhead.

[rriddle]

In D132645#3750776, @bondhugula wrote:

In D132645#3750379, @rriddle wrote:

In D132645#3749286, @lattner wrote:

The goal of this makes sense to me. That said, would it be possible to key this off of "whether the printed form of an op has \n's in it"?

I think that is the actual predicate that matters here. It is possible to have ops with regions that have custom printing hooks (e.g. if the region has an op to reduce and prints the op inline when it is simple and singular), and it is possible to have ops with embedded \n's in their printed form. Keying off "contains a \n" seems more stable/correct to me.

+1 here. I would key off of the generated output, instead of assuming based on the number of regions.

Wouldn't checking the printed form (for a region holding op in particular) be undesirable (too much unnecessary work for too little)? Is it possible to do this cleanly?

I used regions as a proxy here -- that every op with a region will almost always have a new line and that every op without a region will almost always be printed on a single line. I haven't myself seen a single case deviating from this in the tree or in an open-source downstream project. In fact, printing on a single line for region-holding ops hurts readability (you are likely to mix the external attribute list with the region since braces are at play in both), and printing on multiple lines for a non-region op is similarly confusing.

The change is really addressing the overwhelmingly common case with near-zero overhead.

Not sure what you mean, just checking the number of regions has no indication. You'd have to also check that they aren't empty for that to be a reasonable heuristic (e.g. every "external" function has one region, and is printed on a single line).

[bondhugula]

In D132645#3750777, @rriddle wrote:

Not sure what you mean, just checking the number of regions has no indication. You'd have to also check that they aren't empty for that to be a reasonable heuristic (e.g. every "external" function has one region, and is printed on a single line).

Thanks for spotting this. I should at least check for empty regions.

[bondhugula]

Check for non-empty regions; add test case for the single line case as well.

[lattner]

As mentioned above, this isn't enough and is just wrong. Some ops may print empty regions as {} on different lines.

Checking the printing looks trivial, just change Diagnostic::appendOp (which is already printing to a string before emitting the op) to add a \n when os.str() contains a \n. Likely two line patch.

[mehdi_amini]

It is slightly more than two lines because we don't have a std::string Operation::toString(OpPrintingFlags &flags); method:

std::string str;
llvm::raw_string_ostream os(str);
op.print(os, adjustPrintingFlags(flags, severity));

What about adding such toString method though?

We actually just added a free function helper in TensorFlow for all MLIR object that can be streamed to llvm::raw_ostream: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/compiler/xla/mlir/utils/to_string.h#L28-L39
(it works with Type, Attr, Operation, ... but does not take OpPrintingFlags, maybe an overload would do?)

[lattner]

All that code already exists in Diagnostic::appendOp, we need incremental ~+2 more lines on what is in main.

[bondhugula]

Check for new line in the op's printed form.

[bondhugula]

In D132645#3752081, @mehdi_amini wrote:

It is slightly more than two lines because we don't have a std::string Operation::toString(OpPrintingFlags &flags); method:

std::string str;
llvm::raw_string_ostream os(str);
op.print(os, adjustPrintingFlags(flags, severity));

What about adding such toString method though?

We actually just added a free function helper in TensorFlow for all MLIR object that can be streamed to llvm::raw_ostream: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/compiler/xla/mlir/utils/to_string.h#L28-L39
(it works with Type, Attr, Operation, ... but does not take OpPrintingFlags, maybe an overload would do?)

Thank you for the comments. It looks useful to have a toString anyway; I've gone ahead and added that while on this and made appendOp use it.

In D132645#3752036, @lattner wrote:

Checking the printing looks trivial, just change Diagnostic::appendOp (which is already printing to a string before emitting the op) to add a \n when os.str() contains a \n. Likely two line patch.

By putting the logic in appendOp, one won't know whether to print a trailing white space at the end of "see current operation: ". But I've gone ahead with this.

[rriddle]

mlir/include/mlir/IR/Operation.h

672 ↗

(On Diff #456231)

I'm not really a fan of this method, can you remove it from this diff? There is already something similar in https://github.com/llvm/llvm-project/blob/1a1c59f9958609438c13af73444061d85caa749c/mlir/include/mlir/Support/DebugStringHelper.h#L28 which could just be changed to invoke "print()" and have an optional set of additional arguments to that.

mlir/lib/IR/Diagnostics.cpp

150

Why the const? Can you remove that?

[bondhugula]

toString -> str

[bondhugula]

In D132645#3752081, @mehdi_amini wrote:

It is slightly more than two lines because we don't have a std::string Operation::toString(OpPrintingFlags &flags); method:

std::string str;
llvm::raw_string_ostream os(str);
op.print(os, adjustPrintingFlags(flags, severity));

What about adding such toString method though?

Should this be named str() to be consistent with Diagnostic::str() and other similar things?

mlir/lib/IR/Diagnostics.cpp

150

It's to indicate that its contents will not be changed - trivial here, though.

[bondhugula]

Update check in DebugString.

mlir/include/mlir/IR/Operation.h

672 ↗

(On Diff #456231)

Updated this check.

[rriddle]

mlir/include/mlir/IR/Operation.h

672 ↗

(On Diff #456231)

I should be more clear, I'm not a fan of having a str method on operation.

[rriddle]

mlir/include/mlir/IR/Operation.h

672 ↗

(On Diff #456231)

For clarity, print methods are extremely common and I'd rather have something generic that wraps the boilerplate of calling print than to duplicate it for every potential class.

[bondhugula]

mlir/include/mlir/IR/Operation.h

672 ↗

(On Diff #456231)

I see, thanks. I went ahead and added that since @mehdi_amini felt we should have one. I can take this new method out of this revision. Having something generic would be useful (for Operation, Type, Attribute, etc.).

[mehdi_amini]

mlir/include/mlir/IR/Operation.h

672 ↗

(On Diff #456231)

Right, it's a matter of taste, but we have a dump method that makes it "convenient" to call print with stderr, so having a toString() seemed like on the same scale of convenience :)

[mehdi_amini]

(that said I thought of this having forgotten that we already have the free function debugString available in mlir/include/mlir/Support/DebugStringHelper.h)

[lattner]

fwiw, the reason that LLVM conventionally has the ->dump() methods is for use in debuggers. Modern debuggers still aren't able to resolve call thing->print(llvm::errs()) super reliably.

[lattner]

I agree that it would be useful to have a "get as string" convenience next to these things generally. Please name it "getAsString()" though to follow the general naming conventions which prefer "get". LLVM is wildly inconsistent across subprojects, but MLIR is relatively good about following this convention.

[mehdi_amini]

Thanks for the reminder about debuggers, I had forgotten this motivation )

[rriddle]

In D132645#3774821, @lattner wrote:

I agree that it would be useful to have a "get as string" convenience next to these things generally. Please name it "getAsString()" though to follow the general naming conventions which prefer "get". LLVM is wildly inconsistent across subprojects, but MLIR is relatively good about following this convention.

I'm not really in favor of adding a getAsString to every class with a print, especially given that in some contexts it's going to be confusing. I'm just imagining the confusion of Attr::getAsString, given we have a StringAttr and getAsString isn't going to give you the value of the StringAttr, it's going to print to a string. I'd rather name this more explicitly as printToString/printAsString/dumpAsString or something, but even then I'd be really sad if we have to carbon copy this same 4 lines of boilerplate all over the code base.

[lattner]

Yes, that's a fair point, I didn't think about StringAttr. This discussion is pretty far afield from the title on this PR - can we move it to its own issue?

[bondhugula]

Drop the str() accessor.

[bondhugula]

In D132645#3776061, @lattner wrote:

Yes, that's a fair point, I didn't think about StringAttr. This discussion is pretty far afield from the title on this PR - can we move it to its own issue?

Done.

Right, it's a matter of taste, but we have a dump method that makes it "convenient" to call print with stderr, so having a toString() seemed like on the same scale of convenience :)

That's precisely how I had thought about it too.

[bondhugula]

Add missed change.

[bondhugula]

NFC tweak.

[lattner]

Thank you for improving this Uday! (and also tolerating the long and winding side discussion) :-) :-)

[bondhugula]

Fix oversights; rebase. check-mlir passes.

mlir/include/mlir/IR/Diagnostics.h

public:
   Diagnostic &operator<<(char val);
   Diagnostic &operator<<(const Twine &val);
   Diagnostic &operator<<(Twine &&val);
 
   /// Stream in an OperationName.
   Diagnostic &operator<<(OperationName val);
 
   /// Stream in an Operation.
-  Diagnostic &operator<<(Operation &val);
-  Diagnostic &operator<<(Operation *val) { return *this << *val; }
+  Diagnostic &operator<<(Operation &op);
+  Diagnostic &operator<<(Operation *op) { return *this << *op; }
   /// Append an operation with the given printing flags.
-  Diagnostic &appendOp(Operation &val, const OpPrintingFlags &flags);
+  Diagnostic &appendOp(Operation &op, const OpPrintingFlags &flags);
 
   /// Stream in a Value.
   Diagnostic &operator<<(Value val);
 
   /// Stream in a range.
   template <typename T, typename ValueT = llvm::detail::ValueOfRange<T>>
   std::enable_if_t<!std::is_constructible<DiagnosticArgument, T>::value,
                    Diagnostic &>

mlir/lib/IR/Diagnostics.cpp

static OpPrintingFlags adjustPrintingFlags(OpPrintingFlags flags,
   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::operator<<(Operation &op) {
+  return appendOp(op, OpPrintingFlags());
 }
-Diagnostic &Diagnostic::appendOp(Operation &val, const OpPrintingFlags &flags) {
+
+Diagnostic &Diagnostic::appendOp(Operation &op, const OpPrintingFlags &flags) {
   std::string str;
   llvm::raw_string_ostream os(str);
-  val.print(os, adjustPrintingFlags(flags, severity));
+  op.print(os, adjustPrintingFlags(flags, severity));
+  // Print on a new line for better readability if the op will be printed on
+  // multiple lines.
+  if (str.find('\n') != std::string::npos)
+    *this << '\n';
   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, adjustPrintingFlags(OpPrintingFlags(), severity));
   return *this << os.str();
 }
 

mlir/test/IR/print-op-on-diagnostic.mlir

-// RUN: not mlir-opt %s -mlir-print-op-on-diagnostic 2>&1 | FileCheck %s
+// RUN: not mlir-opt -split-input-file %s -mlir-print-op-on-diagnostic 2>&1 | FileCheck %s
 
 // This file tests the functionality of 'mlir-print-op-on-diagnostic'.
 
 // CHECK: {{invalid to use 'test.invalid_attr'}}
-// CHECK: {{see current operation: "builtin.module"()}}
+// CHECK: see current operation:
+// CHECK-NEXT: "builtin.module"()
 module attributes {test.invalid_attr} {}
+
+// -----
+
+func.func @foo() {
+  "test.foo"(%cst) : (index) -> ()
+  // CHECK: {{operand #0 does not dominate this use}}
+  // CHECK: {{see current operation: "test.foo"(.*)}}
+  %cst = arith.constant 0 : index
+  return
+}