Re: Repost [PATCH 1/6] Add -mcpu=future
on 2024/2/21 15:19, Michael Meissner wrote: > On Tue, Feb 20, 2024 at 06:35:34PM +0800, Kewen.Lin wrote: >> Hi Mike, >> >> Sorry for late reply (just back from vacation). >> >> on 2024/2/8 03:58, Michael Meissner wrote: >>> On Wed, Feb 07, 2024 at 05:21:10PM +0800, Kewen.Lin wrote: on 2024/2/6 14:01, Michael Meissner wrote: Sorry for the possible confusion here, the "tune_proc" that I referred to is the variable in the above else branch: enum processor_type tune_proc = (TARGET_POWERPC64 ? PROCESSOR_DEFAULT64 : PROCESSOR_DEFAULT); It's either PROCESSOR_DEFAULT64 or PROCESSOR_DEFAULT, so it doesn't have a chance to be PROCESSOR_FUTURE, so the checking "tune_proc == PROCESSOR_FUTURE" is useless. >>> >>> PROCESSOR_DEFAULT can be PROCESSOR_FUTURE if somebody configures GCC with >>> --with-cpu=future. While in general it shouldn't occur, it is helpful to >>> consider all of the corner cases. >> >> But it sounds not true, I think you meant TARGET_CPU_DEFAULT instead? >> >> On one local ppc64le machine I tried to configure with --with-cpu=power10, >> I got {,OPTION_}TARGET_CPU_DEFAULT "power10" but PROCESSOR_DEFAULT is still >> PROCESSOR_POWER7 (PROCESSOR_DEFAULT64 is PROCESSOR_POWER8). I think these >> PROCESSOR_DEFAULT{,64} are defined by various headers: > > Yes, I was mistaken. You are correct TARGET_CPU_DEFAULT is set. I will > change > the comments. Thanks! > >> gcc/config/rs6000/aix71.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER7 >> gcc/config/rs6000/aix71.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER7 >> gcc/config/rs6000/aix72.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER7 >> gcc/config/rs6000/aix72.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER7 >> gcc/config/rs6000/aix73.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER8 >> gcc/config/rs6000/aix73.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER8 >> gcc/config/rs6000/darwin.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC7400 >> gcc/config/rs6000/darwin.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER4 >> gcc/config/rs6000/freebsd64.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC7450 >> gcc/config/rs6000/freebsd64.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER8 >> gcc/config/rs6000/linux64.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER7 >> gcc/config/rs6000/linux64.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER8 >> gcc/config/rs6000/rs6000.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC603 >> gcc/config/rs6000/rs6000.h:#define PROCESSOR_DEFAULT64 PROCESSOR_RS64A >> gcc/config/rs6000/vxworks.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC604 >> >> , and they are unlikely to be updated later, no? >> >> btw, the given --with-cpu=future will make cpu_index never negative so >> >> ... >> else if (cpu_index >= 0) >> rs6000_tune_index = tune_index = cpu_index; >> else >> ... >> >> so there is no chance to enter "else" arm, that is, that arm only takes >> effect when no cpu/tune is given (neither -m{cpu,tune} nor --with-cpu=). > > Note, this is existing code. I didn't modify it. If we want to change it, we > should do it as another patch. Yes, I agree. Just to clarify, I didn't suggest changing it but instead suggested almost keeping them, since we don't need any changes in "else" arm, so instead of updating in arms "if" and "else if" for "future cpu type", it seems a bit more clear to just check it after this, ie.: bool explicit_tune = false; if (rs6000_tune_index >= 0) { tune_index = rs6000_tune_index; explicit_tune = true; } else if (cpu_index >= 0) // as before rs6000_tune_index = tune_index = cpu_index; else { //as before ... } // Check tune_index here instead. if (processor_target_table[tune_index].processor == PROCESSOR_FUTURE) { tune_index = rs6000_cpu_index_lookup (PROCESSOR_POWER10); if (explicit_tune) warn ... } // as before rs6000_tune = processor_target_table[tune_index].processor; , copied from previous comment: https://gcc.gnu.org/pipermail/gcc-patches/2024-January/643681.html BR, Kewen
Re: Repost [PATCH 1/6] Add -mcpu=future
On Tue, Feb 20, 2024 at 06:35:34PM +0800, Kewen.Lin wrote: > on 2024/2/8 03:58, Michael Meissner wrote: > $ grep -r "define PROCESSOR_DEFAULT" gcc/config/rs6000/ > gcc/config/rs6000/aix71.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER7 > gcc/config/rs6000/aix71.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER7 > gcc/config/rs6000/aix72.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER7 > gcc/config/rs6000/aix72.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER7 > gcc/config/rs6000/aix73.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER8 > gcc/config/rs6000/aix73.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER8 > gcc/config/rs6000/darwin.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC7400 > gcc/config/rs6000/darwin.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER4 > gcc/config/rs6000/freebsd64.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC7450 > gcc/config/rs6000/freebsd64.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER8 > gcc/config/rs6000/linux64.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER7 > gcc/config/rs6000/linux64.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER8 > gcc/config/rs6000/rs6000.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC603 > gcc/config/rs6000/rs6000.h:#define PROCESSOR_DEFAULT64 PROCESSOR_RS64A > gcc/config/rs6000/vxworks.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC604 > > , and they are unlikely to be updated later, no? In most cases did would be an ABI change. Almost never an acceptable thing to do. Segher
Re: Repost [PATCH 1/6] Add -mcpu=future
On Tue, Feb 20, 2024 at 06:35:34PM +0800, Kewen.Lin wrote: > Hi Mike, > > Sorry for late reply (just back from vacation). > > on 2024/2/8 03:58, Michael Meissner wrote: > > On Wed, Feb 07, 2024 at 05:21:10PM +0800, Kewen.Lin wrote: > >> on 2024/2/6 14:01, Michael Meissner wrote: > >> Sorry for the possible confusion here, the "tune_proc" that I referred to > >> is > >> the variable in the above else branch: > >> > >>enum processor_type tune_proc = (TARGET_POWERPC64 ? PROCESSOR_DEFAULT64 > >> : PROCESSOR_DEFAULT); > >> > >> It's either PROCESSOR_DEFAULT64 or PROCESSOR_DEFAULT, so it doesn't have a > >> chance to be PROCESSOR_FUTURE, so the checking "tune_proc == > >> PROCESSOR_FUTURE" > >> is useless. > > > > PROCESSOR_DEFAULT can be PROCESSOR_FUTURE if somebody configures GCC with > > --with-cpu=future. While in general it shouldn't occur, it is helpful to > > consider all of the corner cases. > > But it sounds not true, I think you meant TARGET_CPU_DEFAULT instead? > > On one local ppc64le machine I tried to configure with --with-cpu=power10, > I got {,OPTION_}TARGET_CPU_DEFAULT "power10" but PROCESSOR_DEFAULT is still > PROCESSOR_POWER7 (PROCESSOR_DEFAULT64 is PROCESSOR_POWER8). I think these > PROCESSOR_DEFAULT{,64} are defined by various headers: Yes, I was mistaken. You are correct TARGET_CPU_DEFAULT is set. I will change the comments. > gcc/config/rs6000/aix71.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER7 > gcc/config/rs6000/aix71.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER7 > gcc/config/rs6000/aix72.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER7 > gcc/config/rs6000/aix72.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER7 > gcc/config/rs6000/aix73.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER8 > gcc/config/rs6000/aix73.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER8 > gcc/config/rs6000/darwin.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC7400 > gcc/config/rs6000/darwin.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER4 > gcc/config/rs6000/freebsd64.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC7450 > gcc/config/rs6000/freebsd64.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER8 > gcc/config/rs6000/linux64.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER7 > gcc/config/rs6000/linux64.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER8 > gcc/config/rs6000/rs6000.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC603 > gcc/config/rs6000/rs6000.h:#define PROCESSOR_DEFAULT64 PROCESSOR_RS64A > gcc/config/rs6000/vxworks.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC604 > > , and they are unlikely to be updated later, no? > > btw, the given --with-cpu=future will make cpu_index never negative so > > ... > else if (cpu_index >= 0) > rs6000_tune_index = tune_index = cpu_index; > else > ... > > so there is no chance to enter "else" arm, that is, that arm only takes > effect when no cpu/tune is given (neither -m{cpu,tune} nor --with-cpu=). Note, this is existing code. I didn't modify it. If we want to change it, we should do it as another patch. -- Michael Meissner, IBM PO Box 98, Ayer, Massachusetts, USA, 01432 email: meiss...@linux.ibm.com
Re: Repost [PATCH 1/6] Add -mcpu=future
Hi Mike, Sorry for late reply (just back from vacation). on 2024/2/8 03:58, Michael Meissner wrote: > On Wed, Feb 07, 2024 at 05:21:10PM +0800, Kewen.Lin wrote: >> on 2024/2/6 14:01, Michael Meissner wrote: >> Sorry for the possible confusion here, the "tune_proc" that I referred to is >> the variable in the above else branch: >> >>enum processor_type tune_proc = (TARGET_POWERPC64 ? PROCESSOR_DEFAULT64 : >> PROCESSOR_DEFAULT); >> >> It's either PROCESSOR_DEFAULT64 or PROCESSOR_DEFAULT, so it doesn't have a >> chance to be PROCESSOR_FUTURE, so the checking "tune_proc == >> PROCESSOR_FUTURE" >> is useless. > > PROCESSOR_DEFAULT can be PROCESSOR_FUTURE if somebody configures GCC with > --with-cpu=future. While in general it shouldn't occur, it is helpful to > consider all of the corner cases. But it sounds not true, I think you meant TARGET_CPU_DEFAULT instead? On one local ppc64le machine I tried to configure with --with-cpu=power10, I got {,OPTION_}TARGET_CPU_DEFAULT "power10" but PROCESSOR_DEFAULT is still PROCESSOR_POWER7 (PROCESSOR_DEFAULT64 is PROCESSOR_POWER8). I think these PROCESSOR_DEFAULT{,64} are defined by various headers: $ grep -r "define PROCESSOR_DEFAULT" gcc/config/rs6000/ gcc/config/rs6000/aix71.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER7 gcc/config/rs6000/aix71.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER7 gcc/config/rs6000/aix72.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER7 gcc/config/rs6000/aix72.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER7 gcc/config/rs6000/aix73.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER8 gcc/config/rs6000/aix73.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER8 gcc/config/rs6000/darwin.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC7400 gcc/config/rs6000/darwin.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER4 gcc/config/rs6000/freebsd64.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC7450 gcc/config/rs6000/freebsd64.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER8 gcc/config/rs6000/linux64.h:#define PROCESSOR_DEFAULT PROCESSOR_POWER7 gcc/config/rs6000/linux64.h:#define PROCESSOR_DEFAULT64 PROCESSOR_POWER8 gcc/config/rs6000/rs6000.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC603 gcc/config/rs6000/rs6000.h:#define PROCESSOR_DEFAULT64 PROCESSOR_RS64A gcc/config/rs6000/vxworks.h:#define PROCESSOR_DEFAULT PROCESSOR_PPC604 , and they are unlikely to be updated later, no? btw, the given --with-cpu=future will make cpu_index never negative so ... else if (cpu_index >= 0) rs6000_tune_index = tune_index = cpu_index; else ... so there is no chance to enter "else" arm, that is, that arm only takes effect when no cpu/tune is given (neither -m{cpu,tune} nor --with-cpu=). BR, Kewen
Re: Repost [PATCH 1/6] Add -mcpu=future
On Fri, Jan 05, 2024 at 06:35:37PM -0500, Michael Meissner wrote: > * config/rs6000/rs6000.opt (-mfuture): New undocumented debug switch. No. Never ever use a flag that does what -mcpu= should do. We're still trying to recover from previous such mistakes. Don't add more please. > +++ b/gcc/config/rs6000/rs6000-c.cc > @@ -447,6 +447,8 @@ rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT > flags) > rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR9"); >if ((flags & OPTION_MASK_POWER10) != 0) > rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR10"); > + if ((flags & OPTION_MASK_FUTURE) != 0) > +rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR_FUTURE"); if a & B) != 0) != 0) != 0) ? You can do just if (a & B) Yes, existing code already does the silly thing, but just fix it then, don't add more :-) (And no if ((a & B)) either please). > +static int > +rs600_cpu_index_lookup (enum processor_type processor) > +{ > + for (size_t i = 0; i < ARRAY_SIZE (processor_target_table); i++) > +if (processor_target_table[i].processor == processor) > + return i; > + > + return -1; > +} "int i" please, not "size_t". This has nothing to do with object sizes. The loop counter will always be a small number. > + /* At the moment, we don't have explict -mtune=future support. If the user "At the moment" is out of date almost as soon as you write it. It is better to avoid such terms ;-) > + explicitly tried to use -mtune=future, give a warning. If not, use the > + power10 tuning until future tuning is added. */ There should be Power11 tuning now, please use that? So please post this -- as a separate series, and not as a single patch -- after fixing the things Ke Wen pointed out. Thanks! Segher
Re: Repost [PATCH 1/6] Add -mcpu=future
On Wed, Feb 07, 2024 at 05:21:10PM +0800, Kewen.Lin wrote: > on 2024/2/6 14:01, Michael Meissner wrote: > > It was more as a separation. The MPCCORE, CELL, PPCA2, and TITAN are rather > > old processors. I'll probably remove Titan soonish, btw. We have adjusted code around it for what, fifteen years? But the hardware never materialized. There are more silly things in our backend, but this one takes the prize. > OK, considering we only get this warning once for a simple case, I'm inclined > not to keep a static variable for it, it's the same as what we do currently > for option conflict errors emission. But I'm fine for either. Whatever is easiest. Segher
Re: Repost [PATCH 1/6] Add -mcpu=future
On Tue, Feb 06, 2024 at 01:01:52AM -0500, Michael Meissner wrote: > > Nit: Named as "ISA_FUTURE_MASKS_SERVER" seems more accurate as it's > > constituted > > with ISA_3_1_MASKS_**SERVER** ... > > Well the _SERVER stuff was due to the power7 days when we still had to support > the E500 in the main rs6000 tree. But I will change it to be more consistant > in the future patches. "_SERVER" still is a good shortish name for the server systems ;-) > > > @@ -67,7 +67,9 @@ enum processor_type > > > PROCESSOR_MPCCORE, > > > PROCESSOR_CELL, > > > PROCESSOR_PPCA2, > > > - PROCESSOR_TITAN > > > + PROCESSOR_TITAN, > > > + > > > > Nit: unintentional empty line? > > > > > + PROCESSOR_FUTURE > > > }; > > It was more as a separation. The MPCCORE, CELL, PPCA2, and TITAN are rather > old processors. I don't recall why we kept them after the POWER. Please don't add random separations. > Logically we should re-order the list and move MPCCORE, etc. earlier, but I > will delete the blank line in future patches. Don't randomly reorder, either. _FUTURE should be added after POWER11. > > I think we should also update asm_names in driver-rs6000.cc. > > Ok. Though the driver-rs6000.cc stuff won't kick in until we have a real > system that matches "future". Or when during development you have that faked. You did test it, right? :-) Segher
Re: Repost [PATCH 1/6] Add -mcpu=future
On Wed, Feb 07, 2024 at 05:21:10PM +0800, Kewen.Lin wrote: > on 2024/2/6 14:01, Michael Meissner wrote: > Sorry for the possible confusion here, the "tune_proc" that I referred to is > the variable in the above else branch: > >enum processor_type tune_proc = (TARGET_POWERPC64 ? PROCESSOR_DEFAULT64 : > PROCESSOR_DEFAULT); > > It's either PROCESSOR_DEFAULT64 or PROCESSOR_DEFAULT, so it doesn't have a > chance to be PROCESSOR_FUTURE, so the checking "tune_proc == PROCESSOR_FUTURE" > is useless. PROCESSOR_DEFAULT can be PROCESSOR_FUTURE if somebody configures GCC with --with-cpu=future. While in general it shouldn't occur, it is helpful to consider all of the corner cases. -- Michael Meissner, IBM PO Box 98, Ayer, Massachusetts, USA, 01432 email: meiss...@linux.ibm.com
Re: Repost [PATCH 1/6] Add -mcpu=future
on 2024/2/6 14:01, Michael Meissner wrote: > On Tue, Jan 23, 2024 at 04:44:32PM +0800, Kewen.Lin wrote: ... >>> diff --git a/gcc/config/rs6000/rs6000-opts.h >>> b/gcc/config/rs6000/rs6000-opts.h >>> index 33fd0efc936..25890ae3034 100644 >>> --- a/gcc/config/rs6000/rs6000-opts.h >>> +++ b/gcc/config/rs6000/rs6000-opts.h >>> @@ -67,7 +67,9 @@ enum processor_type >>> PROCESSOR_MPCCORE, >>> PROCESSOR_CELL, >>> PROCESSOR_PPCA2, >>> - PROCESSOR_TITAN >>> + PROCESSOR_TITAN, >>> + >> >> Nit: unintentional empty line? >> >>> + PROCESSOR_FUTURE >>> }; > > It was more as a separation. The MPCCORE, CELL, PPCA2, and TITAN are rather > old processors. I don't recall why we kept them after the POWER. > > Logically we should re-order the list and move MPCCORE, etc. earlier, but I > will delete the blank line in future patches. Thanks for clarifying, the re-order thing can be done in a separate patch and in this context one comment line would be better than a blank line. :) ... >>> + power10 tuning until future tuning is added. */ >>>if (rs6000_tune_index >= 0) >>> -tune_index = rs6000_tune_index; >>> +{ >>> + enum processor_type cur_proc >>> + = processor_target_table[rs6000_tune_index].processor; >>> + >>> + if (cur_proc == PROCESSOR_FUTURE) >>> + { >>> + static bool issued_future_tune_warning = false; >>> + if (!issued_future_tune_warning) >>> + { >>> + issued_future_tune_warning = true; >> >> This seems to ensure we only warn this once, but I noticed that in rs6000/ >> only some OPT_Wpsabi related warnings adopt this way, I wonder if we don't >> restrict it like this, for a tiny simple case, how many times it would warn? > > In a simple case, you would only get the warning once. But if you use > __attribute__((__target__(...))) or #pragma target ... you might see it more > than once. OK, considering we only get this warning once for a simple case, I'm inclined not to keep a static variable for it, it's the same as what we do currently for option conflict errors emission. But I'm fine for either. >>>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; >>> - } >>> + tune_index = rs600_cpu_index_lookup (tune_proc == PROCESSOR_FUTURE >>> + ? PROCESSOR_POWER10 >>> + : tune_proc); >> >> This part looks useless, as tune_proc is impossible to be PROCESSOR_FUTURE. > > Well in theory, you could configure the compiler with --with-cpu=future or > --with-tune=future. Sorry for the possible confusion here, the "tune_proc" that I referred to is the variable in the above else branch: enum processor_type tune_proc = (TARGET_POWERPC64 ? PROCESSOR_DEFAULT64 : PROCESSOR_DEFAULT); It's either PROCESSOR_DEFAULT64 or PROCESSOR_DEFAULT, so it doesn't have a chance to be PROCESSOR_FUTURE, so the checking "tune_proc == PROCESSOR_FUTURE" is useless. That's why I suggested the below flow, it does a final check out of those checks, it looks a bit more clear IMHO. > >>> } >> >> Maybe re-structure the above into: >> >> bool explicit_tune = false; >> if (rs6000_tune_index >= 0) >> { >> tune_index = rs6000_tune_index; >> explicit_tune = true; >> } >> else if (cpu_index >= 0) >> // as before >> rs6000_tune_index = tune_index = cpu_index; >> else >> { >>//as before >>... >> } >> >> // Check tune_index here instead. >> >> if (processor_target_table[tune_index].processor == PROCESSOR_FUTURE) >> { >> tune_index = rs6000_cpu_index_lookup (PROCESSOR_POWER10); >> if (explicit_tune) >> warn ... >> } >> >> // as before >> rs6000_tune = processor_target_table[tune_index].processor; >> >>> BR, Kewen
Re: Repost [PATCH 1/6] Add -mcpu=future
On Tue, Jan 23, 2024 at 04:44:32PM +0800, Kewen.Lin wrote: > > --- a/gcc/config/rs6000/rs6000-cpus.def > > +++ b/gcc/config/rs6000/rs6000-cpus.def > > @@ -88,6 +88,10 @@ > > | OPTION_MASK_POWER10 \ > > | OTHER_POWER10_MASKS) > > > > +/* Flags for a potential future processor that may or may not be > > delivered. */ > > +#define ISA_FUTURE_MASKS (ISA_3_1_MASKS_SERVER \ > > +| OPTION_MASK_FUTURE) > > + > > Nit: Named as "ISA_FUTURE_MASKS_SERVER" seems more accurate as it's > constituted > with ISA_3_1_MASKS_**SERVER** ... Well the _SERVER stuff was due to the power7 days when we still had to support the E500 in the main rs6000 tree. But I will change it to be more consistant in the future patches. > ..., then this need to be updated accordingly. > > > diff --git a/gcc/config/rs6000/rs6000-opts.h > > b/gcc/config/rs6000/rs6000-opts.h > > index 33fd0efc936..25890ae3034 100644 > > --- a/gcc/config/rs6000/rs6000-opts.h > > +++ b/gcc/config/rs6000/rs6000-opts.h > > @@ -67,7 +67,9 @@ enum processor_type > > PROCESSOR_MPCCORE, > > PROCESSOR_CELL, > > PROCESSOR_PPCA2, > > - PROCESSOR_TITAN > > + PROCESSOR_TITAN, > > + > > Nit: unintentional empty line? > > > + PROCESSOR_FUTURE > > }; It was more as a separation. The MPCCORE, CELL, PPCA2, and TITAN are rather old processors. I don't recall why we kept them after the POWER. Logically we should re-order the list and move MPCCORE, etc. earlier, but I will delete the blank line in future patches. > > +static int > > +rs600_cpu_index_lookup (enum processor_type processor) > > s/rs600_cpu_index_lookup/rs6000_cpu_index_lookup/ I'm going to redo it, and eliminate rs600_cpu_index_lookup. Thanks for catching the spelling of rs600 instead of rs6000. > > +{ > > + for (size_t i = 0; i < ARRAY_SIZE (processor_target_table); i++) > > +if (processor_target_table[i].processor == processor) > > + return i; > > + > > + return -1; > > +} > > Nit: Since this is given with a valid enum processor_type, I think it should > never return -1? If so, may be more clear with gcc_unreachable () or adjust > with initial -1, break when hits and assert it's not -1. As I said, in looking at it, I think I will rewrite the code that uses it to call rs6000_cpu_name_lookup instead. > > + > > > > /* Return number of consecutive hard regs needed starting at reg REGNO > > to hold something of mode MODE. > > @@ -3756,23 +3768,45 @@ rs6000_option_override_internal (bool global_init_p) > > rs6000_isa_flags &= ~OPTION_MASK_POWERPC64; > > #endif > > > > + /* At the moment, we don't have explict -mtune=future support. If the > > user > > Nit: s/explict/explicit/ Thanks. > > > + explicitly tried to use -mtune=future, give a warning. If not, use > > the > > Nit: s/tried/tries/? Thanks. I will reword the comment. > > + power10 tuning until future tuning is added. */ > >if (rs6000_tune_index >= 0) > > -tune_index = rs6000_tune_index; > > +{ > > + enum processor_type cur_proc > > + = processor_target_table[rs6000_tune_index].processor; > > + > > + if (cur_proc == PROCESSOR_FUTURE) > > + { > > + static bool issued_future_tune_warning = false; > > + if (!issued_future_tune_warning) > > + { > > + issued_future_tune_warning = true; > > This seems to ensure we only warn this once, but I noticed that in rs6000/ > only some OPT_Wpsabi related warnings adopt this way, I wonder if we don't > restrict it like this, for a tiny simple case, how many times it would warn? In a simple case, you would only get the warning once. But if you use __attribute__((__target__(...))) or #pragma target ... you might see it more than once. > > + warning (0, "%qs is not currently supported", "-mtune=future"); > > + } > > +> + rs6000_tune_index = rs600_cpu_index_lookup > > (PROCESSOR_POWER10); > > + } > > + tune_index = rs6000_tune_index; > > +} > >else if (cpu_index >= 0) > > -rs6000_tune_index = tune_index = cpu_index; > > +{ > > + enum processor_type cur_cpu > > + = processor_target_table[cpu_index].processor; > > + > > + rs6000_tune_index = tune_index > > + = (cur_cpu == PROCESSOR_FUTURE > > + ? rs600_cpu_index_lookup (PROCESSOR_POWER10) > > s/rs600_cpu_index_lookup/rs6000_cpu_index_lookup/ See above. > > + : cpu_index); > > +} > >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; > > - } > > + tune_index = rs600_cpu_index_lookup (tune_proc == PROCESSOR_FUTURE > > +
Re: Repost [PATCH 1/6] Add -mcpu=future
Hi Mike, on 2024/1/6 07:35, Michael Meissner wrote: > This patch implements support for a potential future PowerPC cpu. Features > added with -mcpu=future, may or may not be added to new PowerPC processors. > > This patch adds support for the -mcpu=future option. If you use -mcpu=future, > the macro __ARCH_PWR_FUTURE__ is defined, and the assembler .machine directive > "future" is used. Future patches in this series will add support for new > instructions that may be present in future PowerPC processors. > > This particular patch does not any new features. It exists as a ground work > for future patches to support for a possible PowerPC processor in the future. > > This patch does not implement any differences in tuning when -mcpu=future is > used compared to -mcpu=power10. If -mcpu=future is used, GCC will use power10 > tuning. If you explicitly use -mtune=future, you will get a warning that > -mtune=future is not supported, and default tuning will be set for power10. > > The patches have been tested on both little and big endian systems. Can I > check > it into the master branch? > > 2024-01-05 Michael Meissner > > gcc/ > > * config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define > __ARCH_PWR_FUTURE__ if -mcpu=future. > * config/rs6000/rs6000-cpus.def (ISA_FUTURE_MASKS): New macro. > (POWERPC_MASKS): Add -mcpu=future support. > * config/rs6000/rs6000-opts.h (enum processor_type): Add > PROCESSOR_FUTURE. > * config/rs6000/rs6000-tables.opt: Regenerate. > * config/rs6000/rs6000.cc (rs600_cpu_index_lookup): New helper > function. > (rs6000_option_override_internal): Make -mcpu=future set > -mtune=power10. If the user explicitly uses -mtune=future, give a > warning and reset the tuning to power10. > (rs6000_option_override_internal): Use power10 costs for future > machine. > (rs6000_machine_from_flags): Add support for -mcpu=future. > (rs6000_opt_masks): Likewise. > * config/rs6000/rs6000.h (ASM_CPU_SUPPORT): Likewise. > * config/rs6000/rs6000.md (cpu attribute): Likewise. > * config/rs6000/rs6000.opt (-mfuture): New undocumented debug switch. > * doc/invoke.texi (IBM RS/6000 and PowerPC Options): Document > -mcpu=future. > --- > gcc/config/rs6000/rs6000-c.cc | 2 + > gcc/config/rs6000/rs6000-cpus.def | 6 +++ > gcc/config/rs6000/rs6000-opts.h | 4 +- > gcc/config/rs6000/rs6000-tables.opt | 3 ++ > gcc/config/rs6000/rs6000.cc | 58 - > gcc/config/rs6000/rs6000.h | 1 + > gcc/config/rs6000/rs6000.md | 2 +- > gcc/config/rs6000/rs6000.opt| 4 ++ > gcc/doc/invoke.texi | 2 +- > 9 files changed, 69 insertions(+), 13 deletions(-) > > diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc > index ce0b14a8d37..f2fb5bef678 100644 > --- a/gcc/config/rs6000/rs6000-c.cc > +++ b/gcc/config/rs6000/rs6000-c.cc > @@ -447,6 +447,8 @@ rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT > flags) > rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR9"); >if ((flags & OPTION_MASK_POWER10) != 0) > rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR10"); > + if ((flags & OPTION_MASK_FUTURE) != 0) > +rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR_FUTURE"); >if ((flags & OPTION_MASK_SOFT_FLOAT) != 0) > rs6000_define_or_undefine_macro (define_p, "_SOFT_FLOAT"); >if ((flags & OPTION_MASK_RECIP_PRECISION) != 0) > diff --git a/gcc/config/rs6000/rs6000-cpus.def > b/gcc/config/rs6000/rs6000-cpus.def > index d28cc87eb2a..8754635f3d9 100644 > --- a/gcc/config/rs6000/rs6000-cpus.def > +++ b/gcc/config/rs6000/rs6000-cpus.def > @@ -88,6 +88,10 @@ >| OPTION_MASK_POWER10 \ >| OTHER_POWER10_MASKS) > > +/* Flags for a potential future processor that may or may not be delivered. > */ > +#define ISA_FUTURE_MASKS (ISA_3_1_MASKS_SERVER \ > + | OPTION_MASK_FUTURE) > + Nit: Named as "ISA_FUTURE_MASKS_SERVER" seems more accurate as it's constituted with ISA_3_1_MASKS_**SERVER** ... > /* Flags that need to be turned off if -mno-power9-vector. */ > #define OTHER_P9_VECTOR_MASKS(OPTION_MASK_FLOAT128_HW > \ >| OPTION_MASK_P9_MINMAX) > @@ -135,6 +139,7 @@ >| OPTION_MASK_LOAD_VECTOR_PAIR \ >| OPTION_MASK_POWER10 \ >| OPTION_MASK_P10_FUSION \ > + | OPTION_MASK_FUTURE \ >| OPTION_MASK_HTM \ >| OPTION_MASK_ISEL \ >| OPTION_MASK_MFCRF
Repost [PATCH 1/6] Add -mcpu=future
This patch implements support for a potential future PowerPC cpu. Features added with -mcpu=future, may or may not be added to new PowerPC processors. This patch adds support for the -mcpu=future option. If you use -mcpu=future, the macro __ARCH_PWR_FUTURE__ is defined, and the assembler .machine directive "future" is used. Future patches in this series will add support for new instructions that may be present in future PowerPC processors. This particular patch does not any new features. It exists as a ground work for future patches to support for a possible PowerPC processor in the future. This patch does not implement any differences in tuning when -mcpu=future is used compared to -mcpu=power10. If -mcpu=future is used, GCC will use power10 tuning. If you explicitly use -mtune=future, you will get a warning that -mtune=future is not supported, and default tuning will be set for power10. The patches have been tested on both little and big endian systems. Can I check it into the master branch? 2024-01-05 Michael Meissner gcc/ * config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define __ARCH_PWR_FUTURE__ if -mcpu=future. * config/rs6000/rs6000-cpus.def (ISA_FUTURE_MASKS): New macro. (POWERPC_MASKS): Add -mcpu=future support. * config/rs6000/rs6000-opts.h (enum processor_type): Add PROCESSOR_FUTURE. * config/rs6000/rs6000-tables.opt: Regenerate. * config/rs6000/rs6000.cc (rs600_cpu_index_lookup): New helper function. (rs6000_option_override_internal): Make -mcpu=future set -mtune=power10. If the user explicitly uses -mtune=future, give a warning and reset the tuning to power10. (rs6000_option_override_internal): Use power10 costs for future machine. (rs6000_machine_from_flags): Add support for -mcpu=future. (rs6000_opt_masks): Likewise. * config/rs6000/rs6000.h (ASM_CPU_SUPPORT): Likewise. * config/rs6000/rs6000.md (cpu attribute): Likewise. * config/rs6000/rs6000.opt (-mfuture): New undocumented debug switch. * doc/invoke.texi (IBM RS/6000 and PowerPC Options): Document -mcpu=future. --- gcc/config/rs6000/rs6000-c.cc | 2 + gcc/config/rs6000/rs6000-cpus.def | 6 +++ gcc/config/rs6000/rs6000-opts.h | 4 +- gcc/config/rs6000/rs6000-tables.opt | 3 ++ gcc/config/rs6000/rs6000.cc | 58 - gcc/config/rs6000/rs6000.h | 1 + gcc/config/rs6000/rs6000.md | 2 +- gcc/config/rs6000/rs6000.opt| 4 ++ gcc/doc/invoke.texi | 2 +- 9 files changed, 69 insertions(+), 13 deletions(-) diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc index ce0b14a8d37..f2fb5bef678 100644 --- a/gcc/config/rs6000/rs6000-c.cc +++ b/gcc/config/rs6000/rs6000-c.cc @@ -447,6 +447,8 @@ rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT flags) rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR9"); if ((flags & OPTION_MASK_POWER10) != 0) rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR10"); + if ((flags & OPTION_MASK_FUTURE) != 0) +rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR_FUTURE"); if ((flags & OPTION_MASK_SOFT_FLOAT) != 0) rs6000_define_or_undefine_macro (define_p, "_SOFT_FLOAT"); if ((flags & OPTION_MASK_RECIP_PRECISION) != 0) diff --git a/gcc/config/rs6000/rs6000-cpus.def b/gcc/config/rs6000/rs6000-cpus.def index d28cc87eb2a..8754635f3d9 100644 --- a/gcc/config/rs6000/rs6000-cpus.def +++ b/gcc/config/rs6000/rs6000-cpus.def @@ -88,6 +88,10 @@ | OPTION_MASK_POWER10 \ | OTHER_POWER10_MASKS) +/* Flags for a potential future processor that may or may not be delivered. */ +#define ISA_FUTURE_MASKS (ISA_3_1_MASKS_SERVER \ +| OPTION_MASK_FUTURE) + /* Flags that need to be turned off if -mno-power9-vector. */ #define OTHER_P9_VECTOR_MASKS (OPTION_MASK_FLOAT128_HW\ | OPTION_MASK_P9_MINMAX) @@ -135,6 +139,7 @@ | OPTION_MASK_LOAD_VECTOR_PAIR \ | OPTION_MASK_POWER10 \ | OPTION_MASK_P10_FUSION \ +| OPTION_MASK_FUTURE \ | OPTION_MASK_HTM \ | OPTION_MASK_ISEL \ | OPTION_MASK_MFCRF\ @@ -267,3 +272,4 @@ RS6000_CPU ("powerpc64", PROCESSOR_POWERPC64, OPTION_MASK_PPC_GFXOPT RS6000_CPU ("powerpc64le", PROCESSOR_POWER8, MASK_POWERPC64 | ISA_2_7_MASKS_SERVER | OPTION_MASK_HTM) RS6000_CPU ("rs64", PROCESSOR_RS64A, OPTION_MASK_PPC_GFXOPT | MASK_POWERPC64) +RS6000_CP