This is an archive of the discontinued LLVM Phabricator instance.

[mlir][tosa][fix] Add proper type checking trait for tosa mul

Authored by tatwaichong on Jul 11 2023, 10:34 AM.



when operating integer type tensors, tosa elementwise multiplication
requires the element type of result to be a 32-bit integer rather
than the same type as inputs.

Change-Id: Ifd3d7ebd879be5c6b2c8e23aa6d7ef41f39c6d41

Diff Detail

Event Timeline

tatwaichong created this revision.Jul 11 2023, 10:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2023, 10:34 AM
tatwaichong requested review of this revision.Jul 11 2023, 10:34 AM
jpienaar added inline comments.

So this is a Tensor of I32? E.g., could this just be Tosa_Int32Tensor and would this cover that?

This is a fix to response the mistake introduced in D150472.
Quote ""tosa.mul"(%0, %1) {shift = 0 : i32} : (tensor<4xi8>, tensor<4xi8>) -> tensor<4xi32> is marked as illegal because result and operand types don't match, even though i8xi8 = i32 is according to the spec".

I propose to create a custom for tosa mul to check if its operands and result meet the spec requirement. Do you think if this a good manner to cope with this demand?

tatwaichong added inline comments.Jul 11 2023, 10:42 AM

Smart idea, but the input could be floating type.

tatwaichong added inline comments.Jul 11 2023, 10:44 AM

Or Tosa_Int32OrFpTensor?

missing < in the expected output string of the test.

tatwaichong added inline comments.Jul 11 2023, 11:34 AM

but this way doesn't cover the type checking between operands and result of the operation. e.g. won't check if the operands and result are the same type when operating floating type. Is it right?

Thanks for working on this!
Some clarification from my side:

  • We should make sure that tosa.mul"(%0, %1) {shift = 0 : i32} : (tensor<4xi8>, tensor<4xi8>) -> tensor<4xi32> compiles (because its legal according to the standard).
  • We don't need to disallow tosa.mul"(%0, %1) {shift = 0 : i32} : (tensor<4xi16>, tensor<4xi16>) -> tensor<4xi16>; the LLVM implementation of TOSA is more relaxed than the standard in many ways,

and I think that freedom has been useful.

What do you think?

update the trait in a relexed way to allow the result type can be other size but not i32 as long as this is a valid size.

Yes. This demand is reasonable. We loosen the trait to accept those use cases.

mgehre-amd accepted this revision.Jul 14 2023, 12:18 AM


This revision is now accepted and ready to land.Jul 14 2023, 12:18 AM

Is there any remaining issues? could you land this for me?

Can you rebase this? There is a conflict and probably easiest for you to fix. Then I can land it.

rebase and reslove conflict

This revision was landed with ongoing or failed builds.Jul 21 2023, 4:40 PM
This revision was automatically updated to reflect the committed changes.