[Mesa-dev] [PATCH 12/59] i965: add brw_imm_df

[Samuel Iglesias Gonsálvez]

On 11/05/16 22:30, Francisco Jerez wrote:
> Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
> 
>> On 11/05/16 05:56, Francisco Jerez wrote:
>>> Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
>>>
>>>> From: Connor Abbott <connor.w.abbott at intel.com>
>>>>
>>>> v2 (Iago)
>>>>   - Fixup accessibility in backend_reg
>>>>
>>>> Signed-off-by: Iago Toral Quiroga <itoral at igalia.com>
>>>
>>> I've just noticed (while running valgrind) that this patch causes
>>> serious breakage in the back-end.  The reason is that the extra bits
>>> required to make room for the df field of the union don't get
>>> initialized in all codepaths, so backend_reg comparisons done using
>>> memcmp() can basically return random results now.  Can you please look
>>> into this?  Some ways to fix it would be to make sure we zero-initialize
>>> the whole brw_reg in all cases (or at least the union padding), or stop
>>> using memcmp() to compare registers -- I guess the latter might be
>>> somewhat less intrusive and increase the likelihood that we can get this
>>> sorted out timely.
>>>
>>
>> Attached is a patch for it, I initialized all union bits to zero before
>> setting them in brw_reg(). Can you test it? If it is not fixed, Would
>> you mind sending me an example to run it with valgrind here?
>>
> I'm afraid it's not fixed, I still see plenty of "Conditional jump or
> move depends on uninitialised value(s)" errors while running pretty much
> any piglit test on valgrind with the patch below applied.
> 

:-(

OK, I will fix it.

Thanks,

Sam

>> I am thinking that maybe we want to change backend_reg::equals() if this
>> doesn't work.
>>
>> Sam
>>
>>>> ---
>>>>  src/mesa/drivers/dri/i965/brw_reg.h    | 9 +++++++++
>>>>  src/mesa/drivers/dri/i965/brw_shader.h | 1 +
>>>>  2 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_reg.h b/src/mesa/drivers/dri/i965/brw_reg.h
>>>> index b84c709..6d51623 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_reg.h
>>>> +++ b/src/mesa/drivers/dri/i965/brw_reg.h
>>>> @@ -254,6 +254,7 @@ struct brw_reg {
>>>>           unsigned pad1:1;
>>>>        };
>>>>  
>>>> +      double df;
>>>>        float f;
>>>>        int   d;
>>>>        unsigned ud;
>>>> @@ -544,6 +545,14 @@ brw_imm_reg(enum brw_reg_type type)
>>>>  
>>>>  /** Construct float immediate register */
>>>>  static inline struct brw_reg
>>>> +brw_imm_df(double df)
>>>> +{
>>>> +   struct brw_reg imm = brw_imm_reg(BRW_REGISTER_TYPE_DF);
>>>> +   imm.df = df;
>>>> +   return imm;
>>>> +}
>>>> +
>>>> +static inline struct brw_reg
>>>>  brw_imm_f(float f)
>>>>  {
>>>>     struct brw_reg imm = brw_imm_reg(BRW_REGISTER_TYPE_F);
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_shader.h b/src/mesa/drivers/dri/i965/brw_shader.h
>>>> index fc228f6..f6f6167 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_shader.h
>>>> +++ b/src/mesa/drivers/dri/i965/brw_shader.h
>>>> @@ -90,6 +90,7 @@ struct backend_reg : private brw_reg
>>>>     using brw_reg::width;
>>>>     using brw_reg::hstride;
>>>>  
>>>> +   using brw_reg::df;
>>>>     using brw_reg::f;
>>>>     using brw_reg::d;
>>>>     using brw_reg::ud;
>>>> -- 
>>>> 2.5.0
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> From 35254624d63b77aa2024bc2b08612e28cae4bb98 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Samuel=20Iglesias=20Gons=C3=A1lvez?= <siglesias at igalia.com>
>> Date: Wed, 11 May 2016 07:44:10 +0200
>> Subject: [PATCH] i965: initialize struct brw_reg's union bits to zero.
>> MIME-Version: 1.0
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 8bit
>>
>> Extra bits required to make room for the df field of the union don't get
>> initialized in all codepaths, so backend_reg comparisons done using
>> memcmp() can basically return random results.
>>
>> Initialize them to zero before setting the rest of union's fields.
>>
>> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
>> Reported-by: Francisco Jerez <currojerez at riseup.net>
>> ---
>>  src/mesa/drivers/dri/i965/brw_reg.h | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_reg.h b/src/mesa/drivers/dri/i965/brw_reg.h
>> index 6d51623..3b76d7d 100644
>> --- a/src/mesa/drivers/dri/i965/brw_reg.h
>> +++ b/src/mesa/drivers/dri/i965/brw_reg.h
>> @@ -338,6 +338,9 @@ brw_reg(enum brw_reg_file file,
>>     reg.subnr = subnr * type_sz(type);
>>     reg.nr = nr;
>>  
>> +   /* Initialize all union's bits to zero before setting them. */
>> +   reg.df = 0;
>> +
>>     /* Could do better: If the reg is r5.3<0;1,0>, we probably want to
>>      * set swizzle and writemask to W, as the lower bits of subnr will
>>      * be lost when converted to align16.  This is probably too much to
>> -- 
>> 2.5.0