This is an archive of the discontinued LLVM Phabricator instance.

[mlir][tosa] Shape inference for a few remaining easy cases:
ClosedPublic

Authored by rsuderman on Jul 12 2021, 2:26 PM.

Details

Summary

Handles shape inference for identity, cast, and rescale. These were missed
during the initialy elementwise work. This includes resize shape propagation
which includes both attribute and input type based propagation.

Diff Detail

Event Timeline

rsuderman created this revision.Jul 12 2021, 2:26 PM
rsuderman requested review of this revision.Jul 12 2021, 2:26 PM

I don't know the ops that well, I could verify these if interested, but was going to rely on Suraj else :)

mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
699

Could we use ShapedType::kDynamic here and below?

728

fpMode makes me think floating point mode, and then I get confused why it is triggered by strideInt. Could you add a comment here?

Looking at the op, does it always have stride and stride_fp? And is only one of them set at a time? Why not have both be optional and then verify that exactly one is set?

736

So this is ceiling of size + 1 if sizeFp was already int sized? Would any of llvm/Support/MathExtras.h work here?

I don't know the ops that well, I could verify these if interested, but was going to rely on Suraj else :)

Suraj is on sabbatical, so won't be looking at this. (At least he shouldn't be :) )

There's a problem with adding shape inference to the resize operator. With the integer version, you could end up with an off by one error in the output size when trying to calculate it from the integer shift/stride values. I think this is the only spot in the specification that explicitly requires the output shape to be specified. We can try to look and see if there is something we can change in the spec to make sure the inferred shape works.

Identity/Cast/Rescale all seem fine.

rsuderman updated this revision to Diff 359903.Jul 19 2021, 1:50 PM

Added kDynamicSize use instead of explicit -1 with comment
about fpmode.

rsuderman marked 3 inline comments as done.Jul 19 2021, 1:54 PM

@eric-k256 We should see about removing that ambiguity to the spec. Its pretty valuable to determine output shapes entirely based on attributes / input shapes whenever possible. Specifying output_shape on construction helps with this but for something like resize, we should be able to find a better solution.

mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
728

I think the specification should be updated to this. At this point TosaOps.td specifies both by values and branches on set value. I agree it would be worth changing.

736

I couldn't find any that seemed useful. The expectation is to round up, even in cases where the value is an exact integer. Floating point can be a bit ambiguous but hopefully this is not depended on too much (we should depend more on output_shape anyway).

rsuderman marked 2 inline comments as done.Jul 26 2021, 10:28 AM
rsuderman edited reviewers, added: mehdi_amini; removed: hanchung, scotttodd.Jul 29 2021, 3:01 PM
jpienaar accepted this revision.Aug 3 2021, 4:22 PM

Looks good thanks, I wonder if we could in a way fuzz this with the reference implementation

mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
1566

You can use an OpTraitList to create a convenient "alias" for this.

This revision is now accepted and ready to land.Aug 3 2021, 4:22 PM

Looks good thanks, I wonder if we could in a way fuzz this with the reference implementation

It should be possible to strip all types excluding the surrounding contract then perform shape propagation. In theory it should produce the same output. The main problem is converting TOSA flatbuffers to sample cases, recent changes in the flatbuffer broke my test generator.