Page MenuHomePhabricator

[MLIR][Shape] Allow unsafe `shape.broadcast`
ClosedPublic

Authored by frgossen on Jul 30 2020, 4:38 AM.

Details

Summary

In a context in which shape.broadcast is known not to produce an error value,
we want it to operate solely on extent tensors. The operation's behavior is
then undefined in the error case as the result type cannot hold this value.

Diff Detail

Event Timeline

frgossen created this revision.Jul 30 2020, 4:38 AM
Herald added a reviewer: silvas. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
frgossen requested review of this revision.Jul 30 2020, 4:38 AM
herhut added inline comments.Jul 30 2020, 5:21 AM
mlir/test/Dialect/Shape/canonicalize.mlir
69

I would disallow this case. You can only have a tensor result of both inputs are tensors. Otherwise, there is no guarantee that the inputs will not be an error.

frgossen added inline comments.Jul 30 2020, 7:40 AM
mlir/test/Dialect/Shape/canonicalize.mlir
69

If I understand the semantics correctly, even if both inputs are of type tensor<?xindex> the result can be an error: [1,2], [3,4] -> error
That is ofc looking locally at this operation only.
We can disallow this case but we will still have undefined behaviour in some cases.

herhut added inline comments.Jul 30 2020, 9:09 AM
mlir/test/Dialect/Shape/canonicalize.mlir
69

Yes, you can only have a tensor result if both inputs are tensors but having tensor inputs is not sufficient to mandate a tensor output. Still, we should disallow cases that are statically wrong.

frgossen updated this revision to Diff 282156.Jul 31 2020, 1:45 AM
frgossen marked 2 inline comments as done.

Address comment

herhut accepted this revision.Jul 31 2020, 1:51 AM

Thanks. With one more test this is good to land!

mlir/test/Dialect/Shape/canonicalize.mlir
65

Could you add the tensor, tensor -> shape case as well? Just to make sure it stays supported.

frgossen updated this revision to Diff 282212.Jul 31 2020, 6:26 AM

Rebase, add test

frgossen marked an inline comment as done.Jul 31 2020, 6:26 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 31 2020, 7:18 AM
This revision was automatically updated to reflect the committed changes.