This is an archive of the discontinued LLVM Phabricator instance.

Add a `qualified` directive to the Op, Attribute, and Type declarative assembly format
ClosedPublic

Authored by mehdi_amini on Jan 9 2022, 4:20 PM.

Details

Summary

This patch introduces a new directive that allow to parse/print attributes and types fully
qualified.
This is a follow-up to ee0908703d29 which introduces the eliding of the !dialect.mnemonic by default and allows to force to fully qualify each type/attribute
individually.

Diff Detail

Event Timeline

mehdi_amini created this revision.Jan 9 2022, 4:20 PM
mehdi_amini requested review of this revision.Jan 9 2022, 4:20 PM
mehdi_amini retitled this revision from Add a `qualified` directive to the Op declarative assembly format to Add a `qualified` directive to the Op, Attribute, and Type declarative assembly format.Jan 9 2022, 4:23 PM
jpienaar added inline comments.
mlir/docs/Tutorials/DefiningAttributesAndTypes.md
493

Copy pasta?

523

And by nesting here this also means for nested types inside type nested in directive?

jpienaar added inline comments.Jan 9 2022, 4:26 PM
mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
92

shouldBeQualified?

mehdi_amini added inline comments.Jan 9 2022, 4:45 PM
mlir/docs/Tutorials/DefiningAttributesAndTypes.md
523

It does not mean "recursive" if this is what you're asking for

mehdi_amini marked an inline comment as done.

Addess Jacques' comments

mehdi_amini marked an inline comment as done.Jan 9 2022, 4:53 PM
rriddle accepted this revision.Jan 9 2022, 8:09 PM

LGTM after resolving comments. It's nice to be able to opt out for those that don't want the more implicit behavior.

mlir/docs/OpDefinitions.md
656
mlir/docs/Tutorials/DefiningAttributesAndTypes.md
523–525

?

I also found the wording including nesting a bit confusing.

mlir/test/lib/Dialect/Test/TestAttrDefs.td
151–154

^ better matches how we format everywhere else, not sure why this file formats this way.

mlir/test/lib/Dialect/Test/TestTypeDefs.td
80–83

same comment as other file, but I guess this is fine for the sake of conformance with the other tests (we should NFC fix that though)

mlir/test/mlir-tblgen/op-format.mlir
314–316
mlir/test/mlir-tblgen/testdialect-typedefs.mlir
33–35

semi unrelated, but could this just be a declaration?

mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
92–95

Can you document these functions?

96–97
684–685

In the failed(var) case, is that going to lead to a double error message?

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

Is this necessary? I wouldn't expect so, given that there is no element class added.

122–125

Can you add documentation here?

3225–3227

nit: I might have added braces here given that this statement stretches 3 lines.

3229

Not sure why we are qualifying ::mlir:: in this code. I should really go through and clean this up.

This revision is now accepted and ready to land.Jan 9 2022, 8:09 PM
stellaraccident accepted this revision.Jan 9 2022, 8:20 PM

Thanks - lg modulo comments.

Nice!

mlir/docs/OpDefinitions.md
659

, -> ;

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

Nit: Flat to indicate ... -> Indicates ....

mehdi_amini marked 14 inline comments as done.

Address comments

mehdi_amini added inline comments.Jan 10 2022, 5:29 PM
mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
92–95

Moving the comment from the member variable to the accessors then.

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

No this is from an early attempt...

mehdi_amini marked 2 inline comments as done.Jan 10 2022, 5:29 PM