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

[Matt Turner]

On Thu, Jan 17, 2019 at 12:18 PM Jason Ekstrand <jason at jlekstrand.net> 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).
>> +          */

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 at gmail.com>