[Mesa-dev] [PATCH 02/14] intel/compiler: new shuffle_for_32bit_write and shuffle_from_32bit_read

[Chema Casanova]

On 14/06/18 03:02, 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:
> 
>     These new shuffle functions deal with the shuffle/unshuffle operations
>     needed for read/write operations using 32-bit components when the
>     read/written components have a different bit-size (8, 16, 64-bits).
>     Shuffle from 32-bit to 32-bit becomes a simple MOV.
> 
>     As the new function shuffle_src_to_dst takes of doing a shuffle or an
>     unshuffle based on the different type_sz of source an destination this
>     generic functions work with any source/destination assuming that writes
>     use a 32-bit destination or reads use a 32-bit source.
> 
> 
> I'm having a lot of trouble understanding this paragraph.  Would you
> mind rephrasing it?
>  

Sure, that English didn't compile:

"shuffle_src_to_dst takes care of doing a shuffle when source type is
smaller than destination type and an unshuffle when source type is
bigger than destination. So this new read/write functions just need
to call shuffle_src_to_dst assuming that writes use a 32-bit
destination and reads use a 32-bit source."

I included also this comment in the commit log:

"As shuffle_for_32bit_write/from_32bit_read components take components
in unit of source/destination types and shuffle_src_to_dst takes units
of the smallest type component we adjust the components and
first_component parameters."

> 
>     To enable this new functions it is needed than there is no
>     source/destination overlap in the case of shuffle_from_32bit_read.
>     That never happens on shuffle_for_32bit_write as it allocates a new
>     destination register as it was at shuffle_64bit_data_for_32bit_write.
>     ---
>      src/intel/compiler/brw_fs.h       | 11 +++++++++
>      src/intel/compiler/brw_fs_nir.cpp | 38 +++++++++++++++++++++++++++++++
>      2 files changed, 49 insertions(+)
> 
>     diff --git a/src/intel/compiler/brw_fs.h b/src/intel/compiler/brw_fs.h
>     index faf51568637..779170ecc95 100644
>     --- a/src/intel/compiler/brw_fs.h
>     +++ b/src/intel/compiler/brw_fs.h
>     @@ -519,6 +519,17 @@ void shuffle_16bit_data_for_32bit_write(const
>     brw::fs_builder &bld,
>                                              const fs_reg &src,
>                                              uint32_t components);
> 
>     +void shuffle_from_32bit_read(const brw::fs_builder &bld,
>     +                             const fs_reg &dst,
>     +                             const fs_reg &src,
>     +                             uint32_t first_component,
>     +                             uint32_t components);
>     +
>     +fs_reg shuffle_for_32bit_write(const brw::fs_builder &bld,
>     +                               const fs_reg &src,
>     +                               uint32_t first_component,
>     +                               uint32_t components);
>     +
>      fs_reg setup_imm_df(const brw::fs_builder &bld,
>                          double v);
> 
>     diff --git a/src/intel/compiler/brw_fs_nir.cpp
>     b/src/intel/compiler/brw_fs_nir.cpp
>     index 1a9d3c41d1d..1f684149fd5 100644
>     --- a/src/intel/compiler/brw_fs_nir.cpp
>     +++ b/src/intel/compiler/brw_fs_nir.cpp
>     @@ -5454,6 +5454,44 @@ shuffle_src_to_dst(const fs_builder &bld,
>         }
>      }
> 
>     +void
>     +shuffle_from_32bit_read(const fs_builder &bld,
>     +                        const fs_reg &dst,
>     +                        const fs_reg &src,
>     +                        uint32_t first_component,
>     +                        uint32_t components)
>     +{
>     +   assert(type_sz(src.type) == 4);
>     +
> 
> 
> /* This function takes components in units of the destination type while
> shuffle_src_to_dst takes components in units of the smallest type */

Done.

>     +   if (type_sz(dst.type) > 4) {
>     +      assert(type_sz(dst.type) == 8);
>     +      first_component *= 2;
>     +      components *= 2;
>     +   }
>     +
>     +   shuffle_src_to_dst(bld, dst, src, first_component, components);
>     +}
>     +
>     +fs_reg
>     +shuffle_for_32bit_write(const fs_builder &bld,
>     +                        const fs_reg &src,
>     +                        uint32_t first_component,
>     +                        uint32_t components)
>     +{
>     +   fs_reg dst = bld.vgrf(BRW_REGISTER_TYPE_D,
>     +                         DIV_ROUND_UP (components *
>     type_sz(src.type), 4));
>     +
> 
> 
> /* This function takes components in units of the source type while
> shuffle_src_to_dst takes components in units of the smallest type */

Done.

> With those added and the commit message re-worded a bit,
> 
> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net
> <mailto:jason at jlekstrand.net>>

Thanks for the review.

Chema

>     +   if (type_sz(src.type) > 4) {
>     +      assert(type_sz(src.type) == 8);
>     +      first_component *= 2;
>     +      components *= 2;
>     +   }
>     +
>     +   shuffle_src_to_dst(bld, dst, src, first_component, components);
>     +
>     +   return dst;
>     +}
>     +
>      fs_reg
>      setup_imm_df(const fs_builder &bld, double v)
>      {
>     -- 
>     2.17.1
> 
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.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
>