This is an archive of the discontinued LLVM Phabricator instance.

[mlir][tosa] Fix tosa average_pool2d to linalg type issue
ClosedPublic

Authored by rsuderman on Oct 11 2021, 5:24 PM.

Details

Summary

Average pool assumed the same input/output type. Result type for integers
is always an i32, should be updated appropriately.

Diff Detail

Event Timeline

rsuderman created this revision.Oct 11 2021, 5:24 PM
rsuderman requested review of this revision.Oct 11 2021, 5:24 PM

If the output should always be i32, could you add verification to the op for this? That would have avoided this error.

Added tosa.avg_pool2d verifier.

GMNGeoffrey added inline comments.Oct 12 2021, 12:28 PM
mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
72 ↗(On Diff #379144)

So this is limited to f32, i8, or i32? It would be nice to express those constraints with type constraints instead of custom verification.

mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
355–360 ↗(On Diff #379144)

Please add failure messages with emitOpError(). Longer term, it would be better to do this with return type inference.

Added op error.

rsuderman marked 2 inline comments as done.Oct 12 2021, 12:58 PM
rsuderman added inline comments.
mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
72 ↗(On Diff #379144)

I agree we should update this restriction, but that should be done as a separate pass with fixing tests. It is surprisingly common to find tests that use slightly incorrect types.

GMNGeoffrey accepted this revision.Oct 12 2021, 12:58 PM
GMNGeoffrey added inline comments.
mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
362–364 ↗(On Diff #379151)

This returns LogicalResult so you can combine them.

This revision is now accepted and ready to land.Oct 12 2021, 12:58 PM
rsuderman marked an inline comment as done.

Changed return statement.

GMNGeoffrey added inline comments.Oct 12 2021, 12:59 PM
mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
72 ↗(On Diff #379144)

Yeah upsettingly common :-/ That's why I push hard for verifiers

rsuderman updated this revision to Diff 379161.Oct 12 2021, 1:09 PM

Rebase on head.

This revision was landed with ongoing or failed builds.Oct 12 2021, 1:11 PM
This revision was automatically updated to reflect the committed changes.