[Mesa-dev] [PATCH v2 19/30] i965/fs: add shuffle_64bit_data_for_32bit_write helper

[Iago Toral]

On Thu, 2016-05-12 at 20:05 -0700, Francisco Jerez wrote:
> Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
> 
> > From: Iago Toral Quiroga <itoral at igalia.com>
> >
> > This does the inverse operation of shuffle_32bit_load_result_to_64bit_data
> > and we will use it when we need to write 64-bit data in the layout expected
> > by untyped write messages.
> >
> > v2 (curro):
> > - Use subscript() instead of stride()
> > - Assert on the input types rather than silently retyping.
> > - Use offset() instead of horiz_offset(), drop the multiplier definition.
> > - Drop the temporary vgrf and force_writemask_all.
> > - Make component_i const.
> > ---
> >  src/mesa/drivers/dri/i965/brw_fs.cpp | 40 ++++++++++++++++++++++++++++++++++++
> >  src/mesa/drivers/dri/i965/brw_fs.h   |  5 +++++
> >  2 files changed, 45 insertions(+)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> > index ebc5128..3c58ccb 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> > @@ -280,6 +280,46 @@ fs_visitor::shuffle_32bit_load_result_to_64bit_data(const fs_builder &bld,
> >  }
> >  
> >  /**
> > + * This helper does the inverse operation of
> > + * SHUFFLE_32BIT_LOAD_RESULT_TO_64BIT_DATA.
> > + *
> > + * We need to do this when we are going to use untyped write messsages that
> > + * operate with 32-bit components in order to arrange our 64-bit data to be
> > + * in the expected layout.
> > + */
> > +void
> > +fs_visitor::shuffle_64bit_data_for_32bit_write(const fs_builder &bld,
> > +                                               const fs_reg dst,
> > +                                               const fs_reg src,
> > +                                               uint32_t components)
> > +{
> > +   assert(type_sz(src.type) == 8);
> > +   assert(type_sz(dst.type) == 4);
> > +
> > +   /* A temporary that we will use to shuffle the 64-bit data of each
> > +    * component in the vector into valid 32-bit data that we can write.
> > +    * We can't write directly to dst because dst can be (and would usually be)
> > +    * the same as src and in that case the first MOV in the loop below would
> > +    * overwrite the data read in the second MOV.
> > +    */
> > +   fs_reg tmp = retype(bld.vgrf(src.type), dst.type);
> > +

For the record, I have been thinking that unlike the case of the other
shuffling function that we use when we need to shuffle the data of a
read, it should usually be unsafe to call this one with src == dst (and
in fact our code doesn't) because the original data representing valid
64-bit elements might still be used after the write but it would no
longer represent valid 64-bit data elements if we do the shuffling
in-place, so in this case I think we can remove the temporary and write
to the dst directly as you originally suggested.

I'll do that change and if I don't see anything wrong with it I'll send
a v2.

> This is just 'bld.vgrf(dst.type, 2)' as I suggested off-list, other than
> that:
> 
> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
> 
> > +   for (unsigned i = 0; i < components; i++) {
> > +      const fs_reg component_i = offset(src, bld, i);
> > +
> > +      bld.MOV(tmp, subscript(component_i, dst.type, 0));
> > +      bld.MOV(offset(tmp, bld, 1), subscript(component_i, dst.type, 1));
> > +
> > +      /* Because we are shuffling our 64-bit data to a 32-bit layout, we
> > +       * need these MOVs to be 32-bit (instead of doing this as a single
> > +       * DF MOV), otherwise we would not be respecting the channel enables.
> > +       */
> > +      bld.MOV(offset(dst, bld, 2 * i), tmp);
> > +      bld.MOV(offset(dst, bld, 2 * i + 1), offset(tmp, bld, 1));
> > +   }
> > +}
> > +
> > +/**
> >   * A helper for MOV generation for fixing up broken hardware SEND dependency
> >   * handling.
> >   */
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
> > index 1aeacae..4c17ff4 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs.h
> > +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> > @@ -110,6 +110,11 @@ public:
> >                                                  const fs_reg src,
> >                                                  uint32_t components);
> >  
> > +   void shuffle_64bit_data_for_32bit_write(const brw::fs_builder &bld,
> > +                                           const fs_reg dst,
> > +                                           const fs_reg src,
> > +                                           uint32_t components);
> > +
> >     void do_untyped_vector_read(const brw::fs_builder &bld,
> >                                 const fs_reg surf_index,
> >                                 const fs_reg offset_reg,
> > -- 
> > 2.5.0
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > 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