This is an archive of the discontinued LLVM Phabricator instance.

Differential D81611

Updated IndexCastOp to support tensor<index> cast
ClosedPublic

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

Details

Reviewers
rriddle
jpienaar
Commits
rG3d56f166bd3c: [mlir][StandardOps] Updated IndexCastOp to support tensor<index> cast
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
Herald added subscribers: msifontes, jurahul, Kayjukh and 15 others. · View Herald Transcript
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
Harbormaster completed remote builds in B59870: Diff 269956.Jun 10 2020, 2:28 PM
rsuderman updated this revision to Diff 269974.Jun 10 2020, 2:55 PM

Updated to include IR tests for index_cast.

Herald added a subscriber: aartbik. · View Herald TranscriptJun 10 2020, 2:55 PM
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
6

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
6

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
6

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.

Harbormaster completed remote builds in B59878: Diff 269974.Jun 10 2020, 3:35 PM
jpienaar accepted this revision.Jun 10 2020, 3:43 PM

Thanks!

mlir/test/Dialect/Standard/ops.mlir
2

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

Harbormaster completed remote builds in B59883: Diff 269980.Jun 10 2020, 4:08 PM
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
2

Extra from copy-paste of another test.

Closed by commit rG3d56f166bd3c: [mlir][StandardOps] Updated IndexCastOp to support tensor<index> cast (authored by rsuderman, committed by rriddle). · Explain WhyJun 10 2020, 5:47 PM
This revision was automatically updated to reflect the committed changes.
Harbormaster completed remote builds in B59895: Diff 269993.Jun 10 2020, 6:20 PM

Revision Contents

PathSize
mlir/
lib/
Dialect/
StandardOps/
IR/
9 lines
test/
Dialect/
Standard/
17 lines
24 lines

Diff 269979

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

Show First 20 Lines • Show All 1,759 Lines • ▼ Show 20 Lines
} }
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
// IndexCastOp // IndexCastOp
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
// Index cast is applicable from index to integer and backwards. // Index cast is applicable from index to integer and backwards.
bool IndexCastOp::areCastCompatible(Type a, Type b) { bool IndexCastOp::areCastCompatible(Type a, Type b) {
if (a.isa<ShapedType>() && b.isa<ShapedType>()) {
auto aShaped = a.cast<ShapedType>();
auto bShaped = b.cast<ShapedType>();
return (aShaped.getShape() == bShaped.getShape()) &&
rriddleUnsubmitted
Not Done
ReplyInline Actions

Is this intended to support mismatch static/dynamic dimensions?

rriddle: Is this intended to support mismatch static/dynamic dimensions?
rsudermanAuthorUnsubmitted
Done
ReplyInline Actions

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: I wasn't planning to. It the input/output shapes should match and it would be strange to have a…
areCastCompatible(aShaped.getElementType(),
bShaped.getElementType());
}
return (a.isIndex() && b.isSignlessInteger()) || return (a.isIndex() && b.isSignlessInteger()) ||
(a.isSignlessInteger() && b.isIndex()); (a.isSignlessInteger() && b.isIndex());
} }
OpFoldResult IndexCastOp::fold(ArrayRef<Attribute> cstOperands) { OpFoldResult IndexCastOp::fold(ArrayRef<Attribute> cstOperands) {
// Fold IndexCast(IndexCast(x)) -> x // Fold IndexCast(IndexCast(x)) -> x
rriddleUnsubmitted
Done
ReplyInline Actions

Can you add a test for this?

rriddle: Can you add a test for this?
rsudermanAuthorUnsubmitted
Done
ReplyInline Actions

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

rsuderman: Turns out its unneeded as index_cast does not allow noop casts.
auto cast = getOperand().getDefiningOp<IndexCastOp>(); auto cast = getOperand().getDefiningOp<IndexCastOp>();
if (cast && cast.getOperand().getType() == getType()) if (cast && cast.getOperand().getType() == getType())
return cast.getOperand(); return cast.getOperand();
// Fold IndexCast(constant) -> constant // Fold IndexCast(constant) -> constant
// A little hack because we go through int. Otherwise, the size // A little hack because we go through int. Otherwise, the size
// of the constant might need to change. // of the constant might need to change.
if (auto value = cstOperands[0].dyn_cast_or_null<IntegerAttr>()) if (auto value = cstOperands[0].dyn_cast_or_null<IntegerAttr>())
▲ Show 20 Lines • Show All 1,443 Lines • Show Last 20 Lines

mlir/test/Dialect/Standard/invalid.mlir

  • This file was added.
// RUN: mlir-opt -split-input-file %s -verify-diagnostics
// CHECK-LABEL: test_index_cast_shape_error
func @test_index_cast_shape_error(%arg0 : tensor<index>) -> tensor<2xi64> {
// expected-error @+1 {{operand type 'tensor<index>' and result type 'tensor<2xi64>' are cast incompatible}}
%0 = index_cast %arg0 : tensor<index> to tensor<2xi64>
return %0 : tensor<2xi64>
}
// -----
// CHECK-LABEL: test_index_cast_tensor_error
func @test_index_cast_tensor_error(%arg0 : tensor<index>) -> i64 {
// expected-error @+1 {{operand type 'tensor<index>' and result type 'i64' are cast incompatible}}
%0 = index_cast %arg0 : tensor<index> to i64
return %0 : i64
}

mlir/test/Dialect/Standard/ops.mlir

  • This file was added.
// RUN: mlir-opt -split-input-file %s | mlir-opt | FileCheck %s
// Verify the printed output can be parsed.
jpienaarUnsubmitted
Not Done
ReplyInline Actions

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

jpienaar: What does split input do here given fed back into mlir-opt? (and also no file splits in here)
rsudermanAuthorUnsubmitted
Done
ReplyInline Actions

Extra from copy-paste of another test.

rsuderman: Extra from copy-paste of another test.
// RUN: mlir-opt %s | mlir-opt | FileCheck %s
// Verify the generic form can be parsed.
// RUN: mlir-opt -mlir-print-op-generic %s | mlir-opt | FileCheck %s
rriddleUnsubmitted
Not Done
ReplyInline Actions

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

rriddle: This looks more like a test for the core parser than for the Standard dialect. Not sure that we…
rsudermanAuthorUnsubmitted
Done
ReplyInline Actions

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?

rsuderman: My goal is to match the invalid tests and have correctly parsed tests pass through (both with…
rriddleUnsubmitted
Not Done
ReplyInline Actions

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.

rriddle: I meant the last RUN line. Parsing the generic input seems like it is testing the core parser…
// CHECK-LABEL: test_index_cast
func @test_index_cast(%arg0 : index) -> i64 {
%0 = index_cast %arg0 : index to i64
return %0 : i64
}
// CHECK-LABEL: test_index_cast_tensor
func @test_index_cast_tensor(%arg0 : tensor<index>) -> tensor<i64> {
%0 = index_cast %arg0 : tensor<index> to tensor<i64>
return %0 : tensor<i64>
}
// CHECK-LABEL: test_index_cast_tensor_reverse
func @test_index_cast_tensor_reverse(%arg0 : tensor<i64>) -> tensor<index> {
%0 = index_cast %arg0 : tensor<i64> to tensor<index>
return %0 : tensor<index>
}