rs6000: Move the hunk affecting VSX/ALTIVEC ahead [PR103627]

  Hi,

There is one hunk checking for functions with target attribute/pragma
have the same altivec abi as the one of main_target_opt, it can update
both VSX and ALTIVEC flags.  Meanwhile, we have some codes to check or
warn for some isa flags related to VSX and ALTIVEC, that sit where the
mentioned hunk is proposed to be moved to in this patch.

Since the flags update in the mentioned hunk happen behind those
adjustments based on VSX and ALTIVEC flags, it can cause the
incompatibility and result in unexpected behaviors, the associated test
case is one typical case.

Besides, we already have the code which sets TARGET_FLOAT128_TYPE and
lays after where the hunk is moved to, and OPTION_MASK_FLOAT128_KEYWORD
will rely on TARGET_FLOAT128_TYPE, so this patch just simply removes them.

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

Is it ok for trunk?

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

	PR target/103627
	* config/rs6000/rs6000.c (rs6000_option_override_internal): Move the
	hunk affecting VSX and ALTIVEC to the appropriate place.

gcc/testsuite/ChangeLog:

	PR target/103627
	* gcc.target/powerpc/pr103627-3.c: New test.
---
 gcc/config/rs6000/rs6000.c                    | 21 ++++++++-----------
 gcc/testsuite/gcc.target/powerpc/pr103627-3.c | 20 ++++++++++++++++++
 2 files changed, 29 insertions(+), 12 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103627-3.c

--
2.27.0
  

Gentle ping:

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

on 2021/12/23 上午10:12, Kewen.Lin via Gcc-patches wrote:
> Hi,
> 
> There is one hunk checking for functions with target attribute/pragma
> have the same altivec abi as the one of main_target_opt, it can update
> both VSX and ALTIVEC flags.  Meanwhile, we have some codes to check or
> warn for some isa flags related to VSX and ALTIVEC, that sit where the
> mentioned hunk is proposed to be moved to in this patch.
> 
> Since the flags update in the mentioned hunk happen behind those
> adjustments based on VSX and ALTIVEC flags, it can cause the
> incompatibility and result in unexpected behaviors, the associated test
> case is one typical case.
> 
> Besides, we already have the code which sets TARGET_FLOAT128_TYPE and
> lays after where the hunk is moved to, and OPTION_MASK_FLOAT128_KEYWORD
> will rely on TARGET_FLOAT128_TYPE, so this patch just simply removes them.
> 
> Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
> powerpc64-linux-gnu P8 and P7.
> 
> Is it ok for trunk?
> 
> BR,
> Kewen
> -----
> gcc/ChangeLog:
> 
> 	PR target/103627
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Move the
> 	hunk affecting VSX and ALTIVEC to the appropriate place.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR target/103627
> 	* gcc.target/powerpc/pr103627-3.c: New test.
> ---
>  gcc/config/rs6000/rs6000.c                    | 21 ++++++++-----------
>  gcc/testsuite/gcc.target/powerpc/pr103627-3.c | 20 ++++++++++++++++++
>  2 files changed, 29 insertions(+), 12 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103627-3.c
> 
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index ec3b46682a7..0b09713b2f5 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -3955,6 +3955,15 @@ rs6000_option_override_internal (bool global_init_p)
>    else if (TARGET_ALTIVEC)
>      rs6000_isa_flags |= (OPTION_MASK_PPC_GFXOPT & ~ignore_masks);
> 
> +  /* Disable VSX and Altivec silently if the user switched cpus to power7 in a
> +     target attribute or pragma which automatically enables both options,
> +     unless the altivec ABI was set.  This is set by default for 64-bit, but
> +     not for 32-bit.  Don't move this before the above code using ignore_masks,
> +     since it can reset the cleared VSX/ALTIVEC flag again.  */
> +  if (main_target_opt != NULL && !main_target_opt->x_rs6000_altivec_abi)
> +    rs6000_isa_flags &= ~((OPTION_MASK_VSX | OPTION_MASK_ALTIVEC)
> +			  & ~rs6000_isa_flags_explicit);
> +
>    if (TARGET_CRYPTO && !TARGET_ALTIVEC)
>      {
>        if (rs6000_isa_flags_explicit & OPTION_MASK_CRYPTO)
> @@ -4373,18 +4382,6 @@ rs6000_option_override_internal (bool global_init_p)
>  	}
>      }
> 
> -  /* Disable VSX and Altivec silently if the user switched cpus to power7 in a
> -     target attribute or pragma which automatically enables both options,
> -     unless the altivec ABI was set.  This is set by default for 64-bit, but
> -     not for 32-bit.  */
> -  if (main_target_opt != NULL && !main_target_opt->x_rs6000_altivec_abi)
> -    {
> -      TARGET_FLOAT128_TYPE = 0;
> -      rs6000_isa_flags &= ~((OPTION_MASK_VSX | OPTION_MASK_ALTIVEC
> -			     | OPTION_MASK_FLOAT128_KEYWORD)
> -			    & ~rs6000_isa_flags_explicit);
> -    }
> -
>    /* Enable Altivec ABI for AIX -maltivec.  */
>    if (TARGET_XCOFF
>        && (TARGET_ALTIVEC || TARGET_VSX)
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr103627-3.c b/gcc/testsuite/gcc.target/powerpc/pr103627-3.c
> new file mode 100644
> index 00000000000..9df2b73fe85
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr103627-3.c
> @@ -0,0 +1,20 @@
> +/* There are no error messages for either LE or BE 64bit.  */
> +/* { dg-require-effective-target be }*/
> +/* { dg-require-effective-target ilp32 } */
> +/* We don't have one powerpc.*_ok for Power6, use altivec_ok conservatively.  */
> +/* { dg-require-effective-target powerpc_altivec_ok } */
> +/* { dg-options "-mdejagnu-cpu=power6" } */
> +
> +/* Verify compiler emits error message instead of ICE.  */
> +
> +#pragma GCC target "cpu=power10"
> +int
> +main ()
> +{
> +  float *b;
> +  __vector_quad c;
> +  __builtin_mma_disassemble_acc (b, &c);
> +  /* { dg-error "'__builtin_mma_disassemble_acc' requires the '-mmma' option" "" { target *-*-* } .-1 } */
> +  return 0;
> +}
> +
> --
> 2.27.0
>
  

Gentle ping:

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

BR,
Kewen

> on 2021/12/23 上午10:12, Kewen.Lin via Gcc-patches wrote:
>> Hi,
>>
>> There is one hunk checking for functions with target attribute/pragma
>> have the same altivec abi as the one of main_target_opt, it can update
>> both VSX and ALTIVEC flags.  Meanwhile, we have some codes to check or
>> warn for some isa flags related to VSX and ALTIVEC, that sit where the
>> mentioned hunk is proposed to be moved to in this patch.
>>
>> Since the flags update in the mentioned hunk happen behind those
>> adjustments based on VSX and ALTIVEC flags, it can cause the
>> incompatibility and result in unexpected behaviors, the associated test
>> case is one typical case.
>>
>> Besides, we already have the code which sets TARGET_FLOAT128_TYPE and
>> lays after where the hunk is moved to, and OPTION_MASK_FLOAT128_KEYWORD
>> will rely on TARGET_FLOAT128_TYPE, so this patch just simply removes them.
>>
>> Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
>> powerpc64-linux-gnu P8 and P7.
>>
>> Is it ok for trunk?
>>
>> BR,
>> Kewen
>> -----
>> gcc/ChangeLog:
>>
>> 	PR target/103627
>> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Move the
>> 	hunk affecting VSX and ALTIVEC to the appropriate place.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR target/103627
>> 	* gcc.target/powerpc/pr103627-3.c: New test.
>> ---
>>  gcc/config/rs6000/rs6000.c                    | 21 ++++++++-----------
>>  gcc/testsuite/gcc.target/powerpc/pr103627-3.c | 20 ++++++++++++++++++
>>  2 files changed, 29 insertions(+), 12 deletions(-)
>>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103627-3.c
>>
>> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
>> index ec3b46682a7..0b09713b2f5 100644
>> --- a/gcc/config/rs6000/rs6000.c
>> +++ b/gcc/config/rs6000/rs6000.c
>> @@ -3955,6 +3955,15 @@ rs6000_option_override_internal (bool global_init_p)
>>    else if (TARGET_ALTIVEC)
>>      rs6000_isa_flags |= (OPTION_MASK_PPC_GFXOPT & ~ignore_masks);
>>
>> +  /* Disable VSX and Altivec silently if the user switched cpus to power7 in a
>> +     target attribute or pragma which automatically enables both options,
>> +     unless the altivec ABI was set.  This is set by default for 64-bit, but
>> +     not for 32-bit.  Don't move this before the above code using ignore_masks,
>> +     since it can reset the cleared VSX/ALTIVEC flag again.  */
>> +  if (main_target_opt != NULL && !main_target_opt->x_rs6000_altivec_abi)
>> +    rs6000_isa_flags &= ~((OPTION_MASK_VSX | OPTION_MASK_ALTIVEC)
>> +			  & ~rs6000_isa_flags_explicit);
>> +
>>    if (TARGET_CRYPTO && !TARGET_ALTIVEC)
>>      {
>>        if (rs6000_isa_flags_explicit & OPTION_MASK_CRYPTO)
>> @@ -4373,18 +4382,6 @@ rs6000_option_override_internal (bool global_init_p)
>>  	}
>>      }
>>
>> -  /* Disable VSX and Altivec silently if the user switched cpus to power7 in a
>> -     target attribute or pragma which automatically enables both options,
>> -     unless the altivec ABI was set.  This is set by default for 64-bit, but
>> -     not for 32-bit.  */
>> -  if (main_target_opt != NULL && !main_target_opt->x_rs6000_altivec_abi)
>> -    {
>> -      TARGET_FLOAT128_TYPE = 0;
>> -      rs6000_isa_flags &= ~((OPTION_MASK_VSX | OPTION_MASK_ALTIVEC
>> -			     | OPTION_MASK_FLOAT128_KEYWORD)
>> -			    & ~rs6000_isa_flags_explicit);
>> -    }
>> -
>>    /* Enable Altivec ABI for AIX -maltivec.  */
>>    if (TARGET_XCOFF
>>        && (TARGET_ALTIVEC || TARGET_VSX)
>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr103627-3.c b/gcc/testsuite/gcc.target/powerpc/pr103627-3.c
>> new file mode 100644
>> index 00000000000..9df2b73fe85
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr103627-3.c
>> @@ -0,0 +1,20 @@
>> +/* There are no error messages for either LE or BE 64bit.  */
>> +/* { dg-require-effective-target be }*/
>> +/* { dg-require-effective-target ilp32 } */
>> +/* We don't have one powerpc.*_ok for Power6, use altivec_ok conservatively.  */
>> +/* { dg-require-effective-target powerpc_altivec_ok } */
>> +/* { dg-options "-mdejagnu-cpu=power6" } */
>> +
>> +/* Verify compiler emits error message instead of ICE.  */
>> +
>> +#pragma GCC target "cpu=power10"
>> +int
>> +main ()
>> +{
>> +  float *b;
>> +  __vector_quad c;
>> +  __builtin_mma_disassemble_acc (b, &c);
>> +  /* { dg-error "'__builtin_mma_disassemble_acc' requires the '-mmma' option" "" { target *-*-* } .-1 } */
>> +  return 0;
>> +}
>> +
>> --
>> 2.27.0
>>
  

Hi!

On Thu, Dec 23, 2021 at 10:12:19AM +0800, Kewen.Lin wrote:
> 	PR target/103627
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Move the
> 	hunk affecting VSX and ALTIVEC to the appropriate place.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR target/103627
> 	* gcc.target/powerpc/pr103627-3.c: New test.


> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -3955,6 +3955,15 @@ rs6000_option_override_internal (bool global_init_p)
>    else if (TARGET_ALTIVEC)
>      rs6000_isa_flags |= (OPTION_MASK_PPC_GFXOPT & ~ignore_masks);
> 
> +  /* Disable VSX and Altivec silently if the user switched cpus to power7 in a
> +     target attribute or pragma which automatically enables both options,
> +     unless the altivec ABI was set.  This is set by default for 64-bit, but
> +     not for 32-bit.  Don't move this before the above code using ignore_masks,
> +     since it can reset the cleared VSX/ALTIVEC flag again.  */
> +  if (main_target_opt != NULL && !main_target_opt->x_rs6000_altivec_abi)
> +    rs6000_isa_flags &= ~((OPTION_MASK_VSX | OPTION_MASK_ALTIVEC)
> +			  & ~rs6000_isa_flags_explicit);

Could you at the same time get rid of the != NULL please?
  if (bla != NULL)
is sillier than
  if (bla != 0)
which is about the same as
  if (!!bla)
but that is certainly better than
  if (bla != 0 != 0)
although I am not sure about the more stylish
  if (bla != 0 != 0 != 0 != 0 != 0)
but what is wrong with
  if (bla)
?  :-)

> +/* There are no error messages for either LE or BE 64bit.  */
> +/* { dg-require-effective-target be }*/

(space before */)

> +/* { dg-require-effective-target ilp32 } */
> +/* We don't have one powerpc.*_ok for Power6, use altivec_ok conservatively.  */
> +/* { dg-require-effective-target powerpc_altivec_ok } */
> +/* { dg-options "-mdejagnu-cpu=power6" } */

It is okay always, no _ok at all please.

Okay for trunk with those things (but do test of course).  Thanks!


Segher
  

Hi Segher,

on 2022/1/27 上午6:42, Segher Boessenkool wrote:
> Hi!
> 
> On Thu, Dec 23, 2021 at 10:12:19AM +0800, Kewen.Lin wrote:
>> 	PR target/103627
>> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Move the
>> 	hunk affecting VSX and ALTIVEC to the appropriate place.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR target/103627
>> 	* gcc.target/powerpc/pr103627-3.c: New test.
> 
> 
>> --- a/gcc/config/rs6000/rs6000.c
>> +++ b/gcc/config/rs6000/rs6000.c
>> @@ -3955,6 +3955,15 @@ rs6000_option_override_internal (bool global_init_p)
>>    else if (TARGET_ALTIVEC)
>>      rs6000_isa_flags |= (OPTION_MASK_PPC_GFXOPT & ~ignore_masks);
>>
>> +  /* Disable VSX and Altivec silently if the user switched cpus to power7 in a
>> +     target attribute or pragma which automatically enables both options,
>> +     unless the altivec ABI was set.  This is set by default for 64-bit, but
>> +     not for 32-bit.  Don't move this before the above code using ignore_masks,
>> +     since it can reset the cleared VSX/ALTIVEC flag again.  */
>> +  if (main_target_opt != NULL && !main_target_opt->x_rs6000_altivec_abi)
>> +    rs6000_isa_flags &= ~((OPTION_MASK_VSX | OPTION_MASK_ALTIVEC)
>> +			  & ~rs6000_isa_flags_explicit);
> 
> Could you at the same time get rid of the != NULL please?
>   if (bla != NULL)
> is sillier than
>   if (bla != 0)
> which is about the same as
>   if (!!bla)
> but that is certainly better than
>   if (bla != 0 != 0)
> although I am not sure about the more stylish
>   if (bla != 0 != 0 != 0 != 0 != 0)
> but what is wrong with
>   if (bla)
> ?  :-)
> 
>> +/* There are no error messages for either LE or BE 64bit.  */
>> +/* { dg-require-effective-target be }*/
> 
> (space before */)
> 
>> +/* { dg-require-effective-target ilp32 } */
>> +/* We don't have one powerpc.*_ok for Power6, use altivec_ok conservatively.  */
>> +/* { dg-require-effective-target powerpc_altivec_ok } */
>> +/* { dg-options "-mdejagnu-cpu=power6" } */
> 
> It is okay always, no _ok at all please.
> 
> Okay for trunk with those things (but do test of course).  Thanks!
> 
> 

Thanks for the review comments!  I've addressed them in the attached patch.
Besides, I updated the case a bit by adding -mno-avoid-indexed-addresses,
otherwise the associated case would have one unexpected warning which is
supposed to be fixed by [1].  But even with this adjustment, this patch
still relies on [2], otherwise the associated case will get ICE instead.
btw, it can survives in all the testings as before together with [2].

So I'll hold this to commit until [2] gets landed.

[1] https://gcc.gnu.org/pipermail/gcc-patches/2021-December/587449.html
[2] https://gcc.gnu.org/pipermail/gcc-patches/2022-January/589325.html

BR,
Kewen
From 90565d869395df08a65d521ddad8684a99e844a5 Mon Sep 17 00:00:00 2001
From: Kewen Lin <linkw@linux.ibm.com>
Date: Thu, 27 Jan 2022 00:25:29 -0600
Subject: [PATCH 3/3] rs6000: Move the hunk affecting VSX/ALTIVEC ahead
 [PR103627]

The modified hunk can update VSX and ALTIVEC flag, we have some codes
to check/warn for some flags related to VSX and ALTIVEC sitting where
the hunk is proprosed to be moved to.  Without this adjustment, the
VSX and ALTIVEC update is too late, it can cause the incompatibility
and result in unexpected behaviors, the associated test case is one
typical case.

Since we already have the code which sets TARGET_FLOAT128_TYPE and lays
after the moved place, and OPTION_MASK_FLOAT128_KEYWORD will rely on
TARGET_FLOAT128_TYPE, so it just simply remove them.

gcc/ChangeLog:

	PR target/103627
	* config/rs6000/rs6000.cc (rs6000_option_override_internal): Move the
	hunk affecting VSX and ALTIVEC to appropriate place.

gcc/testsuite/ChangeLog:

	PR target/103627
	* gcc.target/powerpc/pr103627-3.c: New test.
---
 gcc/config/rs6000/rs6000.cc                   | 21 ++++++++-----------
 gcc/testsuite/gcc.target/powerpc/pr103627-3.c | 20 ++++++++++++++++++
 2 files changed, 29 insertions(+), 12 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103627-3.c

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index 634937e052f..f90fd8955cd 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -3943,6 +3943,15 @@ rs6000_option_override_internal (bool global_init_p)
   else if (TARGET_ALTIVEC)
     rs6000_isa_flags |= (OPTION_MASK_PPC_GFXOPT & ~ignore_masks);
 
+  /* Disable VSX and Altivec silently if the user switched cpus to power7 in a
+     target attribute or pragma which automatically enables both options,
+     unless the altivec ABI was set.  This is set by default for 64-bit, but
+     not for 32-bit.  Don't move this before the above code using ignore_masks,
+     since it can reset the cleared VSX/ALTIVEC flag again.  */
+  if (main_target_opt && !main_target_opt->x_rs6000_altivec_abi)
+    rs6000_isa_flags &= ~((OPTION_MASK_VSX | OPTION_MASK_ALTIVEC)
+			  & ~rs6000_isa_flags_explicit);
+
   if (TARGET_CRYPTO && !TARGET_ALTIVEC)
     {
       if (rs6000_isa_flags_explicit & OPTION_MASK_CRYPTO)
@@ -4359,18 +4368,6 @@ rs6000_option_override_internal (bool global_init_p)
 	}
     }
 
-  /* Disable VSX and Altivec silently if the user switched cpus to power7 in a
-     target attribute or pragma which automatically enables both options,
-     unless the altivec ABI was set.  This is set by default for 64-bit, but
-     not for 32-bit.  */
-  if (main_target_opt != NULL && !main_target_opt->x_rs6000_altivec_abi)
-    {
-      TARGET_FLOAT128_TYPE = 0;
-      rs6000_isa_flags &= ~((OPTION_MASK_VSX | OPTION_MASK_ALTIVEC
-			     | OPTION_MASK_FLOAT128_KEYWORD)
-			    & ~rs6000_isa_flags_explicit);
-    }
-
   /* Enable Altivec ABI for AIX -maltivec.  */
   if (TARGET_XCOFF
       && (TARGET_ALTIVEC || TARGET_VSX)
diff --git a/gcc/testsuite/gcc.target/powerpc/pr103627-3.c b/gcc/testsuite/gcc.target/powerpc/pr103627-3.c
new file mode 100644
index 00000000000..5a4d5ba5701
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr103627-3.c
@@ -0,0 +1,20 @@
+/* There are no error messages for either LE or BE 64bit.  */
+/* { dg-require-effective-target be } */
+/* { dg-require-effective-target ilp32 } */
+/* { dg-options "-mdejagnu-cpu=power6" } */
+
+/* Verify compiler emits error message instead of ICE.  */
+
+/* Option -mno-avoid-indexed-addresses is to disable the unexpected
+   warning on indexed addressing which can affect dg checks.  */
+#pragma GCC target "cpu=power10,no-avoid-indexed-addresses"
+int
+main ()
+{
+  float *b;
+  __vector_quad c;
+  __builtin_mma_disassemble_acc (b, &c);
+  /* { dg-error "'__builtin_mma_disassemble_acc' requires the '-mmma' option" "" { target *-*-* } .-1 } */
+  return 0;
+}
+