This is an archive of the discontinued LLVM Phabricator instance.

[mlir] resolve types from attributes in assemblyFormat
ClosedPublic

Authored by tali on Apr 18 2020, 12:14 PM.

Details

Summary

An operation can specify that an operation or result type matches the
type of another operation, result, or attribute via the AllTypesMatch
or TypesMatchWith constraints.

Use these constraints to also automatically resolve types in the
automatically generated assembly parser.
This way, only the attribute needs to be listed in assemblyFormat,
e.g. for constant operations.

Diff Detail

Event Timeline

tali created this revision.Apr 18 2020, 12:14 PM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
317

Can you use an llvm::PointerUnion for the variable and attribute to avoid another field?

1356

Same here, could you use PointerUnion instead?

1750

Is there a reason why we couldn't use the transformer with the attribute type?

tali updated this revision to Diff 259722.Apr 23 2020, 3:04 PM

Use llvm::PointerUnion and add some tests

tali marked 3 inline comments as done.Apr 23 2020, 3:06 PM
tali added inline comments.
mlir/tools/mlir-tblgen/OpFormatGen.cpp
1750

Haven't found a need to do so, but added it in next revision.

rriddle requested changes to this revision.Apr 28 2020, 10:58 AM

Looking really good.

mlir/test/lib/Dialect/Test/TestOps.td
1332

Can you add an example for AllTypesMatch?

mlir/tools/mlir-tblgen/OpFormatGen.cpp
298–299

Please run clang-format.

1707

Can you add support for attributes here as well?

This revision now requires changes to proceed.Apr 28 2020, 10:58 AM
rriddle added inline comments.Apr 28 2020, 11:00 AM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
741

We will also need to add verification of the attribute here if the transformer is non-trivial.

rriddle added inline comments.Apr 28 2020, 11:02 AM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
741

Although I suspect 99% of the cases are already handled because we check the storage type when parsing, happy to leave this as a followup for now.

rriddle accepted this revision.Apr 28 2020, 11:04 AM

LGTM after adding handling for AllTypesMatch and a test for it.

mlir/test/lib/Dialect/Test/TestOps.td
1332

With attributes I mean.

This revision is now accepted and ready to land.Apr 28 2020, 11:04 AM
tali marked 5 inline comments as done.May 1 2020, 11:54 PM
tali added inline comments.
mlir/tools/mlir-tblgen/OpFormatGen.cpp
741

Which checks would you find useful here?

tali updated this revision to Diff 261620.May 1 2020, 11:55 PM

Add attribute support for AllTypesMatch

tali marked an inline comment as done.May 2 2020, 1:22 AM
tali added inline comments.
mlir/test/mlir-tblgen/op-format.mlir
113

I would really like to be able to format this example as test.format_all_types_match_attr 1, %i64 : i64, with the type at the end.
We could add support for type($attribute), and suppress printing of the attribute type if it is specified explicitly or when it can be inferred.

rriddle added inline comments.May 2 2020, 1:41 AM
mlir/test/mlir-tblgen/op-format.mlir
113

The type of the attribute is required to be resolved when parsing it(i.e. at creation time). You would need some fairly fundamental changes to attributes to support that.

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

I mean that if the transformer is dependent on specific invariants of the attribute constraint, the attribute has to be verified first otherwise the transformer could assert/crash. This is the same reasoning as to why the operand/result variables get verified here.

tali edited the summary of this revision. (Show Details)May 2 2020, 12:39 PM
tali updated this revision to Diff 263102.May 10 2020, 9:53 PM

rebase against HEAD

tali updated this revision to Diff 274606.Jun 30 2020, 1:40 PM

Rebase to HEAD

Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2020, 1:40 PM
tali updated this revision to Diff 274609.Jun 30 2020, 2:09 PM

Rename NamedArgOrAttr to ConstArgument

mehdi_amini added inline comments.Jun 30 2020, 3:25 PM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
1773

Can you name the types instead of auto? (when the type isn't immediately obvious and/or it does not improve readability significantly)

tali updated this revision to Diff 274912.Jul 1 2020, 2:04 PM

Used more concrete types instead of auto and simplified findSeenArg().

tali marked an inline comment as done.Jul 1 2020, 2:05 PM
tali updated this revision to Diff 275288.Jul 2 2020, 9:41 PM

Rebase on top of HEAD.

This revision was automatically updated to reflect the committed changes.