Page MenuHomePhabricator

[mlir][DeclarativeParser] Emit an error if a `:` follows an attribute with a non-constant type.
ClosedPublic

Authored by rriddle on Apr 2 2020, 2:48 AM.

Details

Summary

The attribute grammar includes an optional trailing colon type, so for attributes without a constant buildable type this will generally lead to unexpected and undesired behavior. Given that, it's better to just error out on these cases.

Diff Detail

Event Timeline

rriddle created this revision.Apr 2 2020, 2:48 AM
antiagainst accepted this revision.Apr 2 2020, 8:16 AM
This revision is now accepted and ready to land.Apr 2 2020, 8:16 AM
lattner accepted this revision.Apr 2 2020, 8:55 AM

Awesome, thank you River! Is this also introducing TypedStrAttr as a new way to specify an attribute that cannot have a ": type" production in the parser? Is there a way to spell that?

bondhugula added inline comments.
mlir/include/mlir/Dialect/Affine/IR/AffineOpsBase.td
23

Shouldn't this be I64? The affine map's results / inputs are Index type, but when the affine map is a constant, that constant is an int64 (and not of index bit width).

Awesome, thank you River! Is this also introducing TypedStrAttr as a new way to specify an attribute that cannot have a ": type" production in the parser? Is there a way to spell that?

Not exactly, TypedStrAttr is intended to be a way to specify a StrAttr with a non-None type; given that NoneType is the overwhelmingly common case.

You can already use the valueType field, used here, to specify that an attribute has a static type and to print/parser without it. For example, if you have an I32Attr today it won't print/parse
the type because we can statically construct an I32 type from the builder. This revision uses that knowledge to check if the attribute parses the type or not.

rriddle marked 2 inline comments as done.Apr 2 2020, 12:21 PM
rriddle added inline comments.
mlir/include/mlir/Dialect/Affine/IR/AffineOpsBase.td
23

I'm basing this off of the type of the MLIR attribute, which is always Index.

https://github.com/llvm/llvm-project/blob/6acd3003755db3944f7fcbea7542bd574a41c0a0/mlir/lib/IR/AttributeDetail.h#L34

bondhugula marked an inline comment as done.Apr 2 2020, 12:33 PM
bondhugula added inline comments.
mlir/include/mlir/Dialect/Affine/IR/AffineOpsBase.td
23

I see - looks like I completed missed what valueType here meant.

rriddle updated this revision to Diff 254982.Apr 3 2020, 7:19 PM
rriddle marked an inline comment as done.

Rebase

This revision was automatically updated to reflect the committed changes.