Page MenuHomePhabricator

[mlir][Shape] Make sure tensor_cast(constant_shape) folding uses the correct type
ClosedPublic

Authored by bkramer on Dec 9 2020, 3:08 AM.

Details

Summary

This is still subtle, but I think the test cases are sufficient to show
that it works.

Diff Detail

Event Timeline

bkramer created this revision.Dec 9 2020, 3:08 AM
bkramer requested review of this revision.Dec 9 2020, 3:08 AM
herhut added a comment.Dec 9 2020, 7:10 AM

This seems to do the right thing, I just don't understand why.

mlir/lib/Dialect/Shape/IR/ShapeCanonicalization.td
38

I am somewhat puzzled what this means. It creates a new Shape_ConstShapeOp with the given $ty which is the value? Why is it called ty then? How does the type come into this? Is it somehow inherited from the original operation?

bkramer updated this revision to Diff 310525.Dec 9 2020, 7:13 AM
  • Rename argument from $ty to $arg
mlir/lib/Dialect/Shape/IR/ShapeCanonicalization.td
38

$ty is just me coming up with a weird name. Renamed it to be more consistent.

The type is inherited from the original.

silvas accepted this revision.Dec 9 2020, 11:19 AM
silvas added inline comments.
mlir/lib/Dialect/Shape/IR/ShapeCanonicalization.td
37

Maybe add a small comment here about how the types are preserved.

mlir/test/Dialect/Shape/canonicalize.mlir
897

I think both these test cases and be reduced. The select isn't needed (return/func are sufficiently picky about the type to be representative for this test) and the shape.const_shape that we don't fold is not needed after the select is removed.

906

random comment (unrelated to patch): I'm surprised that shape.const_shape [3] : tensor<1xindex> isn't rejected by the verifier. The ODS declaration has the return of Shape_ShapeOrExtentTensorType, which only allows !shape.shape or tensor<?xindex> (but seemingly not tensor<1xindex>).

This revision is now accepted and ready to land.Dec 9 2020, 11:19 AM
jurahul added inline comments.Dec 9 2020, 11:22 AM
mlir/test/Dialect/Shape/canonicalize.mlir
906

That's because:

def Shape_ExtentTensorType :
    1DTensorOf<[Index]>,

Shape_ExtentTensorType allows any 1D tensor of index elements.

silvas added inline comments.Dec 9 2020, 11:25 AM
mlir/test/Dialect/Shape/canonicalize.mlir
906

Oh, I think I found it. Shape_ExtentTensorType inherits from 1DTensorOf<[Index]> but is BuildableType that creates tensor<?xindex>.

https://github.com/llvm/llvm-project/blob/df282215d497e15104ae9e182e083cdfa0bae3c2/mlir/include/mlir/Dialect/Shape/IR/ShapeBase.td#L110

That's kind of sketchy because BuildableType is supposed to be for types that only have one representation, which doesn't seem to be the case for Shape_ExtentTensorType if it also allows tensor<Nxindex> along with tensor<?xindex>.

https://mlir.llvm.org/docs/OpDefinitions/#type-inference

Buildable Types
Some type constraints may only have one representation, allowing for them to be directly buildable; for example the I32 or Index types. Types in ODS may mark themselves as buildable by setting the builderCall field or inheriting from the BuildableType class.
This revision was landed with ongoing or failed builds.Dec 10 2020, 1:50 AM
This revision was automatically updated to reflect the committed changes.