This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Refactor the Parser library in preparation for an MLIR binary format
ClosedPublic

Authored by rriddle on Jul 12 2022, 4:51 PM.

Details

Summary

The current Parser library is solely focused on providing API for
the textual MLIR format, but MLIR will soon also provide a binary
format. This commit renames the current Parser library to AsmParser to
better correspond to what the library is actually intended for. A new
Parser library is added which will act as a unified parser interface
between both text and binary formats. Most parser clients are
unaffected, given that the unified interface is essentially the same as
the current interface. Only clients that rely on utilizing the
AsmParserState, or those that want to parse Attributes/Types need to be
updated to point to the AsmParser library.

Diff Detail

Event Timeline

rriddle created this revision.Jul 12 2022, 4:51 PM
Herald added a project: Restricted Project. · View Herald Transcript
rriddle requested review of this revision.Jul 12 2022, 4:51 PM

In general this seems like a good mechanical move to make space for the more generic.

mlir/lib/AsmParser/Parser.h
1

Is end state here that all will actually include Parser/Parser.h unless they explicitly care about textual format? If so, does it make sense to move all to point to AsmParser/Parser.h as intermediate state? (E.g., what if we kept Parser/Parser.h and had it include AsmParser/Parser.h and then when there is the binary one the ones that need the generic interface still points here and the others get flipped but we don't get a flip one way and then the next)

rriddle added inline comments.Jul 13 2022, 2:06 PM
mlir/lib/AsmParser/Parser.h
1

Yeah, the end state is that 99% of users will include Parser/Parser.h. Only those that need to parse textual attributes/types(rare), or use the specialized AsmState stuff (generally only LSP or other very rare tools), should be including AsmParser/Parser.h.

What is the benefit of the intermediate state? I don't think any current usages upstream will change after this patch, and users should generally prefer the generic interface over the Asm one.

jpienaar accepted this revision.Jul 15 2022, 7:52 AM

Moving to make space for generic interface and bin parser SGTM.

mlir/lib/AsmParser/Parser.h
1

Oh just that we don't delete Parser/Parser.h and then reintroduce it if that's what most will be using. And until there is the generic one, AsmParser and Parser can just be alias that would seem to result in no back and forth where Parser is actually to be used. Not strong preference just thinking of we can avoid changing users that will be using Parser to use AsmParser and back to Parser.

This revision is now accepted and ready to land.Jul 15 2022, 7:52 AM

Does your plan already set us up to build MLIR with support for the binary parser but stripping out the ASM parsing entirely?
(I'd really want this for the printer as well, but this is slightly more tricky)

Does your plan already set us up to build MLIR with support for the binary parser but stripping out the ASM parsing entirely?
(I'd really want this for the printer as well, but this is slightly more tricky)

It should. I think we need to figure out the concrete details there though. If we go with something like a build-time flag, it should be trivial. The new Parser lib simply dispatches to either binary/text based on sniffing the input (i.e. checking for binary magic), so stripping that out text would be simple and contained to one place. We could also provide a nice error message in those cases (if you provide text but haven't built with it). If someone never wants the textual format, they can always rely on the binary lib directly (but the more interesting case for me is stripping out text parsing with a simple build time change)

the more interesting case for me is stripping out text parsing with a simple build time change

Yes that's what I'm looking for.

This revision was landed with ongoing or failed builds.Jul 25 2022, 4:33 PM
This revision was automatically updated to reflect the committed changes.
mlir/lib/Parser/DialectSymbolParser.cpp