[mlir] Add `Operation::dumpPrettyPrinted` #120117

weiweichen · 2024-12-16T17:30:47Z

Add Operation::dumpPrettyPrinted to get more readable print during debugging when the IR may not be able to pass verify yet.

Pretty print Operation that may not be verified for ease of readabilty while debugging.

…eic/mlir/dump-pretty-printed

llvmbot · 2024-12-16T17:31:22Z

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: weiwei chen (weiweichen)

Changes

Add Operation::dumpPrettyPrinted to get more readable print during debugging when the IR may not be able to pass verify yet.

Full diff: https://github.com/llvm/llvm-project/pull/120117.diff

2 Files Affected:

(modified) mlir/include/mlir/IR/Operation.h (+3)
(modified) mlir/lib/IR/AsmPrinter.cpp (+5)

diff --git a/mlir/include/mlir/IR/Operation.h b/mlir/include/mlir/IR/Operation.h
index f0dd7c51780566..4f6bea3dcf113c 100644
--- a/mlir/include/mlir/IR/Operation.h
+++ b/mlir/include/mlir/IR/Operation.h
@@ -322,6 +322,9 @@ class alignas(8) Operation final
   void print(raw_ostream &os, AsmState &state);
   void dump();
 
+  /// Pretty print for ease of debugging readabilty with unverified IR.
+  void dumpPrettyPrinted();
+
   //===--------------------------------------------------------------------===//
   // Operands
   //===--------------------------------------------------------------------===//
diff --git a/mlir/lib/IR/AsmPrinter.cpp b/mlir/lib/IR/AsmPrinter.cpp
index 61b90bc9b0a7bb..7beb2ab1fb0444 100644
--- a/mlir/lib/IR/AsmPrinter.cpp
+++ b/mlir/lib/IR/AsmPrinter.cpp
@@ -3985,6 +3985,11 @@ void Operation::dump() {
   llvm::errs() << "\n";
 }
 
