[Mesa-dev] [PATCH 2/2] i965: check each field separately in backend_end::equals()

[Samuel Iglesias Gonsálvez]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 13/05/16 05:38, Francisco Jerez wrote:
> Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
> 
>> 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. Check field by field to avoid this.
>> 
>> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com> 
>> Reported-by: Francisco Jerez <currojerez at riseup.net>
> 
> This seemed to avoid the problem on at least the few tests I ran 
> manually under valgrind, but I have doubts that it is a complete
> fix, grepping for 'memcmp' in the back-end gives the following uses
> with a brw_reg as argument that will likely still give
> non-deterministic results even with this patch applied:
> 
> brw_fs_generator.cpp:1009:      if (memcmp(&surface_reg,
> &sampler_reg, sizeof(surface_reg)) == 0) { 
> brw_vec4_generator.cpp:298:      if (memcmp(&surface_reg,
> &sampler_reg, sizeof(surface_reg)) == 0) {
> 
> Might be a good idea to factor out the manual comparison into a
> static function in brw_reg.h you could use in the FS and VEC4
> generators to avoid duplicating the complex expression?  Or maybe
> find out why PATCH 1 is not sufficient to fix the issue?  Most
> likely something else other than brw_reg() is attempting to
> initialize brw_reg structs manually?
> 

OK. I am looking into it today.

Thanks for the suggestions and comments.

Sam

> One more comment below.
> 
>> --- src/mesa/drivers/dri/i965/brw_reg.h      |  1 + 
>> src/mesa/drivers/dri/i965/brw_shader.cpp | 21
>> +++++++++++++++++++-- 2 files changed, 20 insertions(+), 2
>> deletions(-)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/brw_reg.h
>> b/src/mesa/drivers/dri/i965/brw_reg.h index 3b76d7d..68128bd
>> 100644 --- a/src/mesa/drivers/dri/i965/brw_reg.h +++
>> b/src/mesa/drivers/dri/i965/brw_reg.h @@ -233,6 +233,7 @@
>> uint32_t brw_swizzle_immediate(enum brw_reg_type type, uint32_t
>> x, unsigned swz) * WM programs to implement shaders decomposed
>> into "channel serial" * or "structure of array" form: */ +/*
>> IMPORTANT: update backend_reg::equals() if you add a new field
>> here. */ struct brw_reg { enum brw_reg_type type:4; enum
>> brw_reg_file file:3;      /* :2 hardware format */ diff --git
>> a/src/mesa/drivers/dri/i965/brw_shader.cpp
>> b/src/mesa/drivers/dri/i965/brw_shader.cpp index a23f14e..7871f51
>> 100644 --- a/src/mesa/drivers/dri/i965/brw_shader.cpp +++
>> b/src/mesa/drivers/dri/i965/brw_shader.cpp @@ -687,8 +687,25 @@
>> backend_shader::backend_shader(const struct brw_compiler
>> *compiler, bool backend_reg::equals(const backend_reg &r) const 
>> { -   return memcmp((brw_reg *)this, (brw_reg *)&r,
>> sizeof(brw_reg)) == 0 && -          reg_offset == r.reg_offset; +
>> bool equal = true;
> 
> No need for this variable, you can just return false early.
> 

OK.

>> + +   if (this->reg_offset != r.reg_offset || +       this->type
>> != r.type || +       this->file != r.file || +       this->negate
>> != r.negate || +       this->abs != r.abs || +
>> this->address_mode != r.address_mode || +       this->pad0 !=
>> r.pad0 || +       this->subnr != r.subnr || +       this->nr !=
>> r.nr || +       this->ud != r.ud) +      equal = false; + +   /*
>> Check higher 32 bits of the brw_reg's union if they are filled
>> */ +   if (equal && this->type == BRW_REGISTER_TYPE_DF &&
>> this->df != r.df) +      equal = false; + +   return equal; }
>> 
>> bool -- 2.5.0
>> 
>> _______________________________________________ mesa-dev mailing
>> list mesa-dev at lists.freedesktop.org 
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJXNWJRAAoJEH/0ujLxfcND0lsQAMQQiEQ9zeo06iPPE+xHNHdY
MpZLE+4+pWsBS5Q0PnQPZ9rJMOJQmNI55aA/htUH4tOmShyg8q3tSI4PIZzdSrrS
0XjQCMZI7O43Sca/aBG633TFrqxDBEr/2JKpjXXm//ZRvnJ8pTfVtro/ams/pAed
jOC1gNs/qfIVlHnQMZWOAViKLSt9oG0jNsBnsvp3VAlnveq/RaHxApPGhHKosbbi
x6WjmNI5UihEiyJqOuaCVzIejeKJFOBKEORPybi6YLRwjwvbg7PVYDdSWixuVsLF
vz1X2VCQRUw/4uck9uqhzFk37afkftU8RamjYyXLfhOZ2F+kc1+QVFlf5ptUvXiz
cmUZGIwvLHz9y1USNzLuIiIhHbGV+Qb2oMeo2pkK/teHsIeDJbfEaCTrDeGbLQmH
qdboQtRR1XwMKcDm8qBvrqG7aQ9N0cLMQH4Cfk1mApOZovNkdEa7CjYkBGDvshUt
VWLjq0v2lYUsgKtHYB4r8GXxdlSEXhK312VrzFoDOIwVfVahQupDOKCNEcLZj2jC
ngLyoFsCPg1YnyFLXnAlgcvbyqKAUev6kSZHogSaeojQJZJlnfJMqZi2IMAUvlt+
GQSjsMzHXh/kKQkVIreMQNaBDWaXgIfbzDBiNHcYVvrNti94zt/p8r3J7DU+McHw
+NhNlvEn/rd177sJRtPB
=VZah
-----END PGP SIGNATURE-----