Page MenuHomePhabricator

[MLIR] Attribute and type formats in ODS
ClosedPublic

Authored by Mogball on Oct 11 2021, 6:06 PM.

Details

Summary

Declarative attribute and type formats with assembly formats. Define an
assemblyFormat field in attribute and type defs with a mnemonic to
generate a parser and printer.

tablegen
def MyAttr : AttrDef<MyDialect, "MyAttr"> {
  let parameters = (ins "int64_t":$count, "AffineMap":$map);
  let mnemonic = "my_attr";
  let assemblyFormat = "`<` $count `,` $map `>`";
}

Use struct to define a comma-separated list of key-value pairs:

tablegen
def MyType : TypeDef<MyDialect, "MyType"> {
  let parameters = (ins "int":$one, "int":$two, "int":$three);
  let mnemonic = "my_attr";
  let assemblyFormat = "`<` $three `:` struct($one, $two) `>`";
}

Use struct(*) to capture all parameters.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a reviewer: sjarus. · View Herald Transcript
Herald added a reviewer: jsetoain. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Mogball updated this revision to Diff 379222.Oct 12 2021, 4:52 PM

pushed wrong diff...

Mogball removed a project: Restricted Project.Oct 12 2021, 4:55 PM
Mogball updated this revision to Diff 380108.Oct 15 2021, 3:12 PM

parsers of attributes or types with verifiers should call
getChecked

rriddle added inline comments.Oct 18 2021, 7:55 AM
mlir/test/lib/Dialect/Test/TestTypeDefs.td
217

Is this wrapped at 80?

mlir/test/mlir-tblgen/attr-or-type-format.mlir
5

Can you move all of these expected-error to the previous line and use either expected-error@+1 or expected-error@below?

mlir/test/mlir-tblgen/attr-or-type-format.td
36–48

Can you clean these up? Splitting them into different lines doesn't really help understand what this is supposed to be testing. (Same applies throughout this file).

mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
109
110

Can this method be const?

183

Please document this.

208

Include mlir/Support/LLVM.h and you can drop the llvm:: on all of these raw_ostream.

231–232

Please document these.

243–244

You don't need to specify the comment with the definition, especially if it is in the same file.

260–261

Spell out auto here.

273

Spell out auto here.

286

same here.

367

I'd drop all of these hardcoded indents honestly. They don't provide much over explicitly specifying the spaces.

378–384

Is it really important to let them be in any order as opposed to say always alphabetical order (or order as specified in the directive)?

465–469
642–643

These methods should be defined top-level. Only the class should be in the namespace.

655

Spell out auto here.

691–701

I would have expected this to be defined with the ODS gen, as opposed to the format generator.

716

Spell out auto here.

718

It's kind of weird seeing the format namespace next to the variable name.

mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.h
25

Please document this.

Mogball marked 19 inline comments as done.Oct 18 2021, 3:27 PM
Mogball added inline comments.
mlir/test/lib/Dialect/Test/TestTypeDefs.td
217

This line's 76 characters.

mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
378–384

If this were a fresh feature, I'd say "probably not", but since the struct directive is meant as a gateway to deprecate StructAttr, which is a DictionaryAttr, it needs to allow the parameters in any order (or else all the test cases will need to be fixed).

642–643

Ah, didn't know it also applied to member function definitions of anonymous-namespaced classes

Mogball updated this revision to Diff 380533.Oct 18 2021, 3:29 PM
Mogball marked an inline comment as done.

Review comments

Can you document this in Tutorials/DefiningAttributesAndTypes.md?

mlir/test/mlir-tblgen/attr-or-type-format.td
155–157

Can you trim this down to 2? Or is there something being tested for 4 params?

mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
527–528

Please remove the format token here. It makes the parser less local, because it relies on someone else having parsed the next token. This should use a similar form (i.e. have a currentToken, consumeToken, etc) as the other parser.

565–566

When does this happen? Why not let it be caught in the next call to parseElement?

571–576

Given that you need the index, why not llvm::enumerate?

655–656

Nit: You could just use SmallVector here (and give it a hint for the size, or let it pick one which would honestly be a pretty good choice here).

