Page MenuHomePhabricator

[mlir] Don't allow dynamic extent tensor types for ConstShapeOp.
ClosedPublic

Authored by akuegel on Oct 5 2021, 5:40 AM.

Details

Summary

ConstShapeOp has a constant shape, so its type can always be static.
We still allow it to have ShapeType though.

Diff Detail

Event Timeline

akuegel created this revision.Oct 5 2021, 5:40 AM
akuegel requested review of this revision.Oct 5 2021, 5:40 AM

I agree, dynamically shaped constants seem counter-intuitive. They may be needed for folding ops w/o changing their result type though.

Sean, Frederik told me that due to folding, we may want to still allow dynamic extent tensors, and that you initially suggested such a pattern. Therefore asking for your opinion on this change :)
With this change, I was able to find a lowering from Tensorflow dialect to HLO dialect where we did create dynamic extent tensors although it would have been possible to create static extent tensors. And also the bug fixed in https://reviews.llvm.org/D111127 would not have happened if we don't allow dynamic extent tensors for ConstantShapeOp.

I think it's fine for const_shape to always be static and require a tensor.cast to erase the static shape to dynamic.

Thanks Sean, then I guess we are in agreement that this change might make sense. I guess we can give this change a try, and if it turns out that we actually still better want to allow dynamic type for const_shape, I can revert it?
Anyone wants to approve?

This revision is now accepted and ready to land.Oct 7 2021, 1:38 AM