[Mesa-dev] [PATCH 46/59] intel/compiler: fix integer to/from half-float in atom platforms

[Iago Toral]

Yes, I mentioned this in another of my replies. I think we probably
want a combination of these things. For some restrictions  like the
cases where we need to split a conversion in two instructions going
through an intermediate type I think a NIR lowering would be good, but
then I think we might also want to create one or two specialized
conversion helpers in the backend that deal with specific groups of
situations such as maybe conversions to/from 64-bit or to/from lower
bit-sizes, or maybe to/from integer, etc I need to think a bit more
about what would be a good strategy, but in any case, I share your view
of the situation and I agree we should do something about it.
Iago
On Fri, 2018-12-07 at 13:53 -0600, Jason Ekstrand wrote:
> We are starting to get a *lot* of special cases in the conversion
> code.  I'm not sure what the best thing to do is.  Maybe some master
> conversion function that just does it all?  Maybe some NIR lowering? 
> In any case, I think we can do better than the pile of special cases
> we are starting to accumulate.
> 
> On Tue, Dec 4, 2018 at 1:18 AM Iago Toral Quiroga <itoral at igalia.com>
> wrote:
> > Section Register Region Restriction of the 3D Media GPGPU chapter
> > states:
> > 
> > 
> > 
> >    "Conversion between Integer and HF (Half Float) must be DWord
> > 
> >     aligned and strided by a DWord on the destination."
> > 
> > 
> > 
> > The same restriction shows up in all hardware platforms that
> > support
> > 
> > half-float, however, empirical testing suggests that only atom
> > 
> > platforms are affected.
> > 
> > ---
> > 
> >  src/intel/compiler/brw_fs_nir.cpp | 41
> > +++++++++++++++++++++++++++++--
> > 
> >  1 file changed, 39 insertions(+), 2 deletions(-)
> > 
> > 
> > 
> > diff --git a/src/intel/compiler/brw_fs_nir.cpp
> > b/src/intel/compiler/brw_fs_nir.cpp
> > 
> > index 3f98c6a4474..db3a8812ae3 100644
> > 
> > --- a/src/intel/compiler/brw_fs_nir.cpp
> > 
> > +++ b/src/intel/compiler/brw_fs_nir.cpp
> > 
> > @@ -917,6 +917,25 @@ fs_visitor::nir_emit_alu(const fs_builder
> > &bld, nir_alu_instr *instr)
> > 
> >           inst->saturate = instr->dest.saturate;
> > 
> >           break;
> > 
> >        }
> > 
> > +
> > 
> > +      /* 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)) &&
> > 
> > +          nir_dest_bit_size(instr->dest.dest) == 16) {
> > 
> > +         assert(result.type == BRW_REGISTER_TYPE_HF);
> > 
> > +         fs_reg tmp =
> > 
> > +            horiz_stride(retype(bld.vgrf(BRW_REGISTER_TYPE_F, 1),
> > result.type), 2);
> > 
> > +         bld.MOV(tmp, op[0]);
> > 
> > +         op[0] = tmp;
> > 
> > +      }
> > 
> > +
> > 
> >        inst = bld.MOV(result, op[0]);
> > 
> >        inst->saturate = instr->dest.saturate;
> > 
> >        break;
> > 
> > @@ -939,11 +958,29 @@ fs_visitor::nir_emit_alu(const fs_builder
> > &bld, nir_alu_instr *instr)
> > 
> >        }
> > 
> >        /* Fallthrough */
> > 
> > 
> > 
> > +   case nir_op_f2i16:
> > 
> > +   case nir_op_f2u16:
> > 
> > +      /* 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)) &&
> > 
> > +          nir_src_bit_size(instr->src[0].src) == 16) {
> > 
> > +         fs_reg tmp =
> > 
> > +            horiz_stride(retype(bld.vgrf(BRW_REGISTER_TYPE_D, 1),
> > result.type), 2);
> > 
> > +         bld.MOV(tmp, op[0]);
> > 
> > +         op[0] = tmp;
> > 
> > +      }
> > 
> > +      /* Fallthrough */
> > 
> > +
> > 
> >     case nir_op_f2f32:
> > 
> >     case nir_op_f2i32:
> > 
> >     case nir_op_f2u32:
> > 
> > -   case nir_op_f2i16:
> > 
> > -   case nir_op_f2u16:
> > 
> >     case nir_op_i2i32:
> > 
> >     case nir_op_u2u32:
> > 
> >     case nir_op_i2i16:
> > 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181210/b7da64b5/attachment.html>