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

[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%2Fgallium.readthedocs.io%2Fen%2Flatest%2Ftgsi.html%23opcode-UMSB&data=02%7C01%7Csroland%40vmware.com%7C7dabc2002d7c4ece269d08d5fe13cba8%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636694283339615256&sdata=j5BGumW2g%2Bj8NtLAxyy6ZFCoCDsbfMqgrkjtWtIlQBQ%3D&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.
And personally, I prefer them to all be unsigned, because bitops on
signed is just always looking crazy.
Although ignoring the type that way is really something which only works
easily because the type width is always the same, even if it's really
doing the same thing anyway.

> 
>> If you really want to move so the dst type is signed, you need to add
>> it to infer_src_type so the src args are unsigned.
> I see the control flow now, so I guess I actually should add these
> three cases to infer_dst_type, and also add the proper src type for
> umsb.
> 
>> FWIW I'd like to think that all 3 of these always operate on unsigned
>> data (even if they might return signed data), imho they are a lot
>> more easier to understand that way (I mean the signed msb you have to
>> look up what the hell it actually does, bit operations on unsigned
>> data are just easier to understand). But of course it doesn't really
>> make a difference, so I can live with the other 2 if the src type
>> indicates signed.
> GLSL bitCount and findLSB are both operating on both types, and I
> assume that these are actually translated to these two TGSI
> instructions, so the src type should not be set,
Well that doesn't really matter, if you don't set them they will be
implicitly be the same type as the instruction type.

Roland



> 
> thanks for the pointers,
> Gert 
> 
>>
>> Roland
>>
>>
>> Am 09.08.2018 um 10:43 schrieb Gert Wollny:
>>> The GLSL functions  findLSB, findMSB, and countBits always return
>>> a signed integer type. Let the according TGSI operations reflect
>>> this.
>>>
>>> Signed-off-by: Gert Wollny <gert.wollny at collabora.com>
>>> ---
>>>  src/gallium/auxiliary/tgsi/tgsi_info.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_info.c
>>> b/src/gallium/auxiliary/tgsi/tgsi_info.c
>>> index bbe1a21e43..a8c2bbe663 100644
>>> --- 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;
>>> @@ -188,6 +185,9 @@ tgsi_opcode_infer_type(enum tgsi_opcode opcode)
>>>     case TGSI_OPCODE_U64SGE:
>>>     case TGSI_OPCODE_I64SLT:
>>>     case TGSI_OPCODE_I64SGE:
>>> +   case TGSI_OPCODE_LSB:
>>> +   case TGSI_OPCODE_POPC:
>>> +   case TGSI_OPCODE_UMSB:
>>>        return TGSI_TYPE_SIGNED;
>>>     case TGSI_OPCODE_DADD:
>>>     case TGSI_OPCODE_DABS:
>>>
>>
>>