[Mesa-dev] [PATCH 07/14] intel/compiler: shuffle_from_32bit_read for 64-bit do_untyped_vector_read

[Jason Ekstrand]

On Thu, Jun 14, 2018 at 10:57 PM, Iago Toral <itoral at igalia.com> wrote:

> On Fri, 2018-06-15 at 00:20 +0200, Chema Casanova wrote:
> >
> > On 14/06/18 03:26, Jason Ekstrand wrote:
> > > On Sat, Jun 9, 2018 at 4:13 AM, Jose Maria Casanova Crespo
> > > <jmcasanova at igalia.com <mailto:jmcasanova at igalia.com>> wrote:
> > >
> > >     do_untyped_vector_read is used at load_ssbo and load_shared.
> > >
> > >     The previous MOVs are removed because shuffle_from_32bit_read
> > >     can handle storing the shuffle results in the expected
> > > destination
> > >     just using the proper offset.
> > >     ---
> > >      src/intel/compiler/brw_fs_nir.cpp | 12 ++----------
> > >      1 file changed, 2 insertions(+), 10 deletions(-)
> > >
> > >     diff --git a/src/intel/compiler/brw_fs_nir.cpp
> > >     b/src/intel/compiler/brw_fs_nir.cpp
> > >     index 7e738ade82e..780a9e228de 100644
> > >     --- a/src/intel/compiler/brw_fs_nir.cpp
> > >     +++ b/src/intel/compiler/brw_fs_nir.cpp
> > >     @@ -2434,16 +2434,8 @@ do_untyped_vector_read(const fs_builder
> > > &bld,
> > >
> > >  BRW_PREDICATE_NONE);
> > >
> > >               /* Shuffle the 32-bit load result into valid 64-bit
> > > data */
> > >     -         const fs_reg packed_result = bld.vgrf(dest.type,
> > >     iter_components);
> > >     -         shuffle_32bit_load_result_to_64bit_data(
> > >     -            bld, packed_result, read_result, iter_components);
> > >     -
> > >     -         /* Move each component to its destination */
> > >     -         read_result = retype(read_result,
> > > BRW_REGISTER_TYPE_DF);
> > >     -         for (int c = 0; c < iter_components; c++) {
> > >     -            bld.MOV(offset(dest, bld, it * 2 + c),
> > >     -                    offset(packed_result, bld, c));
> > >     -         }
> > >
> > >
> > > I really don't know why we needed this extra set of MOVs.  They
> > > seem
> > > pretty pointless to me.  Maybe history?  In any case, this looks
> > > good.v-
> >
> > I've just checked and there is not much history as the 64-bit code of
> > this function hasn't been changed since they landed. I think that the
> > logic was first shuffle and then move to the proper destination
> > instead
> > of just shuffling to the final destination directly.
>
> Could it be related to non-uniform control flow? Does the function
> below disable channel masks to shuffle? If it does, then I think we
> need to do the shuffle to a temporary and the move from there to its
> original destination with channel masking enabled.
>

That could be.  I don't think the function below has any masking problems
though.

--Jason



> Iago
>
> > So maybe Iago remembers if there was any reason why...
> >
> > > Reviewed-by: Jason Ekstrand <jason at jlekstrand.net
> > > <mailto:jason at jlekstrand.net>>
> > >
> > >
> > >     +         shuffle_from_32bit_read(bld, offset(dest, bld, it *
> > > 2),
> > >     +                                 read_result, 0,
> > > iter_components);
> > >
> > >               bld.ADD(read_offset, read_offset, brw_imm_ud(16));
> > >            }
> > >     --
> > >     2.17.1
> > >
> > >     _______________________________________________
> > >     mesa-dev mailing list
> > >     mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedeskt
> > > op.org>
> > >     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > >     <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
> > >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180614/64d55567/attachment.html>