This is an archive of the discontinued LLVM Phabricator instance.

[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

Mogball created this revision.Oct 11 2021, 6:06 PM
Mogball requested review of this revision.Oct 11 2021, 6:06 PM

I have one more feature request please: can we support "default values", like omitting an entry if it matches the default value and setting the default value when it is missing from the text?

I have one more feature request please: can we support "default values", like omitting an entry if it matches the default value and setting the default value when it is missing from the text?

You're talking about for struct directive right?

I have one more feature request please: can we support "default values", like omitting an entry if it matches the default value and setting the default value when it is missing from the text?

You're talking about for struct directive right?

I think it'd be applicable to struct somehow, but also more widely. I'm not sure what would be the best syntax in the declarative assembly for that. But also I wonder if the concept of a default value isn't in itself something to include on the let parameters = instead. This could impact the builder as well maybe.

rriddle requested changes to this revision.Oct 11 2021, 10:48 PM

Took a quick glance, looks pretty good.

mlir/include/mlir/IR/DialectImplementation.h
105–112

nit: Can you move this lambda into a separate variable?

mlir/include/mlir/TableGen/Dialect.h
6

Nice

mlir/include/mlir/TableGen/FormatGen.h
1 ↗(On Diff #378845)

Do we need this in the public Tablegen API? Can we move this to mlir-tblgen instead? I don't think we want this exposes in the public facing API.

mlir/lib/TableGen/FormatGen.cpp
21–22 ↗(On Diff #378845)
mlir/test/lib/Dialect/Test/TestAttributes.cpp
136–153

Mark these as static and drop the anonymous namespace. Only classes should really be placed in those.

mlir/tools/mlir-tblgen/FormatParser.h
1 ↗(On Diff #378845)

Unused?

This revision now requires changes to proceed.Oct 11 2021, 10:48 PM
Mogball added inline comments.Oct 11 2021, 11:22 PM
mlir/test/lib/Dialect/Test/TestAttributes.cpp
136–153

I need to stop forgetting about this rule

mlir/tools/mlir-tblgen/FormatParser.h
1 ↗(On Diff #378845)

Indeed

Mogball added a comment.EditedOct 11 2021, 11:27 PM

I have one more feature request please: can we support "default values", like omitting an entry if it matches the default value and setting the default value when it is missing from the text?

You're talking about for struct directive right?

I think it'd be applicable to struct somehow, but also more widely. I'm not sure what would be the best syntax in the declarative assembly for that. But also I wonder if the concept of a default value isn't in itself something to include on the let parameters = instead. This could impact the builder as well maybe.

I was thinking purely in terms of struct: struct($one = 5, $two = "foo") which would be cleanest to implement IMO.

But of course default-valued parameters should extend beyond that. In which case it could be implemented much the same way default/optional attributes in ops are:

let parameters = (ins DefaultValuedParam<"int", "5">:$one, 
                      DefaultValuedParam<StringRefParameter, "\"foo\"">:$two);

I have one more feature request please: can we support "default values", like omitting an entry if it matches the default value and setting the default value when it is missing from the text?

You're talking about for struct directive right?

I think it'd be applicable to struct somehow, but also more widely. I'm not sure what would be the best syntax in the declarative assembly for that. But also I wonder if the concept of a default value isn't in itself something to include on the let parameters = instead. This could impact the builder as well maybe.

I was thinking purely in terms of struct: struct($one = 5, $two = "foo") which would be cleanest to implement IMO.

But of course default-valued parameters should extend beyond that. In which case it could be implemented much the same way default/optional attributes in ops are:

let parameters = (ins DefaultValuedParam<"int", "5">:$one, 
                      DefaultValuedParam<StringRefParameter, "\"foo\"">:$two);

Yeah, I think the cleanest thing would be to incorporate this in the parameters themselves (i.e. on the attribute definition). The assembly format could then be taught to understand defaults (it might be cleanest to work this into how parameters get anchored for "optional groups").

Mogball updated this revision to Diff 379098.Oct 12 2021, 10:11 AM
Mogball marked 5 inline comments as done.

Fixes for review comments

Will follow-up with a CL implementing default-valued parameters + optional groups in formats

jpienaar added inline comments.
mlir/test/mlir-tblgen/attr-or-type-format.mlir
110

Nit: could you name the functions to reflect what you are testing?

Mogball updated this revision to Diff 379110.Oct 12 2021, 10:49 AM

Describing/naming test cases for attr-or-type-format

Mogball updated this revision to Diff 379115.Oct 12 2021, 11:08 AM
Mogball marked an inline comment as done.

Fix test cases

Mogball updated this revision to Diff 379221.Oct 12 2021, 4:51 PM
This comment was removed by Mogball.
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.Nov 1 2021, 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.Nov 1 2021, 2:59 PM
Mogball added inline comments.Nov 1 2021, 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.Nov 1 2021, 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.Nov 1 2021, 4:20 PM
Mogball added inline comments.Nov 2 2021, 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.Nov 3 2021, 12:36 PM

struct(*) -> struct(params)

And added params directive.

Mogball marked 2 inline comments as done.Nov 3 2021, 12:37 PM
rriddle added inline comments.Nov 4 2021, 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.Nov 4 2021, 10:56 AM
mlir/docs/Tutorials/DefiningAttributesAndTypes.md
499

Very well.

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

Make params directive functional.

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

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

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

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.Nov 12 2021, 12:12 AM
mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
511

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