Page MenuHomePhabricator

[mlir][DeclarativeParser] Add support for formatting enum attributes in the string form.
ClosedPublic

Authored by rriddle on Feb 12 2020, 10:30 PM.

Details

Summary

This revision adds support to the declarative parser for formatting enum attributes in the symbolized form. It uses this new functionality to port several of the SPIRV parsers over to the declarative form.

Diff Detail

Event Timeline

rriddle created this revision.Feb 12 2020, 10:30 PM
antiagainst accepted this revision.Feb 13 2020, 7:52 AM

Nice! Thanks River for adding support for this!

mlir/tools/mlir-tblgen/OpFormatGen.cpp
301

Do we want to specify none type here? It should parse as a string attr for start and we later reply on that for symbolize the string.

This revision is now accepted and ready to land.Feb 13 2020, 7:52 AM
rriddle marked 2 inline comments as done.Feb 13 2020, 11:32 AM
rriddle added inline comments.
mlir/tools/mlir-tblgen/OpFormatGen.cpp
301

Without specifying a type, the parser of StringAttr will check for an optional colon type which isn't a desirable. Passing NoneType makes sure that we only parse the string value.

flaub added a subscriber: flaub.Feb 13 2020, 4:17 PM

This is great! Would it be possible to parse keywords into enum attrs as well?

Actually, what's the general feeling about using keywords vs strings for enum values? I think I prefer keywords a little more, it's less noisy and its distinguished from an actual string in the textual representation. Also, it seems like LLVM IR has 'keywords' (they aren't wrapped in quotes) for many enums.

This revision was automatically updated to reflect the committed changes.
rriddle marked an inline comment as done.

This is great! Would it be possible to parse keywords into enum attrs as well?

Actually, what's the general feeling about using keywords vs strings for enum values? I think I prefer keywords a little more, it's less noisy and its distinguished from an actual string in the textual representation. Also, it seems like LLVM IR has 'keywords' (they aren't wrapped in quotes) for many enums.

I think we could, but we would need additional checks given that only a subset of enums can actually be represented with a keyword. BitEnums would be completely out, but that is to be expected. For the others we would need to check dynamically if the case value can be parsed in as a keyword. I say dynamically instead of statically because it would be really awful if you added a new enum case that wasn't conforming and then had to update all of your existing tests.