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

If this information is useful, please help other people find it:
Share via:

Ilia Mirkin

2015-Jan-11 22:12 UTC

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

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 a2D_ARRAY with a constant as the array element. i.e. a piglit with

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

  -ilia

Tobias Klausmann

2015-Jan-11 22:48 UTC

head link

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

On 11.01.2015 23:12, Ilia Mirkin wrote:> 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));Tested this (hope i did the right thing) and the assert did not get 
triggered, but i am still uncertain of this.
-> move the assert into the F32 case for U32/S32 just to make sure...
switch (i->sType)
case TYPE_F32:
    assert(...)
...

other than that, we are not even going to fold U32 -> U16 ;-)

Greetings,
Tobias

Ilia Mirkin

2015-Jan-11 22:53 UTC

head link

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

On Sun, Jan 11, 2015 at 5:48 PM, Tobias Klausmann
<tobias.johannes.klausmann at mni.thm.de> wrote:>
>
> On 11.01.2015 23:12, Ilia Mirkin wrote:
>>
>> 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));
>
> Tested this (hope i did the right thing) and the assert did not get
> triggered, but i am still uncertain of this.
> -> move the assert into the F32 case for U32/S32 just to make sure...
> switch (i->sType)
> case TYPE_F32:
>    assert(...)
> ...
>
> other than that, we are not even going to fold U32 -> U16 ;-)
Right, and that's the problem. Try it with a piglit that has the code
I suggest... if you don't end up collapsing it, include the TGSI
that's generated (and also the shader test source) and we'll go from
there.

  -ilia

Maybe Matching Threads

Search for more seemingly similar threads

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

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

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

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

Maybe Matching Threads