This is an archive of the discontinued LLVM Phabricator instance.

[mlir:Parser] Don't use strings for the "ugly" form of Attribute/Type syntax
ClosedPublic

Authored by rriddle on Jan 28 2022, 12:14 PM.

Details

Summary

This commit refactors the syntax of "ugly" attribute/type formats to not use
strings for wrapping. This means that moving forward attirbutes and type formats
will always need to be in some recognizable form, i.e. if they use incompatible
characters they will need to manually wrap those in a string, the framework will
no longer do it automatically.

This has the benefit of greatly simplifying how parsing attributes/types work, given
that we currently rely on some extremely complicated nested parser logic which is
quite problematic for a myriad of reasons; unecessary complexity(we create a nested
source manager/lexer/etc.), diagnostic locations can be off/wrong given string escaping,
etc.

Diff Detail

Event Timeline

rriddle created this revision.Jan 28 2022, 12:14 PM
rriddle requested review of this revision.Jan 28 2022, 12:14 PM
rriddle planned changes to this revision.EditedJan 28 2022, 12:15 PM

This patch isn't entirely done, still need to update a few things (essentially just documentation). Just getting it out to show the direction.

Mogball added inline comments.Jan 28 2022, 1:03 PM
mlir/lib/Parser/DialectSymbolParser.cpp
213

I was under the impression from the description that nested parsers would be gone?

rriddle added inline comments.Jan 28 2022, 2:07 PM
mlir/lib/Parser/DialectSymbolParser.cpp
213

This is not a nested parser in the ways that matter, it is a controlled API layered on top of the base parser. The "nested parser" in the description is referring to the fact that we previously had to instantiate a completely new base parser(i.e. new source manager, new lexer, etc.).

rriddle requested review of this revision.Jan 28 2022, 2:41 PM
rriddle updated this revision to Diff 404169.
rriddle edited the summary of this revision. (Show Details)
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2022, 2:41 PM
rriddle planned changes to this revision.Jan 28 2022, 2:41 PM

(updating triggered review, still not done)

bondhugula added inline comments.
flang/lib/Optimizer/Dialect/FIRType.cpp
692

Side comment: this declaration can now be moved inside.

mlir/lib/IR/AsmPrinter.cpp
1552

Single quotes.

rriddle requested review of this revision.Jun 29 2022, 10:52 AM
rriddle updated this revision to Diff 441080.
rriddle edited the summary of this revision. (Show Details)
rriddle marked 2 inline comments as done.
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: bzcheeseman. · View Herald Transcript
rriddle added inline comments.Jun 29 2022, 10:52 AM
flang/lib/Optimizer/Dialect/FIRType.cpp
692

It can't, it's used lower down in the SequenceType::get call. Unless I completely misunderstood what you meant here, which is possible.

Mogball accepted this revision.Jun 29 2022, 11:19 AM

Are ugly attributes and types now no longer allowed to have unbalanced < and > in their format?

This revision is now accepted and ready to land.Jun 29 2022, 11:19 AM
jpienaar accepted this revision.Jul 3 2022, 6:43 AM
jpienaar added a subscriber: jpienaar.

Overall looks good thanks, could you add to commit some guide for downstream folks needing to update?

mlir/lib/IR/AsmPrinter.cpp
1466

Could this be expanded to say when it's simple enough?

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