[v2] rs6000: Rework option -mpowerpc64 handling [PR106680]

  Hi,

PR106680 shows that -m32 -mpowerpc64 is different from
-mpowerpc64 -m32, this is determined by the way how we
handle option powerpc64 in rs6000_handle_option.

Segher pointed out this difference should be taken as
a bug and we should ensure that option powerpc64 is
independent of -m32/-m64.  So this patch removes the
handlings in rs6000_handle_option and add some necessary
supports in rs6000_option_override_internal instead.

With this patch, if users specify -m{no-,}powerpc64, the
specified value is honoured, otherwise, for 64bit it
always enables OPTION_MASK_POWERPC64; while for 32bit
and TARGET_POWERPC64 and OS_MISSING_POWERPC64, it disables
OPTION_MASK_POWERPC64.

btw, following Segher's suggestion, I did some tries to warn
when OPTION_MASK_POWERPC64 is set for OS_MISSING_POWERPC64.
If warn for the case that powerpc64 is specified explicitly,
there are some TCs using -m32 -mpowerpc64 on ppc64-linux,
they need some updates, meanwhile the artificial run
with "--target_board=unix'{-m32/-mpowerpc64}'" will have
noisy warnings on ppc64-linux.  If warn for the case that
it's specified implicitly, they can just be initialized by
TARGET_DEFAULT (like -m32 on ppc64-linux) or set from the 
given cpu mask, we have to special case them and not to warn.
As Segher's latest comment, I decide not to warn them and
keep it consistent with before.

Bootstrapped and regress-tested on:
  - powerpc64-linux-gnu P7 and P8 {-m64,-m32}
  - powerpc64le-linux-gnu P9 and P10
  - powerpc-ibm-aix7.2.0.0 {-maix64,-maix32}

Hi Iain, could you help to test this new patch on darwin
again?  Thanks in advance!

Is it ok for trunk if darwin testing goes well?

BR,
Kewen
-----
	PR target/106680

gcc/ChangeLog:

	* common/config/rs6000/rs6000-common.cc (rs6000_handle_option): Remove
	the adjustment for option powerpc64 in -m64 handling, and remove the
	whole -m32 handling.
	* config/rs6000/rs6000.cc (rs6000_option_override_internal): When no
	explicit powerpc64 option is provided, enable it for -m64.  For 32 bit
	and OS_MISSING_POWERPC64, disable powerpc64 if it's enabled but not
	specified explicitly.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/pr106680-1.c: New test.
	* gcc.target/powerpc/pr106680-2.c: New test.
	* gcc.target/powerpc/pr106680-3.c: New test.
	* gcc.target/powerpc/pr106680-4.c: New test.

2022-10-12  Kewen Lin  <linkw@linux.ibm.com>
	    Iain Sandoe  <iain@sandoe.co.uk>
---
 gcc/common/config/rs6000/rs6000-common.cc     | 11 ------
 gcc/config/rs6000/rs6000.cc                   | 37 ++++++++++++++-----
 gcc/testsuite/gcc.target/powerpc/pr106680-1.c | 13 +++++++
 gcc/testsuite/gcc.target/powerpc/pr106680-2.c | 14 +++++++
 gcc/testsuite/gcc.target/powerpc/pr106680-3.c | 13 +++++++
 gcc/testsuite/gcc.target/powerpc/pr106680-4.c | 17 +++++++++
 6 files changed, 85 insertions(+), 20 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106680-1.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106680-2.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106680-3.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106680-4.c

--
2.27.0
  

Hi Kewen,

> On 12 Oct 2022, at 09:12, Kewen.Lin <linkw@linux.ibm.com> wrote:

> PR106680 shows that -m32 -mpowerpc64 is different from
> -mpowerpc64 -m32, this is determined by the way how we
> handle option powerpc64 in rs6000_handle_option.
> 
> Segher pointed out this difference should be taken as
> a bug and we should ensure that option powerpc64 is
> independent of -m32/-m64.  So this patch removes the
> handlings in rs6000_handle_option and add some necessary
> supports in rs6000_option_override_internal instead.
> 
> With this patch, if users specify -m{no-,}powerpc64, the
> specified value is honoured, otherwise, for 64bit it
> always enables OPTION_MASK_POWERPC64; while for 32bit
> and TARGET_POWERPC64 and OS_MISSING_POWERPC64, it disables
> OPTION_MASK_POWERPC64.
> 
> btw, following Segher's suggestion, I did some tries to warn
> when OPTION_MASK_POWERPC64 is set for OS_MISSING_POWERPC64.
> If warn for the case that powerpc64 is specified explicitly,
> there are some TCs using -m32 -mpowerpc64 on ppc64-linux,
> they need some updates, meanwhile the artificial run
> with "--target_board=unix'{-m32/-mpowerpc64}'" will have
> noisy warnings on ppc64-linux.  If warn for the case that
> it's specified implicitly, they can just be initialized by
> TARGET_DEFAULT (like -m32 on ppc64-linux) or set from the 
> given cpu mask, we have to special case them and not to warn.
> As Segher's latest comment, I decide not to warn them and
> keep it consistent with before.
> 
> Bootstrapped and regress-tested on:
>  - powerpc64-linux-gnu P7 and P8 {-m64,-m32}
>  - powerpc64le-linux-gnu P9 and P10
>  - powerpc-ibm-aix7.2.0.0 {-maix64,-maix32}
> 
> Hi Iain, could you help to test this new patch on darwin
> again?  Thanks in advance!

I kicked off a bootstrap - and 'check-gcc-c' .. if all goes well, there will be an 
answer in ≈ 7hours.  If something fails, the answer will be sooner ;)
cheers
Iain

> 
> Is it ok for trunk if darwin testing goes well?
> 
> BR,
> Kewen
> -----
> 	PR target/106680
> 
> gcc/ChangeLog:
> 
> 	* common/config/rs6000/rs6000-common.cc (rs6000_handle_option): Remove
> 	the adjustment for option powerpc64 in -m64 handling, and remove the
> 	whole -m32 handling.
> 	* config/rs6000/rs6000.cc (rs6000_option_override_internal): When no
> 	explicit powerpc64 option is provided, enable it for -m64.  For 32 bit
> 	and OS_MISSING_POWERPC64, disable powerpc64 if it's enabled but not
> 	specified explicitly.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/powerpc/pr106680-1.c: New test.
> 	* gcc.target/powerpc/pr106680-2.c: New test.
> 	* gcc.target/powerpc/pr106680-3.c: New test.
> 	* gcc.target/powerpc/pr106680-4.c: New test.
> 
> 2022-10-12  Kewen Lin  <linkw@linux.ibm.com>
> 	    Iain Sandoe  <iain@sandoe.co.uk>
> ---
> gcc/common/config/rs6000/rs6000-common.cc     | 11 ------
> gcc/config/rs6000/rs6000.cc                   | 37 ++++++++++++++-----
> gcc/testsuite/gcc.target/powerpc/pr106680-1.c | 13 +++++++
> gcc/testsuite/gcc.target/powerpc/pr106680-2.c | 14 +++++++
> gcc/testsuite/gcc.target/powerpc/pr106680-3.c | 13 +++++++
> gcc/testsuite/gcc.target/powerpc/pr106680-4.c | 17 +++++++++
> 6 files changed, 85 insertions(+), 20 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106680-1.c
> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106680-2.c
> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106680-3.c
> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106680-4.c
> 
> diff --git a/gcc/common/config/rs6000/rs6000-common.cc b/gcc/common/config/rs6000/rs6000-common.cc
> index 8e393d08a23..c76b5c27bb6 100644
> --- a/gcc/common/config/rs6000/rs6000-common.cc
> +++ b/gcc/common/config/rs6000/rs6000-common.cc
> @@ -119,19 +119,8 @@ rs6000_handle_option (struct gcc_options *opts, struct gcc_options *opts_set,
> #else
>     case OPT_m64:
> #endif
> -      opts->x_rs6000_isa_flags |= OPTION_MASK_POWERPC64;
>       opts->x_rs6000_isa_flags |= (~opts_set->x_rs6000_isa_flags
> 				   & OPTION_MASK_PPC_GFXOPT);
> -      opts_set->x_rs6000_isa_flags |= OPTION_MASK_POWERPC64;
> -      break;
> -
> -#ifdef TARGET_USES_AIX64_OPT
> -    case OPT_maix32:
> -#else
> -    case OPT_m32:
> -#endif
> -      opts->x_rs6000_isa_flags &= ~OPTION_MASK_POWERPC64;
> -      opts_set->x_rs6000_isa_flags |= OPTION_MASK_POWERPC64;
>       break;
> 
>     case OPT_mminimal_toc:
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index e6fa3ad0eb7..e37d99deb61 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -3648,17 +3648,12 @@ rs6000_option_override_internal (bool global_init_p)
>       rs6000_pointer_size = 32;
>     }
> 
> -  /* Some OSs don't support saving the high part of 64-bit registers on context
> -     switch.  Other OSs don't support saving Altivec registers.  On those OSs,
> -     we don't touch the OPTION_MASK_POWERPC64 or OPTION_MASK_ALTIVEC settings;
> -     if the user wants either, the user must explicitly specify them and we
> -     won't interfere with the user's specification.  */
> +  /* Some OSs don't support saving Altivec registers.  On those OSs, we don't
> +     touch the OPTION_MASK_ALTIVEC settings; if the user wants it, the user
> +     must explicitly specify it and we won't interfere with the user's
> +     specification.  */
> 
>   set_masks = POWERPC_MASKS;
> -#ifdef OS_MISSING_POWERPC64
> -  if (OS_MISSING_POWERPC64)
> -    set_masks &= ~OPTION_MASK_POWERPC64;
> -#endif
> #ifdef OS_MISSING_ALTIVEC
>   if (OS_MISSING_ALTIVEC)
>     set_masks &= ~(OPTION_MASK_ALTIVEC | OPTION_MASK_VSX
> @@ -3668,6 +3663,18 @@ rs6000_option_override_internal (bool global_init_p)
>   /* Don't override by the processor default if given explicitly.  */
>   set_masks &= ~rs6000_isa_flags_explicit;
> 
> +  /* Without option powerpc64 specified explicitly, we need to ensure
> +     powerpc64 always enabled for 64 bit here, otherwise some following
> +     checks can use unexpected TARGET_POWERPC64 value.  Meanwhile, we
> +     need to ensure set_masks doesn't have OPTION_MASK_POWERPC64 on,
> +     otherwise later processing can clear it.  */
> +  if (!(rs6000_isa_flags_explicit & OPTION_MASK_POWERPC64)
> +      && TARGET_64BIT)
> +    {
> +      rs6000_isa_flags |= OPTION_MASK_POWERPC64;
> +      set_masks &= ~OPTION_MASK_POWERPC64;
> +    }
> +
>   /* Process the -mcpu=<xxx> and -mtune=<xxx> argument.  If the user changed
>      the cpu in a target attribute or pragma, but did not specify a tuning
>      option, use the cpu for the tuning option rather than the option specified
> @@ -3718,6 +3725,18 @@ rs6000_option_override_internal (bool global_init_p)
>       rs6000_isa_flags |= (flags & ~rs6000_isa_flags_explicit);
>     }
> 
> +  /* Don't expect powerpc64 enabled on those OSes with OS_MISSING_POWERPC64,
> +     since they don't support saving the high part of 64-bit registers on
> +     context switch.  If the user explicitly specifies it, we won't interfere
> +     with the user's specification.  */
> +#ifdef OS_MISSING_POWERPC64
> +  if (OS_MISSING_POWERPC64
> +      && TARGET_32BIT
> +      && TARGET_POWERPC64
> +      && !(rs6000_isa_flags_explicit & OPTION_MASK_POWERPC64))
> +    rs6000_isa_flags &= ~OPTION_MASK_POWERPC64;
> +#endif
> +
>   if (rs6000_tune_index >= 0)
>     tune_index = rs6000_tune_index;
>   else if (cpu_index >= 0)
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr106680-1.c b/gcc/testsuite/gcc.target/powerpc/pr106680-1.c
> new file mode 100644
> index 00000000000..d624d43230a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr106680-1.c
> @@ -0,0 +1,13 @@
> +/* { dg-require-effective-target lp64 } */
> +/* { dg-options "-mno-powerpc64" } */
> +
> +/* Verify there is an error message about PowerPC64 requirement.  */
> +
> +int foo ()
> +{
> +  return 1;
> +}
> +
> +/* { dg-error "'-m64' requires a PowerPC64 cpu" "PR106680" { target powerpc*-*-linux* powerpc*-*-freebsd* powerpc-*-rtems* } 0 } */
> +/* { dg-warning "'-m64' requires PowerPC64 architecture, enabling" "PR106680" { target powerpc*-*-darwin* } 0 } */
> +/* { dg-warning "'-maix64' requires PowerPC64 architecture remain enabled" "PR106680" { target powerpc*-*-aix* } 0 } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr106680-2.c b/gcc/testsuite/gcc.target/powerpc/pr106680-2.c
> new file mode 100644
> index 00000000000..a9ed73726ef
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr106680-2.c
> @@ -0,0 +1,14 @@
> +/* { dg-require-effective-target lp64 } */
> +/* { dg-options "-mno-powerpc64 -m64" } */
> +
> +/* Verify option -m64 doesn't override option -mno-powerpc64,
> +   and there is an error message about PowerPC64 requirement.  */
> +
> +int foo ()
> +{
> +  return 1;
> +}
> +
> +/* { dg-error "'-m64' requires a PowerPC64 cpu" "PR106680" { target powerpc*-*-linux* powerpc*-*-freebsd* powerpc-*-rtems* } 0 } */
> +/* { dg-warning "'-m64' requires PowerPC64 architecture, enabling" "PR106680" { target powerpc*-*-darwin* } 0 } */
> +/* { dg-warning "'-maix64' requires PowerPC64 architecture remain enabled" "PR106680" { target powerpc*-*-aix* } 0 } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr106680-3.c b/gcc/testsuite/gcc.target/powerpc/pr106680-3.c
> new file mode 100644
> index 00000000000..b642d5c7a00
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr106680-3.c
> @@ -0,0 +1,13 @@
> +/* { dg-require-effective-target lp64 } */
> +/* { dg-options "-m64 -mno-powerpc64" } */
> +
> +/* Verify there is an error message about PowerPC64 requirement.  */
> +
> +int foo ()
> +{
> +  return 1;
> +}
> +
> +/* { dg-error "'-m64' requires a PowerPC64 cpu" "PR106680" { target powerpc*-*-linux* powerpc*-*-freebsd* powerpc-*-rtems* } 0 } */
> +/* { dg-warning "'-m64' requires PowerPC64 architecture, enabling" "PR106680" { target powerpc*-*-darwin* } 0 } */
> +/* { dg-warning "'-maix64' requires PowerPC64 architecture remain enabled" "PR106680" { target powerpc*-*-aix* } 0 } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr106680-4.c b/gcc/testsuite/gcc.target/powerpc/pr106680-4.c
> new file mode 100644
> index 00000000000..7fee48d39f3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr106680-4.c
> @@ -0,0 +1,17 @@
> +/* Skip this on aix, otherwise it emits the error message like "64-bit
> +   computation with 32-bit addressing not yet supported" on aix.  */
> +/* { dg-skip-if "" { powerpc*-*-aix* } } */
> +/* { dg-require-effective-target ilp32 } */
> +/* { dg-options "-mpowerpc64 -m32 -O2" } */
> +
> +/* Verify option -m32 doesn't override option -mpowerpc64.
> +   If option -mpowerpc64 gets overridden, the assembly would
> +   end up with addc and adde.  */
> +/* { dg-final { scan-assembler-not {\maddc\M} } } */
> +/* { dg-final { scan-assembler-not {\madde\M} } } */
> +/* { dg-final { scan-assembler-times {\madd\M} 1 } } */
> +
> +long long foo (long long a, long long b)
> +{
> +  return a+b;
> +}
> --
> 2.27.0
  

> On 12 Oct 2022, at 09:57, Iain Sandoe <iain@sandoe.co.uk> wrote:
>> On 12 Oct 2022, at 09:12, Kewen.Lin <linkw@linux.ibm.com> wrote:
> 
>> PR106680 shows that -m32 -mpowerpc64 is different from
>> -mpowerpc64 -m32, this is determined by the way how we
>> handle option powerpc64 in rs6000_handle_option.
>> 
>> Segher pointed out this difference should be taken as
>> a bug and we should ensure that option powerpc64 is
>> independent of -m32/-m64.  So this patch removes the
>> handlings in rs6000_handle_option and add some necessary
>> supports in rs6000_option_override_internal instead.
>> 
>> With this patch, if users specify -m{no-,}powerpc64, the
>> specified value is honoured, otherwise, for 64bit it
>> always enables OPTION_MASK_POWERPC64; while for 32bit
>> and TARGET_POWERPC64 and OS_MISSING_POWERPC64, it disables
>> OPTION_MASK_POWERPC64.
>> 
>> btw, following Segher's suggestion, I did some tries to warn
>> when OPTION_MASK_POWERPC64 is set for OS_MISSING_POWERPC64.
>> If warn for the case that powerpc64 is specified explicitly,
>> there are some TCs using -m32 -mpowerpc64 on ppc64-linux,
>> they need some updates, meanwhile the artificial run
>> with "--target_board=unix'{-m32/-mpowerpc64}'" will have
>> noisy warnings on ppc64-linux.  If warn for the case that
>> it's specified implicitly, they can just be initialized by
>> TARGET_DEFAULT (like -m32 on ppc64-linux) or set from the 
>> given cpu mask, we have to special case them and not to warn.
>> As Segher's latest comment, I decide not to warn them and
>> keep it consistent with before.
>> 
>> Bootstrapped and regress-tested on:
>> - powerpc64-linux-gnu P7 and P8 {-m64,-m32}
>> - powerpc64le-linux-gnu P9 and P10
>> - powerpc-ibm-aix7.2.0.0 {-maix64,-maix32}
>> 
>> Hi Iain, could you help to test this new patch on darwin
>> again?  Thanks in advance!
> 
> I kicked off a bootstrap - and 'check-gcc-c' .. if all goes well, there will be an 
> answer in ? 7hours.  If something fails, the answer will be sooner ;)

bootstrapped and tested on powerpc-darwin9, with default CPU configuration.
I have not yet tried tuning or cpu configure options.

testresults compare ?nominal" against a recent set (another day elapsed time
would be needed for a proper regtest).

thanks
Iain
  

Hi Iain,

on 2022/10/13 18:09, Iain Sandoe wrote:
> 
> 
>> On 12 Oct 2022, at 09:57, Iain Sandoe <iain@sandoe.co.uk> wrote:
>>> On 12 Oct 2022, at 09:12, Kewen.Lin <linkw@linux.ibm.com> wrote:
>>
>>> PR106680 shows that -m32 -mpowerpc64 is different from
>>> -mpowerpc64 -m32, this is determined by the way how we
>>> handle option powerpc64 in rs6000_handle_option.
>>>
>>> Segher pointed out this difference should be taken as
>>> a bug and we should ensure that option powerpc64 is
>>> independent of -m32/-m64.  So this patch removes the
>>> handlings in rs6000_handle_option and add some necessary
>>> supports in rs6000_option_override_internal instead.
>>>
>>> With this patch, if users specify -m{no-,}powerpc64, the
>>> specified value is honoured, otherwise, for 64bit it
>>> always enables OPTION_MASK_POWERPC64; while for 32bit
>>> and TARGET_POWERPC64 and OS_MISSING_POWERPC64, it disables
>>> OPTION_MASK_POWERPC64.
>>>
>>> btw, following Segher's suggestion, I did some tries to warn
>>> when OPTION_MASK_POWERPC64 is set for OS_MISSING_POWERPC64.
>>> If warn for the case that powerpc64 is specified explicitly,
>>> there are some TCs using -m32 -mpowerpc64 on ppc64-linux,
>>> they need some updates, meanwhile the artificial run
>>> with "--target_board=unix'{-m32/-mpowerpc64}'" will have
>>> noisy warnings on ppc64-linux.  If warn for the case that
>>> it's specified implicitly, they can just be initialized by
>>> TARGET_DEFAULT (like -m32 on ppc64-linux) or set from the 
>>> given cpu mask, we have to special case them and not to warn.
>>> As Segher's latest comment, I decide not to warn them and
>>> keep it consistent with before.
>>>
>>> Bootstrapped and regress-tested on:
>>> - powerpc64-linux-gnu P7 and P8 {-m64,-m32}
>>> - powerpc64le-linux-gnu P9 and P10
>>> - powerpc-ibm-aix7.2.0.0 {-maix64,-maix32}
>>>
>>> Hi Iain, could you help to test this new patch on darwin
>>> again?  Thanks in advance!
>>
>> I kicked off a bootstrap - and 'check-gcc-c' .. if all goes well, there will be an 
>> answer in ≈ 7hours.  If something fails, the answer will be sooner ;)
> 
> bootstrapped and tested on powerpc-darwin9, with default CPU configuration.
> I have not yet tried tuning or cpu configure options.
> 
> testresults compare “nominal" against a recent set (another day elapsed time
> would be needed for a proper regtest).

Sounds good!  Many thanks again for your help!

BR,
Kewen
  

Hi,

Gentle ping this:

https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603350.html

BR,
Kewen

on 2022/10/12 16:12, Kewen.Lin via Gcc-patches wrote:
> Hi,
> 
> PR106680 shows that -m32 -mpowerpc64 is different from
> -mpowerpc64 -m32, this is determined by the way how we
> handle option powerpc64 in rs6000_handle_option.
> 
> Segher pointed out this difference should be taken as
> a bug and we should ensure that option powerpc64 is
> independent of -m32/-m64.  So this patch removes the
> handlings in rs6000_handle_option and add some necessary
> supports in rs6000_option_override_internal instead.
> 
> With this patch, if users specify -m{no-,}powerpc64, the
> specified value is honoured, otherwise, for 64bit it
> always enables OPTION_MASK_POWERPC64; while for 32bit
> and TARGET_POWERPC64 and OS_MISSING_POWERPC64, it disables
> OPTION_MASK_POWERPC64.
> 
> btw, following Segher's suggestion, I did some tries to warn
> when OPTION_MASK_POWERPC64 is set for OS_MISSING_POWERPC64.
> If warn for the case that powerpc64 is specified explicitly,
> there are some TCs using -m32 -mpowerpc64 on ppc64-linux,
> they need some updates, meanwhile the artificial run
> with "--target_board=unix'{-m32/-mpowerpc64}'" will have
> noisy warnings on ppc64-linux.  If warn for the case that
> it's specified implicitly, they can just be initialized by
> TARGET_DEFAULT (like -m32 on ppc64-linux) or set from the 
> given cpu mask, we have to special case them and not to warn.
> As Segher's latest comment, I decide not to warn them and
> keep it consistent with before.
> 
> Bootstrapped and regress-tested on:
>   - powerpc64-linux-gnu P7 and P8 {-m64,-m32}
>   - powerpc64le-linux-gnu P9 and P10
>   - powerpc-ibm-aix7.2.0.0 {-maix64,-maix32}
> 
> Hi Iain, could you help to test this new patch on darwin
> again?  Thanks in advance!
> 
> Is it ok for trunk if darwin testing goes well?
>
  

Hi,

Gentle ping this:

https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603350.html

BR,
Kewen

> on 2022/10/12 16:12, Kewen.Lin via Gcc-patches wrote:
>> Hi,
>>
>> PR106680 shows that -m32 -mpowerpc64 is different from
>> -mpowerpc64 -m32, this is determined by the way how we
>> handle option powerpc64 in rs6000_handle_option.
>>
>> Segher pointed out this difference should be taken as
>> a bug and we should ensure that option powerpc64 is
>> independent of -m32/-m64.  So this patch removes the
>> handlings in rs6000_handle_option and add some necessary
>> supports in rs6000_option_override_internal instead.
>>
>> With this patch, if users specify -m{no-,}powerpc64, the
>> specified value is honoured, otherwise, for 64bit it
>> always enables OPTION_MASK_POWERPC64; while for 32bit
>> and TARGET_POWERPC64 and OS_MISSING_POWERPC64, it disables
>> OPTION_MASK_POWERPC64.
>>
>> btw, following Segher's suggestion, I did some tries to warn
>> when OPTION_MASK_POWERPC64 is set for OS_MISSING_POWERPC64.
>> If warn for the case that powerpc64 is specified explicitly,
>> there are some TCs using -m32 -mpowerpc64 on ppc64-linux,
>> they need some updates, meanwhile the artificial run
>> with "--target_board=unix'{-m32/-mpowerpc64}'" will have
>> noisy warnings on ppc64-linux.  If warn for the case that
>> it's specified implicitly, they can just be initialized by
>> TARGET_DEFAULT (like -m32 on ppc64-linux) or set from the 
>> given cpu mask, we have to special case them and not to warn.
>> As Segher's latest comment, I decide not to warn them and
>> keep it consistent with before.
>>
>> Bootstrapped and regress-tested on:
>>   - powerpc64-linux-gnu P7 and P8 {-m64,-m32}
>>   - powerpc64le-linux-gnu P9 and P10
>>   - powerpc-ibm-aix7.2.0.0 {-maix64,-maix32}
>>
>> Hi Iain, could you help to test this new patch on darwin
>> again?  Thanks in advance!
>>
>> Is it ok for trunk if darwin testing goes well?
>>
>
  

Hi!

On Wed, Oct 12, 2022 at 04:12:21PM +0800, Kewen.Lin wrote:
> PR106680 shows that -m32 -mpowerpc64 is different from
> -mpowerpc64 -m32, this is determined by the way how we
> handle option powerpc64 in rs6000_handle_option.
> 
> Segher pointed out this difference should be taken as
> a bug and we should ensure that option powerpc64 is
> independent of -m32/-m64.  So this patch removes the
> handlings in rs6000_handle_option and add some necessary
> supports in rs6000_option_override_internal instead.

Sorry for the late review.

> +  /* Don't expect powerpc64 enabled on those OSes with OS_MISSING_POWERPC64,
> +     since they don't support saving the high part of 64-bit registers on
> +     context switch.  If the user explicitly specifies it, we won't interfere
> +     with the user's specification.  */

It depends on the OS, and what you call "context switch".  For example
on Linux the context switches done by the kernel are fine, only things
done by setjmp/longjmp and getcontext/setcontext are not.  So just be a
bit more vague here?  "Since they do not save and restore the high half
of the GPRs correctly in all cases", something like that?

Okay for trunk like that.  Thanks!


Segher
  

Hi Segher,

on 2022/12/24 04:26, Segher Boessenkool wrote:
> Hi!
> 
> On Wed, Oct 12, 2022 at 04:12:21PM +0800, Kewen.Lin wrote:
>> PR106680 shows that -m32 -mpowerpc64 is different from
>> -mpowerpc64 -m32, this is determined by the way how we
>> handle option powerpc64 in rs6000_handle_option.
>>
>> Segher pointed out this difference should be taken as
>> a bug and we should ensure that option powerpc64 is
>> independent of -m32/-m64.  So this patch removes the
>> handlings in rs6000_handle_option and add some necessary
>> supports in rs6000_option_override_internal instead.
> 
> Sorry for the late review.
> 
>> +  /* Don't expect powerpc64 enabled on those OSes with OS_MISSING_POWERPC64,
>> +     since they don't support saving the high part of 64-bit registers on
>> +     context switch.  If the user explicitly specifies it, we won't interfere
>> +     with the user's specification.  */
> 
> It depends on the OS, and what you call "context switch".  For example
> on Linux the context switches done by the kernel are fine, only things
> done by setjmp/longjmp and getcontext/setcontext are not.  So just be a
> bit more vague here?  "Since they do not save and restore the high half
> of the GPRs correctly in all cases", something like that?
> 
> Okay for trunk like that.  Thanks!
> 

Thanks!  Adjusted as you suggested and committed in r13-4894-gacc727cf02a144.

BR,
Kewen
  

Hello,

On 27.12.22 11:16, Kewen.Lin via Gcc-patches wrote:
> Hi Segher,
> 
> on 2022/12/24 04:26, Segher Boessenkool wrote:
>> Hi!
>>
>> On Wed, Oct 12, 2022 at 04:12:21PM +0800, Kewen.Lin wrote:
>>> PR106680 shows that -m32 -mpowerpc64 is different from
>>> -mpowerpc64 -m32, this is determined by the way how we
>>> handle option powerpc64 in rs6000_handle_option.
>>>
>>> Segher pointed out this difference should be taken as
>>> a bug and we should ensure that option powerpc64 is
>>> independent of -m32/-m64.  So this patch removes the
>>> handlings in rs6000_handle_option and add some necessary
>>> supports in rs6000_option_override_internal instead.
>>
>> Sorry for the late review.
>>
>>> +  /* Don't expect powerpc64 enabled on those OSes with OS_MISSING_POWERPC64,
>>> +     since they don't support saving the high part of 64-bit registers on
>>> +     context switch.  If the user explicitly specifies it, we won't interfere
>>> +     with the user's specification.  */
>>
>> It depends on the OS, and what you call "context switch".  For example
>> on Linux the context switches done by the kernel are fine, only things
>> done by setjmp/longjmp and getcontext/setcontext are not.  So just be a
>> bit more vague here?  "Since they do not save and restore the high half
>> of the GPRs correctly in all cases", something like that?
>>
>> Okay for trunk like that.  Thanks!
>>
> 
> Thanks!  Adjusted as you suggested and committed in r13-4894-gacc727cf02a144.

I am a bit late, however, this broke the 32-bit support for -mcpu=e6500. 
For RTEMS, I have the following multilibs:

MULTILIB_REQUIRED += mcpu=e6500/m32
MULTILIB_REQUIRED += mcpu=e6500/m32/mvrsave
MULTILIB_REQUIRED += mcpu=e6500/m32/msoft-float/mno-altivec
MULTILIB_REQUIRED += mcpu=e6500/m64
MULTILIB_REQUIRED += mcpu=e6500/m64/mvrsave

I configured GCC as a bi-arch compiler (32-bit and 64-bit). It seems you 
removed the -m32 handling, so I am not sure how to approach this issue. 
I added a test case to the PR:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106680
  

Hi Sebastian,

on 2024/2/5 18:38, Sebastian Huber wrote:
> Hello,
> 
> On 27.12.22 11:16, Kewen.Lin via Gcc-patches wrote:
>> Hi Segher,
>>
>> on 2022/12/24 04:26, Segher Boessenkool wrote:
>>> Hi!
>>>
>>> On Wed, Oct 12, 2022 at 04:12:21PM +0800, Kewen.Lin wrote:
>>>> PR106680 shows that -m32 -mpowerpc64 is different from
>>>> -mpowerpc64 -m32, this is determined by the way how we
>>>> handle option powerpc64 in rs6000_handle_option.
>>>>
>>>> Segher pointed out this difference should be taken as
>>>> a bug and we should ensure that option powerpc64 is
>>>> independent of -m32/-m64.  So this patch removes the
>>>> handlings in rs6000_handle_option and add some necessary
>>>> supports in rs6000_option_override_internal instead.
>>>
>>> Sorry for the late review.
>>>
>>>> +  /* Don't expect powerpc64 enabled on those OSes with OS_MISSING_POWERPC64,
>>>> +     since they don't support saving the high part of 64-bit registers on
>>>> +     context switch.  If the user explicitly specifies it, we won't interfere
>>>> +     with the user's specification.  */
>>>
>>> It depends on the OS, and what you call "context switch".  For example
>>> on Linux the context switches done by the kernel are fine, only things
>>> done by setjmp/longjmp and getcontext/setcontext are not.  So just be a
>>> bit more vague here?  "Since they do not save and restore the high half
>>> of the GPRs correctly in all cases", something like that?
>>>
>>> Okay for trunk like that.  Thanks!
>>>
>>
>> Thanks!  Adjusted as you suggested and committed in r13-4894-gacc727cf02a144.
> 
> I am a bit late, however, this broke the 32-bit support for -mcpu=e6500. For RTEMS, I have the following multilibs:
> 
> MULTILIB_REQUIRED += mcpu=e6500/m32
> MULTILIB_REQUIRED += mcpu=e6500/m32/mvrsave
> MULTILIB_REQUIRED += mcpu=e6500/m32/msoft-float/mno-altivec
> MULTILIB_REQUIRED += mcpu=e6500/m64
> MULTILIB_REQUIRED += mcpu=e6500/m64/mvrsave
> 
> I configured GCC as a bi-arch compiler (32-bit and 64-bit). It seems you removed the -m32 handling, so I am not sure how to approach this issue. I added a test case to the PR:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106680

Thanks for reporting, I'll have a look at it (but I'm starting to be on vacation, so there may be slow response).

I'm not sure what's happened in bugzilla recently, but I didn't receive any mail notifications on your comments
#c5 and #c6 (sorry for the late response), since PR106680 is in state resolved maybe it's good to file a new
one for further tracking. :)

BR,
Kewen