[Mesa-dev] [PATCH 18/59] i965: fix brw_saturate_immediate() for doubles

[Samuel Iglesias Gonsálvez]

On 02/05/16 09:56, Iago Toral wrote:
> On Mon, 2016-05-02 at 10:54 +0300, Pohjolainen, Topi wrote:
>> On Mon, May 02, 2016 at 09:42:14AM +0200, Iago Toral wrote:
>>> On Mon, 2016-05-02 at 10:34 +0300, Pohjolainen, Topi wrote:
>>>> On Mon, May 02, 2016 at 09:22:49AM +0200, Iago Toral wrote:
>>>>> On Mon, 2016-05-02 at 10:08 +0300, Pohjolainen, Topi wrote:
>>>>>> On Mon, May 02, 2016 at 10:02:50AM +0300, Pohjolainen, Topi wrote:
>>>>>>> On Fri, Apr 29, 2016 at 01:29:15PM +0200, Samuel Iglesias Gons?lvez wrote:
>>>>>>>> From: Iago Toral Quiroga <itoral at igalia.com>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>  src/mesa/drivers/dri/i965/brw_shader.cpp | 28 ++++++++++++++++++++++------
>>>>>>>>  1 file changed, 22 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
>>>>>>>> index d40937b..a063b88 100644
>>>>>>>> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
>>>>>>>> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
>>>>>>>> @@ -476,7 +476,14 @@ brw_saturate_immediate(enum brw_reg_type type, struct brw_reg *reg)
>>>>>>>>        unsigned ud;
>>>>>>>>        int d;
>>>>>>>>        float f;
>>>>>>>> -   } imm = { reg->ud }, sat_imm = { 0 };
>>>>>>>> +      double df;
>>>>>>>> +   } imm, sat_imm = { 0 };
>>>>>>>> +
>>>>>>>> +   unsigned size = type_sz(type);
>>>>>>>
>>>>>>> Could be 'const'.
>>>>>>>
>>>>>>>> +   if (size < 8)
>>>>>>
>>>>>> Thinking a little further, is there a reason we don't write directly:
>>>>>>
>>>>>>         if (type == BRW_REGISTER_TYPE_DF)
>>>>>>            imm.df = reg->df;
>>>>>>         else
>>>>>>            imm.ud = reg->ud;
>>>>>
>>>>> Because in the future we might want to support 64-bit integers too
>>>>> (BRW_REGISTER_TYPE_Q I think?) and then we would need to change this
>>>>> code again. The original implementation would do the right thing in that
>>>>> case without changes.
>>>>
>>>> I was thinking about this as this idiom works elsewhere just fine. But here
>>>> wouldn't 64-bit integers have size >= 8 and therefore use:
>>>>
>>>>                imm.df = reg->df;
>>>>
>>>> Or is this the intention as it copies 64-bits regardless of the type?
>>>
>>> Exactly, that's what we want to do here. Basically, we want to either do
>>> a 32-bit or 64-bit data copy, the type is otherwise irrelevant and doing
>>> it this way makes it so that we don't have to patch the code again in
>>> the future if we support 64-bit integers. Also, it is consistent with
>>> the way in which we were already handling 32-bit types where we used ud
>>> for all of them.
>>
>> Ok. I would personally appreciate a comment clarifying this but as the
>> current logic doesn't have one either, I'll leave it to you to add some text
>> if you feel the same way.
> 
> Sure, an extra comment won't hurt :)
> 
> Iago
> 

As Iago said, we will add an extra comment here.

Thanks,

Sam