+void Operation::dumpPrettyPrinted() {
+  print(llvm::errs(), OpPrintingFlags().useLocalScope().assumeVerified());
+  llvm::errs() << "\n";
+}
+
 void Block::print(raw_ostream &os) {
   Operation *parentOp = getParentOp();
   if (!parentOp) {

River707

I'm not really sure I'm following this API, it seems very narrow and specific. Have you considered instead the possibility of adding an argument to dump for providing extra flags?

weiweichen · 2024-12-16T20:22:51Z

I'm not really sure I'm following this API, it seems very narrow and specific. Have you considered instead the possibility of adding an argument to dump for providing extra flags?

Fair enough! Putting in this patch mostly because @lattner mentioned about 👇 to me the other day when I was asking about why sometime I see a module being printed in generic mode.

I'd love to see an op->dumpPrettyPrinted() method on Operation, 
which would be suitable for being called by the debugger. 
If you're interested, I'd very much +1 a patch to upstream MLIR.

It'd set the assumeVerified pretty printing flag, so it would force the 
"seriously, this is eyebleed, just do the thing I as a human can understand" mode

If debugger is ok to run op->dump(OpPrintingFlags().useLocalScope().assumeVerified()), "adding an argument to dump for providing extra flags" is probably a better approach?

joker-eph · 2024-12-16T20:36:34Z

If debugger is ok to run op->dump(OpPrintingFlags().useLocalScope().assumeVerified()),

I think the idea is that in the debugger you can type: op->dump(true). The prototype for dump() would be changed to void dump(bool assumeVerified = false);

…eic/mlir/dump-pretty-printed

weiweichen · 2024-12-16T20:54:00Z

If debugger is ok to run op->dump(OpPrintingFlags().useLocalScope().assumeVerified()),

I think the idea is that in the debugger you can type: op->dump(true). The prototype for dump() would be changed to void dump(bool assumeVerified = false);

Sounds good! Updated the API accordingly.

joker-eph · 2024-12-16T20:56:01Z

LGTM, assuming you checked in the debugger that this work as expected!

River707 · 2024-12-16T20:58:45Z

mlir/include/mlir/IR/Operation.h

@@ -320,7 +320,7 @@ class alignas(8) Operation final

  void print(raw_ostream &os, const OpPrintingFlags &flags = std::nullopt);
  void print(raw_ostream &os, AsmState &state);
-  void dump();
+  void dump(bool assumeVerified = false);


If we want to ensure dump is available, we should mark it with LLVM_DUMP_METHOD. Helps prevent the symbol from being killed off.

+1, please use LLVM_DUMP_METHOD

lattner · 2024-12-16T21:54:55Z

For me, the #1 usecase is when working on mlir that is under construction when in LLDB. E.g. working on building a funciton, it has no terminator, it's like "please just dump it, not in the horrible way". I'm not sure how well LLDB works with default arguments these days, but this would definitely reduce my suffering on a near daily basis if we had it :-)

lattner

Awesome, thank you! +1 with the LLVM_DUMP_METHOD attribute.

weiweichen · 2024-12-16T22:54:37Z

For me, the #1 usecase is when working on mlir that is under construction when in LLDB. E.g. working on building a funciton, it has no terminator, it's like "please just dump it, not in the horrible way". I'm not sure how well LLDB works with default arguments these days, but this would definitely reduce my suffering on a near daily basis if we had it :-)

lldb doesn't seem to be able to pick up the default arguments 😢. I asked our in-house lldb expert @walter-erquinigo who said that "expr eval is based on the information on DWARF, and not on source code, so it doesn’t know your .h files". I don't think I want to change the behavior of Operation::dump(). Maybe I shall get back to the Operation::dumpPrettyPrinted() API?

joker-eph · 2024-12-16T22:59:06Z

Just a nit on the naming: I would call this dumpPretty() (Printed seems redundant, and since you aim at typing this manually in a debugger, less characters won't hurt ;) )

River707 · 2024-12-16T23:03:24Z

Just a nit on the naming: I would call this dumpPretty() (Printed seems redundant, and since you aim at typing this manually in a debugger, less characters won't hurt ;) )

I'm not sure on the name pretty, I don't see how the pretty is configured via the options used, it's closer to a dumpVerified.

joker-eph · 2024-12-16T23:08:30Z

The only effect of "AssumeVerified" is to not disable pretty-printing (that is: not disable custom printers and falling back to generic printer). From a user point of view, it seemed to me pretty was somehow OK to describe the visible difference in behavior between dump() and dumpXXXXXX(), but I'm open to either possibility really.

River707 · 2024-12-16T23:26:54Z

The only effect of "AssumeVerified" is to not disable pretty-printing (that is: not disable custom printers and falling back to generic printer). From a user point of view, it seemed to me pretty was somehow OK to describe the visible difference in behavior between dump() and dumpXXXXXX(), but I'm open to either possibility really.

We have some options (like prettyPrintLocations or something) that I had in my mind that made it weird (even if I'd love to remove that flag)

…nts.

…eic/mlir/dump-pretty-printed

weiweichen · 2024-12-17T15:07:43Z

Not using default argument (because lldb doesn't understand it) with Operation::dump() and adding Operation::dumpPretty() since pretty feels like similar to other IR printing methods. I don't have a strong opinion on what name to use. Happy to change the API to dumpVerified if we like it better (@River707 👀 ).

lattner

great outcome, thank you!

weiweichen added 2 commits December 16, 2024 12:24

[mlir] Add Operation::dumpPrettyPrinted.

4cbb253

Pretty print Operation that may not be verified for ease of readabilty while debugging.

Merge branch 'main' of https://github.com/llvm/llvm-project into weiw…

0c87100

…eic/mlir/dump-pretty-printed

llvmbot added mlir:core MLIR Core Infrastructure mlir labels Dec 16, 2024

River707 reviewed Dec 16, 2024

View reviewed changes

weiweichen added 2 commits December 16, 2024 15:52

Update API according to comment.

fe13015

Merge branch 'main' of https://github.com/llvm/llvm-project into weiw…

a670522

…eic/mlir/dump-pretty-printed

River707 approved these changes Dec 16, 2024

View reviewed changes

lattner approved these changes Dec 16, 2024

View reviewed changes

weiweichen added 3 commits December 17, 2024 10:01

Use dumpPretty instead because lldb doesn't understand default argume…

4504b3c

…nts.

Merge branch 'main' of https://github.com/llvm/llvm-project into weiw…

27ebd72

…eic/mlir/dump-pretty-printed

Remove extra line.

5870e25

weiweichen merged commit 644643a into llvm:main Dec 18, 2024
8 checks passed

lattner reviewed Dec 18, 2024

View reviewed changes

[mlir] Add Operation::dumpPrettyPrinted #120117

[mlir] Add Operation::dumpPrettyPrinted #120117

Uh oh!

Conversation

weiweichen commented Dec 16, 2024

Uh oh!

llvmbot commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

River707 left a comment

Choose a reason for hiding this comment

Uh oh!

weiweichen commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joker-eph commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weiweichen commented Dec 16, 2024

Uh oh!

joker-eph commented Dec 16, 2024

Uh oh!

River707 Dec 16, 2024

Choose a reason for hiding this comment

Uh oh!

lattner Dec 16, 2024

Choose a reason for hiding this comment

Uh oh!

lattner commented Dec 16, 2024

Uh oh!

lattner left a comment

Choose a reason for hiding this comment

Uh oh!

weiweichen commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joker-eph commented Dec 16, 2024

Uh oh!

River707 commented Dec 16, 2024

Uh oh!

joker-eph commented Dec 16, 2024

Uh oh!

River707 commented Dec 16, 2024

Uh oh!

weiweichen commented Dec 17, 2024

Uh oh!

Uh oh!

lattner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[mlir] Add `Operation::dumpPrettyPrinted` #120117

[mlir] Add `Operation::dumpPrettyPrinted` #120117

llvmbot commented Dec 16, 2024 •

edited

Loading

weiweichen commented Dec 16, 2024 •

edited

Loading

joker-eph commented Dec 16, 2024 •

edited

Loading

weiweichen commented Dec 16, 2024 •

edited

Loading