[Mesa-dev] [PATCH v2] i965/vec4: make offset() operate in terms of channels instead of full registers

[Iago Toral]

On Tue, 2016-08-23 at 11:21 +0200, Michael Schellenberger Costa wrote:
> Hi Iago,
> 
> given that the idea here was to unify vec4 and fs you might want to
> adopt the names/function types accordingly.
> 
> In brw_ir_fs.h there is byte_offset that returns a fs_reg while you
> have
> void add_byte_offset.

Yeah, the reason for that is that in the FS backend we only have
fs_reg, so byte_offset takes that as argument and returns an updated
copy of an fs_reg that we can return directly from the offset() helper.

In the vec4 backend we have to deal with src_reg and dst_reg but I was
trying not want to duplicate the logic for each (the switch statement).
I did not notice that the FS backend was using byte_offset() directly
in other places, so it might be a good idea to make a byte_offset()
helper in the vec4 backend that works in the same fashion. That means
that I'll move the switch statement to another helper that takes a
backend_reg as input parameter and call that from byte_offset.

Iago

> --Michael
> 
> Am 23.08.2016 um 10:24 schrieb Iago Toral Quiroga:
> > 
> > This will make it more consistent with the FS implementation of the
> > same
> > helper and will provide more flexibility that will come in handy,
> > for
> > example, when we add a SIMD lowering pass in the vec4 backend.
> > 
> > v2:
> >  - Move the switch statement to add_byte_offset (Iago)
> >  - Remove the assert on the register file, it is redundant with the
> > switch
> >    (Michael Schellenberger)
> >  - Fix use of '=' instead of '==' (Michael Schellenberger,
> >    Francesco Ansanelli)
> > ---
> >  src/mesa/drivers/dri/i965/brw_ir_vec4.h | 36
> > +++++++++++++++++++++++++--------
> >  1 file changed, 28 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_ir_vec4.h
> > b/src/mesa/drivers/dri/i965/brw_ir_vec4.h
> > index 81b6a13..d55b522 100644
> > --- a/src/mesa/drivers/dri/i965/brw_ir_vec4.h
> > +++ b/src/mesa/drivers/dri/i965/brw_ir_vec4.h
> > @@ -60,12 +60,33 @@ retype(src_reg reg, enum brw_reg_type type)
> >     return reg;
> >  }
> >  
> > +static inline void
> > +add_byte_offset(backend_reg *reg, unsigned delta)
> > +{
> > +   switch (reg->file) {
> > +   case BAD_FILE:
> > +      break;
> > +   case MRF:
> > +   case VGRF:
> > +   case ATTR:
> > +   case UNIFORM: {
> > +      const unsigned suboffset = reg->subreg_offset + delta;
> > +      reg->reg_offset += suboffset / REG_SIZE;
> > +      reg->subreg_offset += suboffset % REG_SIZE;
> > +      /* Align16 requires that register accesses are 16-byte
> > aligned */
> > +      assert(reg->subreg_offset % 16 == 0);
> > +      break;
> > +   }
> > +   default:
> > +      assert(delta == 0);
> > +   }
> > +}
> > +
> >  static inline src_reg
> > -offset(src_reg reg, unsigned delta)
> > +offset(src_reg reg, unsigned width, unsigned delta)
> >  {
> > -   assert(delta == 0 ||
> > -          (reg.file != ARF && reg.file != FIXED_GRF && reg.file !=
> > IMM));
> > -   reg.reg_offset += delta;
> > +   unsigned byte_offset = delta * width * type_sz(reg.type);
> > +   add_byte_offset(&reg, byte_offset);
> >     return reg;
> >  }
> >  
> > @@ -130,11 +151,10 @@ retype(dst_reg reg, enum brw_reg_type type)
> >  }
> >  
> >  static inline dst_reg
> > -offset(dst_reg reg, unsigned delta)
> > +offset(dst_reg reg, unsigned width, unsigned delta)
> >  {
> > -   assert(delta == 0 ||
> > -          (reg.file != ARF && reg.file != FIXED_GRF && reg.file !=
> > IMM));
> > -   reg.reg_offset += delta;
> > +   unsigned byte_offset = delta * width * type_sz(reg.type);
> > +   add_byte_offset(&reg, byte_offset);
> >     return reg;
> >  }
> >  
> >