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

[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. 

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:
       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