This is an archive of the discontinued LLVM Phabricator instance.

Updated IndexCastOp to support tensor<index> cast
ClosedPublic

Authored by rsuderman on Jun 10 2020, 1:09 PM.

Details

Summary

We now support index casting for tensor<index> to tensor<int>. This
better supports compatibility with the Shape dialect.

Diff Detail

Event Timeline

rsuderman created this revision.Jun 10 2020, 1:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2020, 1:09 PM
rriddle requested changes to this revision.Jun 10 2020, 1:27 PM

Can you add test cases?

This revision now requires changes to proceed.Jun 10 2020, 1:27 PM
rsuderman updated this revision to Diff 269974.Jun 10 2020, 2:55 PM

Updated to include IR tests for index_cast.

rriddle added inline comments.Jun 10 2020, 3:01 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1782

Can you add a test for this?

mlir/test/Dialect/Standard/ops.mlir
5 ↗(On Diff #269974)

This looks more like a test for the core parser than for the Standard dialect. Not sure that we should include this here.

rriddle added inline comments.Jun 10 2020, 3:02 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1772

Is this intended to support mismatch static/dynamic dimensions?

rsuderman updated this revision to Diff 269979.Jun 10 2020, 3:07 PM
rsuderman marked an inline comment as done.

Removed unneeded folder.

rsuderman edited the summary of this revision. (Show Details)Jun 10 2020, 3:08 PM
rsuderman marked 2 inline comments as done.
rsuderman added inline comments.
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1782

Turns out its unneeded as index_cast does not allow noop casts.

mlir/test/Dialect/Standard/ops.mlir
5 ↗(On Diff #269974)

My goal is to match the invalid tests and have correctly parsed tests pass through (both with and without tensors). Is there a better way to include examples of valid index_casts?

rriddle accepted this revision.Jun 10 2020, 3:10 PM

Can you revise the commit description if you aren't updating the folder?

mlir/test/Dialect/Standard/ops.mlir
5 ↗(On Diff #269974)

I meant the last RUN line. Parsing the generic input seems like it is testing the core parser and nothing related to the standard dialect that isn't tested by the other two tests.

This revision is now accepted and ready to land.Jun 10 2020, 3:10 PM
rsuderman updated this revision to Diff 269980.Jun 10 2020, 3:16 PM
rsuderman marked 2 inline comments as done.

Removed unneeded parts othe Standard/ops.mlir checks.

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

I wasn't planning to. It the input/output shapes should match and it would be strange to have a incorrectly shape determined side of the case.

rsuderman retitled this revision from Updated IndexCastOp to support tensor<index> cast with updated folder to Updated IndexCastOp to support tensor<index> cast.Jun 10 2020, 3:18 PM

Comment updated.

jpienaar accepted this revision.Jun 10 2020, 3:43 PM

Thanks!

mlir/test/Dialect/Standard/ops.mlir
1 ↗(On Diff #269980)

What does split input do here given fed back into mlir-opt? (and also no file splits in here)

Harbormaster completed remote builds in B59882: Diff 269979.
rsuderman updated this revision to Diff 269993.Jun 10 2020, 4:20 PM

jpienaar@ comment for removing pipe into mlir-opt

rsuderman marked an inline comment as done.Jun 10 2020, 4:20 PM
rsuderman added inline comments.
mlir/test/Dialect/Standard/ops.mlir
1 ↗(On Diff #269980)

Extra from copy-paste of another test.

This revision was automatically updated to reflect the committed changes.