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

[Iago Toral]

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.

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