From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp001-out.apm-internet.net (smtp001-out.apm-internet.net [85.119.248.222]) by sourceware.org (Postfix) with ESMTPS id 4436938582BC for ; Wed, 12 Oct 2022 08:57:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4436938582BC Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=sandoe.co.uk Authentication-Results: sourceware.org; spf=none smtp.mailfrom=sandoe.co.uk Received: (qmail 13225 invoked from network); 12 Oct 2022 08:57:33 -0000 X-APM-Out-ID: 16655650521322 X-APM-Authkey: 257869/1(257869/1) 9 Received: from unknown (HELO ?192.168.1.95?) (81.138.1.83) by smtp001.apm-internet.net with SMTP; 12 Oct 2022 08:57:33 -0000 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.7\)) Subject: Re: [PATCH v2] rs6000: Rework option -mpowerpc64 handling [PR106680] From: Iain Sandoe In-Reply-To: <63afd344-38fa-7a8e-4958-8256c2a9bca7@linux.ibm.com> Date: Wed, 12 Oct 2022 09:57:32 +0100 Cc: GCC Patches , Segher Boessenkool , David Edelsohn , Peter Bergner Content-Transfer-Encoding: quoted-printable Message-Id: <4652D74F-D0EF-4DFD-A87C-D2CA1996E468@sandoe.co.uk> References: <63afd344-38fa-7a8e-4958-8256c2a9bca7@linux.ibm.com> To: "Kewen.Lin" X-Mailer: Apple Mail (2.3608.120.23.2.7) X-Spam-Status: No, score=-14.3 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_COUK,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Kewen, > On 12 Oct 2022, at 09:12, 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. >=20 > 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. >=20 > 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. >=20 > 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=3Dunix'{-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=20 > 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. >=20 > 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} >=20 > 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=20 answer in =E2=89=88 7hours. If something fails, the answer will be = sooner ;) cheers Iain >=20 > Is it ok for trunk if darwin testing goes well? >=20 > BR, > Kewen > ----- > PR target/106680 >=20 > gcc/ChangeLog: >=20 > * 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. >=20 > gcc/testsuite/ChangeLog: >=20 > * 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. >=20 > 2022-10-12 Kewen Lin > Iain Sandoe > --- > 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 >=20 > 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 |=3D OPTION_MASK_POWERPC64; > opts->x_rs6000_isa_flags |=3D (~opts_set->x_rs6000_isa_flags > & OPTION_MASK_PPC_GFXOPT); > - opts_set->x_rs6000_isa_flags |=3D OPTION_MASK_POWERPC64; > - break; > - > -#ifdef TARGET_USES_AIX64_OPT > - case OPT_maix32: > -#else > - case OPT_m32: > -#endif > - opts->x_rs6000_isa_flags &=3D ~OPTION_MASK_POWERPC64; > - opts_set->x_rs6000_isa_flags |=3D OPTION_MASK_POWERPC64; > break; >=20 > 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 =3D 32; > } >=20 > - /* 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. */ >=20 > set_masks =3D POWERPC_MASKS; > -#ifdef OS_MISSING_POWERPC64 > - if (OS_MISSING_POWERPC64) > - set_masks &=3D ~OPTION_MASK_POWERPC64; > -#endif > #ifdef OS_MISSING_ALTIVEC > if (OS_MISSING_ALTIVEC) > set_masks &=3D ~(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 &=3D ~rs6000_isa_flags_explicit; >=20 > + /* 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 |=3D OPTION_MASK_POWERPC64; > + set_masks &=3D ~OPTION_MASK_POWERPC64; > + } > + > /* Process the -mcpu=3D and -mtune=3D 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 |=3D (flags & ~rs6000_isa_flags_explicit); > } >=20 > + /* 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 &=3D ~OPTION_MASK_POWERPC64; > +#endif > + > if (rs6000_tune_index >=3D 0) > tune_index =3D rs6000_tune_index; > else if (cpu_index >=3D 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