Why does TOSA use signless integer types?

MLIR
1

Hi!
There is something confusing about the element types used by TOSA ops.
According to the spec these are the supported types:

image
image697×409 53.4 KB

So int is either signed int or unsigned int.
In TOSA implementation, we have:

def Tosa_Int8 : I<8>;
def Tosa_Int16 : I<16>;
def Tosa_Int32 : I<32>;
def Tosa_Int48 : I<48>;
def Tosa_Int64 : I<64>;

def Tosa_SignedInt : AnyTypeOf<[Tosa_Int8,
                                Tosa_Int16,
                                Tosa_Int32,
                                Tosa_Int48,
                                Tosa_Int64]>;

Here we actually have signless integer because we use:

// Signless integer type of a specific width.
class I<int width>

I would expect it to use:

// Signed integer type of a specific width.
class SI<int width>

Can someone elaborate on the decision to use signless types? It will make things clearer for me.

Thanks in advance,
Maya

1 Like
2

I don’t know if this is going to make it clearer, but when we first created the TOSA dialect, we used signless by mistake. At the time, the TOSA specification was less clear about the types used, so it was easier to miss. We’ve improved the specification so that it defines the types clearly, but we have not changed the dialect.

I think in practice the situation is not too bad, as the majority of the TOSA integer operators only work on signed values, so the op does carry the signedness interpretation. It’s on my todo list to find a better way to document this, and make sure we update the dialect to be more clear on this subject.

Thanks,
Eric

3

Thanks for your answer :slight_smile:
Does it mean TOSA will use signed integer type at some point?

4

Hi again :slight_smile:
I encountered this issue again, since I am using si8 in my IR, and I use tosa ops → verification fails.
Do you mind if I add si8 to the types tosa ops accepts? Or even replace i8 with si8?
Thanks

5

Hi!
Sorry for the delay. I’d prefer not to add/replace i8 with si8, as I believe that would break any consumer of the TOSA dialect, as they may not be able to accept signed ints.
Instead, could you insert UnrealizedConversionCast when creating the TOSA ops? The TOSA ops other than RESIZE are noted in the spec as only supporting (signed) int8. The dialect doesn’t make this as clear as it could. It comes down to a discussion of whether TOSA->(linalg/etc.) is the proper place to do the signed->signless conversion, or whether it should be done frontend->TOSA. Your proposal would take us to the second option, which isn’t unreasonable, but is a bigger change from where we are right now.

This overall topic has been discussed (without a complete resolution from what I can tell) before: [RFC] SignednessCastOp - #15 by gcmn. That thread touches on both MHLO and TOSA, which both have somewhat similar issues. If the compiler is eventually heading down through Linalg or other layers that expect signless, someone needs to do the cast, it is a matter of what point that happens. TOSA is right on the border of where I’d expect the conversion to occur.

Is your design eventually going to the existing backends, or do you have your own backend in mind? If your own, does it expect signed or signless?

Thanks,
Eric

6

Hi!
Thanks for your answer.

I’d prefer not to add/replace i8 with si8, as I believe that would break any consumer of the TOSA dialect, as they may not be able to accept signed ints.

I see what you mean, but this explains why not to replace i8 with si8. By adding si8 we’re not supposed to break anything, just like ui8 was added ⚙ D108427 [mlir][tosa] Support UInt8 inputs and outputs for tosa.rescale.

It comes down to a discussion of whether TOSA->(linalg/etc.) is the proper place to do the signed->signless conversion, or whether it should be done frontend->TOSA. Your proposal would take us to the second option

I didn’t mean TOSA will be signless, so I don’t see why my proposal will do the signed->signless conversion in frontend->TOSA. I support keeping the signedness in TOSA, but doing it as defined in MLIR - ui8 for unsigned and si8 for signed. Currently in TOSA it is ui8 and i8.

If I understand it right, the i8 in TOSA should have been si8 but is i8 by mistake. I assume I can use the UnrealizedConversionCast, but I am wondering if and why this overhead is really required. Assuming it’s really a mistake, I think it’s better to fix it, without having to carry it with us.
It seems that now we use i8 with multiple meanings, for TOSA dialect it means signed int, but for feeding ops from other dialects it is considered signless.
My proposal is:

  • Use ui8 and si8 in TOSA dialect. This way it will fully match the spec where there is unsigned/signed int. And it will use the types with there natural meaning as defined by MLIR.
  • Use UnrealizedConversionCast for TOSA consumers that do not support unsigned/signed int.

Is your design eventually going to the existing backends, or do you have your own backend in mind? If your own, does it expect signed or signless?

My code will eventually run on custom hardware, using llvm compiler.

We can also set an online meeting if you think it will make it easier to discuss.
And of course I will be glad to implement/help with any change I am proposing.

Thanks a lot,
Maya

7

“My code will eventually run on custom hardware, using llvm compiler.”

If it using the llvm compiler, why not use the same method llvm uses?
llvm has just i8, it is not signed or unsigned.
llvm using the operations done on the i8 to determine its type.
Can TOSA do the same, use the operations to determine the type?
e.g. LLVM instruction:

CreateSDiv
CreateUDiv
8

The only thing that would be required to make the TOSA dialect work completely with i8 is to add a new dialect operator for RESCALE that is for unsigned values. The existing RESCALE would be defined to work on signed values only. At that point, the dialect would be consistent, with the signedness carried in the op. Producers/consumers of RESCALE would have to adjust to the new ops, but that would be a minimal change. (Unless they are relying on u8 somewhere not aligned with the spec, but that should be corrected regardless of what we do) This is still consistent with the specification, as the specification ops and dialect would note the proper types.

If we change the TOSA dialect to use ui8 and si8, then producers/consumers of TOSA would all need to be switched. The frontends I know about are TensorFlow(+Lite), torch-mlir, and onnx-mlir. Consumers of the dialect are TosaToLinalg and TosaToTensor in tree. I suspect that there are additional consumers at other companies (like you will have) that aren’t public. I haven’t done any scoping to see the amount of work required, but it would be significant. I’ll tag @rsuderman, he’s more familiar with the TosaToLinalg side than I am, and may have a better sense of the scale.

Thanks,
Eric

9

So there technically is not issue with migrating integer types from i8 to si8 in the cases where we have signed integers vs signless integers, and likely this is something that should be done in the long term. The primary issue with making that migration is that there are a substantial number of lowerings that assume signless vs unsigned integer operations. This includes TosaToLinalg but also includes work in other repositories (e.g. Tensorflow and IREE).

One complexity is that tensorflow-lite’s MLIR dialect assumes signless integers when dealing with signed operations, which means that additional work would be needed int he tflite-to-tosa conversion to convert all signless integers to signed, while preserving unsigned types.

For IREE we would need to modify the sign-stripping required to lower to it’s code generation backend.

Overall it should not be an overwhelming refactor, however the cross repo changes required makes it difficult to land smoothly.

If we wanted to start on the migration, adding support for SI types to the list of types should not cause any meaningful issues. Then we could start validating TosaToLinalg behaves correctly on signed types (and begin similar migration in TensorFlow and IREE).

10

Thank you all!
I uploaded a patch: ⚙ D146380 [mlir] Add signed int to TOSA supported types

11

Are we actually starting this migration to have signed types and makes the dialect uniform on this? I would be concerned to go in a direction where we are still inconsistent, but in a different way.

12

I agree with Mehdi that this should be a migration, with the goal to remove signless types from the dialect interface. Having all of the signed/unsigned/signless types as a transition towards signed types is fine, but it should be temporary, and we should try to make sure users of the dialect are aware of the need to handle signed and should drop support for signless.

I can help with some of the TF changes, but I don’t have direct commit access to that repo, and I can’t speak for the IREE team on that transition. I agree that the code changes themselves shouldn’t be overwhelming.

I should be able to help on the torch-mlir side.

I think a new thread here will help, with a title that describes the change of the dialect interface to signed. This thread may not get the attention as it is posed as a question rather than notifying people of a proposal.

13

We would be happy to help in onnx-mlir for the ONNX to TOSA conversion. @eric-k Are you going to announce when this change lands in upstream MLIR?

14

Thanks for the help. Before we land the current change, I think a thread explicitly noting the proposal, and its affects on existing TOSA dialect users, along with the benefits/drawbacks of the solution. It would also need a target point in time to remove signless to allow for planning.

At the point we remove signless, it has the potential to break out of tree users, which I would like to be very careful about doing.