[Mesa-dev,2/2] tgsi: handle bitwise opcodes in tgsi_opcode_infer_type

Submitted by Marek Olšák on March 2, 2015, 11:52 a.m.

Details

Message ID 1425297146-25938-2-git-send-email-maraeo@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Marek Olšák March 2, 2015, 11:52 a.m.
From: Marek Olšák <marek.olsak@amd.com>

---
 src/gallium/auxiliary/tgsi/tgsi_info.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/gallium/auxiliary/tgsi/tgsi_info.c b/src/gallium/auxiliary/tgsi/tgsi_info.c
index e6e0a60..258173d 100644
--- a/src/gallium/auxiliary/tgsi/tgsi_info.c
+++ b/src/gallium/auxiliary/tgsi/tgsi_info.c
@@ -341,6 +341,8 @@  tgsi_opcode_infer_type( uint opcode )
    case TGSI_OPCODE_USNE:
    case TGSI_OPCODE_SVIEWINFO:
    case TGSI_OPCODE_UMUL_HI:
+   case TGSI_OPCODE_UBFE:
+   case TGSI_OPCODE_UMSB:
       return TGSI_TYPE_UNSIGNED;
    case TGSI_OPCODE_ARL:
    case TGSI_OPCODE_ARR:
@@ -362,6 +364,12 @@  tgsi_opcode_infer_type( uint opcode )
    case TGSI_OPCODE_IABS:
    case TGSI_OPCODE_ISSG:
    case TGSI_OPCODE_IMUL_HI:
+   case TGSI_OPCODE_POPC:
+   case TGSI_OPCODE_IBFE:
+   case TGSI_OPCODE_BFI:
+   case TGSI_OPCODE_BREV:
+   case TGSI_OPCODE_LSB:
+   case TGSI_OPCODE_IMSB:
       return TGSI_TYPE_SIGNED;
    default:
       return TGSI_TYPE_FLOAT;

Comments

Am 02.03.2015 um 12:52 schrieb Marek Olšák:
> From: Marek Olšák <marek.olsak@amd.com>
> 
> ---
>  src/gallium/auxiliary/tgsi/tgsi_info.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/gallium/auxiliary/tgsi/tgsi_info.c b/src/gallium/auxiliary/tgsi/tgsi_info.c
> index e6e0a60..258173d 100644
> --- a/src/gallium/auxiliary/tgsi/tgsi_info.c
> +++ b/src/gallium/auxiliary/tgsi/tgsi_info.c
> @@ -341,6 +341,8 @@ tgsi_opcode_infer_type( uint opcode )
>     case TGSI_OPCODE_USNE:
>     case TGSI_OPCODE_SVIEWINFO:
>     case TGSI_OPCODE_UMUL_HI:
> +   case TGSI_OPCODE_UBFE:
> +   case TGSI_OPCODE_UMSB:
>        return TGSI_TYPE_UNSIGNED;
>     case TGSI_OPCODE_ARL:
>     case TGSI_OPCODE_ARR:
> @@ -362,6 +364,12 @@ tgsi_opcode_infer_type( uint opcode )
>     case TGSI_OPCODE_IABS:
>     case TGSI_OPCODE_ISSG:
>     case TGSI_OPCODE_IMUL_HI:
> +   case TGSI_OPCODE_POPC:
> +   case TGSI_OPCODE_IBFE:
> +   case TGSI_OPCODE_BFI:
> +   case TGSI_OPCODE_BREV:
> +   case TGSI_OPCODE_LSB:
> +   case TGSI_OPCODE_IMSB:
>        return TGSI_TYPE_SIGNED;
>     default:
>        return TGSI_TYPE_FLOAT;
> 

This doesn't look all that consistent to me, in particular it doesn't
match what tgsi_exec.c thinks these arguments are wrt signed/unsigned.
Granted it probably doesn't make much of a difference, but it should be
fixed imho - though not sure if sticking to what tgsi_exec.c thinks
these arguments are (sometimes with different result and src type) is
always better.

