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

[Iago Toral]

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/20181210/5d708f9f/attachment.html>