On Thu, Jul 18, 2024 at 08:08:44AM -0500, Segher Boessenkool wrote:
> Hi!
>
> [ I reviewed this together with Ke Wen. All blame should go to me, all
> praise to him. ]
>
> On Wed, Jul 10, 2024 at 01:34:26PM -0400, Michael Meissner wrote:
> > [This is a repost of the June 4th patch]
>
> It is not a repost. It is v2. It has important changes.
The first 2 patches for the compiler proper are exactly the same as the June
4th post. Yes, the June 4th post had changes from the previous post.
The only change was to the testsuite.
> > * config.gcc (rs6000*-*-*, powerpc*-*-*): Add support for power11.
>
> Please do not add new rs6000-* stuff. There haven't been systems like
> that for twenty years or so now. It is mostly harmless to add this
> stuff, but it just makes more work to delete later. And more
> importantly, this was not tested; this *cannot* be tested.
The code I added was just extra code in the powerpc section. I did not change
the case statement. That code had the rs6000* line, so I was just quoting it
in the ChangeLog.
>
> > * config/rs6000/rs6000-cpus.def (ISA_POWER11_MASKS_SERVER): New define.
> > (POWERPC_MASKS): Add power11 isa bit.
>
> There is no Power11 ISA bit. There cannnot be a Power11 ISA bit. There
> is no new ISA for Power11.
>
> "Add power11 bit"? It is a hack to easily distinguish between power10
> and power11, nothing more. It abuses existing mechanisms a bit. Not
> the nicest thing to do, but it works :-)
>
> But please do not call it what it is not. And mark it up as a hack in
> some nice place.
The issue is right now the mechanism to distinguish between the cpus is the
bits in rs6000_isa_flags. Sure other mechanisms can be done, and perhaps we
should create them, but if you want the correct defines and .machines to be
done for things like #pragma GCC tar and the target attribute, the simplest way
is to create a new bit in rs6000_isa_flags, and just mark the option as giving
an error if the user explicity uses it.
> > * config/rs6000/rs6000.opt (-mpower11): Add internal power11 ISA flag.
>
> s//ISA //
>
> It is not nice to have a user-selectable option for this at all :-(
>
> > --- a/gcc/config.gcc
> > +++ b/gcc/config.gcc
> > @@ -533,7 +533,9 @@ powerpc*-*-*)
> > extra_headers="${extra_headers} ppu_intrinsics.h spu2vmx.h vec_types.h
> > si2vmx.h"
> > extra_headers="${extra_headers} amo.h"
> > case x$with_cpu in
> > -
> > xpowerpc64|xdefault64|x6[23]0|x970|xG5|xpower[3456789]|xpower10|xpower6x|xrs64a|xcell|xa2|xe500mc64|xe5500|xe6500)
> > + xpowerpc64 | xdefault64 | x6[23]0 | x970 | xG5 | xpower[3456789] \
> > + | xpower1[01] | xpower6x | xrs64a | xcell | xa2 | xe500mc64 \
> > + | xe5500 | xe6500)
> > cpu_is_64bit=yes
> > ;;
> > esac
>
> Please do not change form and function at the same time. It is much
> easier to see what is going on, to verify the change is correct, if you
> do it as two patches (either the cleanup first, which is nicer, or as a
> later step, which is often easier).
I can just make the line even longer if you prefer.
> > --- a/gcc/config/rs6000/ppc-auxv.h
> > +++ b/gcc/config/rs6000/ppc-auxv.h
> > @@ -47,9 +47,8 @@
> > #define PPC_PLATFORM_PPC476 12
> > #define PPC_PLATFORM_POWER8 13
> > #define PPC_PLATFORM_POWER9 14
> > -
> > -/* This is not yet official. */
> > #define PPC_PLATFORM_POWER10 15
> > +#define PPC_PLATFORM_POWER11 16
>
> Please add a comment where the official thing is?
>
> It is in glibc dl-procinfo.h, and there *cannot* be more than 16
> platforms currently, so how can this work? Or do we get data
> directly from the kernel, or what?
>
> But you tested it, so you do obviously get PPC_PLATFORM_POWER11 from
> somewhere. I cannot see from where though?
In other discussions, I was told that 16 with be the platform number for the
kernel in the future.
> > --- a/gcc/config/rs6000/rs6000-opts.h
> > +++ b/gcc/config/rs6000/rs6000-opts.h
> > @@ -67,7 +67,8 @@ enum processor_type
> > PROCESSOR_MPCCORE,
> > PROCESSOR_CELL,
> > PROCESSOR_PPCA2,
> > - PROCESSOR_TITAN
> > + PROCESSOR_TITAN,
> > + PROCESSOR_POWER11
>
> Please insert this after the p10 entry. There is a (very vague)
> ordering here, we do not put the newest at the end.
Ok.
> > -/* Instruction costs on POWER10 processors. */
> > +/* Instruction costs on POWER10/POWER11 processors. */
>
> The official names are Power10 and Power11. POWER9 is still in SHOUTING
> CAPS, but later CPUs have the more relaxed spelling as official names
> (just like the Power ISA has had for longer already).
Ok, but I was just using the current code's format.
> > - /* Do Power10 dependent reordering. */
> > - if (rs6000_tune == PROCESSOR_POWER10 && last_scheduled_insn)
> > + /* Do Power10/power11 dependent reordering. */
>
> Power11.
Ok.
> > + /* Do Power10/power11 dependent reordering. */
>
> Again.
>
> > --- a/gcc/config/rs6000/rs6000.md
> > +++ b/gcc/config/rs6000/rs6000.md
> > @@ -351,7 +351,7 @@ (define_attr "cpu"
> > ppc403,ppc405,ppc440,ppc476,
> >
> > ppc8540,ppc8548,ppce300c2,ppce300c3,ppce500mc,ppce500mc64,ppce5500,ppce6500,
> > power4,power5,power6,power7,power8,power9,power10,
> > - rs64a,mpccore,cell,ppca2,titan"
> > + rs64a,mpccore,cell,ppca2,titan,power11"
>
> Add this after power10.
Ok.
> > --- a/gcc/config/rs6000/rs6000.opt
> > +++ b/gcc/config/rs6000/rs6000.opt
> > @@ -585,6 +585,9 @@ Target Undocumented
> > Var(rs6000_speculate_indirect_jumps) Init(1) Save
> > mpower10
> > Target Undocumented Mask(POWER10) Var(rs6000_isa_flags) WarnRemoved
> >
> > +mpower11
> > +Target Undocumented Mask(POWER11) Var(rs6000_isa_flags) Warn(Do not use
> > %<-mpower11>)
>
> So why have this option at all? It is a hack because you want to look
> at this bitfield instead of the "which cpu/tune do we have selected"
> field in some places? That is only acceptable if you do not hide that
> fact!
>
> Why is it not "WarnRemoved" like mpower10 is? That wasn't factually
> true there either. Emulating this manually with some custom Warn is
> farther from ideal.
>
> It would be much better if we did not need to use an ugly hack like
> this. Future improvement :-)
Ok.
> > --- a/gcc/doc/invoke.texi
> > +++ b/gcc/doc/invoke.texi
> > @@ -31488,8 +31488,9 @@ Supported values for @var{cpu_type} are @samp{401},
> > @samp{403},
> > @samp{e6500}, @samp{ec603e}, @samp{G3}, @samp{G4}, @samp{G5},
> > @samp{titan}, @samp{power3}, @samp{power4}, @samp{power5}, @samp{power5+},
> > @samp{power6}, @samp{power6x}, @samp{power7}, @samp{power8},
> > -@samp{power9}, @samp{power10}, @samp{powerpc}, @samp{powerpc64},
> > -@samp{powerpc64le}, @samp{rs64}, and @samp{native}.
> > +@samp{power9}, @samp{power10}, @samp{power11},
> > +@samp{powerpc}, @samp{powerpc64}, @samp{powerpc64le},
> > +@samp{rs64}, and @samp{native}.
>
> Hint: you do not need to layout TeXinfo source code. TeX itself takes
> care of that much better than you can. It is fine to keep some short
> lines, this will be rewrapped in output anyway.
Ok.
> Okay for trunk with those changes. Also fine for backport to 14.
> Thank you!
>
>
> Segher
--
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meiss...@linux.ibm.com
