[Mesa-dev] [PATCH v3 21/42] intel/compiler: set correct precision fields for 3-source float instructions

[Iago Toral]

On Fri, 2019-01-18 at 06:54 -0600, Jason Ekstrand wrote:
> 
> 
> 
> 
> 
> On January 18, 2019 04:47:51 Iago Toral <itoral at igalia.com> wrote:
> > On Thu, 2019-01-17 at 14:18 -0600, Jason Ekstrand wrote:
> > > On Tue, Jan 15, 2019 at 7:55 AM Iago Toral Quiroga <
> > > itoral at igalia.com> wrote:
> > > > Source0 and Destination extract the floating-point precision
> > > > automatically
> > > > 
> > > > from the SrcType and DstType instruction fields respectively
> > > > when they are
> > > > 
> > > > set to types :F or :HF. For Source1 and Source2 operands, we
> > > > use the new
> > > > 
> > > > 1-bit fields Src1Type and Src2Type, where 0 means normal
> > > > precision and 1
> > > > 
> > > > means half-precision. Since we always use the type of the
> > > > destination for
> > > > 
> > > > all operands when we emit 3-source instructions, we only need
> > > > set Src1Type
> > > > 
> > > > and Src2Type to 1 when we are emitting a half-precision
> > > > instruction.
> > > > 
> > > > 
> > > > 
> > > > v2:
> > > > 
> > > >  - Set the bit separately for each source based on its type so
> > > > we can
> > > > 
> > > >    do mixed floating-point mode in the future (Topi).
> > > > 
> > > > 
> > > > 
> > > > Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > > > 
> > > > ---
> > > > 
> > > >  src/intel/compiler/brw_eu_emit.c | 16 ++++++++++++++++
> > > > 
> > > >  1 file changed, 16 insertions(+)
> > > > 
> > > > 
> > > > 
> > > > diff --git a/src/intel/compiler/brw_eu_emit.c
> > > > b/src/intel/compiler/brw_eu_emit.c
> > > > 
> > > > index a785f96b650..2fa89f8a2a3 100644
> > > > 
> > > > --- a/src/intel/compiler/brw_eu_emit.c
> > > > 
> > > > +++ b/src/intel/compiler/brw_eu_emit.c
> > > > 
> > > > @@ -801,6 +801,22 @@ brw_alu3(struct brw_codegen *p, unsigned
> > > > opcode, struct brw_reg dest,
> > > > 
> > > >            */
> > > > 
> > > >           brw_inst_set_3src_a16_src_type(devinfo, inst,
> > > > dest.type);
> > > > 
> > > >           brw_inst_set_3src_a16_dst_type(devinfo, inst,
> > > > dest.type);
> > > > 
> > > > +
> > > > 
> > > > +         /* From the Bspec: Instruction types
> > > > 
> > > > +          *
> > > > 
> > > > +          * Three source instructions can use operands with
> > > > mixed-mode
> > > > 
> > > > +          * precision. When SrcType field is set to :f or :hf
> > > > it defines
> > > > 
> > > > +          * precision for source 0 only, and fields Src1Type
> > > > and Src2Type
> > > > 
> > > > +          * define precision for other source operands:
> > > > 
> > > > +          *
> > > > 
> > > > +          *   0b = :f. Single precision Float (32-bit).
> > > > 
> > > > +          *   1b = :hf. Half precision Float (16-bit).
> > > > 
> > > > +          */
> > > > 
> > > > +         if (src1.type == BRW_REGISTER_TYPE_HF)
> > > > 
> > > > +            brw_inst_set_3src_a16_src1_type(devinfo, inst, 1);
> > > 
> > > Maybe worth throwing in an
> > > 
> > > assert(src0.type == BRW_REGISTER_TYPE_F || src0.type ==
> > > BRW_REGISTER_TYPE_HF);
> > > 
> > > just to be sure? 
> > 
> > If we are going to do this I guess we should also check the same
> > for src2.
> 
> Yeah, it'd probably be good to have a general assertion that the
> three sources have the same type with the caveat that they can vary
> to mix half and full float.  Maybe that would be better than
> something specific right here.

Actually, I don't think that is going to work. There is this comment
right above this hunk:
         /* Set both the source and destination types based on
dest.type,          * ignoring the source register types.  The MAD and
LRP emitters ensure          * that all four types are float.  The BFE
and BFI2 emitters, however,          * may send us mixed D and UD types
and want us to ignore that and use          * the destination
type.          */
I guess we could still formulate a readable assertion using helpers,
something like this:
assert(all_types_32bit_integer(src0.type, src1.type, src2.type)
||           all_types_mixed_float(src0.type, src1.type, src2.type)
||           all_types_64bit_float(src0.type, src1.type, src2.type));
but creating 3 helpers only for one assertion might a bit excessive...
so maybe just adding the specific asserts for src1 and src2 when they
are HF is more reasonable?
> > >  Either way, this and patch 20 are
> > > Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
> > >  
> > > > +
> > > > 
> > > > +         if (src2.type == BRW_REGISTER_TYPE_HF)
> > > > 
> > > > +            brw_inst_set_3src_a16_src2_type(devinfo, inst, 1);
> > > > 
> > > >        }
> > 
> > And the same here (for src0 and src1)
> > > >     }
> > > > 
> > > > 
> > > > 
> 
> 
> 
> 
> 
> 
> 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190121/7e36d0f8/attachment.html>