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

Submitted by Iago Toral Quiroga on Feb. 12, 2019, 11:56 a.m.

Details

Message ID 20190212115607.21467-34-itoral@igalia.com
State New
Headers show
Series "intel: VK_KHR_shader_float16_int8 implementation" ( rev: 7 6 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga Feb. 12, 2019, 11:56 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.
---
 src/intel/compiler/brw_eu_validate.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 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..000a05cb6ac 100644
--- a/src/intel/compiler/brw_eu_validate.c
+++ b/src/intel/compiler/brw_eu_validate.c
@@ -431,18 +431,14 @@  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 (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 {
+      assert(devinfo->gen >= 8 && src0_exec_type == BRW_REGISTER_TYPE_HF);
+      return BRW_REGISTER_TYPE_HF;
    }
-
-   assert(src0_exec_type == BRW_REGISTER_TYPE_F);
-   return BRW_REGISTER_TYPE_F;
 }
 
 /**

Comments

Matt, Curro,

Could one of you please take a look at this and the other validator patches
in this series?  Region restrictions aren't my strongest area.

On Tue, Feb 12, 2019 at 5:56 AM Iago Toral Quiroga <itoral@igalia.com>
wrote:

> 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.
> ---
>  src/intel/compiler/brw_eu_validate.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/src/intel/compiler/brw_eu_validate.c
> b/src/intel/compiler/brw_eu_validate.c
> index 358a0347a93..000a05cb6ac 100644
> --- a/src/intel/compiler/brw_eu_validate.c
> +++ b/src/intel/compiler/brw_eu_validate.c
> @@ -431,18 +431,14 @@ 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 (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 {
> +      assert(devinfo->gen >= 8 && src0_exec_type == BRW_REGISTER_TYPE_HF);
> +      return BRW_REGISTER_TYPE_HF;
>     }
> -
> -   assert(src0_exec_type == BRW_REGISTER_TYPE_F);
> -   return BRW_REGISTER_TYPE_F;
>  }
>
>  /**
> --
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Still waiting on this, specifically we are missing reviews for patches
33, 34, 36 and 37.
On Sat, 2019-02-16 at 09:58 -0600, Jason Ekstrand wrote:
> Matt, Curro,
> 
> Could one of you please take a look at this and the other validator
> patches in this series?  Region restrictions aren't my strongest
> area.
> 
> 
> On Tue, Feb 12, 2019 at 5:56 AM Iago Toral Quiroga <itoral@igalia.com
> > wrote:
> > 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.
> > 
> > ---
> > 
> >  src/intel/compiler/brw_eu_validate.c | 18 +++++++-----------
> > 
> >  1 file changed, 7 insertions(+), 11 deletions(-)
> > 
> > 
> > 
> > diff --git a/src/intel/compiler/brw_eu_validate.c
> > b/src/intel/compiler/brw_eu_validate.c
> > 
> > index 358a0347a93..000a05cb6ac 100644
> > 
> > --- a/src/intel/compiler/brw_eu_validate.c
> > 
> > +++ b/src/intel/compiler/brw_eu_validate.c
> > 
> > @@ -431,18 +431,14 @@ 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 (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 {
> > 
> > +      assert(devinfo->gen >= 8 && src0_exec_type ==
> > BRW_REGISTER_TYPE_HF);
> > 
> > +      return BRW_REGISTER_TYPE_HF;
> > 
> >     }
> > 
> > -
> > 
> > -   assert(src0_exec_type == BRW_REGISTER_TYPE_F);
> > 
> > -   return BRW_REGISTER_TYPE_F;
> > 
> >  }
> > 
> > 
> > 
> >  /**
> >
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.
> ---
>  src/intel/compiler/brw_eu_validate.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/src/intel/compiler/brw_eu_validate.c b/src/intel/compiler/brw_eu_validate.c
> index 358a0347a93..000a05cb6ac 100644
> --- a/src/intel/compiler/brw_eu_validate.c
> +++ b/src/intel/compiler/brw_eu_validate.c
> @@ -431,18 +431,14 @@ 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 (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 {
> +      assert(devinfo->gen >= 8 && src0_exec_type == BRW_REGISTER_TYPE_HF);
> +      return BRW_REGISTER_TYPE_HF;

I'm having trouble convincing myself that this is correct.  Aren't there
four earlier return statements in this function you may potentially hit
on BDW that will still fail to consider the destination type for
instructions with HF operands?

>     }
> -
> -   assert(src0_exec_type == BRW_REGISTER_TYPE_F);
> -   return BRW_REGISTER_TYPE_F;
>  }
>  
>  /**
> -- 
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Tue, 2019-02-26 at 13:55 -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.
> > ---
> >  src/intel/compiler/brw_eu_validate.c | 18 +++++++-----------
> >  1 file changed, 7 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/intel/compiler/brw_eu_validate.c
> > b/src/intel/compiler/brw_eu_validate.c
> > index 358a0347a93..000a05cb6ac 100644
> > --- a/src/intel/compiler/brw_eu_validate.c
> > +++ b/src/intel/compiler/brw_eu_validate.c
> > @@ -431,18 +431,14 @@ 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 (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 {
> > +      assert(devinfo->gen >= 8 && src0_exec_type ==
> > BRW_REGISTER_TYPE_HF);
> > +      return BRW_REGISTER_TYPE_HF;
> 
> I'm having trouble convincing myself that this is correct.  Aren't
> there
> four earlier return statements in this function you may potentially
> hit
> on BDW that will still fail to consider the destination type for
> instructions with HF operands?

Yes, and it seems this is not only in BDW actually, at least for 2-
source instructions.  I'll fix that.

BTW, I also see we have this code (right above this hunk):

/* Execution data type is independent of destination data type, 
 * except in mixed F/HF instructions on CHV and SKL+.
(...)
if ((devinfo->gen >= 9 || devinfo->is_cherryview) &&
     src0_exec_type == BRW_REGISTER_TYPE_HF) {
   return dst_exec_type;
}

Which suggests that we are considering a conversion from HF to F a
mixed float instruction, is that correct? Until now I have been
assuming that conversion MOVs like this were not mixed mode
instructions, just regular conversions between types.

Iago

> >     }
> > -
> > -   assert(src0_exec_type == BRW_REGISTER_TYPE_F);
> > -   return BRW_REGISTER_TYPE_F;
> >  }
> >  
> >  /**
> > -- 
> > 2.17.1
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev