[Mesa-dev] [PATCH v2 39/53] intel/compiler: add a helper to do conversions between integer and half-float

[Iago Toral]

On Fri, 2019-01-04 at 02:02 -0800, Francisco Jerez wrote:
> Iago Toral <itoral at igalia.com> writes:
> 
> > On Wed, 2019-01-02 at 15:00 -0800, Francisco Jerez wrote:
> > > Iago Toral Quiroga <itoral at igalia.com> writes:
> > > 
> > > > There are hardware restrictions to consider that seem to affect
> > > > atom platforms
> > > > only.
> > > 
> > > Same comment here as for PATCH 13 of this series.  This and PATCH
> > > 40
> > > shouldn't be necessary anymore with [1] in place.  Please drop
> > > them.
> > 
> > Actually, I think your pass doesn't handle this case. I have just
> > tested this and I get various regressions for conversions between
> > integers (specifically integers lower than 32-bit, so I wonder if
> > this
> > restriction only affects these cases) and half-float.
> > 
> > Here is an example of final IR generated with your pass and without
> > the
> > call to fixup_int_half_float_conversion from my series:
> > 
> > mov(8) vgrf1+0.0:HF, vgrf0<2>:W 
> > 
> > Which should be:
> > 
> > mov(8) vgrf1<2>:HF, vgrf0<2>:W
> > 
> > It seems your pass doesn't act on this code since
> > INTEL_DEBUG=optimizer
> > doesn't show any trace of it.
> > 
> > If this is not a bug in your pass and just that it doesn't handle
> > this
> > case, I am happy to add the support for it as part of my series if
> > that
> > makes sense to you, just let me know if that is the case.
> > 
> 
> It's not really a bug, you just need to add a case to
> has_dst_aligned_region_restriction() for it to return true for FP16
> instructions with this restriction, sorry I didn't point that out
> before.

Ok, I'll do that as part of my series then, thanks!

Iago

> > Iago
> > 
> > > [1] 
> > > 
https://lists.freedesktop.org/archives/mesa-dev/2018-December/212775.html
> > > 
> > > > ---
> > > >  src/intel/compiler/brw_fs_nir.cpp | 32
> > > > +++++++++++++++++++++++++++++++
> > > >  1 file changed, 32 insertions(+)
> > > > 
> > > > diff --git a/src/intel/compiler/brw_fs_nir.cpp
> > > > b/src/intel/compiler/brw_fs_nir.cpp
> > > > index 802f5cb0944..a9fd98bab68 100644
> > > > --- a/src/intel/compiler/brw_fs_nir.cpp
> > > > +++ b/src/intel/compiler/brw_fs_nir.cpp
> > > > @@ -696,6 +696,38 @@ fixup_64bit_conversion(const fs_builder
> > > > &bld,
> > > >     return false;
> > > >  }
> > > >  
> > > > +static bool
> > > > +fixup_int_half_float_conversion(const fs_builder &bld,
> > > > +                                fs_reg dst, fs_reg src,
> > > > +                                bool saturate,
> > > > +                                const struct gen_device_info
> > > > *devinfo)
> > > > +{
> > > > +   /* CHV PRM, 3D Media GPGPU Engine, Register Region
> > > > Restrictions,
> > > > +    * Special Restrictions:
> > > > +    *
> > > > +    *    "Conversion between Integer and HF (Half Float) must
> > > > be
> > > > DWord
> > > > +    *     aligned and strided by a DWord on the destination."
> > > > +    *
> > > > +    * The same restriction is listed for other hardware
> > > > platforms,
> > > > however,
> > > > +    * empirical testing suggests that only atom platforms are
> > > > affected.
> > > > +    */
> > > > +   if (!devinfo->is_cherryview &&
> > > > !gen_device_info_is_9lp(devinfo))
> > > > +      return false;
> > > > +
> > > > +   if (!((dst.type == BRW_REGISTER_TYPE_HF &&
> > > > !brw_reg_type_is_floating_point(src.type)) ||
> > > > +         (src.type == BRW_REGISTER_TYPE_HF &&
> > > > !brw_reg_type_is_floating_point(dst.type))))
> > > > +      return false;
> > > > +
> > > > +   fs_reg tmp =
> > > > horiz_stride(retype(bld.vgrf(BRW_REGISTER_TYPE_F,
> > > > 1),
> > > > +                                    dst.type),
> > > > +                             2);
> > > > +   bld.MOV(tmp, src);
> > > > +   fs_inst *inst = bld.MOV(dst, tmp);
> > > > +   inst->saturate = saturate;
> > > > +
> > > > +   return true;
> > > > +}
> > > > +
> > > >  void
> > > >  fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr
> > > > *instr)
> > > >  {
> > > > -- 
> > > > 2.17.1
> > > > 
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev