[Mesa-dev] [PATCH 11/59] intel/compiler: Implement float64/int64 to float16 conversion
Iago Toral
itoral at igalia.com
Tue Dec 11 08:23:05 UTC 2018
On Mon, 2018-12-10 at 10:55 -0600, Jason Ekstrand wrote:
> Trying to reduce the discussion to one e-mail thread and I picked
> this one. :-)
>
> TBH, I'm not sure what the right solution is. What I do know is that
> the special cases are starting to get nuts. I see a three possible
> options:
>
> 1. Unify the current brw_fs_nir code. This could happen a couple of
> ways:
> a. Break the workarounds out into some helpers.
I think this would make sense in any case.
> b. Have a mega convert block (or function) that just handles
> everything in a general way
Considering all the complexity that the conversion code is
getting having a dedicated function probably makes more sense than
having the logic merged in the giant switch statement of the NIR
translator.
> 2. Lower the conversions to multiple conversions in NIR. This would
> let us handle some of the issues such as DF -> F -> HF but not the
> struding issues.
I see this as a nice thing to have. It is only a few cases but it will
help recducing complexity in the backend.
> 3. Just emit the conversion and handle all these cases in
> lower_conversions.
Sounds a bit like the mega convert function from 1), only that we would
be calling it from a different place.
> I kind-of think option 3 may be the best but I'm not at all sure on
> that point and I think I'd be open to any of the above.
Ideally I think we would do most of the above. 1.b) and/or 3) look like
the only ones that would really make things better, but if we are going
to do that we might as well put a bit more effort into it an the other
things as well. They all look nice to have to me.
One thing about moving to lower conversions is that the pass happens
very late and right now we are handling all these cases very early. I
am not sure if this would have any actual implications but it is
something we should verify before we commit to that, so I guess we
might want to have the master conversion function first and design it
so we can call that from lower_conversions or from the NIR translator
and then expriment with both scenarios.
Iago
> --Jason
>
> On Mon, Dec 10, 2018 at 2:36 AM Iago Toral <itoral at igalia.com> wrote:
> > Yes that might be a good thing to do. I mean, handling it in the
> > backend isn't too bad if it was only this, but we now have a lot of
> > conversion conversion cases and different kinds of restrictions on
> > different gens and things start getting a bit messy. If we can pull
> > some of these to NIR I think that at the very least that would make
> > the backend code a bit more reasonable, which is a good thing.
> > Would you prefer that I do that before we land this or is it okay
> > if I fix it up aftwewards?
> > In the backend I was also thinking about maybe pulling some of the
> > conversion logic into one of more specialized conversion helpers
> > that deal with specific things, maybe conversions from integers, or
> > conversions from/to larger or smalller bit-sizes, etc I would need
> > to think if there is one particular way to group conversions that
> > makes things more sane overall.
> >
> > Iago
> > On Fri, 2018-12-07 at 09:34 -0600, Jason Ekstrand wrote:
> > > Now I'm wondering even more about my previous question about just
> > > splitting it into two instructions in NIR.
> > >
> > > On Tue, Dec 4, 2018 at 1:18 AM Iago Toral Quiroga <
> > > itoral at igalia.com> 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;
> > >
> > > I don't think you need both saturates. Just the first one should
> > > be sufficient as it clamps to [0, 1] which is representable in
> > > 16-bit float.
> > >
> > > > + 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 */
> > > >
> > > > + 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]);
> > > >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181211/38cb640b/attachment-0001.html>
More information about the mesa-dev
mailing list