[Mesa-dev] [PATCH 11/59] intel/compiler: Implement float64/int64 to float16 conversion
Iago Toral
itoral at igalia.com
Wed Dec 5 09:56:06 UTC 2018
On Wed, 2018-12-05 at 11:08 +0200, Pohjolainen, Topi wrote:
> 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 <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.
>
> 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;
Ah, yes, I see what you mean now. I guess this is very subjective in
the end but in general my preference was to separate the cases and try
to avoid too many fallthroughs and specially for large blocks of
opcodes, at least for the cases where the main benefit was to reuse
that 3-line block which is basically the MOV instruction that is going
to be there for all conversion cases. I found that as we added more
types (there is also 8-bit conversions coming up later in the series)
and some of these conversions come with additional restrictions for
specific platforms or source/destination types, the combination of
fallthoughs and conditionals started to be a real pain and made it more
difficult to immediately identify which cases where relevant to a
particular code inside a particular block of case statements. We
actually had a couple of bugs during development about this so I ended
up thinking that trying less hard to fallthough was better.
With that being said, if you feel different about this I have no
problem switching to what you suggest.
> >
> > 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
> > >
> > >
>
>
More information about the mesa-dev
mailing list