Page MenuHomePhabricator

[mlir][ODS] Add a new trait `TypesMatchWith`
ClosedPublic

Authored by rriddle on Feb 14 2020, 1:46 PM.

Details

Summary

This trait takes three arguments: lhs, rhs, transformer. It verifies that the type of 'rhs' matches the type of 'lhs' when the given 'transformer' is applied to 'lhs'. This allows for adding constraints like: "the type of 'a' must match the element type of 'b'". A followup revision will add support in the declarative parser for using these equality constraints to port more c++ parsers to the declarative form.

Depends On D74646

Diff Detail

Event Timeline

rriddle created this revision.Feb 14 2020, 1:46 PM

@antiagainst @jpienaar I'm not attached to the name here, I just couldn't think of one so please suggest something better if you can.

Just brief scan :)

mlir/include/mlir/IR/OpBase.td
1725

Could we do something like:

that denotes that transformer(lhs) == rhs ?

(makes it clear where transformer is applied, simpler to read :))

rriddle updated this revision to Diff 245195.Feb 18 2020, 9:56 AM
rriddle marked an inline comment as done.

Resolve comments

flaub added a subscriber: flaub.Feb 18 2020, 10:47 AM
stephenneuendorffer added inline comments.
mlir/include/mlir/Dialect/StandardOps/Ops.td
374

I can't parse this. suggest: result type has i1 element type and same shape as operands

442

I can't parse this...

1056

I can't parse this...

1073–1077

Unrelated change? Why does this need to be IntegerOrFloatLike?

1227

This is similar to other comments but "value type" starts to be ambiguous in a message. maybe "type of value operand matches element type of memref operand" instead?
or "type of 'value' matches element type of 'memref'"

1492

Ditto

1528

Ditto

rriddle updated this revision to Diff 245219.Feb 18 2020, 11:31 AM
rriddle marked 8 inline comments as done.

Resolve comments.

rriddle added inline comments.Feb 18 2020, 11:33 AM
mlir/include/mlir/Dialect/StandardOps/Ops.td
1073–1077

Not unrelated, getI1SameShape asserts that the given type is integer-or-float like. This avoids that assert.

antiagainst accepted this revision.Feb 18 2020, 3:47 PM

LGTM!

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1130

Give it a more descriptive name like checkAttrSizedValueSegmentsCode or something? (Sorry my bad previously using just code. :))

This revision is now accepted and ready to land.Feb 18 2020, 3:47 PM
mlir/include/mlir/Dialect/StandardOps/Ops.td
1073–1077

It seems like this is an unnecessary condition for 'select' which used to be type generic...

rriddle marked an inline comment as done.Feb 18 2020, 4:13 PM
rriddle added inline comments.
mlir/include/mlir/Dialect/StandardOps/Ops.td
1073–1077

It was never truly type generic, it was just never verified.

Um... OK. It's not clear to me *why* select shouldn't type generic, but we can cross that bridge later, I guess :)

Um... OK. It's not clear to me *why* select shouldn't type generic, but we can cross that bridge later, I guess :)

I'm not disagreeing with you :)

rriddle updated this revision to Diff 245303.Feb 18 2020, 4:43 PM

Resolve comments.

rriddle marked an inline comment as done.Feb 18 2020, 4:43 PM
jpienaar accepted this revision.Feb 19 2020, 7:42 AM

Nice, thanks!

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1178

Would front() work here?

rriddle marked 2 inline comments as done.Feb 19 2020, 10:17 AM
rriddle added inline comments.
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1178

Not yet, I need to add more accessors to the indexed_range class.

This revision was automatically updated to reflect the committed changes.
rriddle marked an inline comment as done.