[Mesa-dev] [PATCH] i965: Fix undefined df bits in brw_reg comparisons.

[Samuel Iglesias Gonsálvez]

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

With Curro's comment addressed,

Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>

On 14/05/16 02:44, Kenneth Graunke wrote:
> Commit 5310bca024f77da40ea6f4c275455f9cb0528f9e added a new "double
> df" field to the brw_reg struct, adding an extra 4 bytes of data
> that isn't usually initialized (or may contain irrelevant garbage
> if the struct is mutated).  This means that it's no longer safe to
> memcmp().
> 
> Instead, add a brw_regs_equal() function which ignores the extra df
> bits unless they matter.  To keep the implementation cheap, we wrap
> the first set of fields in a union/struct so that we can use a
> single DWord comparison.
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org> --- 
> src/mesa/drivers/dri/i965/brw_fs_generator.cpp   |  2 +- 
> src/mesa/drivers/dri/i965/brw_reg.h              | 27
> +++++++++++++++++------- src/mesa/drivers/dri/i965/brw_shader.cpp
> |  2 +- src/mesa/drivers/dri/i965/brw_vec4_generator.cpp |  2 +- 4
> files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp index
> 4f6f3a3..3b50a82 100644 ---
> a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp +++
> b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp @@ -1010,7 +1010,7
> @@ fs_generator::generate_tex(fs_inst *inst, struct brw_reg dst,
> struct brw_reg src brw_set_default_mask_control(p,
> BRW_MASK_DISABLE); brw_set_default_access_mode(p, BRW_ALIGN_1);
> 
> -      if (memcmp(&surface_reg, &sampler_reg, sizeof(surface_reg))
> == 0) { +      if (brw_regs_equal(&surface_reg, &sampler_reg)) { 
> brw_MUL(p, addr, sampler_reg, brw_imm_uw(0x101)); } else { 
> brw_SHL(p, addr, sampler_reg, brw_imm_ud(8)); diff --git
> a/src/mesa/drivers/dri/i965/brw_reg.h
> b/src/mesa/drivers/dri/i965/brw_reg.h index 6d51623..71e1024
> 100644 --- a/src/mesa/drivers/dri/i965/brw_reg.h +++
> b/src/mesa/drivers/dri/i965/brw_reg.h @@ -234,14 +234,19 @@
> uint32_t brw_swizzle_immediate(enum brw_reg_type type, uint32_t x,
> unsigned swz) * or "structure of array" form: */ struct brw_reg { -
> enum brw_reg_type type:4; -   enum brw_reg_file file:3;      /* :2
> hardware format */ -   unsigned negate:1;             /* source
> only */ -   unsigned abs:1;                /* source only */ -
> unsigned address_mode:1;       /* relative addressing, hopefully!
> */ -   unsigned pad0:1; -   unsigned subnr:5;              /* :1 in
> align16 */ -   unsigned nr:16; +   union { +      struct { +
> enum brw_reg_type type:4; +         enum brw_reg_file file:3;
> /* :2 hardware format */ +         unsigned negate:1;
> /* source only */ +         unsigned abs:1;                /*
> source only */ +         unsigned address_mode:1;       /* relative
> addressing, hopefully! */ +         unsigned pad0:1; +
> unsigned subnr:5;              /* :1 in align16 */ +
> unsigned nr:16; +      }; +      uint32_t bits; +   };
> 
> union { struct { @@ -261,6 +266,12 @@ struct brw_reg { }; };
> 
> +static inline bool +brw_regs_equal(const struct brw_reg *a, const
> struct brw_reg *b) +{ +   const bool df = a->type ==
> BRW_REGISTER_TYPE_DF && a->file == IMM; +   return a->bits ==
> b->bits && (df ? a->df == b->df : a->ud == b->ud); +}
> 
> struct brw_indirect { unsigned addr_subnr:4; diff --git
> a/src/mesa/drivers/dri/i965/brw_shader.cpp
> b/src/mesa/drivers/dri/i965/brw_shader.cpp index a23f14e..8d9e309
> 100644 --- a/src/mesa/drivers/dri/i965/brw_shader.cpp +++
> b/src/mesa/drivers/dri/i965/brw_shader.cpp @@ -687,7 +687,7 @@
> 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
> && +   return brw_regs_equal((brw_reg *)this, (brw_reg *)&r) && 
> reg_offset == r.reg_offset; }
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp index
> 4b44c3a..baf4422 100644 ---
> a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp +++
> b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp @@ -295,7 +295,7
> @@ generate_tex(struct brw_codegen *p, 
> brw_set_default_mask_control(p, BRW_MASK_DISABLE); 
> brw_set_default_access_mode(p, BRW_ALIGN_1);
> 
> -      if (memcmp(&surface_reg, &sampler_reg, sizeof(surface_reg))
> == 0) { +      if (brw_regs_equal(&surface_reg, &sampler_reg)) { 
> brw_MUL(p, addr, sampler_reg, brw_imm_uw(0x101)); } else { 
> brw_SHL(p, addr, sampler_reg, brw_imm_ud(8));
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJXNs3JAAoJEH/0ujLxfcNDbNoP/A1sDKrs2iHW7rN/pCT59Qvy
xe4ZoPaQU++gUzQbizOvrdKaibIj5SgwY6Cs9gvWoOy8FsOjRrQs2ptnCKyzfEog
TDrwQ/4CDY6Kc0NykVQgxJHmw/363XHqWo3FF6mpVyl7MZHyA9ffBxzFO1OSEhx8
uwpY6mt0NfDtBh/R4Pju5UAV7WT9VnIYWh4Te7M098EsEgf6iqg2I833ct1FbNWu
LJYu6g46cIN3Mig4Bak5H495Ws4phP+vBcIPKe+wcWSS/p3bGG0OOk3fcm3fDsD4
h7pE8sYBh/t5TInAMFfjAm9SSnXHmxe1zqkvh5XwD8WQPIAx9E9HnzccGDAs35o6
gU3O43DAkYqXm53oOJi5qOWeisllxQcrXB13/9itp6J6S9y4FUPyVJ4ra8yMvfwR
d8iD4DFZYxUQKgiFND59rES6xkyh1t4XbxW+Y2RMWc86Da8hnt9XhaOhx0McXT6f
VMrX7R1gfxXlGr+z97lp4am9t/F//sDhjupxfut+tf1nSJ9dOrVufbtX+kCMU1EO
BMwG/qWwWKduE0SVTnmYixFU3CUSR9mvvt00LdER9/KGNcgcGcC1ZPcdfz1laqD9
NVF8KBG6X2ELHppAcRpmsINfX8Rjn4JIqIUFeHnbyWXYuHDW31mq2YduMoslU7S0
DBAPVrqvEL2S5zhrGrI7
=nx5e
-----END PGP SIGNATURE-----