On Wed, Dec 05, 2018 at 09:49:29AM +0100, Iago Toral wrote:
> 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 <sigles...@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 <sigles...@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.
I'm not sure if I understood correctly your answer but I didn't suggest to
merge f2f16 case with {i,u}2f16 cases. I thought that having:
case nir_op_f2f16:
...
break;
case nir_op_i2f16:
case nir_op_u2f16:
...
break;
case nir_op_b2i:
...
would have yielded smaller diff than:
case nir_op_f2f16:
...
break;
case nir_op_b2i:
...
case nir_op_u2u64:
...
/* fallthrough */
case nir_op_i2f16:
case nir_op_u2f16:
...
break;
case nir_op_f2f32
...
break;
>
> 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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> >
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
