[Mesa-stable] [Mesa-dev] [PATCH] intel/fs: Use a pure vertical stride for large register strides

[Jason Ekstrand]

On Mon, Nov 6, 2017 at 4:10 AM, Samuel Iglesias Gonsálvez <
siglesias at igalia.com> wrote:

>
> On Thu, 2017-11-02 at 15:54 -0700, Jason Ekstrand wrote:
> > Register strides higher than 4 are uncommon but they can happen.  For
> > instance, if you have a 64-bit extract_u8 operation, we turn that
> > into
> > UB -> UQ MOV with a source stride of 8.  Our previous calculation
> > would
> > try to generate a stride of <32;8,8>:ub which is invalid because the
> > maximum horizontal stride is 4.  To solve this problem, we instead
> > use a
> > stride of <8;1,0>.  As noted in the comment, this does not work as a
> > destination but that's ok as very few things actually generate that
> > stride.
> >
>
> Great!
>
> > Cc: mesa-stable at lists.freedesktop.org
> > ---
> >  src/intel/compiler/brw_fs_generator.cpp | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/intel/compiler/brw_fs_generator.cpp
> > b/src/intel/compiler/brw_fs_generator.cpp
> > index 46f9a33..a557f80 100644
> > --- a/src/intel/compiler/brw_fs_generator.cpp
> > +++ b/src/intel/compiler/brw_fs_generator.cpp
> > @@ -90,9 +90,18 @@ brw_reg_from_fs_reg(const struct gen_device_info
> > *devinfo, fs_inst *inst,
> >            *       different execution size when the number of
> > components
> >            *       written to each destination GRF is not the same.
> >            */
> > -         const unsigned width = MIN2(reg_width, phys_width);
> > -         brw_reg = brw_vecn_reg(width, brw_file_from_reg(reg), reg-
> > >nr, 0);
> > -         brw_reg = stride(brw_reg, width * reg->stride, width, reg-
> > >stride);
> > +         if (reg->stride > 4) {
> > +            /* For registers with an exceptionally large stride, we
> > use a
> > +             * width of 1 and only use the vertical stride.  This
> > only works
> > +             * for sources since destinations require hstride == 1.
> > +             */
> > +            brw_reg = brw_vec1_reg(brw_file_from_reg(reg), reg->nr,
> > 0);
> > +            brw_reg = stride(brw_reg, reg->stride, 1, 0);
>
> I think it is a good idea to add an assert like:
>
>    assert(reg != &inst->dst)
>
> in order to avoid applying this to dst.
>

We already have an assert that triggers, but this is more direct.  I'll add
it.


> With or without that change,
>
> Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
>

Thanks!


> Sam
>
> > +         } else {
> > +            const unsigned width = MIN2(reg_width, phys_width);
> > +            brw_reg = brw_vecn_reg(width, brw_file_from_reg(reg),
> > reg->nr, 0);
> > +            brw_reg = stride(brw_reg, width * reg->stride, width,
> > reg->stride);
> > +         }
> >
> >           if (devinfo->gen == 7 && !devinfo->is_haswell) {
> >              /* From the IvyBridge PRM (EU Changes by Processor
> > Generation, page 13):
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-stable/attachments/20171106/b9bd7bfd/attachment.html>