Page MenuHomePhabricator

Add a mechanism for Dialects to customize printing/parsing operations when they are unregistered
ClosedPublic

Authored by mehdi_amini on Mar 19 2021, 9:51 PM.

Diff Detail

Event Timeline

mehdi_amini created this revision.Mar 19 2021, 9:51 PM
mehdi_amini requested review of this revision.Mar 19 2021, 9:51 PM
rriddle added inline comments.Mon, Mar 22, 10:47 AM
mlir/include/mlir/IR/Dialect.h
46

Do we need the bool result here? This is only used for name shadowing (IIRC), so do we have an actual need to allow unregistered operations to name shadow? Also, I would expect that if an unregistered op does have that trait, we would develop a proper mechanism (e.g. via a dialect fallback) to query if the op has that trait. WDYT?

mlir/lib/IR/AsmPrinter.cpp
2412
mlir/lib/Parser/Parser.cpp
1658–1660

nit: Drop the llvm:: here.

mehdi_amini added inline comments.Mon, Mar 22, 11:19 AM
mlir/include/mlir/IR/Dialect.h
46

Yeah that's a good point!

mehdi_amini marked 3 inline comments as done.

address comments

jpienaar added inline comments.
mlir/include/mlir/IR/Dialect.h
42

This one I struggle to read (perhaps as I keep on reading bookmark). Is isolated from above returned/required still?

mlir/lib/Parser/Parser.cpp
1708

So this is only place where it can be changed to true?

rriddle accepted this revision.Mon, Mar 22, 3:46 PM
rriddle added inline comments.
mlir/include/mlir/IR/Dialect.h
46

Could we return function_ref (or llvm::unique_function) instead? I imagine that a majority of the functions passed back are going to be function-pointer-like(i.e. cheap to construct/copy).

mlir/lib/Parser/Parser.cpp
930

nit: Drop the llvm:: here.

mlir/test/lib/Dialect/Test/TestDialect.cpp
213

nit: Add braces here.

This revision is now accepted and ready to land.Mon, Mar 22, 3:46 PM
mehdi_amini added inline comments.Mon, Mar 22, 3:56 PM
mlir/include/mlir/IR/Dialect.h
42

Oh I forgot to update the doc when I changed the API...

mehdi_amini marked 4 inline comments as done.

Address River's most recent comments

mlir/lib/Parser/Parser.cpp
1708

Yes!
If/when we have Traits fallback to the dialect, we can also implement it in the else branch

Harbormaster completed remote builds in B95107: Diff 332454.