This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Shape] Generalize `shape.const_shape` to extent tensors
ClosedPublic

Authored by frgossen on Jul 20 2020, 5:30 AM.

Details

Summary

The operation shape.const_shape was used for constants of type shape only.
We can now also use it to create constant extent tensors.

Depends On D84156

Diff Detail

Event Timeline

frgossen created this revision.Jul 20 2020, 5:30 AM
Herald added a reviewer: silvas. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

Shouldn't shape.const_shape always return a tensor? It cannot return an error currently, right?

pifon2a accepted this revision.Jul 20 2020, 6:02 AM

Not at the moment but I think eventually it should also be possible to create constant error shapes.
We can split const_shape into const_shape and const_error later if we want that.
If the return type of const_shape was constrained to an extent tensor at this point then we'd have to update all the operations before (because it's all over in the tests).

frgossen updated this revision to Diff 279257.Jul 20 2020, 8:17 AM

Add test case

What do you think about

%1 = shape.const_shape [1, 2, 3]
%2 = shape.const_extent_tensor [4, 5, 6]

instead of

%1 = shape.const_shape [1, 2, 3] : !shape.shape
%2 = shape.const_shape [4, 5, 6] : tensor<?xindex>

?

That seems more succinct.

Also, the right thing here, given our current rationale, is to always return an extent tensor from this op since it cannot be an error. Are you planning on doing that migration as well? It seems that this patch puts us in an awkward intermediate state.

stellaraccident added inline comments.
mlir/test/Dialect/Shape/ops.mlir
84

What is the rationale for having a shape-specific constant op for this? Since tensor<?xindex> is a standard type, [std.]constant is the right way to be constructing constants for it, no?

frgossen marked an inline comment as done.Jul 21 2020, 2:37 AM

This is indeed an intermediate state.

We would like to update all operations so that they accept extent tensors. At that point, the operation const_shape could be split in two operations: const_shape for extent tensors and const_error.

Eventually, we would like to replace const_shape with std.constant. However, that requires type refinement as we can only create statically shaped extent tensors this way. When fed into a return operation, e.g., that causes problems otherwise.

mlir/test/Dialect/Shape/ops.mlir
84

That is what we wanted but it turns out that std.constant cannot create dynamically shaped tensor constants like tensor<?xindex>. The example here, shape.const_shape [1, 2, 3], would always be of type tensor<3xindex> which is problematic when consumed by, e.g., a return operation. Also casting is not really an option for constant folding.

In the long run, we would still like to use std.constant for this. However, this required type refinement. Maybe we can discuss this in our meeting today.

frgossen updated this revision to Diff 279502.Jul 21 2020, 6:38 AM

Make helper static

frgossen marked an inline comment as done.Jul 21 2020, 6:39 AM
jpienaar accepted this revision.Jul 21 2020, 9:21 AM
This revision is now accepted and ready to land.Jul 21 2020, 9:21 AM
herhut accepted this revision.Jul 22 2020, 12:32 AM
This revision was automatically updated to reflect the committed changes.