[Mesa-dev] [PATCH] Gallium/tgsi: Correct signdness of return value of bit operations

[Roland Scheidegger]

Am 09.08.2018 um 19:09 schrieb Gert Wollny:
> Am Donnerstag, den 09.08.2018, 18:52 +0200 schrieb Roland Scheidegger:
>> Am 09.08.2018 um 18:18 schrieb Gert Wollny:
>>> Am Donnerstag, den 09.08.2018, 17:10 +0200 schrieb Roland
>>> Scheidegger:
>>>> This is incorrect for umsb.
>>>
>>> Hmm, according to the TGSI doc all of those operations including
>>> UMSB are supposed to return -1 if no bits are set [1], for me that
>>> implies that their return type should be signed. 
>>>
>>> [1] https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2
>>> Fgallium.readthedocs.io%2Fen%2Flatest%2Ftgsi.html%23opcode-
>>> UMSB&data=02%7C01%7Csroland%40vmware.com%7C7dabc2002d7c4ece269d
>>> 08d5fe13cba8%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636694283
>>> 339615256&sdata=j5BGumW2g%2Bj8NtLAxyy6ZFCoCDsbfMqgrkjtWtIlQBQ%3
>>> D&reserved=0
>>
>> Yes, I guess that's why glsl has them defined as signed.
>> But you could just as well say the definition is they return unsigned
>> 0xffffffff (tgsi is really more like d3d10 asm there, so as you know
>> the register file is untyped, and d3d10 says to return 0xffffffff for
>> such things, not saying what type this is at all).
>> tgsi doesn't really directly have to mirror glsl opcodes, and
>> certainly not in cases where this amounts to just cosmetic
>> differences.
> Well, it's not that cosmetic when you look at virglrenderer where TGSI
> gets translated back to GLSL. Obviously there one can also force a
> certain return type in other was,, and this is what I initally
> proposed, but Dave asked whether this could also be done via the
> infer_type mechanism, so I did this and to limit the amount the
> virglrenderer/gallium and the mesa/gallium diverge, I also proposed
> this here too. (I added Dave directly to the loop in case he wants to
> add something).  
> 
>> And personally, I prefer them to all be unsigned, because bitops on
>> signed is just always looking crazy. 
> I can understand this, but in the case of the return value I don't
> really see declaring it as signed would be a bad thing.
Yes, as I said I can live with it. If it helps something alright.



> 
> A different approch would then look more or less like this: 
> 
> --- a/src/gallium/auxiliary/tgsi/tgsi_info.c
> +++ b/src/gallium/auxiliary/tgsi/tgsi_info.c
> @@ -150,9 +150,6 @@ tgsi_opcode_infer_type(enum tgsi_opcode opcode)
>     case TGSI_OPCODE_UBFE:
>     case TGSI_OPCODE_BFI:
>     case TGSI_OPCODE_BREV:
> -   case TGSI_OPCODE_POPC:
> -   case TGSI_OPCODE_LSB:
> -   case TGSI_OPCODE_UMSB:
>     case TGSI_OPCODE_IMG2HND:
>     case TGSI_OPCODE_SAMP2HND:
>        return TGSI_TYPE_UNSIGNED;
> @@ -274,6 +271,7 @@ tgsi_opcode_infer_src_type(enum tgsi_opcode opcode,
> uint src_idx)
>     case TGSI_OPCODE_I2F:
>     case TGSI_OPCODE_I2D:
>     case TGSI_OPCODE_I2I64:
> +   case TGSI_OPCODE_UMSB:
You probably wanted to add that to the unsigned section...

Roland



>        return TGSI_TYPE_SIGNED;
>     case TGSI_OPCODE_ARL:
>     case TGSI_OPCODE_ARR:
> @@ -324,5 +322,12 @@ tgsi_opcode_infer_dst_type(enum tgsi_opcode
> opcode, uint dst_idx)
>     if (dst_idx == 1 && opcode == TGSI_OPCODE_DFRACEXP)
>        return TGSI_TYPE_SIGNED;
>  
> -   return tgsi_opcode_infer_type(opcode);
> +   switch (opcode) {
> +   case TGSI_OPCODE_LSB:
> +   case TGSI_OPCODE_POPC:
> +   case TGSI_OPCODE_UMSB:
> +      return TGSI_TYPE_SIGNED;
> +   default:
> +      return tgsi_opcode_infer_type(opcode);
> +   }
>  }
> 
> Best, 
> Gert
>