Nouveau - Jan 2015 - [PATCH] nv50/ir: Handle OP_CVT when folding constant expressions

On Sun, Jan 11, 2015 at 4:40 PM, Tobias Klausmann
<tobias.johannes.klausmann at mni.thm.de> wrote:
> Folding for conversions: F32->(U{16/32}, S{16/32}) and (U{16/32},
{S16/32})->F32
>
> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann at
mni.thm.de>
> ---
> V2: Split out F64 parts
> V3: remove handling of saturate for (U/S)32,
>
>  .../drivers/nouveau/codegen/nv50_ir_peephole.cpp   | 73
++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> index 21d20ca..aaf0d0d 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> @@ -997,6 +997,79 @@ ConstantFolding::opnd(Instruction *i, ImmediateValue
&imm0, int s)
>        i->op = OP_MOV;
>        break;
>     }
> +   case OP_CVT: {
> +      Storage res;
> +      bld.setPosition(i, true); /* make sure bld is init'ed */
> +      switch(i->dType) {
> +      case TYPE_U16:
> +         switch (i->sType) {
> +         case TYPE_F32:
> +            if (i->saturate)
> +               res.data.u16 = util_iround(CLAMP(imm0.reg.data.f32, 0,
> +                                                UINT16_MAX));
> +            else
> +              res.data.u16 = util_iround(imm0.reg.data.f32);
> +            break;
> +         default:
> +            return;
> +         }
This won't get hit for the U32 -> U16 conversion though right? Did you
test that case? Am I misreading/misunderstanding perhaps?

  -ilia

On 11.01.2015 22:54, Ilia Mirkin wrote:
> On Sun, Jan 11, 2015 at 4:40 PM, Tobias Klausmann
> <tobias.johannes.klausmann at mni.thm.de> wrote:
>> Folding for conversions: F32->(U{16/32}, S{16/32}) and (U{16/32},
{S16/32})->F32
>>
>> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann at
mni.thm.de>
>> ---
>> V2: Split out F64 parts
>> V3: remove handling of saturate for (U/S)32,
>>
>>   .../drivers/nouveau/codegen/nv50_ir_peephole.cpp   | 73
++++++++++++++++++++++
>>   1 file changed, 73 insertions(+)
>>
>> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>> index 21d20ca..aaf0d0d 100644
>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>> @@ -997,6 +997,79 @@ ConstantFolding::opnd(Instruction *i,
ImmediateValue &imm0, int s)
>>         i->op = OP_MOV;
>>         break;
>>      }
>> +   case OP_CVT: {
>> +      Storage res;
>> +      bld.setPosition(i, true); /* make sure bld is init'ed */
>> +      switch(i->dType) {
>> +      case TYPE_U16:
>> +         switch (i->sType) {
>> +         case TYPE_F32:
>> +            if (i->saturate)
>> +               res.data.u16 = util_iround(CLAMP(imm0.reg.data.f32, 0,
>> +                                                UINT16_MAX));
>> +            else
>> +              res.data.u16 = util_iround(imm0.reg.data.f32);
>> +            break;
>> +         default:
>> +            return;
>> +         }
> This won't get hit for the U32 -> U16 conversion though right? Did
you
> test that case? Am I misreading/misunderstanding perhaps?
A complete piglit run did not hit i->saturate for U32 or S32. That said, 
i kept the assert() there on purpose for now to actually make sure we 
are no hitting such a case. Do i misread you now? :)

On Sun, Jan 11, 2015 at 5:08 PM, Tobias Klausmann
<tobias.johannes.klausmann at mni.thm.de> wrote:
>
>
> On 11.01.2015 22:54, Ilia Mirkin wrote:
>>
>> On Sun, Jan 11, 2015 at 4:40 PM, Tobias Klausmann
>> <tobias.johannes.klausmann at mni.thm.de> wrote:
>>>
>>> Folding for conversions: F32->(U{16/32}, S{16/32}) and
(U{16/32},
>>> {S16/32})->F32
>>>
>>> Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann at
mni.thm.de>
>>> ---
>>> V2: Split out F64 parts
>>> V3: remove handling of saturate for (U/S)32,
>>>
>>>   .../drivers/nouveau/codegen/nv50_ir_peephole.cpp   | 73
>>> ++++++++++++++++++++++
>>>   1 file changed, 73 insertions(+)
>>>
>>> diff --git
a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>>> b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>>> index 21d20ca..aaf0d0d 100644
>>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>>> @@ -997,6 +997,79 @@ ConstantFolding::opnd(Instruction *i,
ImmediateValue
>>> &imm0, int s)
>>>         i->op = OP_MOV;
>>>         break;
>>>      }
>>> +   case OP_CVT: {
>>> +      Storage res;
>>> +      bld.setPosition(i, true); /* make sure bld is init'ed */
>>> +      switch(i->dType) {
>>> +      case TYPE_U16:
>>> +         switch (i->sType) {
>>> +         case TYPE_F32:
>>> +            if (i->saturate)
>>> +               res.data.u16 = util_iround(CLAMP(imm0.reg.data.f32,
0,
>>> +                                                UINT16_MAX));
>>> +            else
>>> +              res.data.u16 = util_iround(imm0.reg.data.f32);
>>> +            break;
>>> +         default:
>>> +            return;
>>> +         }
>>
>> This won't get hit for the U32 -> U16 conversion though right?
Did you
>> test that case? Am I misreading/misunderstanding perhaps?
>
> A complete piglit run did not hit i->saturate for U32 or S32. That said,
i
> kept the assert() there on purpose for now to actually make sure we are no
> hitting such a case. Do i misread you now? :)
>From my read of the code, we'd hit that case now with TXF on a
2D_ARRAY with a constant as the array element. i.e. a piglit with

uniform sampler2DArray foo;
texelFetch(foo, ivec3(1, 2, 3));

  -ilia