Page MenuHomePhabricator

[mlir][ods] Default-valued parameters in attribute or type defs
ClosedPublic

Authored by Mogball on Jan 29 2022, 3:38 PM.

Details

Summary

Optional parameters with defaultValue set will be populated with that value if they aren't encountered during parsing. Moreover, parameters equal to their default values are elided when printing.

Depends on D118210

Diff Detail

Event Timeline

Mogball created this revision.Jan 29 2022, 3:38 PM
Mogball requested review of this revision.Jan 29 2022, 3:38 PM
rriddle added inline comments.Jan 31 2022, 1:22 AM
mlir/docs/Tutorials/DefiningAttributesAndTypes.md
525
527

Parameters have a comparator field, which I assume should be what this is driven through (which could mean the == of the type, but doesn't have to be).

529–534

Would be good to show example outputs here.

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

This should use the comparator field. Would likely be good to add a test with something like APInt.

87
302

Storing a twine value is playing with fire a bit, Twine generally takes variables by reference. I would inline this (param.getCppStorageType() + "()").str()/ generate the string right here.

Mogball updated this revision to Diff 404819.Jan 31 2022, 11:17 PM
Mogball marked 6 inline comments as done.

review comments + new test case for APFloat with comparator

lattner added a subscriber: lattner.Feb 1 2022, 5:55 PM

This is really cool Jeff!

rriddle added inline comments.Feb 7 2022, 11:39 AM
mlir/docs/Tutorials/DefiningAttributesAndTypes.md
541–542

Can you give an example on how this interacts with optional groups?

mlir/test/lib/Dialect/Test/TestTypeDefs.td
334–335

nit: Prefer the original namespace if we need to specify it (i.e. llvm in this case).

350–351
mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
65

Is the llvm:: here necessary?

rriddle added inline comments.Feb 7 2022, 11:41 AM
mlir/include/mlir/IR/OpBase.td
3148–3152

It looks like this requires that the parameter be statically constructible. Conceptually though, we could at least provide an instance of the context to allow for building things like default attributes/types.

Mogball marked 4 inline comments as done.Feb 8 2022, 3:25 PM
Mogball added inline comments.
mlir/include/mlir/IR/OpBase.td
3148–3152

getContext() would work, but I didn't want to add a substitution because I wonder if the default values should be also added to the getters:

SomeAttr get(MLIRContext *ctx, int value = 5);

Evidently, having a context in the default value string would not be valid here. If default values were not set in the getters, then the default values of the parameters would not exist outside of the parser/printer. If I wanted to get an attribute with default-valued parameters in C++, there would be no way to do so.

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

Yes. Otherwise it conflicts with VariableElement::Optional.

Mogball updated this revision to Diff 407001.Feb 8 2022, 3:33 PM
Mogball marked an inline comment as done.

review comments

rriddle added inline comments.Feb 10 2022, 11:53 AM
mlir/include/mlir/IR/OpBase.td
3148–3152

I wouldn't worry about that given that you can just do what the C++ compiler will do for you anyways:

// This:
SomeAttr get(MLIRContext *ctx, int value = 5);

// Just turns into (generally speaking):
SomeAttr get(MLIRContext *ctx, int value);
SomeAttr get(MLIRContext *ctx) { return get(ctx, 5); }
rriddle added inline comments.Feb 10 2022, 11:55 AM
mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
69

Should we just set this as the default value of the comparator field?

Mogball marked 3 inline comments as done.Feb 11 2022, 12:31 PM
Mogball added inline comments.
mlir/include/mlir/IR/OpBase.td
3148–3152

Oh, true.

Mogball updated this revision to Diff 407993.Feb 11 2022, 12:38 PM
Mogball marked an inline comment as done.

review comments

Mogball updated this revision to Diff 408003.Feb 11 2022, 12:52 PM

Add the context substitution and add a test case

rriddle accepted this revision.Feb 14 2022, 10:49 AM

Really awesome stuff!

mlir/docs/Tutorials/DefiningAttributesAndTypes.md
546

Is ctxt what we use everywhere else? IMO, we should really unify around _ctx (which more closely aligns with what we shorten it to in C++).

mlir/include/mlir/IR/OpBase.td
3152
This revision is now accepted and ready to land.Feb 14 2022, 10:49 AM
Mogball marked 2 inline comments as done.Feb 15 2022, 10:57 AM
Mogball added inline comments.
mlir/docs/Tutorials/DefiningAttributesAndTypes.md
546

It's actually inconsistent. There is one use of _ctx and four (including this one) of _ctxt. I also thought it was weird that it's _ctxt and not _ctx. I'll swap this one to _ctx

Mogball updated this revision to Diff 408968.Feb 15 2022, 11:01 AM
Mogball marked an inline comment as done.

change _ctxt to _ctx

This revision was landed with ongoing or failed builds.Feb 15 2022, 11:02 AM
This revision was automatically updated to reflect the committed changes.