This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Shape] Canonicalize casted dynamic extent tensor
ClosedPublic

Authored by frgossen on Mar 23 2021, 2:29 AM.

Diff Detail

Event Timeline

frgossen created this revision.Mar 23 2021, 2:29 AM
frgossen requested review of this revision.Mar 23 2021, 2:29 AM
tpopp added inline comments.Mar 23 2021, 7:36 AM
mlir/lib/Dialect/Shape/IR/Shape.cpp
1001

Can we generalize this? Like can we also canonicalize

%0 = shape.shape_of %arg : tensor<?x?x?xf32> -> tensor<?xindex>
%1 = tensor.cast %0 : tensor<?xindex> to tensor<3xindex>

I would assume it can work both ways. The only case I'm not sure is safe is with unranked tensors like.

%0 = shape.shape_of %arg : tensor<*xf32> -> tensor<?xindex>
%1 = tensor.cast %0 : tensor<?xindex> to tensor<3xindex>
herhut accepted this revision.Mar 23 2021, 7:57 AM
herhut added a reviewer: jpienaar.

This moves us a little further in the direction of having more precise types in the shape dialect in the non-error bearing case. I think this is the right direction to go but also want to give Jacques a chance to comment.

mlir/lib/Dialect/Shape/IR/Shape.cpp
949–950

This change seems unrelated. Can you add a motivation to the diff description?

951

nit: Move into conditional.

This revision is now accepted and ready to land.Mar 23 2021, 7:57 AM
herhut added inline comments.Mar 23 2021, 7:59 AM
mlir/lib/Dialect/Shape/IR/Shape.cpp
1001

Good point, yes, the former would also be a valid canonicalization. The latter is not, unless we give shape_of asserting behavior wrt. its return type, which tensor.cast has. I think we should not and rather use the cast in this case.

This is independent of the fact that asserting behavior is not implemented for tensor.cast I think.

frgossen updated this revision to Diff 332694.Mar 23 2021, 9:14 AM

Address comments

frgossen updated this revision to Diff 332699.Mar 23 2021, 9:27 AM
frgossen marked 2 inline comments as done.

Split CL

frgossen marked 2 inline comments as done.Mar 23 2021, 9:28 AM

Thanks!

mlir/lib/Dialect/Shape/IR/Shape.cpp
949–950

Right, it does not really belong here. Will make a separate CL

frgossen updated this revision to Diff 333818.Mar 29 2021, 4:44 AM
frgossen marked an inline comment as done.

Address comments

This revision was landed with ongoing or failed builds.Mar 29 2021, 4:59 AM
This revision was automatically updated to reflect the committed changes.