Page MenuHomePhabricator

[mlir] Add an interface for Cast-Like operations
ClosedPublic

Authored by rriddle on Jan 15 2021, 2:48 PM.

Details

Summary

A cast-like operation is one that converts from a set of input types to a set of output types. The arity of the inputs may be from 0-N, whereas the arity of the outputs may be anything from 1-N. Cast-like operations are removable in cases where they produce a "no-op", i.e when the input types and output types match 1-1.

Diff Detail

Event Timeline

rriddle created this revision.Jan 15 2021, 2:48 PM
rriddle requested review of this revision.Jan 15 2021, 2:48 PM
silvas added inline comments.Jan 15 2021, 2:54 PM
mlir/lib/IR/Operation.cpp
1229

This fold doesn't seem right, as many things that we could consider to be "casts" can be lossy. for example, consider if sitofp and fptosi were a single op (like we do for index_cast an others). Then this could fold away an intermediate truncation to integer.

Thoughts?

rriddle added inline comments.Jan 15 2021, 2:55 PM
mlir/lib/IR/Operation.cpp
1229

Thanks for the catch, I wrote the builtin op first and then just copied it over. This shouldn't be here.

silvas added inline comments.Jan 15 2021, 2:56 PM
mlir/include/mlir/Interfaces/CastInterfaces.td
23

what is the 0 case? That just seems more like a constant op.

rriddle added inline comments.Jan 15 2021, 3:02 PM
mlir/include/mlir/Interfaces/CastInterfaces.td
23

I was thinking of the cases that happen during dialect conversion. During conversion you can end up with a 1->0 conversion (most often during function signature conversion), i.e. where a type gets dropped. If uses of the original value still linger around, we have to insert a cast operation that temporarily materializes that value.

silvas accepted this revision.Jan 15 2021, 3:02 PM

Thanks for cleaning this up!

mlir/test/IR/invalid-ops.mlir
1077 ↗(On Diff #317082)

nit: kind of confusing to see this plural but only used for one type. Would suggest saying type(s).

Also, I don't see a test for the multiple-result case, multiple operand case, or 0-operand case. I don't think the anycast patch will include it (it allows all type combinations, right?), so it might be worth adding a test op.

This revision is now accepted and ready to land.Jan 15 2021, 3:02 PM
rriddle edited the summary of this revision. (Show Details)Jan 15 2021, 3:36 PM
rriddle updated this revision to Diff 317092.Jan 15 2021, 3:36 PM
rriddle edited the summary of this revision. (Show Details)

Fix accidental incorrect behavior for casts

lattner accepted this revision.Jan 15 2021, 5:29 PM
lattner added a subscriber: lattner.

nice!

mlir/docs/Tutorials/Toy/Ch-4.md
224

Can't this code be simpler, something like:

return input && output && input == output && input.hasRank())

since the tensor types are uniqued? You only need the more complicated code if you're using verifyCompatibleShape instead of shape equality IIRC.

mlir/examples/toy/Ch4/mlir/Dialect.cpp
249

same code

mlir/examples/toy/Ch5/mlir/Dialect.cpp
249

same code

mlir/examples/toy/Ch6/mlir/Dialect.cpp
250

same code

mlir/examples/toy/Ch7/mlir/Dialect.cpp
301

same code

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
48–49

btw Paul taught me that !listconcat(x, y) can be spelled as x # y which is much nicer for this sort of use case

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2740

Random question, but should this be an assert that input/output.size() == 1 instead of a check?

Shouldn't this be called only when the arity is already checked by the verifier?

rriddle updated this revision to Diff 317141.Jan 15 2021, 6:43 PM
rriddle marked 10 inline comments as done.

Address comments

rriddle added inline comments.Jan 15 2021, 6:44 PM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
48–49

Nice!

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2740

These methods are sometimes called from outside of the verifier. That coupled with the fact that these are now interface methods, I figured it's better to gracefully fail instead of assert.

rriddle edited the summary of this revision. (Show Details)Jan 15 2021, 6:44 PM
herhut added a subscriber: herhut.Jan 18 2021, 12:31 AM

Nice cleanup, thanks!

mlir/docs/Tutorials/Toy/Ch-4.md
216

nit: They - The

mlir/examples/toy/Ch4/mlir/Dialect.cpp
241

nit: They -> The

mlir/examples/toy/Ch5/mlir/Dialect.cpp
241

Same nit here and in every copy :)

rriddle updated this revision to Diff 317657.Jan 19 2021, 12:41 PM
rriddle marked 3 inline comments as done.

Address feedback

This revision was automatically updated to reflect the committed changes.