662–670

llvm::enumerate?

674

nit: Spell out auto here.

Mogball updated this revision to Diff 380837.Oct 19 2021, 6:31 PM
Mogball marked 6 inline comments as done.
  • Change FormatParser to use curToken and consumeToken instead of passing tokens around
  • Other review comments
Mogball marked an inline comment as done.Oct 19 2021, 6:31 PM
Mogball added inline comments.
mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
527–528

This "paradigm" personally made more sense to me, but I will change it to match the other parser. If I ever have to write a third format parser like this one, I will definitely try to abstract a lot of this duplicated logic away.

571–576

Because I didn't know it existed :)

Did a scan and code looks alright, but still missing documentation?

mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
593

Leftover debugging?

624–626

Drop the trivial braces here.

Mogball updated this revision to Diff 383189.Oct 28 2021, 4:04 PM
Mogball marked 3 inline comments as done.

Add documentation for attribute and type assembly formats
(and fixing review comments)

rriddle accepted this revision.Mon, Nov 1, 2:59 PM

LGTM after resolving the last comment.

mlir/docs/Tutorials/DefiningAttributesAndTypes.md
521

Would it be better to have a directive that represents all parameters? This seems a bit magical to me, and having a parameters/params directive also better matches the op format.

This revision is now accepted and ready to land.Mon, Nov 1, 2:59 PM
Mogball added inline comments.Mon, Nov 1, 4:16 PM
mlir/docs/Tutorials/DefiningAttributesAndTypes.md
521

Personally, params, drawing a parallel with op formats, would intuitively expand to a comma-separated list of the parameters. But struct(...) indicates that the parameters captured inside the directive will be parsed and printed in a key-value list form, where order doesn't matter, and the * is just a shorthand for "capture all the parameters".

I debated with just using struct instead of struct(*) but struct on its own doesn't do enough to indicate what's going on.

rriddle requested changes to this revision.Mon, Nov 1, 4:20 PM
rriddle added inline comments.
mlir/docs/Tutorials/DefiningAttributesAndTypes.md
521

But struct(...) indicates that the parameters captured inside the directive will be parsed and printed in a key-value list form, where order doesn't matter, and the * is just a shorthand for "capture all the parameters".

I'm not sure I agree here. Why would struct(params) be different from struct($a, $b, $c)?

This revision now requires changes to proceed.Mon, Nov 1, 4:20 PM
Mogball added inline comments.Tue, Nov 2, 9:49 AM
mlir/docs/Tutorials/DefiningAttributesAndTypes.md
521

Oh, I thought you meant params entirely instead of struct(*). I'm okay with struct(params)

Mogball updated this revision to Diff 384559.Wed, Nov 3, 12:36 PM

struct(*) -> struct(params)

And added params directive.

Mogball marked 2 inline comments as done.Wed, Nov 3, 12:37 PM
rriddle added inline comments.Thu, Nov 4, 8:14 AM
mlir/docs/Tutorials/DefiningAttributesAndTypes.md
499

Can we have it just mean a comma list of parameter values when used outside of struct?

mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
685–687

Drop trivial braces here.

699–701

Can we move this to the parser for the params directive instead? It feels like it would compose better, especially if the params directive is usable outside of struct.

Mogball added inline comments.Thu, Nov 4, 10:56 AM
mlir/docs/Tutorials/DefiningAttributesAndTypes.md
499

Very well.

Mogball updated this revision to Diff 385132.Fri, Nov 5, 10:52 AM

Make params directive functional.

Mogball marked 3 inline comments as done.Fri, Nov 5, 10:52 AM
rriddle accepted this revision.Sun, Nov 7, 1:26 PM

LG, let's land and then iterate in tree.

This revision is now accepted and ready to land.Sun, Nov 7, 1:26 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Fri, Nov 12, 12:10 AM
mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
510

I'm a bit puzzled by the conditional here, the emitted code isn't just "insert a space" like the comment indicates. What am I misunderstanding here?

mehdi_amini added inline comments.Fri, Nov 12, 12:12 AM
mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
510

Scratch it, I misread the code, to many meta levels here...