<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jun 14, 2018 at 2:39 PM, Chema Casanova <<a href="mailto:jmcasanova@igalia.com" target="_blank">jmcasanova@igalia.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 14/06/18 03:02, Jason Ekstrand wrote: > On Sat, Jun 9, 2018 at 4:13 AM, Jose Maria Casanova Crespo > <<a href="mailto:jmcasanova@igalia.com">jmcasanova@igalia.com</a> <mailto:<a href="mailto:jmcasanova@igalia.com">jmcasanova@igalia.com</a>>> 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." <div><div class="h5"></div></div></blockquote><div> </div><div>Those both look good. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > > 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 */ </div></div>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 <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a> > <mailto:<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>>> 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 > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <mailto:<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a>> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > <<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>> <div class="HOEnZb"><div class="h5">> > > > > _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > </div></div></blockquote></div> </div></div>