rs6000: Disable MMA if no P9 VECTOR support [PR103627]

  Hi,

As PR103627 shows, there is an unexpected case where !TARGET_VSX
and TARGET_MMA co-exist.  As ISA3.1 claims, SIMD is a requirement
for MMA.  By looking into the ICE, I noticed that the current
MMA implementation depends on vector pairs load/store, but since
we don't have a separated option to control Power10 vector, this
patch is to check for Power9 vector instead.

Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
powerpc64-linux-gnu P8.

Is it ok for trunk?

BR,
Kewen
-----
gcc/ChangeLog:

	PR target/103627
	* config/rs6000/rs6000.c (rs6000_option_override_internal): Disable
	MMA if !TARGET_P9_VECTOR.

gcc/testsuite/ChangeLog:

	PR target/103627
	* gcc.target/powerpc/pr103627-1.c: New test.
	* gcc.target/powerpc/pr103627-2.c: New test.
---
 gcc/config/rs6000/rs6000.c                    | 11 +++++++++++
 gcc/testsuite/gcc.target/powerpc/pr103627-1.c | 16 ++++++++++++++++
 gcc/testsuite/gcc.target/powerpc/pr103627-2.c | 16 ++++++++++++++++
 3 files changed, 43 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103627-1.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103627-2.c

--
2.27.0
  

Gentle ping:

https://gcc.gnu.org/pipermail/gcc-patches/2021-December/587310.html

on 2021/12/23 上午10:09, Kewen.Lin via Gcc-patches wrote:
> Hi,
> 
> As PR103627 shows, there is an unexpected case where !TARGET_VSX
> and TARGET_MMA co-exist.  As ISA3.1 claims, SIMD is a requirement
> for MMA.  By looking into the ICE, I noticed that the current
> MMA implementation depends on vector pairs load/store, but since
> we don't have a separated option to control Power10 vector, this
> patch is to check for Power9 vector instead.
> 
> Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
> powerpc64-linux-gnu P8.
> 
> Is it ok for trunk?
> 
> BR,
> Kewen
> -----
> gcc/ChangeLog:
> 
> 	PR target/103627
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Disable
> 	MMA if !TARGET_P9_VECTOR.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR target/103627
> 	* gcc.target/powerpc/pr103627-1.c: New test.
> 	* gcc.target/powerpc/pr103627-2.c: New test.
> ---
>  gcc/config/rs6000/rs6000.c                    | 11 +++++++++++
>  gcc/testsuite/gcc.target/powerpc/pr103627-1.c | 16 ++++++++++++++++
>  gcc/testsuite/gcc.target/powerpc/pr103627-2.c | 16 ++++++++++++++++
>  3 files changed, 43 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103627-1.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103627-2.c
> 
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index c020947abc8..ec3b46682a7 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -4505,6 +4505,17 @@ rs6000_option_override_internal (bool global_init_p)
>        rs6000_isa_flags &= ~OPTION_MASK_MMA;
>      }
> 
> +  /* MMA requires SIMD support as ISA 3.1 claims and our implementation
> +     such as "*movoo" uses vector pair access which are only supported
> +     from ISA 3.1.  But since we don't have one separated option to
> +     control Power10 vector, check for Power9 vector instead.  */
> +  if (TARGET_MMA && !TARGET_P9_VECTOR)
> +    {
> +      if ((rs6000_isa_flags_explicit & OPTION_MASK_MMA) != 0)
> +	error ("%qs requires %qs", "-mmma", "-mpower9-vector");
> +      rs6000_isa_flags &= ~OPTION_MASK_MMA;
> +    }
> +
>    if (!TARGET_PCREL && TARGET_PCREL_OPT)
>      rs6000_isa_flags &= ~OPTION_MASK_PCREL_OPT;
> 
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr103627-1.c b/gcc/testsuite/gcc.target/powerpc/pr103627-1.c
> new file mode 100644
> index 00000000000..6c6c16188fb
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr103627-1.c
> @@ -0,0 +1,16 @@
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-options "-mdejagnu-cpu=power10 -mno-power9-vector" } */
> +
> +/* Verify compiler emits error message instead of ICE.  */
> +
> +extern float *dest;
> +extern __vector_quad src;
> +
> +int
> +foo ()
> +{
> +  __builtin_mma_disassemble_acc (dest, &src);
> +  /* { dg-error "'__builtin_mma_disassemble_acc' requires the '-mmma' option" "" { target *-*-* } .-1 } */
> +  return 0;
> +}
> +
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr103627-2.c b/gcc/testsuite/gcc.target/powerpc/pr103627-2.c
> new file mode 100644
> index 00000000000..6604872c0e8
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr103627-2.c
> @@ -0,0 +1,16 @@
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-options "-mdejagnu-cpu=power10 -mmma -mno-power9-vector" } */
> +
> +/* Verify the emitted error message.  */
> +
> +extern float *dest;
> +extern __vector_quad src;
> +
> +int
> +foo ()
> +{
> +  __builtin_mma_disassemble_acc (dest, &src);
> +  /* { dg-error "'-mmma' requires '-mpower9-vector'" "mma" { target *-*-* } 0 } */
> +  return 0;
> +}
> +
> --
> 2.27.0
>
  

Gentle ping:

https://gcc.gnu.org/pipermail/gcc-patches/2021-December/587310.html

BR,
Kewen

> on 2021/12/23 上午10:09, Kewen.Lin via Gcc-patches wrote:
>> Hi,
>>
>> As PR103627 shows, there is an unexpected case where !TARGET_VSX
>> and TARGET_MMA co-exist.  As ISA3.1 claims, SIMD is a requirement
>> for MMA.  By looking into the ICE, I noticed that the current
>> MMA implementation depends on vector pairs load/store, but since
>> we don't have a separated option to control Power10 vector, this
>> patch is to check for Power9 vector instead.
>>
>> Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
>> powerpc64-linux-gnu P8.
>>
>> Is it ok for trunk?
>>
>> BR,
>> Kewen
>> -----
>> gcc/ChangeLog:
>>
>> 	PR target/103627
>> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Disable
>> 	MMA if !TARGET_P9_VECTOR.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR target/103627
>> 	* gcc.target/powerpc/pr103627-1.c: New test.
>> 	* gcc.target/powerpc/pr103627-2.c: New test.
>> ---
>>  gcc/config/rs6000/rs6000.c                    | 11 +++++++++++
>>  gcc/testsuite/gcc.target/powerpc/pr103627-1.c | 16 ++++++++++++++++
>>  gcc/testsuite/gcc.target/powerpc/pr103627-2.c | 16 ++++++++++++++++
>>  3 files changed, 43 insertions(+)
>>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103627-1.c
>>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103627-2.c
>>
>> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
>> index c020947abc8..ec3b46682a7 100644
>> --- a/gcc/config/rs6000/rs6000.c
>> +++ b/gcc/config/rs6000/rs6000.c
>> @@ -4505,6 +4505,17 @@ rs6000_option_override_internal (bool global_init_p)
>>        rs6000_isa_flags &= ~OPTION_MASK_MMA;
>>      }
>>
>> +  /* MMA requires SIMD support as ISA 3.1 claims and our implementation
>> +     such as "*movoo" uses vector pair access which are only supported
>> +     from ISA 3.1.  But since we don't have one separated option to
>> +     control Power10 vector, check for Power9 vector instead.  */
>> +  if (TARGET_MMA && !TARGET_P9_VECTOR)
>> +    {
>> +      if ((rs6000_isa_flags_explicit & OPTION_MASK_MMA) != 0)
>> +	error ("%qs requires %qs", "-mmma", "-mpower9-vector");
>> +      rs6000_isa_flags &= ~OPTION_MASK_MMA;
>> +    }
>> +
>>    if (!TARGET_PCREL && TARGET_PCREL_OPT)
>>      rs6000_isa_flags &= ~OPTION_MASK_PCREL_OPT;
>>
>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr103627-1.c b/gcc/testsuite/gcc.target/powerpc/pr103627-1.c
>> new file mode 100644
>> index 00000000000..6c6c16188fb
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr103627-1.c
>> @@ -0,0 +1,16 @@
>> +/* { dg-require-effective-target power10_ok } */
>> +/* { dg-options "-mdejagnu-cpu=power10 -mno-power9-vector" } */
>> +
>> +/* Verify compiler emits error message instead of ICE.  */
>> +
>> +extern float *dest;
>> +extern __vector_quad src;
>> +
>> +int
>> +foo ()
>> +{
>> +  __builtin_mma_disassemble_acc (dest, &src);
>> +  /* { dg-error "'__builtin_mma_disassemble_acc' requires the '-mmma' option" "" { target *-*-* } .-1 } */
>> +  return 0;
>> +}
>> +
>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr103627-2.c b/gcc/testsuite/gcc.target/powerpc/pr103627-2.c
>> new file mode 100644
>> index 00000000000..6604872c0e8
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr103627-2.c
>> @@ -0,0 +1,16 @@
>> +/* { dg-require-effective-target power10_ok } */
>> +/* { dg-options "-mdejagnu-cpu=power10 -mmma -mno-power9-vector" } */
>> +
>> +/* Verify the emitted error message.  */
>> +
>> +extern float *dest;
>> +extern __vector_quad src;
>> +
>> +int
>> +foo ()
>> +{
>> +  __builtin_mma_disassemble_acc (dest, &src);
>> +  /* { dg-error "'-mmma' requires '-mpower9-vector'" "mma" { target *-*-* } 0 } */
>> +  return 0;
>> +}
>> +
>> --
>> 2.27.0
>>
  

