[v3,19/42] intel/compiler: don't compact 3-src instructions with Src1Type or Src2Type bits

Submitted by Iago Toral Quiroga on Jan. 15, 2019, 1:53 p.m.

Details

Message ID 20190115135414.2313-20-itoral@igalia.com
State New
Headers show
Series "intel: VK_KHR_shader_float16_int8 implementation" ( rev: 5 4 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga Jan. 15, 2019, 1:53 p.m.
We are now using these bits, so don't assert that they are not set, just
avoid compaction in that case.

Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
---
 src/intel/compiler/brw_eu_compact.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/compiler/brw_eu_compact.c b/src/intel/compiler/brw_eu_compact.c
index ae14ef10ec0..20fed254331 100644
--- a/src/intel/compiler/brw_eu_compact.c
+++ b/src/intel/compiler/brw_eu_compact.c
@@ -928,8 +928,11 @@  has_3src_unmapped_bits(const struct gen_device_info *devinfo,
       assert(!brw_inst_bits(src, 127, 126) &&
              !brw_inst_bits(src, 105, 105) &&
              !brw_inst_bits(src, 84, 84) &&
-             !brw_inst_bits(src, 36, 35) &&
              !brw_inst_bits(src, 7,  7));
+
+      /* Src1Type and Src2Type, used for mixed-precision floating point */
+      if (brw_inst_bits(src, 36, 35))
+         return true;
    }
 
    return false;

Comments

On Tue, Jan 15, 2019 at 7:55 AM Iago Toral Quiroga <itoral@igalia.com>
wrote:

> We are now using these bits, so don't assert that they are not set, just
> avoid compaction in that case.
>
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
> ---
>  src/intel/compiler/brw_eu_compact.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/intel/compiler/brw_eu_compact.c
> b/src/intel/compiler/brw_eu_compact.c
> index ae14ef10ec0..20fed254331 100644
> --- a/src/intel/compiler/brw_eu_compact.c
> +++ b/src/intel/compiler/brw_eu_compact.c
> @@ -928,8 +928,11 @@ has_3src_unmapped_bits(const struct gen_device_info
> *devinfo,
>        assert(!brw_inst_bits(src, 127, 126) &&
>               !brw_inst_bits(src, 105, 105) &&
>               !brw_inst_bits(src, 84, 84) &&
> -             !brw_inst_bits(src, 36, 35) &&
>               !brw_inst_bits(src, 7,  7));
> +
> +      /* Src1Type and Src2Type, used for mixed-precision floating point */
> +      if (brw_inst_bits(src, 36, 35))
> +         return true;
>

You're only doing this in the broadwell case.  What about SKL+ and CHV?
Can we compact mixed-precision stuff there?  Looks like maybe we can but
there should be at least something in the commit message about that.


>     }
>
>     return false;
> --
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
On Thu, 2019-01-17 at 14:14 -0600, Jason Ekstrand wrote:
> On Tue, Jan 15, 2019 at 7:55 AM Iago Toral Quiroga <itoral@igalia.com
> > wrote:
> > We are now using these bits, so don't assert that they are not set,
> > just
> > 
> > avoid compaction in that case.
> > 
> > 
> > 
> > Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
> > 
> > ---
> > 
> >  src/intel/compiler/brw_eu_compact.c | 5 ++++-
> > 
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > 
> > 
> > diff --git a/src/intel/compiler/brw_eu_compact.c
> > b/src/intel/compiler/brw_eu_compact.c
> > 
> > index ae14ef10ec0..20fed254331 100644
> > 
> > --- a/src/intel/compiler/brw_eu_compact.c
> > 
> > +++ b/src/intel/compiler/brw_eu_compact.c
> > 
> > @@ -928,8 +928,11 @@ has_3src_unmapped_bits(const struct
> > gen_device_info *devinfo,
> > 
> >        assert(!brw_inst_bits(src, 127, 126) &&
> > 
> >               !brw_inst_bits(src, 105, 105) &&
> > 
> >               !brw_inst_bits(src, 84, 84) &&
> > 
> > -             !brw_inst_bits(src, 36, 35) &&
> > 
> >               !brw_inst_bits(src, 7,  7));
> > 
> > +
> > 
> > +      /* Src1Type and Src2Type, used for mixed-precision floating
> > point */
> > 
> > +      if (brw_inst_bits(src, 36, 35))
> > 
> > +         return true;
> 
> You're only doing this in the broadwell case.  What about SKL+ and
> CHV?  Can we compact mixed-precision stuff there?  Looks like maybe
> we can but there should be at least something in the commit message
> about that.

In these platforms compaction is possible in some cases
and  set_3src_control_index() takescare of this by including these bits
in a tablre lookup for accepted combinations. I can add thisto the
commit message:
"We are now using these bits, so don't assert that they are not set. In
gen8, if these bits are setcompaction is not possible. On gen9 and CHV
platforms set_3src_control_index() checks thesebits (and others)
against a table to validate if the particular bit combination is
eligible forcompaction or not."
With that said, if I am reading this correctly, it looks like all
entries in gen8_3src_control_index_table that allow compaction require
these bits to be zero at present, so I guess that right now we could
also just extend the check I am adding here for BDW to other platforms,
however, I guess that relying on the array with accepted bit
combinations for compaction is more reliable should any future
platforms allow for more combinations.
> >     }
> > 
> > 
> > 
> >     return false;
> >
On January 18, 2019 04:12:44 Iago Toral <itoral@igalia.com> wrote:
> On Thu, 2019-01-17 at 14:14 -0600, Jason Ekstrand wrote:
>> On Tue, Jan 15, 2019 at 7:55 AM Iago Toral Quiroga <itoral@igalia.com> wrote:
>>> We are now using these bits, so don't assert that they are not set, just
>>> avoid compaction in that case.
>>>
>>> Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
>>> ---
>>> src/intel/compiler/brw_eu_compact.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/intel/compiler/brw_eu_compact.c 
>>> b/src/intel/compiler/brw_eu_compact.c
>>> index ae14ef10ec0..20fed254331 100644
>>> --- a/src/intel/compiler/brw_eu_compact.c
>>> +++ b/src/intel/compiler/brw_eu_compact.c
>>> @@ -928,8 +928,11 @@ has_3src_unmapped_bits(const struct gen_device_info 
>>> *devinfo,
>>>       assert(!brw_inst_bits(src, 127, 126) &&
>>>              !brw_inst_bits(src, 105, 105) &&
>>>              !brw_inst_bits(src, 84, 84) &&
>>> -             !brw_inst_bits(src, 36, 35) &&
>>>              !brw_inst_bits(src, 7,  7));
>>> +
>>> +      /* Src1Type and Src2Type, used for mixed-precision floating point */
>>> +      if (brw_inst_bits(src, 36, 35))
>>> +         return true;
>>
>> You're only doing this in the broadwell case.  What about SKL+ and CHV?  
>> Can we compact mixed-precision stuff there?  Looks like maybe we can but 
>> there should be at least something in the commit message about that.
>
> In these platforms compaction is possible in some cases and 
> set_3src_control_index() takes
> care of this by including these bits in a tablre lookup for accepted 
> combinations. I can add this
> to the commit message:
>
> "We are now using these bits, so don't assert that they are not set. In 
> gen8, if these bits are set
> compaction is not possible. On gen9 and CHV platforms 
> set_3src_control_index() checks these
> bits (and others) against a table to validate if the particular bit 
> combination is eligible for
> compaction or not."
>
> With that said, if I am reading this correctly, it looks like all entries 
> in gen8_3src_control_index_table that allow compaction require these bits 
> to be zero at present, so I guess that right now we could also just extend 
> the check I am adding here for BDW to other platforms, however, I guess 
> that relying on the array with accepted bit combinations for compaction is 
> more reliable should any future platforms allow for more combinations.

Sounds good. With that commit message update, r-b me.

>
>>>    }
>>>
>>>    return false;
On Tue, Jan 15, 2019 at 5:55 AM Iago Toral Quiroga <itoral@igalia.com> wrote:
>
> We are now using these bits, so don't assert that they are not set, just
> avoid compaction in that case.
>
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
> ---
>  src/intel/compiler/brw_eu_compact.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/intel/compiler/brw_eu_compact.c b/src/intel/compiler/brw_eu_compact.c
> index ae14ef10ec0..20fed254331 100644
> --- a/src/intel/compiler/brw_eu_compact.c
> +++ b/src/intel/compiler/brw_eu_compact.c
> @@ -928,8 +928,11 @@ has_3src_unmapped_bits(const struct gen_device_info *devinfo,
>        assert(!brw_inst_bits(src, 127, 126) &&
>               !brw_inst_bits(src, 105, 105) &&
>               !brw_inst_bits(src, 84, 84) &&
> -             !brw_inst_bits(src, 36, 35) &&
>               !brw_inst_bits(src, 7,  7));
> +
> +      /* Src1Type and Src2Type, used for mixed-precision floating point */
> +      if (brw_inst_bits(src, 36, 35))
> +         return true;
>     }


These bits are used on SKL+ and CHV (which is handled immediately
above this hunk), so this is only modifying BDW. All looks correct to
me.

Reviewed-by: Matt Turner <mattst88@gmail.com>