Roland
On Mon, Mar 2, 2015 at 5:12 PM, Roland Scheidegger <sroland@vmware.com> wrote:
> Am 02.03.2015 um 12:52 schrieb Marek Olšák:
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> ---
>>  src/gallium/auxiliary/tgsi/tgsi_info.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_info.c b/src/gallium/auxiliary/tgsi/tgsi_info.c
>> index e6e0a60..258173d 100644
>> --- a/src/gallium/auxiliary/tgsi/tgsi_info.c
>> +++ b/src/gallium/auxiliary/tgsi/tgsi_info.c
>> @@ -341,6 +341,8 @@ tgsi_opcode_infer_type( uint opcode )
>>     case TGSI_OPCODE_USNE:
>>     case TGSI_OPCODE_SVIEWINFO:
>>     case TGSI_OPCODE_UMUL_HI:
>> +   case TGSI_OPCODE_UBFE:
>> +   case TGSI_OPCODE_UMSB:
>>        return TGSI_TYPE_UNSIGNED;
>>     case TGSI_OPCODE_ARL:
>>     case TGSI_OPCODE_ARR:
>> @@ -362,6 +364,12 @@ tgsi_opcode_infer_type( uint opcode )
>>     case TGSI_OPCODE_IABS:
>>     case TGSI_OPCODE_ISSG:
>>     case TGSI_OPCODE_IMUL_HI:
>> +   case TGSI_OPCODE_POPC:
>> +   case TGSI_OPCODE_IBFE:
>> +   case TGSI_OPCODE_BFI:
>> +   case TGSI_OPCODE_BREV:
>> +   case TGSI_OPCODE_LSB:
>> +   case TGSI_OPCODE_IMSB:
>>        return TGSI_TYPE_SIGNED;
>>     default:
>>        return TGSI_TYPE_FLOAT;
>>
>
> This doesn't look all that consistent to me, in particular it doesn't
> match what tgsi_exec.c thinks these arguments are wrt signed/unsigned.
> Granted it probably doesn't make much of a difference, but it should be
> fixed imho - though not sure if sticking to what tgsi_exec.c thinks
> these arguments are (sometimes with different result and src type) is
> always better.

So how should it look like then? Note that the main purpose of this
code is for gallivm to choose between f32 and i32 correctly to avoid
type failures. LLVM doesn't care about the signedness and it looks
like gallivm doesn't care either. At least I can't find code in
gallivm that behaves differently for SIGNED vs UNSIGNED.

Marek
Am 02.03.2015 um 20:38 schrieb Marek Olšák:
> On Mon, Mar 2, 2015 at 5:12 PM, Roland Scheidegger <sroland@vmware.com> wrote:
>> Am 02.03.2015 um 12:52 schrieb Marek Olšák:
>>> From: Marek Olšák <marek.olsak@amd.com>
>>>
>>> ---
>>>  src/gallium/auxiliary/tgsi/tgsi_info.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_info.c b/src/gallium/auxiliary/tgsi/tgsi_info.c
>>> index e6e0a60..258173d 100644
>>> --- a/src/gallium/auxiliary/tgsi/tgsi_info.c
>>> +++ b/src/gallium/auxiliary/tgsi/tgsi_info.c
>>> @@ -341,6 +341,8 @@ tgsi_opcode_infer_type( uint opcode )
>>>     case TGSI_OPCODE_USNE:
>>>     case TGSI_OPCODE_SVIEWINFO:
>>>     case TGSI_OPCODE_UMUL_HI:
>>> +   case TGSI_OPCODE_UBFE:
>>> +   case TGSI_OPCODE_UMSB:
>>>        return TGSI_TYPE_UNSIGNED;
>>>     case TGSI_OPCODE_ARL:
>>>     case TGSI_OPCODE_ARR:
>>> @@ -362,6 +364,12 @@ tgsi_opcode_infer_type( uint opcode )
>>>     case TGSI_OPCODE_IABS:
>>>     case TGSI_OPCODE_ISSG:
>>>     case TGSI_OPCODE_IMUL_HI:
>>> +   case TGSI_OPCODE_POPC:
>>> +   case TGSI_OPCODE_IBFE:
>>> +   case TGSI_OPCODE_BFI:
>>> +   case TGSI_OPCODE_BREV:
>>> +   case TGSI_OPCODE_LSB:
>>> +   case TGSI_OPCODE_IMSB:
>>>        return TGSI_TYPE_SIGNED;
>>>     default:
>>>        return TGSI_TYPE_FLOAT;
>>>
>>
>> This doesn't look all that consistent to me, in particular it doesn't
>> match what tgsi_exec.c thinks these arguments are wrt signed/unsigned.
>> Granted it probably doesn't make much of a difference, but it should be
>> fixed imho - though not sure if sticking to what tgsi_exec.c thinks
>> these arguments are (sometimes with different result and src type) is
>> always better.
> 
> So how should it look like then? Note that the main purpose of this
> code is for gallivm to choose between f32 and i32 correctly to avoid
> type failures. LLVM doesn't care about the signedness and it looks
> like gallivm doesn't care either. At least I can't find code in
> gallivm that behaves differently for SIGNED vs UNSIGNED.
> 

Yes, even tgsi_exec.c doesn't really care much. Maybe this is a somewhat
artificial distinction. Honestly I'm not quite sure why for some of the
opcodes signed or unsigned was chosen, Ilja wrote it so he might know.
The gallium docs of course just refer to sm5 opcodes, which don't
distinguish this at all. For some of the opcodes I think it might make
sense to stick with glsl - e.g. the dst type of findLSB/findMSB (which
translates to lsb/umsb/imsb) is always signed regardless if the input
was signed or unsigned. But for most operations glsl just has two
definitions for signed and unsigned types which are just identical, I
don't know which one should be chosen. But I'd prefer if whatever we
specify in tgsi_exec.c matches what we say about it in tgsi_info.c, no
matter which way...
I can't remember what the reason to distinguish signed from unsigned
there actually was, might simplify some things if we didn't (for
gallivm, this changes the build context used for fetching arguments, but
all we use it for is to bitcast it to the corresponding llvm type, which
of course doesn't distinguish between signed or unsigned).

Roland
On Mon, Mar 2, 2015 at 4:48 PM, Roland Scheidegger <sroland@vmware.com> wrote:
> Maybe this is a somewhat
> artificial distinction. Honestly I'm not quite sure why for some of the
> opcodes signed or unsigned was chosen, Ilja wrote it so he might know.

I picked whatever made sense for the return value range... not a whole
lot of thought went into it though tbh. Happy to elaborate on my
thought process (if any!) for any specific choice.

  -ilia
We're just talking about unused and useless things here. I think that
TGSI_TYPE_UNSIGNED should be removed and TGSI_TYPE_SIGNED should be
renamed to TGSI_TYPE_INT. Until then, this type distiction serves no
purpose.

I'll gladly move all those opcodes under TGSI_TYPE_SIGNED, but please
don't try to convince me that TGSI_TYPE_UNSIGNED is better for some of
them, because in practice there is no difference in behavior.

Marek

On Mon, Mar 2, 2015 at 10:48 PM, Roland Scheidegger <sroland@vmware.com> wrote:
> Am 02.03.2015 um 20:38 schrieb Marek Olšák:
>> On Mon, Mar 2, 2015 at 5:12 PM, Roland Scheidegger <sroland@vmware.com> wrote:
>>> Am 02.03.2015 um 12:52 schrieb Marek Olšák:
>>>> From: Marek Olšák <marek.olsak@amd.com>
>>>>
>>>> ---
>>>>  src/gallium/auxiliary/tgsi/tgsi_info.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_info.c b/src/gallium/auxiliary/tgsi/tgsi_info.c
>>>> index e6e0a60..258173d 100644
>>>> --- a/src/gallium/auxiliary/tgsi/tgsi_info.c
>>>> +++ b/src/gallium/auxiliary/tgsi/tgsi_info.c
>>>> @@ -341,6 +341,8 @@ tgsi_opcode_infer_type( uint opcode )
>>>>     case TGSI_OPCODE_USNE:
>>>>     case TGSI_OPCODE_SVIEWINFO:
>>>>     case TGSI_OPCODE_UMUL_HI:
>>>> +   case TGSI_OPCODE_UBFE:
>>>> +   case TGSI_OPCODE_UMSB:
>>>>        return TGSI_TYPE_UNSIGNED;
>>>>     case TGSI_OPCODE_ARL:
>>>>     case TGSI_OPCODE_ARR:
>>>> @@ -362,6 +364,12 @@ tgsi_opcode_infer_type( uint opcode )
>>>>     case TGSI_OPCODE_IABS:
>>>>     case TGSI_OPCODE_ISSG:
>>>>     case TGSI_OPCODE_IMUL_HI:
>>>> +   case TGSI_OPCODE_POPC:
>>>> +   case TGSI_OPCODE_IBFE:
>>>> +   case TGSI_OPCODE_BFI:
>>>> +   case TGSI_OPCODE_BREV:
>>>> +   case TGSI_OPCODE_LSB:
>>>> +   case TGSI_OPCODE_IMSB:
>>>>        return TGSI_TYPE_SIGNED;
>>>>     default:
>>>>        return TGSI_TYPE_FLOAT;
>>>>
>>>
>>> This doesn't look all that consistent to me, in particular it doesn't
>>> match what tgsi_exec.c thinks these arguments are wrt signed/unsigned.
>>> Granted it probably doesn't make much of a difference, but it should be
>>> fixed imho - though not sure if sticking to what tgsi_exec.c thinks
>>> these arguments are (sometimes with different result and src type) is
>>> always better.
>>
>> So how should it look like then? Note that the main purpose of this
>> code is for gallivm to choose between f32 and i32 correctly to avoid
>> type failures. LLVM doesn't care about the signedness and it looks
>> like gallivm doesn't care either. At least I can't find code in
>> gallivm that behaves differently for SIGNED vs UNSIGNED.
>>
>
> Yes, even tgsi_exec.c doesn't really care much. Maybe this is a somewhat
> artificial distinction. Honestly I'm not quite sure why for some of the
> opcodes signed or unsigned was chosen, Ilja wrote it so he might know.
> The gallium docs of course just refer to sm5 opcodes, which don't
> distinguish this at all. For some of the opcodes I think it might make
> sense to stick with glsl - e.g. the dst type of findLSB/findMSB (which
> translates to lsb/umsb/imsb) is always signed regardless if the input
> was signed or unsigned. But for most operations glsl just has two
> definitions for signed and unsigned types which are just identical, I
> don't know which one should be chosen. But I'd prefer if whatever we
> specify in tgsi_exec.c matches what we say about it in tgsi_info.c, no
> matter which way...
> I can't remember what the reason to distinguish signed from unsigned
> there actually was, might simplify some things if we didn't (for
> gallivm, this changes the build context used for fetching arguments, but
> all we use it for is to bitcast it to the corresponding llvm type, which
> of course doesn't distinguish between signed or unsigned).
>
> Roland
>
>