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

[Kenneth Graunke]

On Friday, May 13, 2016 5:58:25 PM PDT Francisco Jerez wrote:
> Kenneth Graunke <kenneth at whitecape.org> writes:
> 
> > 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) &&
> 
> These casts should be redundant now, if the upcast is safe the compiler
> will be able to figure it out for you.  With that cleaned up:
> 
> Reviewed-by: Francisco Jerez <currojerez at riseup.net>

Right, they're not necessary.  Dropped locally, thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160513/f6530727/attachment.sig>