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

Submitted by Iago Toral Quiroga on Feb. 27, 2019, 10:01 a.m.

Details

Message ID 20190227100131.20092-1-itoral@igalia.com
State New
Headers show
Series "intel: VK_KHR_shader_float16_int8 implementation" ( rev: 8 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga Feb. 27, 2019, 10:01 a.m.
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(-)

Patch hide | download patch | download mbox

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;
 }
 
 /**

Comments

Iago Toral Quiroga <itoral@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:

Acked-by: Francisco Jerez <currojerez@riseup.net>

>  }
>  
>  /**
> -- 
> 2.17.1
On Wed, 2019-02-27 at 15:44 -0800, Francisco Jerez wrote:
> Iago Toral Quiroga <itoral@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?

> Acked-by: Francisco Jerez <currojerez@riseup.net>
> 
> >  }
> >  
> >  /**
> > -- 
> > 2.17.1
Iago Toral <itoral@igalia.com> writes:

> On Wed, 2019-02-27 at 15:44 -0800, Francisco Jerez wrote:
>> Iago Toral Quiroga <itoral@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@riseup.net>
>> 
>> >  }
>> >  
>> >  /**
>> > -- 
>> > 2.17.1