[Mesa-dev] [PATCH v5 33/40] intel/compiler: also set F execution type for mixed float mode in BDW

[Francisco Jerez]

Iago Toral <itoral at igalia.com> writes:

> On Wed, 2019-02-27 at 15:44 -0800, Francisco Jerez wrote:
>> Iago Toral Quiroga <itoral at igalia.com> writes:
>> 
>> > The section 'Execution Data Types' of 3D Media GPGPU volume, which
>> > describes execution types, is exactly the same in BDW and SKL+.
>> > 
>> > Also, this section states that there is a single execution type, so
>> > it
>> > makes sense that this is the wider of the two floating point types
>> > involved in mixed float mode, which is what we do for SKL+ and CHV.
>> > 
>> > v2:
>> >  - Make sure we also account for the destination type in mixed mode
>> > (Curro).
>> > ---
>> >  src/intel/compiler/brw_eu_validate.c | 39 +++++++++++++++++-------
>> > ----
>> >  1 file changed, 24 insertions(+), 15 deletions(-)
>> > 
>> > diff --git a/src/intel/compiler/brw_eu_validate.c
>> > b/src/intel/compiler/brw_eu_validate.c
>> > index 358a0347a93..e0010f0fb07 100644
>> > --- a/src/intel/compiler/brw_eu_validate.c
>> > +++ b/src/intel/compiler/brw_eu_validate.c
>> > @@ -348,6 +348,17 @@ is_unsupported_inst(const struct
>> > gen_device_info *devinfo,
>> >     return brw_opcode_desc(devinfo, brw_inst_opcode(devinfo, inst))
>> > == NULL;
>> >  }
>> >  
>> > +/**
>> > + * Returns whether a combination of two types would qualify as
>> > mixed float
>> > + * operation mode
>> > + */
>> > +static inline bool
>> > +types_are_mixed_float(enum brw_reg_type t0, enum brw_reg_type t1)
>> > +{
>> > +   return (t0 == BRW_REGISTER_TYPE_F && t1 ==
>> > BRW_REGISTER_TYPE_HF) ||
>> > +          (t1 == BRW_REGISTER_TYPE_F && t0 ==
>> > BRW_REGISTER_TYPE_HF);
>> > +}
>> > +
>> >  static enum brw_reg_type
>> >  execution_type_for_type(enum brw_reg_type type)
>> >  {
>> > @@ -390,20 +401,24 @@ execution_type(const struct gen_device_info
>> > *devinfo, const brw_inst *inst)
>> >     enum brw_reg_type src0_exec_type, src1_exec_type;
>> >  
>> >     /* Execution data type is independent of destination data type,
>> > except in
>> > -    * mixed F/HF instructions on CHV and SKL+.
>> > +    * mixed F/HF instructions.
>> >      */
>> >     enum brw_reg_type dst_exec_type = brw_inst_dst_type(devinfo,
>> > inst);
>> >  
>> >     src0_exec_type =
>> > execution_type_for_type(brw_inst_src0_type(devinfo, inst));
>> >     if (num_sources == 1) {
>> > -      if ((devinfo->gen >= 9 || devinfo->is_cherryview) &&
>> > -          src0_exec_type == BRW_REGISTER_TYPE_HF) {
>> > +      if (src0_exec_type == BRW_REGISTER_TYPE_HF)
>> >           return dst_exec_type;
>> > -      }
>> >        return src0_exec_type;
>> >     }
>> >  
>> >     src1_exec_type =
>> > execution_type_for_type(brw_inst_src1_type(devinfo, inst));
>> > +   if (types_are_mixed_float(src0_exec_type, src1_exec_type) ||
>> > +       types_are_mixed_float(src0_exec_type, dst_exec_type) ||
>> > +       types_are_mixed_float(src1_exec_type, dst_exec_type)) {
>> > +      return BRW_REGISTER_TYPE_F;
>> > +   }
>> > +
>> >     if (src0_exec_type == src1_exec_type)
>> >        return src0_exec_type;
>> >  
>> > @@ -431,18 +446,12 @@ execution_type(const struct gen_device_info
>> > *devinfo, const brw_inst *inst)
>> >         src1_exec_type == BRW_REGISTER_TYPE_DF)
>> >        return BRW_REGISTER_TYPE_DF;
>> >  
>> > -   if (devinfo->gen >= 9 || devinfo->is_cherryview) {
>> > -      if (dst_exec_type == BRW_REGISTER_TYPE_F ||
>> > -          src0_exec_type == BRW_REGISTER_TYPE_F ||
>> > -          src1_exec_type == BRW_REGISTER_TYPE_F) {
>> > -         return BRW_REGISTER_TYPE_F;
>> > -      } else {
>> > -         return BRW_REGISTER_TYPE_HF;
>> > -      }
>> > -   }
>> > +   if (src0_exec_type == BRW_REGISTER_TYPE_F ||
>> > +       src1_exec_type == BRW_REGISTER_TYPE_F)
>> > +      return BRW_REGISTER_TYPE_F;
>> >  
>> > -   assert(src0_exec_type == BRW_REGISTER_TYPE_F);
>> > -   return BRW_REGISTER_TYPE_F;
>> > +   assert(src0_exec_type == BRW_REGISTER_TYPE_HF);
>> > +   return BRW_REGISTER_TYPE_HF;
>> 
>> Not really convinced the function is fully correct, but it should be
>> strictly better with this patch:
>
> Is it because of this patch in particular or are you talking about the
> function in general?
>

Talking about the function in general, patch looks okay to me.

>> Acked-by: Francisco Jerez <currojerez at riseup.net>
>> 
>> >  }
>> >  
>> >  /**
>> > -- 
>> > 2.17.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 227 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190228/93024b8d/attachment.sig>