[Mesa-dev] [PATCH 27/59] i965/fs: add a stride helper

[Iago Toral]

On Wed, 2016-05-04 at 09:50 +0200, Iago Toral wrote:
> On Mon, 2016-05-02 at 18:48 -0700, Francisco Jerez wrote:
> > Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
> > 
> > > From: Connor Abbott <connor.w.abbott at intel.com>
> > >
> > > Similar to retype() and offset().
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_ir_fs.h | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_ir_fs.h b/src/mesa/drivers/dri/i965/brw_ir_fs.h
> > > index e4f20f4..abda2c3 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_ir_fs.h
> > > +++ b/src/mesa/drivers/dri/i965/brw_ir_fs.h
> > > @@ -78,6 +78,14 @@ retype(fs_reg reg, enum brw_reg_type type)
> > >  }
> > >  
> > >  static inline fs_reg
> > > +stride(fs_reg reg, unsigned stride)
> > > +{
> > > +   if (reg.stride != 0)
> > > +      reg.stride = stride;
> > > +   return reg;
> > > +}
> > > +
> > 
> > This only works if reg.stride == 0 or 1, we need to honour the stride of
> > the original register (e.g. by doing reg.stride *= stride) or you'll end
> > up taking components not part of the region given as argument.  It gets
> > messy with ARF and HW registers because they represent the stride
> > differently...  I suggest that instead of fixing this you take the
> > following patch from my SIMD32 branch that introduces a somewhat easier
> > to use helper: Instead of doing 'stride(horiz_offset(retype(reg, t), i),
> > type_sz(reg.type) / type_sz(t))' to take the i-th subcomponent of type t
> > of the original register you would just do 'subscript(reg, t, i)'. 
> 
> Actually, I think this is not what we want in a number of places.
> 
> Sometimes we just want to apply a stride to a register, no type changes
> or anything. That is, we just want to do something like stride(reg, 2).
> Of course, we can code that with subscript as subscript(reg, retype(reg,
> type_that is_half_the_size), 0) but that is certainly forced. For
> example, we have code such as:
> 
> bld.MOV(tmp, stride(component_i, 2));
> bld.MOV(horiz_offset(tmp, 8 * multiplier),
>         stride(horiz_offset(component_i, 1), 2));
> 
> where component_i is of type F. Of course, we could retype component_i
> to DF when we pass it to subscript, and use type F for the type
> parameter, so that subscript does what we want, but it is certainly not
> very natural. Subscript is all about accessing data with a different
> type and we have some places where we want to do that and subscript does
> what we want, but we also want to use a "normal" stride sometimes.
> 
> Another example, in lowering of PACK, we do something like this:
> 
> ibld.MOV(stride(horiz_offset(retype(dst, inst->src[i].type), i), 
>                 inst->sources),
>          inst->src[i]);
> 
> 
> Here the stride we want is not based on any relation between types (even
> if I think in this case it would work because inst->sources will be 2
> and the type of inst->src[i] will be half of the type of the dst).
> 
> I think that even if subscript can be useful in some of our cases, we
> still want the stride helper for a bunch of other cases.

I did not make a specific proposal:

How about we use subscript in the places where we are actually open
coding it and we keep the stride() helper (fixed as per the comment
above to consider the original stride of the register) for the other
cases? maybe with an assert to make sure we don't use that with ARF/HW
registers (at least for now).

> >  I
> > think I've looked through all the uses of stride() in your branch and
> > they are all sort of an open-coded version of subscript().  This will
> > also address the issue Ken pointed out in patch 29 related to the use of
> > horiz_offset() on uniforms (or anything with stride other than one,
> > really) -- See the attached patch (yes, correct ARF/HW reg handling will
> > be required for SIMD32...).
> > 
> > > +static inline fs_reg
> > >  byte_offset(fs_reg reg, unsigned delta)
> > >  {
> > >     switch (reg.file) {
> > > -- 
> > > 2.5.0
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev