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

[Iago Toral]

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.

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
> > 
> 
>