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

Submitted by Iago Toral Quiroga on Jan. 15, 2019, 1:53 p.m.

Details

Message ID 20190115135414.2313-22-itoral@igalia.com
State New
Headers show
Series "intel: VK_KHR_shader_float16_int8 implementation" ( rev: 5 4 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga Jan. 15, 2019, 1:53 p.m.
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@intel.com>
---
 src/intel/compiler/brw_eu_emit.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Patch hide | download patch | download mbox

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);
+
+         if (src2.type == BRW_REGISTER_TYPE_HF)
+            brw_inst_set_3src_a16_src2_type(devinfo, inst, 1);
       }
    }
 

Comments

On Tue, Jan 15, 2019 at 7:55 AM Iago Toral Quiroga <itoral@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@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?  Either way, this and patch 20 are

Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>


> +
> +         if (src2.type == BRW_REGISTER_TYPE_HF)
> +            brw_inst_set_3src_a16_src2_type(devinfo, inst, 1);
>        }
>     }
>
> --
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
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@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@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.
>  Either way, this and patch 20 are
> Reviewed-by: Jason Ekstrand <jason@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)
> >     }
> > 
> > 
> >
On January 18, 2019 04:47:51 Iago Toral <itoral@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@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@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.

>
>
>> Either way, this and patch 20 are
>>
>> Reviewed-by: Jason Ekstrand <jason@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)
>
>>>    }
On Fri, 2019-01-18 at 06:54 -0600, Jason Ekstrand wrote:
> 
> 
> 
> 
> 
> On January 18, 2019 04:47:51 Iago Toral <itoral@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@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@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@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)
> > > >     }
> > > > 
> > > > 
> > > > 
> 
> 
> 
> 
> 
> 
> 
>
On Mon, Jan 21, 2019 at 3:17 AM Iago Toral <itoral@igalia.com> wrote:

> On Fri, 2019-01-18 at 06:54 -0600, Jason Ekstrand wrote:
>
>
> On January 18, 2019 04:47:51 Iago Toral <itoral@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@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@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?
>

Yeah, I think we're spending way too much time trying to figure out how to
assert.  I think the right thing to do here is just to do it when src1 and
src2 are HF.  Don't over-complicate it.


>
> Either way, this and patch 20 are
>
> Reviewed-by: Jason Ekstrand <jason@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)
>
>     }
>
>
>
>
On Thu, Jan 17, 2019 at 12:18 PM Jason Ekstrand <jason@jlekstrand.net> wrote:
>
> On Tue, Jan 15, 2019 at 7:55 AM Iago Toral Quiroga <itoral@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@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).
>> +          */

I'd put this in our typical block-quote style.

>> +         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);

Please don't. Stuff like this should go in brw_eu_validate.c. If you
add asserts to the emission code, it'll assert fail when you write a
negative test for brw_eu_validate.c.

The code as-is is

Reviewed-by: Matt Turner <mattst88@gmail.com>