[Mesa-dev] [PATCH 11/59] intel/compiler: Implement float64/int64 to float16 conversion

[Iago Toral]

On Tue, 2018-12-04 at 14:57 +0200, Pohjolainen, Topi wrote:
> On Tue, Dec 04, 2018 at 08:16:35AM +0100, Iago Toral Quiroga wrote:
> > From: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> > 
> > It is not supported directly in the HW, we need to convert to a 32-
> > bit
> > type first as intermediate step.
> > 
> > v2 (Iago): handle conversions from 64-bit integers as well
> > 
> > Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> > ---
> >  src/intel/compiler/brw_fs_nir.cpp | 42
> > ++++++++++++++++++++++++++++---
> >  1 file changed, 39 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/intel/compiler/brw_fs_nir.cpp
> > b/src/intel/compiler/brw_fs_nir.cpp
> > index 7294f49ddc0..9f3d3bf9762 100644
> > --- a/src/intel/compiler/brw_fs_nir.cpp
> > +++ b/src/intel/compiler/brw_fs_nir.cpp
> > @@ -784,6 +784,19 @@ fs_visitor::nir_emit_alu(const fs_builder
> > &bld, nir_alu_instr *instr)
> >         */
> >  
> >     case nir_op_f2f16:
> > +      /* BDW PRM, vol02, Command Reference Instructions, mov -
> > MOVE:
> > +       *
> > +       *   "There is no direct conversion from HF to DF or DF to
> > HF.
> > +       *    Use two instructions and F (Float) as an intermediate
> > type.
> > +       */
> > +      if (nir_src_bit_size(instr->src[0].src) == 64) {
> > +         fs_reg tmp = bld.vgrf(BRW_REGISTER_TYPE_F, 1);
> > +         inst = bld.MOV(tmp, op[0]);
> > +         inst->saturate = instr->dest.saturate;
> > +         inst = bld.MOV(result, tmp);
> > +         inst->saturate = instr->dest.saturate;
> > +         break;
> > +      }
> >        inst = bld.MOV(result, op[0]);
> >        inst->saturate = instr->dest.saturate;
> >        break;
> > @@ -864,7 +877,32 @@ fs_visitor::nir_emit_alu(const fs_builder
> > &bld, nir_alu_instr *instr)
> >           inst->saturate = instr->dest.saturate;
> >           break;
> >        }
> > -      /* fallthrough */
> 
> This is more or less nit-picking but I thought I ask anyway. The
> fallthru
> comment gets now dropped also for other cases than "i2f16" and
> "u2f16". And if
> we added the logic for nir_op_i2f16/nir_op_u2f16 cases just after the
> f2f16
> case that would yield a diff without the following three copy-paste
> lines as
> well. Or amd I missing something?

Yes, I think you're right and if you look at this patch standalone I
think it would make sense to do that. The thing is that later on in the
series we have to change this further to incorporate more restrictions
for conversions to/from integer and half-float for atom platforms, so
having the f2f16 case separated from the {i,u}2f16 cases will make more
sense. That would be patch 46 in the series, which comes later because
that is when we addressed integer conversions from 8-bit and noticed
this whole thing on atom.

I can still make the change you suggest in this patch and then do the
split later on if you think that helps though. I could also try to move
the fix for atom earlier in the series, that will lead to conflicts and
I'd need to slightly rewrite other patches in the series to accomodate
to that, but it is certainly doable if you that makes the commit
history better.

Iago

> > +      inst = bld.MOV(result, op[0]);
> > +      inst->saturate = instr->dest.saturate;
> > +      break;
> > +
> > +   case nir_op_i2f16:
> > +   case nir_op_u2f16:
> > +      /* BDW PRM, vol02, Command Reference Instructions, mov -
> > MOVE:
> > +       *
> > +       *    "There is no direct conversion from HF to Q/UQ or Q/UQ
> > to HF.
> > +       *     Use two instructions and F (Float) or a word integer
> > type or a
> > +       *     DWord integer type as an intermediate type."
> > +       */
> > +      if (nir_src_bit_size(instr->src[0].src) == 64) {
> > +         brw_reg_type reg_type = instr->op == nir_op_i2f16 ?
> > +            BRW_REGISTER_TYPE_D : BRW_REGISTER_TYPE_UD;
> > +         fs_reg tmp = bld.vgrf(reg_type, 1);
> > +         inst = bld.MOV(tmp, op[0]);
> > +         inst->saturate = instr->dest.saturate;
> > +         inst = bld.MOV(result, tmp);
> > +         inst->saturate = instr->dest.saturate;
> > +         break;
> > +      }
> > +      inst = bld.MOV(result, op[0]);
> > +      inst->saturate = instr->dest.saturate;
> > +      break;
> > +
> >     case nir_op_f2f32:
> >     case nir_op_f2i32:
> >     case nir_op_f2u32:
> > @@ -874,8 +912,6 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
> > nir_alu_instr *instr)
> >     case nir_op_u2u32:
> >     case nir_op_i2i16:
> >     case nir_op_u2u16:
> > -   case nir_op_i2f16:
> > -   case nir_op_u2f16:
> >     case nir_op_i2i8:
> >     case nir_op_u2u8:
> >        inst = bld.MOV(result, op[0]);
> > -- 
> > 2.17.1
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
>