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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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) |
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. |
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. |
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.
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)