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

[Jason Ekstrand]

On Mon, Jan 21, 2019 at 3:17 AM Iago Toral <itoral at 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 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?
>

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 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/e4958f33/attachment.html>