This is an archive of the discontinued LLVM Phabricator instance.

[mlir][NFC] Make 'printOp' public in AsmPrinter
ClosedPublic

Authored by dcaballe on Sep 30 2022, 6:36 PM.

Details

Summary

This patch moves the 'printOp' functionality to the public API of
AsmPrinter. No 'parseOp' seems to be needed at this time as existing
APIs are able to parse operations producing results where results are
omitted in the textual form (the LHS of an operation is redundant
when it comes to building the operation itself as it only contains
the result names).

Diff Detail

Event Timeline

dcaballe created this revision.Sep 30 2022, 6:36 PM
Herald added a project: Restricted Project. · View Herald Transcript
dcaballe requested review of this revision.Sep 30 2022, 6:36 PM
dcaballe added inline comments.Sep 30 2022, 6:38 PM
mlir/lib/IR/AsmPrinter.cpp
2674

We have too many similar print methods for Operation *. We should rename them but I prefer to get your feedback first.

Can you add a test in the TestDialect?

mlir/lib/IR/AsmPrinter.cpp
2674

Yeah, it'd be nice to rename them to be explicit about how they are printing. The current ones are really hard to reason about.

dcaballe updated this revision to Diff 464770.Oct 3 2022, 12:27 PM

Renaming + testing

rriddle accepted this revision.Oct 4 2022, 10:28 PM
This revision is now accepted and ready to land.Oct 4 2022, 10:28 PM
bondhugula added inline comments.
mlir/lib/IR/AsmPrinter.cpp
2665

What exactly is "etc." here?

2666

The names print and printFullOp look confusing now: the former is printing more than the latter now.

dcaballe added inline comments.Oct 4 2022, 10:47 PM
mlir/lib/IR/AsmPrinter.cpp
2665

I guess it refers to the location context but we only have indentation and location for now. I'll remove the etc. part.

2666

I kept print because I saw another print method for blocks but I now see it's the only one? I can rename it to printFullOpWithIndent or printFullOpWithIndentAndLoc but I'm not sure if this will scale well if we add more information to be printed. Perhaps something like printFullOpInContext or something like that?

bondhugula added inline comments.Oct 5 2022, 12:04 AM
mlir/lib/IR/AsmPrinter.cpp
2666

I don't have any more comments. Please feel free to go with what you think is better.

This revision was automatically updated to reflect the committed changes.
mlir/test/IR/print-op-custom-or-generic.mlir