Hi!

On Thu, Dec 23, 2021 at 10:09:27AM +0800, Kewen.Lin wrote:
> As PR103627 shows, there is an unexpected case where !TARGET_VSX
> and TARGET_MMA co-exist.  As ISA3.1 claims, SIMD is a requirement
> for MMA.  By looking into the ICE, I noticed that the current
> MMA implementation depends on vector pairs load/store, but since
> we don't have a separated option to control Power10 vector, this
> patch is to check for Power9 vector instead.
> 
> Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
> powerpc64-linux-gnu P8.
> 
> Is it ok for trunk?

No, sorry.

> +  /* MMA requires SIMD support as ISA 3.1 claims and our implementation
> +     such as "*movoo" uses vector pair access which are only supported
> +     from ISA 3.1.  But since we don't have one separated option to
> +     control Power10 vector, check for Power9 vector instead.  */
> +  if (TARGET_MMA && !TARGET_P9_VECTOR)
> +    {
> +      if ((rs6000_isa_flags_explicit & OPTION_MASK_MMA) != 0)
> +	error ("%qs requires %qs", "-mmma", "-mpower9-vector");
> +      rs6000_isa_flags &= ~OPTION_MASK_MMA;
> +    }

-mpower9-vector is a workaround that should go away.  TARGET_MMA should
require ISA 3.1 (or POWER10) directly, and require VSX.

> +/* { dg-options "-mdejagnu-cpu=power10 -mno-power9-vector" } */

It should be impossible to select this at all.  Either you have vectors
or you don't, but it should be impossible to selectively disable part of
vector support.  That way madness lies.

We can allow no VSRs, only VRs, or all VSRs.  There is precedent for
that (see -msoft-float for example, which means "do not use FPRs") --
when compiling certain code we want to disallow whole register banks.
But disallowing or allowing some instructions separately is not a good
idea: it doesn't give any useful extra functionality, it is hard to use,
and it is a lot of extra work for us (it is impossible to test, already,
too many combinations).


Segher
  

Hi Segher,

on 2022/1/27 上午6:19, Segher Boessenkool wrote:
> Hi!
> 
> On Thu, Dec 23, 2021 at 10:09:27AM +0800, Kewen.Lin wrote:
>> As PR103627 shows, there is an unexpected case where !TARGET_VSX
>> and TARGET_MMA co-exist.  As ISA3.1 claims, SIMD is a requirement
>> for MMA.  By looking into the ICE, I noticed that the current
>> MMA implementation depends on vector pairs load/store, but since
>> we don't have a separated option to control Power10 vector, this
>> patch is to check for Power9 vector instead.
>>
>> Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
>> powerpc64-linux-gnu P8.
>>
>> Is it ok for trunk?
> 
> No, sorry.
> 
>> +  /* MMA requires SIMD support as ISA 3.1 claims and our implementation
>> +     such as "*movoo" uses vector pair access which are only supported
>> +     from ISA 3.1.  But since we don't have one separated option to
>> +     control Power10 vector, check for Power9 vector instead.  */
>> +  if (TARGET_MMA && !TARGET_P9_VECTOR)
>> +    {
>> +      if ((rs6000_isa_flags_explicit & OPTION_MASK_MMA) != 0)
>> +	error ("%qs requires %qs", "-mmma", "-mpower9-vector");
>> +      rs6000_isa_flags &= ~OPTION_MASK_MMA;
>> +    }
> 
> -mpower9-vector is a workaround that should go away.  TARGET_MMA should
> require ISA 3.1 (or POWER10) directly, and require VSX.


OK, I see.  Thanks for the detailed explanation below.  I guess that's why
we don't have one option like "-mpower10-vector" any more?

I posted patch v2 guarded with VSX at the below link instead
 
https://gcc.gnu.org/pipermail/gcc-patches/2022-January/589325.html
 
Hope it looks better to you.  :) 

BR,
Kewen

> 
>> +/* { dg-options "-mdejagnu-cpu=power10 -mno-power9-vector" } */
> 
> It should be impossible to select this at all.  Either you have vectors
> or you don't, but it should be impossible to selectively disable part of
> vector support.  That way madness lies.
> 
> We can allow no VSRs, only VRs, or all VSRs.  There is precedent for
> that (see -msoft-float for example, which means "do not use FPRs") --
> when compiling certain code we want to disallow whole register banks.
> But disallowing or allowing some instructions separately is not a good
> idea: it doesn't give any useful extra functionality, it is hard to use,
> and it is a lot of extra work for us (it is impossible to test, already,
> too many combinations).
> 
> 
> Segher
>