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

[Iago Toral]

On Wed, 2016-05-04 at 01:15 -0700, Francisco Jerez wrote:
> Iago Toral <itoral at igalia.com> writes:
> 
> > 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));
> >
> Oh no, this is precisely the use-case I had in mind: component_i is a
> vector of 64-bit channels laid out contiguously, and you want to extract
> each 32-bit field from each channel separately.  You happen to have
> declared component_i as a 32-bit type but that's kind of a lie because
> each component of "component_i" only represents half a channel, it would
> definitely make more sense to make it a DF because that's what it
> contains...

I think that whether component_i is really 64-bit or 32-bit is not
relevant, in the sense that what matters is how the algorithm
needs/wants to interpret the data it has to manipulate. The way I
thought about this is that we have our 32-bit channels mixed up and we
need to re-arrange them, so from my perspective, this is all about
32-bit data that we need to move around in a particular fashion. That's
why I find the stride() helper a lot more natural to use, where I just
tell the access type and the stride explicitly, while with subscript() I
have to think about a register type and an access type instead and the
stride is a consequence of that relation. I admit this is very
subjective though.

In any case, let's change it, as you say we should be able to use
subscript with some minor changes so let's do that. I just wanted to
explain why I personally think that stride() might be more
flexible/natural in some scenarios, at least for some people like me :)

For the record, Ken suggested we land things as they are now (with
horiz_offset/stride) and fix these things later to use subscript()
instead, so we will do that.

> > 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]);
> >
> Same here.  If inst->sources hadn't matched the ratio between the two
> types you'd have ended up reading cross channel boundaries and
> miscompiling the program.  Using subscript() instead would guarantee
> that this is never the case even if e.g. one of the sources happens to
> be missing.
> 
> >
> > 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
> >> 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