| Message ID | 9d9f1f43-b528-387d-45a7-1d89400de0fc@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000: Rework option -mpowerpc64 handling [PR106680] | expand |
Hi Kewen, > On 28 Sep 2022, at 06:30, Kewen.Lin via Gcc-patches <gcc-patches@gcc.gnu.org> 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 > it disables OPTION_MASK_POWERPC64 if OS_MISSING_POWERPC64. > > 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 on darwin to ensure > it won't break darwin's build and new tests are fine? > Thanks in advance! Will do, it will take a day or so, thanks, Iain
(reduced CC list, if folks want to be re-included .. please add them back). > On 28 Sep 2022, at 07:37, Iain Sandoe <iain@sandoe.co.uk> wrote: >> On 28 Sep 2022, at 06:30, Kewen.Lin via Gcc-patches <gcc-patches@gcc.gnu.org> 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 >> it disables OPTION_MASK_POWERPC64 if OS_MISSING_POWERPC64. >> >> 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 on darwin to ensure >> it won't break darwin's build and new tests are fine? >> Thanks in advance! > > Will do, it will take a day or so, thanks, Perhaps a small exposition on the target: powerpc-apple-darwin, is perhaps somewhat unusual in that it is nominally a 32b kernel, but the OS supports 64b processes on suitable hardware (and the OS does preserve the upper bits of 64b regs in the context). ----- I bootstrapped (all supported languages) and tested r13-2892 yesterday with “nominal” results. Then I added this patch .. and did a clean bootstrap (same configuration). the bootstrap fails on the stage3 libgomp (building the ppc64 multilib) with the error below What is somewhat odd here is that libgomp is bootstrapped with the compiler and, apparently, openacc-init.c built OK at stage2. —— Of course, powerpc-darwin is not a blocker for anything, it should not hold you up (but sometimes it manages to find a glitch missed elsewhere). I will try to take a look at this this evening see if I can throw any more light on it. ------ /src-local/gcc-master/libgomp/oacc-init.c:876:1: internal compiler error: ‘global_options’ are modified in local context 876 | { | ^ 0xe940d7 cl_optimization_compare(gcc_options*, gcc_options*) /scratch/10-5-leo/gcc-master/gcc/options-save.cc:14082 0x15f8fb handle_optimize_attribute /src-local/gcc-master/gcc/c-family/c-attribs.cc:5619 0x8447 decl_attributes(tree_node**, tree_node*, int, tree_node*) /src-local/gcc-master/gcc/attribs.cc:875 0x307bb start_function(c_declspecs*, c_declarator*, tree_node*) /src-local/gcc-master/gcc/c/c-decl.cc:9537 0xb4f27 c_parser_declaration_or_fndef /src-local/gcc-master/gcc/c/c-parser.cc:2466 0xc164f c_parser_external_declaration /src-local/gcc-master/gcc/c/c-parser.cc:1800 0xc2323 c_parser_translation_unit /src-local/gcc-master/gcc/c/c-parser.cc:1666 0xc2323 c_parse_file() ???:0 0x13a5db c_common_parse_file() /src-local/gcc-master/gcc/c-family/c-opts.cc:1255 Please submit a full bug report, with preprocessed source (by using -freport-bug). Please include the complete backtrace with any bug report. See <https://gcc.gnu.org/bugs/> for instructions.
Hi Kewen > On 28 Sep 2022, at 17:18, Iain Sandoe <iain@sandoe.co.uk> wrote: > > (reduced CC list, if folks want to be re-included .. please add them back). > >> On 28 Sep 2022, at 07:37, Iain Sandoe <iain@sandoe.co.uk> wrote: > >>> On 28 Sep 2022, at 06:30, Kewen.Lin via Gcc-patches <gcc-patches@gcc.gnu.org> 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 >>> it disables OPTION_MASK_POWERPC64 if OS_MISSING_POWERPC64. >>> >>> 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 on darwin to ensure >>> it won't break darwin's build and new tests are fine? >>> Thanks in advance! >> >> Will do, it will take a day or so, thanks, > > Perhaps a small exposition on the target: > > powerpc-apple-darwin, is perhaps somewhat unusual in that it is nominally a 32b kernel, but the OS supports 64b processes on suitable hardware (and the OS does preserve the upper bits of 64b regs in the context). > > ----- > > I bootstrapped (all supported languages) and tested r13-2892 yesterday with “nominal” results. > > Then I added this patch .. and did a clean bootstrap (same configuration). > > the bootstrap fails on the stage3 libgomp (building the ppc64 multilib) with the error below > What is somewhat odd here is that libgomp is bootstrapped with the compiler and, apparently, > openacc-init.c built OK at stage2. > > —— > > Of course, powerpc-darwin is not a blocker for anything, it should not hold you up (but sometimes it > manages to find a glitch missed elsewhere). I will try to take a look at this this evening see if I can throw > any more light on it. > > ------ > > /src-local/gcc-master/libgomp/oacc-init.c:876:1: internal compiler error: ‘global_options’ are modified in local context > 876 | { > | ^ > 0xe940d7 cl_optimization_compare(gcc_options*, gcc_options*) > /scratch/10-5-leo/gcc-master/gcc/options-save.cc:14082 This repeats on a cross from x86_64-darwin to powerpc-darwin .. (makes debug a bit quicker) this is the failing case - which does not (immediately) seem directly connected .. does it ring any bells for you? 16649 if (ptr1->x_rs6000_sched_restricted_insns_priority != ptr2->x_rs6000_sched_restricted_insns_priority) -> 16650 internal_error ("%<global_options%> are modified in local context”); > 0x15f8fb handle_optimize_attribute > /src-local/gcc-master/gcc/c-family/c-attribs.cc:5619 > 0x8447 decl_attributes(tree_node**, tree_node*, int, tree_node*) > /src-local/gcc-master/gcc/attribs.cc:875 > 0x307bb start_function(c_declspecs*, c_declarator*, tree_node*) > /src-local/gcc-master/gcc/c/c-decl.cc:9537 > 0xb4f27 c_parser_declaration_or_fndef > /src-local/gcc-master/gcc/c/c-parser.cc:2466 > 0xc164f c_parser_external_declaration > /src-local/gcc-master/gcc/c/c-parser.cc:1800 > 0xc2323 c_parser_translation_unit > /src-local/gcc-master/gcc/c/c-parser.cc:1666 > 0xc2323 c_parse_file() > ???:0 > 0x13a5db c_common_parse_file() > /src-local/gcc-master/gcc/c-family/c-opts.cc:1255 > Please submit a full bug report, with preprocessed source (by using -freport-bug). > Please include the complete backtrace with any bug report. > See <https://gcc.gnu.org/bugs/> for instructions. >
Hi! On Wed, Sep 28, 2022 at 05:18:47PM +0100, Iain Sandoe wrote: > > On 28 Sep 2022, at 07:37, Iain Sandoe <iain@sandoe.co.uk> wrote: > >> On 28 Sep 2022, at 06:30, Kewen.Lin via Gcc-patches <gcc-patches@gcc.gnu.org> 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 > >> it disables OPTION_MASK_POWERPC64 if OS_MISSING_POWERPC64. It probably shouldn't have to disable it, it can only be on if it is explicitly enabled by the user? So warning would be much better than implicitly disabling it. > Perhaps a small exposition on the target: > > powerpc-apple-darwin, is perhaps somewhat unusual in that it is nominally a 32b kernel, but the OS supports 64b processes on suitable hardware Just like Linux was before there was powerpc64-linux. I think it should still work even? > (and the OS does preserve the upper bits of 64b regs in the context). That works on Linux as well. What still does not work is user-mode context switches in 32-bit processes (so setjmp and getcontext stuff). > Of course, powerpc-darwin is not a blocker for anything, it should not hold you up (but sometimes it > manages to find a glitch missed elsewhere). It is stage 1, nothing blocks nothing :-) But indeed this looks like a bug not specific to Darwin. It would be nice if it was fixed before this goes in. > I will try to take a look at this this evening see if I can throw > any more light on it. Thanks! Segher
On Wed, Sep 28, 2022 at 01:30:46PM +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. Thanks! > 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 > it disables OPTION_MASK_POWERPC64 if OS_MISSING_POWERPC64. If the user says -m64 -mno-powerpc64 it should error, and perhaps -m32 -mpowerpc64 should warn if OS_MISSING_POWERPC64? > - /* 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_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. */ > > set_masks = POWERPC_MASKS; > -#ifdef OS_MISSING_POWERPC64 > - if (OS_MISSING_POWERPC64) > - set_masks &= ~OPTION_MASK_POWERPC64; > -#endif As I said elsewhere, it probably is helpful if we still warn here for -m32 -mpowerpc64 with OS_MISSING_POWERPC64 (or without the -m32 even, same thing). > + /* With option powerpc64 specified explicitly (either on or off), even if > + being compiled for 64 bit we don't need to check if it's disabled here, > + since subtargets will check and raise an error message if necessary > + later. But without option powerpc64 specified explicitly, we need to > + ensure powerpc64 enabled for 64 bit and disabled on those OSes with > + OS_MISSING_POWERPC64, since they don't support saving the high part of > + 64-bit registers on context switch. */ > + if (!(rs6000_isa_flags_explicit & OPTION_MASK_POWERPC64)) > + { > + if (TARGET_64BIT) > + /* Make sure we always enable it by default for 64 bit. */ > + rs6000_isa_flags |= OPTION_MASK_POWERPC64; > +#ifdef OS_MISSING_POWERPC64 > + else if (OS_MISSING_POWERPC64) > + /* It's unexpected to have OPTION_MASK_POWERPC64 on for OSes which > + miss powerpc64 support, so disable it. */ > + rs6000_isa_flags &= ~OPTION_MASK_POWERPC64; > +#endif > + } Aha. Please don't, just warn instead? Silently disabling such stuff is the worst option :-( > +/* { dg-error "'-m64' requires a PowerPC64 cpu" "PR106680" { target powerpc*-*-linux* powerpc-*-rtems* } 0 } */ Everything except AIX even? So it will include Darwin as well (and the BSDs, and powerpc*-elf, etc.) > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr106680-4.c > @@ -0,0 +1,16 @@ > +/* 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" } */ If you have -m32 you don't need ilp32, and the other way around. > +/* 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 "addc" } } */ > +/* { dg-final { scan-assembler-not "adde" } } */ Lol, nice :-) "adde" is a frequent substring, use \m \M please? You will always get these exact insns anyway. And you could add a -times {\madd\M} 1 ? - - - The Darwin problem might be something in darwin*.h, but I don't see it. Maybe it is a more generic problem? Segher
Hi Folks, > On 28 Sep 2022, at 22:30, Segher Boessenkool <segher@kernel.crashing.org> wrote: > On Wed, Sep 28, 2022 at 05:18:47PM +0100, Iain Sandoe wrote: >>> On 28 Sep 2022, at 07:37, Iain Sandoe <iain@sandoe.co.uk> wrote: >>>> On 28 Sep 2022, at 06:30, Kewen.Lin via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >> powerpc-apple-darwin, is perhaps somewhat unusual in that it is nominally a 32b kernel, but the OS supports 64b processes on suitable hardware > > Just like Linux was before there was powerpc64-linux. I think it should > still work even? > >> (and the OS does preserve the upper bits of 64b regs in the context). > > That works on Linux as well. What still does not work is user-mode > context switches in 32-bit processes (so setjmp and getcontext stuff). AFAIU the Darwin impl. it is the same - the user context only contains 32b register images. Since one can only use the feature between function calls, I guess that the setjmp/longjmp stuff is not so critical on Darwin***. However, even being able to use 64b insns between calls could give a massive win in allowing, for example, lock-free 64b atomics. Sometime, I need to spend some time with this and make a set of ppc970 library slices (the dynamic loader should pick the right arch for the resident cpu). >> I will try to take a look at this this evening see if I can throw >> any more light on it. > > Thanks! adding —with-tune=G5 to the configure line .. the cross-build then succeeded (at "-O1 -g" as I was building to debug) - maybe that will provide a clue, but I’m out of time for today. Iain. === *** revisiting this topic, did make me wonder about non-call exceptions tho, not sure if they were considered in the original recipes.
> On 29 Sep 2022, at 00:04, Iain Sandoe <iain@sandoe.co.uk> wrote: > > adding —with-tune=G5 to the configure line .. the cross-build then succeeded > (at "-O1 -g" as I was building to debug) - maybe that will provide a clue, but I’m > out of time for today. perhaps we also need a check that the m32 CPU has support for 64b insns? so perhaps —with-cpu-32=<G5, 970…> (or the moral equivalent) should be required? Iain
Hi Iain, Thanks very much for your time!!! on 2022/9/29 03:09, Iain Sandoe wrote: > Hi Kewen > >> On 28 Sep 2022, at 17:18, Iain Sandoe <iain@sandoe.co.uk> wrote: >> >> (reduced CC list, if folks want to be re-included .. please add them back). >> >>> On 28 Sep 2022, at 07:37, Iain Sandoe <iain@sandoe.co.uk> wrote: >> >>>> On 28 Sep 2022, at 06:30, Kewen.Lin via Gcc-patches <gcc-patches@gcc.gnu.org> 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 >>>> it disables OPTION_MASK_POWERPC64 if OS_MISSING_POWERPC64. >>>> >>>> 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 on darwin to ensure >>>> it won't break darwin's build and new tests are fine? >>>> Thanks in advance! >>> >>> Will do, it will take a day or so, thanks, >> >> Perhaps a small exposition on the target: >> >> powerpc-apple-darwin, is perhaps somewhat unusual in that it is nominally a 32b kernel, but the OS supports 64b processes on suitable hardware (and the OS does preserve the upper bits of 64b regs in the context). >> >> ----- >> >> I bootstrapped (all supported languages) and tested r13-2892 yesterday with “nominal” results. >> >> Then I added this patch .. and did a clean bootstrap (same configuration). >> >> the bootstrap fails on the stage3 libgomp (building the ppc64 multilib) with the error below >> What is somewhat odd here is that libgomp is bootstrapped with the compiler and, apparently, >> openacc-init.c built OK at stage2. >> >> —— >> >> Of course, powerpc-darwin is not a blocker for anything, it should not hold you up (but sometimes it >> manages to find a glitch missed elsewhere). I will try to take a look at this this evening see if I can throw >> any more light on it. >> >> ------ >> >> /src-local/gcc-master/libgomp/oacc-init.c:876:1: internal compiler error: ‘global_options’ are modified in local context >> 876 | { >> | ^ >> 0xe940d7 cl_optimization_compare(gcc_options*, gcc_options*) >> /scratch/10-5-leo/gcc-master/gcc/options-save.cc:14082 > > This repeats on a cross from x86_64-darwin to powerpc-darwin .. (makes debug a bit quicker) > > this is the failing case - which does not (immediately) seem directly connected .. does it ring > any bells for you? > > 16649 if (ptr1->x_rs6000_sched_restricted_insns_priority != ptr2->x_rs6000_sched_restricted_insns_priority) > -> 16650 internal_error ("%<global_options%> are modified in local context”); > I found this flag is mainly related to tune setting and spotted that we have some code for tune setting when no explicit cpu is given. ... else { size_t i; enum processor_type tune_proc = (TARGET_POWERPC64 ? PROCESSOR_DEFAULT64 : PROCESSOR_DEFAULT); tune_index = -1; for (i = 0; i < ARRAY_SIZE (processor_target_table); i++) if (processor_target_table[i].processor == tune_proc) { tune_index = i; break; } } It checks TARGET_POWERPC64 directly here, my proposed patch will adjust TARGET_POWERPC64 after this hunk, so it seems to be problematic for some case. I'm testing the attached diff which can be applied on top of the previous proposed patch on ppc64 and ppc64le, could you help to test it can fix the issue? BR, Kewen diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc index 605d35893f9..3bfbb4eac21 100644 --- a/gcc/config/rs6000/rs6000.cc +++ b/gcc/config/rs6000/rs6000.cc @@ -3702,7 +3702,7 @@ rs6000_option_override_internal (bool global_init_p) else { /* PowerPC 64-bit LE requires at least ISA 2.07. */ - const char *default_cpu = (!TARGET_POWERPC64 + const char *default_cpu = (!TARGET_POWERPC64 && TARGET_32BIT ? "powerpc" : (BYTES_BIG_ENDIAN ? "powerpc64" @@ -3713,6 +3713,26 @@ rs6000_option_override_internal (bool global_init_p) rs6000_isa_flags |= (flags & ~rs6000_isa_flags_explicit); } + /* With option powerpc64 specified explicitly (either on or off), even if + being compiled for 64 bit we don't need to check if it's disabled here, + since subtargets will check and raise an error message if necessary + later. But without option powerpc64 specified explicitly, we need to + ensure powerpc64 enabled for 64 bit and disabled on those OSes with + OS_MISSING_POWERPC64, since they don't support saving the high part of + 64-bit registers on context switch. */ + if (!(rs6000_isa_flags_explicit & OPTION_MASK_POWERPC64)) + { + if (TARGET_64BIT) + /* Make sure we always enable it by default for 64 bit. */ + rs6000_isa_flags |= OPTION_MASK_POWERPC64; +#ifdef OS_MISSING_POWERPC64 + else if (OS_MISSING_POWERPC64) + /* It's unexpected to have OPTION_MASK_POWERPC64 on for OSes which + miss powerpc64 support, so disable it. */ + rs6000_isa_flags &= ~OPTION_MASK_POWERPC64; +#endif + } + if (rs6000_tune_index >= 0) tune_index = rs6000_tune_index; else if (cpu_index >= 0) @@ -3748,26 +3768,6 @@ rs6000_option_override_internal (bool global_init_p) error ("AltiVec not supported in this target"); } - /* With option powerpc64 specified explicitly (either on or off), even if - being compiled for 64 bit we don't need to check if it's disabled here, - since subtargets will check and raise an error message if necessary - later. But without option powerpc64 specified explicitly, we need to - ensure powerpc64 enabled for 64 bit and disabled on those OSes with - OS_MISSING_POWERPC64, since they don't support saving the high part of - 64-bit registers on context switch. */ - if (!(rs6000_isa_flags_explicit & OPTION_MASK_POWERPC64)) - { - if (TARGET_64BIT) - /* Make sure we always enable it by default for 64 bit. */ - rs6000_isa_flags |= OPTION_MASK_POWERPC64; -#ifdef OS_MISSING_POWERPC64 - else if (OS_MISSING_POWERPC64) - /* It's unexpected to have OPTION_MASK_POWERPC64 on for OSes which - miss powerpc64 support, so disable it. */ - rs6000_isa_flags &= ~OPTION_MASK_POWERPC64; -#endif - } - /* If we are optimizing big endian systems for space, use the load/store multiple instructions. */ if (BYTES_BIG_ENDIAN && optimize_size)
Hi Segher! Thanks for the review comments!! on 2022/9/29 06:04, Segher Boessenkool wrote: > On Wed, Sep 28, 2022 at 01:30:46PM +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. > > Thanks! > >> 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 >> it disables OPTION_MASK_POWERPC64 if OS_MISSING_POWERPC64. > > If the user says -m64 -mno-powerpc64 it should error, and perhaps -m32 > -mpowerpc64 should warn if OS_MISSING_POWERPC64? OK ... > >> - /* 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_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. */ >> >> set_masks = POWERPC_MASKS; >> -#ifdef OS_MISSING_POWERPC64 >> - if (OS_MISSING_POWERPC64) >> - set_masks &= ~OPTION_MASK_POWERPC64; >> -#endif > > As I said elsewhere, it probably is helpful if we still warn here for > -m32 -mpowerpc64 with OS_MISSING_POWERPC64 (or without the -m32 even, > same thing). > OK ... >> + /* With option powerpc64 specified explicitly (either on or off), even if >> + being compiled for 64 bit we don't need to check if it's disabled here, >> + since subtargets will check and raise an error message if necessary >> + later. But without option powerpc64 specified explicitly, we need to >> + ensure powerpc64 enabled for 64 bit and disabled on those OSes with >> + OS_MISSING_POWERPC64, since they don't support saving the high part of >> + 64-bit registers on context switch. */ >> + if (!(rs6000_isa_flags_explicit & OPTION_MASK_POWERPC64)) >> + { >> + if (TARGET_64BIT) >> + /* Make sure we always enable it by default for 64 bit. */ >> + rs6000_isa_flags |= OPTION_MASK_POWERPC64; >> +#ifdef OS_MISSING_POWERPC64 >> + else if (OS_MISSING_POWERPC64) >> + /* It's unexpected to have OPTION_MASK_POWERPC64 on for OSes which >> + miss powerpc64 support, so disable it. */ >> + rs6000_isa_flags &= ~OPTION_MASK_POWERPC64; >> +#endif >> + } > > Aha. Please don't, just warn instead? Silently disabling such stuff is > the worst option :-( ... I'll update this to warn instead as you suggested. :) > >> +/* { dg-error "'-m64' requires a PowerPC64 cpu" "PR106680" { target powerpc*-*-linux* powerpc-*-rtems* } 0 } */ > > Everything except AIX even? So it will include Darwin as well (and the > BSDs, and powerpc*-elf, etc.) I found this message only existed in file rtems.h and function rs6000_linux64_override_options, the latter is used by files linux64.h and freebsd64.h, I guess we just want to add one more powerpc*-*-freebsd*, but leave the others alone (and update this as needed later)? > >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/powerpc/pr106680-4.c >> @@ -0,0 +1,16 @@ >> +/* 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" } */ > > If you have -m32 you don't need ilp32, and the other way around. > Will update! I was afraid the dejagnu version mattered, it can be: "-mpowerpc64 -m32 -O2 -m64" or "-m64 -mpowerpc64 -m32 -O2", but just realized -mpowerpc64 would always take effect, useless worry. :) >> +/* 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 "addc" } } */ >> +/* { dg-final { scan-assembler-not "adde" } } */ > > Lol, nice :-) > > "adde" is a frequent substring, use \m \M please? You will always get > these exact insns anyway. And you could add a -times {\madd\M} 1 ? Will update, thanks again for all the comments! > > The Darwin problem might be something in darwin*.h, but I don't see it. > Maybe it is a more generic problem? > Yeah, it's probably a generic problem but only got exposed on darwin, I just made a trial diff, hope it can work. BR, Kewen
Hi Kewen, thanks for looking at this! (I suspect it would also affect a 32b linux host with a 64b multilib) quite likely powerpc-darwin is the only 32b ppc host in regular testing. > On 29 Sep 2022, at 06:45, Kewen.Lin via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > on 2022/9/29 03:09, Iain Sandoe wrote: >> Hi Kewen >> >>> On 28 Sep 2022, at 17:18, Iain Sandoe <iain@sandoe.co.uk> wrote: >>> >>> (reduced CC list, if folks want to be re-included .. please add them back). >>> >>>> On 28 Sep 2022, at 07:37, Iain Sandoe <iain@sandoe.co.uk> wrote: >>> >>>>> On 28 Sep 2022, at 06:30, Kewen.Lin via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >>>> >>> ------ >>> >>> /src-local/gcc-master/libgomp/oacc-init.c:876:1: internal compiler error: ‘global_options’ are modified in local context >>> 876 | { >>> | ^ >>> 0xe940d7 cl_optimization_compare(gcc_options*, gcc_options*) >>> /scratch/10-5-leo/gcc-master/gcc/options-save.cc:14082 >> >> This repeats on a cross from x86_64-darwin to powerpc-darwin .. (makes debug a bit quicker) >> >> this is the failing case - which does not (immediately) seem directly connected .. does it ring >> any bells for you? >> >> 16649 if (ptr1->x_rs6000_sched_restricted_insns_priority != ptr2->x_rs6000_sched_restricted_insns_priority) >> -> 16650 internal_error ("%<global_options%> are modified in local context”); >> > > I found this flag is mainly related to tune setting and spotted that we have some code > for tune setting when no explicit cpu is given. > > ... > > else > { > size_t i; > enum processor_type tune_proc > = (TARGET_POWERPC64 ? PROCESSOR_DEFAULT64 : PROCESSOR_DEFAULT); > > tune_index = -1; > for (i = 0; i < ARRAY_SIZE (processor_target_table); i++) > if (processor_target_table[i].processor == tune_proc) > { > tune_index = i; > break; > } > } > > It checks TARGET_POWERPC64 directly here, my proposed patch will adjust TARGET_POWERPC64 > after this hunk, so it seems to be problematic for some case. > > I'm testing the attached diff which can be applied on top of the previous proposed patch > on ppc64 and ppc64le, could you help to test it can fix the issue? It does work on a cross from x86_64-darwin => powerpc-darwin, I can also do compile-only tests there with a dummy board and the new tests pass with one minor tweak as described below. full regstrap on the G5 will take a day or so .. but I’ll do the C target tests first to get a heads up. ==== OK. So one small wrinkle, Darwin already has if (TARGET_64BIT && ! TARGET_POWERPC64) { rs6000_isa_flags |= OPTION_MASK_POWERPC64; warning (0, "%qs requires PowerPC64 architecture, enabling", "-m64"); } in darwin_rs6000_override_options() Which means that we do not report an error, but a warning, and then we force 64b on (taking the user’s intention to be specified by the explicit ‘-m64’). If there’s a strong feeling that this should really be an error, then I could make that change and see what fallout results. the patch below amends the test expectations to include Darwin with the warning it currently reports. cheers Iain
Hi Iain, Thanks again for your help!! on 2022/9/29 16:16, Iain Sandoe wrote: > Hi Kewen, > > thanks for looking at this! > (I suspect it would also affect a 32b linux host with a 64b multilib) > Quite reasonable suspicion. > quite likely powerpc-darwin is the only 32b ppc host in regular testing. > [...snip...] >> >> I'm testing the attached diff which can be applied on top of the previous proposed patch >> on ppc64 and ppc64le, could you help to test it can fix the issue? > > It does work on a cross from x86_64-darwin => powerpc-darwin, I can also do compile-only > tests there with a dummy board and the new tests pass with one minor tweak as described > below. > Nice! How blind I was, I should have searched for "requires.*PowerPC64". > full regstrap on the G5 will take a day or so .. but I’ll do the C target tests first to get a heads up. > Thanks! I think the C target tests is enough for now. I just refined the patch by addressing Segher's review comments and some other adjustments, I'm going to test it on ppc64/ppc64le/aix first, if everything goes well, I'll ask your help for a full regstrap on the new version. > ==== > > OK. So one small wrinkle, > > Darwin already has > > if (TARGET_64BIT && ! TARGET_POWERPC64) > { > rs6000_isa_flags |= OPTION_MASK_POWERPC64; > warning (0, "%qs requires PowerPC64 architecture, enabling", "-m64"); > } > > in darwin_rs6000_override_options() > > Which means that we do not report an error, but a warning, and then we force 64b on (taking > the user’s intention to be specified by the explicit ‘-m64’). > > If there’s a strong feeling that this should really be an error, then I could make that change and > see what fallout results. IMHO it's fine to leave it unchanged, aix also follows the same idea emitting warning instead of error, there are probably some actual user cases relying on this behavior, changing it can affect them. Thanks for bringing this up anyway! > > the patch below amends the test expectations to include Darwin with the warning it currently > reports. Will incorporate! Thanks agian! BR, Kewen
Hi Kewen, > On 29 Sep 2022, at 10:12, Kewen.Lin via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > on 2022/9/29 16:16, Iain Sandoe wrote: >>> >>> I'm testing the attached diff which can be applied on top of the previous proposed patch >>> on ppc64 and ppc64le, could you help to test it can fix the issue? >> >> It does work on a cross from x86_64-darwin => powerpc-darwin, I can also do compile-only >> tests there with a dummy board and the new tests pass with one minor tweak as described >> below. >> full regstrap on the G5 will take a day or so .. but I’ll do the C target tests first to get a heads up > > Thanks! I think the C target tests is enough for now. Bootstrap (powerpc-darwin9 on G5) succeeded and the C tests look nominal. Cheers Iain
Hi! On Thu, Sep 29, 2022 at 09:16:33AM +0100, Iain Sandoe wrote: > OK. So one small wrinkle, > > Darwin already has > > if (TARGET_64BIT && ! TARGET_POWERPC64) > { > rs6000_isa_flags |= OPTION_MASK_POWERPC64; > warning (0, "%qs requires PowerPC64 architecture, enabling", "-m64"); > } > > in darwin_rs6000_override_options() This should be in generic code, there is nothing special about Darwin for this. All 64-bit ABIs require 64-bit insns (stdu for example). > Which means that we do not report an error, but a warning, and then we force 64b on (taking > the user’s intention to be specified by the explicit ‘-m64’). And that is wrong. Any silent overriding of what the user says is bad. Not overriding it (and then later ICEing) is bad as well, so it should be an error here. And in generic code anyway. Segher
On Thu, Sep 29, 2022 at 01:45:16PM +0800, Kewen.Lin wrote: > I found this flag is mainly related to tune setting and spotted that we have some code > for tune setting when no explicit cpu is given. > > ... > > else > { > size_t i; > enum processor_type tune_proc > = (TARGET_POWERPC64 ? PROCESSOR_DEFAULT64 : PROCESSOR_DEFAULT); > > tune_index = -1; > for (i = 0; i < ARRAY_SIZE (processor_target_table); i++) > if (processor_target_table[i].processor == tune_proc) > { > tune_index = i; > break; > } > } Ah cool, that needs fixing yes. > --- a/gcc/config/rs6000/rs6000.cc > +++ b/gcc/config/rs6000/rs6000.cc > @@ -3702,7 +3702,7 @@ rs6000_option_override_internal (bool global_init_p) > else > { > /* PowerPC 64-bit LE requires at least ISA 2.07. */ > - const char *default_cpu = (!TARGET_POWERPC64 > + const char *default_cpu = (!TARGET_POWERPC64 && TARGET_32BIT > ? "powerpc" > : (BYTES_BIG_ENDIAN > ? "powerpc64" ... but not like that. If this snippet should happen later just move it later. Or introduce a new variable to make the control flow less confused. Or something else. But don't make the code more complex, introducing more special cases like this. > +#ifdef OS_MISSING_POWERPC64 > + else if (OS_MISSING_POWERPC64) > + /* It's unexpected to have OPTION_MASK_POWERPC64 on for OSes which > + miss powerpc64 support, so disable it. */ > + rs6000_isa_flags &= ~OPTION_MASK_POWERPC64; > +#endif All silent stuff is always bad. If things are done well, we will end up with *less* code than what we had before, not more! Segher
On Thu, Sep 29, 2022 at 12:04:05AM +0100, Iain Sandoe wrote: > > On 28 Sep 2022, at 22:30, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > That works on Linux as well. What still does not work is user-mode > > context switches in 32-bit processes (so setjmp and getcontext stuff). > > AFAIU the Darwin impl. it is the same - the user context only contains 32b > register images. Huh, I thought Darwin did this properly. > Since one can only use the feature between function calls, You still have to preserve the non-volatile GPRs. All 64 bits of it. > I guess that the > setjmp/longjmp stuff is not so critical on Darwin***. However, even being able > to use 64b insns between calls could give a massive win in allowing, for > example, lock-free 64b atomics. But that is not how GCC with -mpowerpc64 works: the calling convention is the usual 32-bit one, but the functions are 64-bit otherwise; it uses all 64 bits of GPRs everywhere except in function calls. Segher
On Thu, Sep 29, 2022 at 12:16:38AM +0100, Iain Sandoe wrote: > > On 29 Sep 2022, at 00:04, Iain Sandoe <iain@sandoe.co.uk> wrote: > > adding —with-tune=G5 to the configure line .. the cross-build then succeeded > > (at "-O1 -g" as I was building to debug) - maybe that will provide a clue, but I’m > > out of time for today. > > perhaps we also need a check that the m32 CPU has support for 64b insns? > > so perhaps —with-cpu-32=<G5, 970…> (or the moral equivalent) should be > required? In principle, yes. But -mpowerpc64 has been independently selectable in the past. Compare to -maltivec, which often is used with -mcpu=750 and stuff like that. We want to have less like this (much less), to reduce exponential special cases and exponential testing requirements to something manageable, but we also want to not break the world :-) Segher
Hi Segher > On 29 Sep 2022, at 18:04, Segher Boessenkool <segher@kernel.crashing.org> wrote: > On Thu, Sep 29, 2022 at 09:16:33AM +0100, Iain Sandoe wrote: >> OK. So one small wrinkle, >> >> Darwin already has >> >> if (TARGET_64BIT && ! TARGET_POWERPC64) >> { >> rs6000_isa_flags |= OPTION_MASK_POWERPC64; >> warning (0, "%qs requires PowerPC64 architecture, enabling", "-m64"); >> } >> >> in darwin_rs6000_override_options() > > This should be in generic code, there is nothing special about Darwin > for this. All 64-bit ABIs require 64-bit insns (stdu for example). Fine by me. >> Which means that we do not report an error, but a warning, and then we force 64b on (taking >> the user’s intention to be specified by the explicit ‘-m64’). > > And that is wrong. Any silent overriding of what the user says is bad. It is not silent - it warns and then carries on, > Not overriding it (and then later ICEing) is bad as well, so it should > be an error here. And in generic code anyway. As noted, if that change is made we will see what the fallout is :) cheers Iain
Hi Segher > On 29 Sep 2022, at 18:18, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Thu, Sep 29, 2022 at 12:04:05AM +0100, Iain Sandoe wrote: >>> On 28 Sep 2022, at 22:30, Segher Boessenkool <segher@kernel.crashing.org> wrote: >>> That works on Linux as well. What still does not work is user-mode >>> context switches in 32-bit processes (so setjmp and getcontext stuff). >> >> AFAIU the Darwin impl. it is the same - the user context only contains 32b >> register images. > > Huh, I thought Darwin did this properly. > >> Since one can only use the feature between function calls, > > You still have to preserve the non-volatile GPRs. All 64 bits of it. The OS does do that - e.g. on an interrupt .. but AFAIR, the user-visible mcontext in a 32b process only shows the lower 32 bits. ( i’d better stop making too many assertions here from memory, ;) ) >> I guess that the >> setjmp/longjmp stuff is not so critical on Darwin***. However, even being able >> to use 64b insns between calls could give a massive win in allowing, for >> example, lock-free 64b atomics. > > But that is not how GCC with -mpowerpc64 works: the calling convention > is the usual 32-bit one, but the functions are 64-bit otherwise; it uses > all 64 bits of GPRs everywhere except in function calls. I think we said the same thing with different words. The CC is unchanged (so that we can only use 64b insns between calls, since the upper 32b of callee-saved regs are not preserved). cheers Iain
Hi! On Thu, Sep 29, 2022 at 07:25:44PM +0100, Iain Sandoe wrote: > > On 29 Sep 2022, at 18:04, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Thu, Sep 29, 2022 at 09:16:33AM +0100, Iain Sandoe wrote: > >> Which means that we do not report an error, but a warning, and then we force 64b on (taking > >> the user’s intention to be specified by the explicit ‘-m64’). > > > > And that is wrong. Any silent overriding of what the user says is bad. > > It is not silent - it warns and then carries on, Yes, but I meant the status quo. We agree :-) > > Not overriding it (and then later ICEing) is bad as well, so it should > > be an error here. And in generic code anyway. > > As noted, if that change is made we will see what the fallout is :) Hopefully it magically makes everything fine ;-) Segher
On Thu, Sep 29, 2022 at 07:33:14PM +0100, Iain Sandoe wrote: > > On 29 Sep 2022, at 18:18, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Thu, Sep 29, 2022 at 12:04:05AM +0100, Iain Sandoe wrote: > >>> On 28 Sep 2022, at 22:30, Segher Boessenkool <segher@kernel.crashing.org> wrote: > >>> That works on Linux as well. What still does not work is user-mode > >>> context switches in 32-bit processes (so setjmp and getcontext stuff). > >> > >> AFAIU the Darwin impl. it is the same - the user context only contains 32b > >> register images. > > > > Huh, I thought Darwin did this properly. > > > >> Since one can only use the feature between function calls, > > > > You still have to preserve the non-volatile GPRs. All 64 bits of it. > > The OS does do that - e.g. on an interrupt .. but AFAIR, the user-visible mcontext > in a 32b process only shows the lower 32 bits. AFAIR the Darwin setjmp/longjmp and setcontext/getcontext do the full 64-bit registers. > ( i’d better stop making too many assertions here from memory, ;) ) Yeah, my memory might not work so well either, for stuff 20 years back! > > But that is not how GCC with -mpowerpc64 works: the calling convention > > is the usual 32-bit one, but the functions are 64-bit otherwise; it uses > > all 64 bits of GPRs everywhere except in function calls. > > I think we said the same thing with different words. > > The CC is unchanged (so that we can only use 64b insns between calls, since > the upper 32b of callee-saved regs are not preserved). But non-volatile GPRs (r21..r31 say) retain the full 64 bits over calls. This needs to be handled by those libc routines, to be compliant at all. Of course a lot of code will work fine, for example the whole GCC testsuite, if you only have the kernel context switches preserve the whole registers. But almost all code that uses setjmp (which is done by some libraries btw, behind the back of the user / programmer) fails spectacularly. Segher
Hi! On Thu, Sep 29, 2022 at 02:16:04PM +0800, Kewen.Lin wrote: > >> +/* { dg-error "'-m64' requires a PowerPC64 cpu" "PR106680" { target powerpc*-*-linux* powerpc-*-rtems* } 0 } */ > > > > Everything except AIX even? So it will include Darwin as well (and the > > BSDs, and powerpc*-elf, etc.) > > I found this message only existed in file rtems.h and function rs6000_linux64_override_options, > the latter is used by files linux64.h and freebsd64.h, I guess we just want to add one more > powerpc*-*-freebsd*, but leave the others alone (and update this as needed later)? Ah. This error should be generated by generic rs6000 code, not separately by separate targets. Dunno if you want to fold that into the current patch series. Segher
Hi Segher & Iain! on 2022/9/30 02:37, Segher Boessenkool wrote: > Hi! > > On Thu, Sep 29, 2022 at 07:25:44PM +0100, Iain Sandoe wrote: >>> On 29 Sep 2022, at 18:04, Segher Boessenkool <segher@kernel.crashing.org> wrote: >>> On Thu, Sep 29, 2022 at 09:16:33AM +0100, Iain Sandoe wrote: >>>> Which means that we do not report an error, but a warning, and then we force 64b on (taking >>>> the user’s intention to be specified by the explicit ‘-m64’). >>> >>> And that is wrong. Any silent overriding of what the user says is bad. >> >> It is not silent - it warns and then carries on, > > Yes, but I meant the status quo. We agree :-) > >>> Not overriding it (and then later ICEing) is bad as well, so it should >>> be an error here. And in generic code anyway. >> >> As noted, if that change is made we will see what the fallout is :) > > Hopefully it magically makes everything fine ;-) Okay, if we want to unify the behavior everywhere in generic code, I'll make a separated follow-up patch for it. :) BR, Kewen
on 2022/9/30 01:11, Segher Boessenkool wrote: > On Thu, Sep 29, 2022 at 01:45:16PM +0800, Kewen.Lin wrote: >> I found this flag is mainly related to tune setting and spotted that we have some code >> for tune setting when no explicit cpu is given. >> >> ... >> >> else >> { >> size_t i; >> enum processor_type tune_proc >> = (TARGET_POWERPC64 ? PROCESSOR_DEFAULT64 : PROCESSOR_DEFAULT); >> >> tune_index = -1; >> for (i = 0; i < ARRAY_SIZE (processor_target_table); i++) >> if (processor_target_table[i].processor == tune_proc) >> { >> tune_index = i; >> break; >> } >> } > > Ah cool, that needs fixing yes. > >> --- a/gcc/config/rs6000/rs6000.cc >> +++ b/gcc/config/rs6000/rs6000.cc >> @@ -3702,7 +3702,7 @@ rs6000_option_override_internal (bool global_init_p) >> else >> { >> /* PowerPC 64-bit LE requires at least ISA 2.07. */ >> - const char *default_cpu = (!TARGET_POWERPC64 >> + const char *default_cpu = (!TARGET_POWERPC64 && TARGET_32BIT >> ? "powerpc" >> : (BYTES_BIG_ENDIAN >> ? "powerpc64" > > ... but not like that. If this snippet should happen later just move it > later. Or introduce a new variable to make the control flow less > confused. Or something else. But don't make the code more complex, > introducing more special cases like this. Agree, the diff was mainly to check if it's the root cause. I think we need to place TARGET_POWERPC64 enablement for 64 bit before this hunk, I've adjusted it in the new version, will post it once it's full tested. > >> +#ifdef OS_MISSING_POWERPC64 >> + else if (OS_MISSING_POWERPC64) >> + /* It's unexpected to have OPTION_MASK_POWERPC64 on for OSes which >> + miss powerpc64 support, so disable it. */ >> + rs6000_isa_flags &= ~OPTION_MASK_POWERPC64; >> +#endif > > All silent stuff is always bad. > OK, with more testings for replacing warning instead of silently disablement I noticed that some disablement is needed, one typical case is -m32 compilation on ppc64, we have OPTION_MASK_POWERPC64 on from TARGET_DEFAULT which is used for initialization (It makes sense to have it on in TARGET_DEFAULT because of it's 64 bit cpu). And -m32 compilation matches OS_MISSING_POWERPC64 (!TARGET_64BIT), so it's the case that we have an implicit OPTION_MASK_POWERPC64 on and OS_MISSING_POWERPC64 holds, but it's unexpected not to disable it but warn it. BR, Kewen > If things are done well, we will end up with *less* code than what we > had before, not more! > > > Segher
On Fri, Sep 30, 2022 at 08:15:37PM +0800, Kewen.Lin wrote: > on 2022/9/30 01:11, Segher Boessenkool wrote: > >> +#ifdef OS_MISSING_POWERPC64 > >> + else if (OS_MISSING_POWERPC64) > >> + /* It's unexpected to have OPTION_MASK_POWERPC64 on for OSes which > >> + miss powerpc64 support, so disable it. */ > >> + rs6000_isa_flags &= ~OPTION_MASK_POWERPC64; > >> +#endif > > > > All silent stuff is always bad. > > OK, with more testings for replacing warning instead of silently disablement > I noticed that some disablement is needed, one typical case is -m32 compilation > on ppc64, we have OPTION_MASK_POWERPC64 on from TARGET_DEFAULT which is used > for initialization (It makes sense to have it on in TARGET_DEFAULT because > of it's 64 bit cpu). And -m32 compilation matches OS_MISSING_POWERPC64 > (!TARGET_64BIT), so it's the case that we have an implicit OPTION_MASK_POWERPC64 > on and OS_MISSING_POWERPC64 holds, but it's unexpected not to disable it but > warn it. Right. If If mpowerpc64 is enabled while OS_MISSING_POWERPC64, warn for that; and if mpowerpc64 was only implicit, disable it as well (and say we did!) Segher
Hi Segher! Thanks for the comments again! on 2022/10/4 05:15, Segher Boessenkool wrote: > On Fri, Sep 30, 2022 at 08:15:37PM +0800, Kewen.Lin wrote: >> on 2022/9/30 01:11, Segher Boessenkool wrote: >>>> +#ifdef OS_MISSING_POWERPC64 >>>> + else if (OS_MISSING_POWERPC64) >>>> + /* It's unexpected to have OPTION_MASK_POWERPC64 on for OSes which >>>> + miss powerpc64 support, so disable it. */ >>>> + rs6000_isa_flags &= ~OPTION_MASK_POWERPC64; >>>> +#endif >>> >>> All silent stuff is always bad. >> >> OK, with more testings for replacing warning instead of silently disablement >> I noticed that some disablement is needed, one typical case is -m32 compilation >> on ppc64, we have OPTION_MASK_POWERPC64 on from TARGET_DEFAULT which is used >> for initialization (It makes sense to have it on in TARGET_DEFAULT because >> of it's 64 bit cpu). And -m32 compilation matches OS_MISSING_POWERPC64 >> (!TARGET_64BIT), so it's the case that we have an implicit OPTION_MASK_POWERPC64 >> on and OS_MISSING_POWERPC64 holds, but it's unexpected not to disable it but >> warn it. > > Right. If If mpowerpc64 is enabled while OS_MISSING_POWERPC64, warn for > that; > Currently if option powerpc64 is enabled explicitly while OS_MISSING_POWERPC64, there is no warning. One typical case is -m32 compilation on ppc64. I made a patch to warn for this case as you suggested (btw, this change can be taken separately from this rework), it caused some test cases to fail as below: gcc.dg/vect/vect-82_64.c gcc.dg/vect/vect-83_64.c gcc.target/powerpc/bswap64-4.c gcc.target/powerpc/ppc64-double-1.c gcc.target/powerpc/pr106680-4.c gcc.target/powerpc/rs6000-fpint-2.c It's fine to fix them with one additional option "-w" to disable the warning. But IIUC one concern is that if we want to test with "--target_board=unix'{-m32, -m32/-mpowerpc64}'", the latter combination will always have this warning, with one extra "-w" (that is -m32/-mpowerpc64/-w) can make some cases which aim to check warning msg ineffective. So maybe we want to re-consider it (like just leaving it as before)? > and if mpowerpc64 was only implicit, disable it as well (and say > we did!) But on ppc64 linux, for -m32 compilation mpowerpc64 is implicitly enabled since it's with bi-arch supported, I made a patch to disable it as well as warn it, it can't be bootstrapped since it warned for -m32 build (-Werror) and failed. So I refined it to something like: + /* With RS6000_BI_ARCH defined (bi-architecture (32/64) supported), + TARGET_DEFAULT has bit MASK_POWERPC64 on by default, to keep the + behavior consistent (like: no warnings for -m32 on ppc64), we + just sliently disable it. Otherwise, disable it and warn. */ + rs6000_isa_flags &= ~OPTION_MASK_POWERPC64; +#ifndef RS6000_BI_ARCH + warning (0, "powerpc64 is unexpected to be enabled on the " + "current OS"); +#endif BR, Kewen
On Mon, Oct 10, 2022 at 10:15:58AM +0800, Kewen.Lin wrote: > on 2022/10/4 05:15, Segher Boessenkool wrote: > > Right. If If mpowerpc64 is enabled while OS_MISSING_POWERPC64, warn for > > that; > > Currently if option powerpc64 is enabled explicitly while OS_MISSING_POWERPC64, > there is no warning. One typical case is -m32 compilation on ppc64. I made > a patch to warn for this case as you suggested (btw, this change can be taken > separately from this rework), it caused some test cases to fail as below: "Explicitly" means the user says "-m32 -mpowerpc64". I wonder what "on powerpc64" means in what you say, and why that would matter? > gcc.dg/vect/vect-82_64.c > gcc.dg/vect/vect-83_64.c > gcc.target/powerpc/bswap64-4.c > gcc.target/powerpc/ppc64-double-1.c > gcc.target/powerpc/pr106680-4.c > gcc.target/powerpc/rs6000-fpint-2.c > > It's fine to fix them with one additional option "-w" to disable the warning. > But IIUC one concern is that if we want to test with "--target_board=unix'{-m32, > -m32/-mpowerpc64}'", the latter combination will always have this warning, > with one extra "-w" (that is -m32/-mpowerpc64/-w) can make some cases which > aim to check warning msg ineffective. So maybe we want to re-consider it > (like just leaving it as before)? There will always be false positives (and negatives!) if you put any warning options in RUNTESTFLAGS. -w is merely louder than most :-) But leave this as further improvement. Maybe put in a comment. > > and if mpowerpc64 was only implicit, disable it as well (and say > > we did!) > > But on ppc64 linux, for -m32 compilation mpowerpc64 is implicitly enabled > since it's with bi-arch supported, I made a patch to disable it as well as > warn it, it can't be bootstrapped since it warned for -m32 build (-Werror) > and failed. So I refined it to something like: > > + /* With RS6000_BI_ARCH defined (bi-architecture (32/64) supported), > + TARGET_DEFAULT has bit MASK_POWERPC64 on by default, to keep the > + behavior consistent (like: no warnings for -m32 on ppc64), we > + just sliently disable it. Otherwise, disable it and warn. */ > + rs6000_isa_flags &= ~OPTION_MASK_POWERPC64; > +#ifndef RS6000_BI_ARCH > + warning (0, "powerpc64 is unexpected to be enabled on the " > + "current OS"); > +#endif It has nothing to do with biarch. Let's just not warn if it is so much work to do it correctly. We never did before, and no one complained, how bad can it be :-) Segher
Hi Segher! on 2022/10/10 21:58, Segher Boessenkool wrote: > On Mon, Oct 10, 2022 at 10:15:58AM +0800, Kewen.Lin wrote: >> on 2022/10/4 05:15, Segher Boessenkool wrote: >>> Right. If If mpowerpc64 is enabled while OS_MISSING_POWERPC64, warn for >>> that; >> >> Currently if option powerpc64 is enabled explicitly while OS_MISSING_POWERPC64, >> there is no warning. One typical case is -m32 compilation on ppc64. I made >> a patch to warn for this case as you suggested (btw, this change can be taken >> separately from this rework), it caused some test cases to fail as below: > > "Explicitly" means the user says "-m32 -mpowerpc64". > > I wonder what "on powerpc64" means in what you say, and why that would > matter? I guess you meant to ask "on ppc64"? I meant to say "ppc64-linux", sorry for the confusion. On ppc64-linux, OS_MISSING_POWERPC64 is defined as !TARGET_64BIT, the explicit option "-m32 -mpowerpc64" doesn't warn before but it's made to warn as the patch mentioned above, then need some test cases updates. > >> gcc.dg/vect/vect-82_64.c >> gcc.dg/vect/vect-83_64.c >> gcc.target/powerpc/bswap64-4.c >> gcc.target/powerpc/ppc64-double-1.c >> gcc.target/powerpc/pr106680-4.c >> gcc.target/powerpc/rs6000-fpint-2.c >> >> It's fine to fix them with one additional option "-w" to disable the warning. >> But IIUC one concern is that if we want to test with "--target_board=unix'{-m32, >> -m32/-mpowerpc64}'", the latter combination will always have this warning, >> with one extra "-w" (that is -m32/-mpowerpc64/-w) can make some cases which >> aim to check warning msg ineffective. So maybe we want to re-consider it >> (like just leaving it as before)? > > There will always be false positives (and negatives!) if you put any > warning options in RUNTESTFLAGS. -w is merely louder than most :-) > > But leave this as further improvement. Maybe put in a comment. OK. > >>> and if mpowerpc64 was only implicit, disable it as well (and say >>> we did!) >> >> But on ppc64 linux, for -m32 compilation mpowerpc64 is implicitly enabled >> since it's with bi-arch supported, I made a patch to disable it as well as >> warn it, it can't be bootstrapped since it warned for -m32 build (-Werror) >> and failed. So I refined it to something like: >> >> + /* With RS6000_BI_ARCH defined (bi-architecture (32/64) supported), >> + TARGET_DEFAULT has bit MASK_POWERPC64 on by default, to keep the >> + behavior consistent (like: no warnings for -m32 on ppc64), we >> + just sliently disable it. Otherwise, disable it and warn. */ >> + rs6000_isa_flags &= ~OPTION_MASK_POWERPC64; >> +#ifndef RS6000_BI_ARCH >> + warning (0, "powerpc64 is unexpected to be enabled on the " >> + "current OS"); >> +#endif > > It has nothing to do with biarch. Let's just not warn if it is so much > work to do it correctly. We never did before, and no one complained, > how bad can it be :-) > OK, I made a patch v2 which doesn't try to warn for them, fully tested it and just posted at: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603350.html BR, Kewen
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..605d35893f9 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_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. */ 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 @@ -3753,6 +3748,26 @@ rs6000_option_override_internal (bool global_init_p) error ("AltiVec not supported in this target"); } + /* With option powerpc64 specified explicitly (either on or off), even if + being compiled for 64 bit we don't need to check if it's disabled here, + since subtargets will check and raise an error message if necessary + later. But without option powerpc64 specified explicitly, we need to + ensure powerpc64 enabled for 64 bit and disabled on those OSes with + OS_MISSING_POWERPC64, since they don't support saving the high part of + 64-bit registers on context switch. */ + if (!(rs6000_isa_flags_explicit & OPTION_MASK_POWERPC64)) + { + if (TARGET_64BIT) + /* Make sure we always enable it by default for 64 bit. */ + rs6000_isa_flags |= OPTION_MASK_POWERPC64; +#ifdef OS_MISSING_POWERPC64 + else if (OS_MISSING_POWERPC64) + /* It's unexpected to have OPTION_MASK_POWERPC64 on for OSes which + miss powerpc64 support, so disable it. */ + rs6000_isa_flags &= ~OPTION_MASK_POWERPC64; +#endif + } + /* If we are optimizing big endian systems for space, use the load/store multiple instructions. */ if (BYTES_BIG_ENDIAN && optimize_size) 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..ff33f6864c3 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr106680-1.c @@ -0,0 +1,12 @@ +/* { 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-*-rtems* } 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..25439910c27 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr106680-2.c @@ -0,0 +1,13 @@ +/* { 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-*-rtems* } 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..f8eea8ea645 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr106680-3.c @@ -0,0 +1,12 @@ +/* { 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-*-rtems* } 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..bfbdd8eb0b4 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr106680-4.c @@ -0,0 +1,16 @@ +/* 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 "addc" } } */ +/* { dg-final { scan-assembler-not "adde" } } */ + +long long foo (long long a, long long b) +{ + return a+b; +}
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 it disables OPTION_MASK_POWERPC64 if OS_MISSING_POWERPC64. 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 on darwin to ensure it won't break darwin's build and new tests are fine? 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 at -m64 and disable it for OS_MISSING_POWERPC64. 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. --- gcc/common/config/rs6000/rs6000-common.cc | 11 ------- gcc/config/rs6000/rs6000.cc | 33 ++++++++++++++----- gcc/testsuite/gcc.target/powerpc/pr106680-1.c | 12 +++++++ gcc/testsuite/gcc.target/powerpc/pr106680-2.c | 13 ++++++++ gcc/testsuite/gcc.target/powerpc/pr106680-3.c | 12 +++++++ gcc/testsuite/gcc.target/powerpc/pr106680-4.c | 16 +++++++++ 6 files changed, 77 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