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.
> * 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.
> * 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.
> * 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).
> --- 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?
> --- 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.
> -/* 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).
> - /* Do Power10 dependent reordering. */
> - if (rs6000_tune == PROCESSOR_POWER10 && last_scheduled_insn)
> + /* Do Power10/power11 dependent reordering. */
Power11.
> + /* 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.
> --- 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 :-)
> --- 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.
Okay for trunk with those changes. Also fine for backport to 14.
Thank you!
Segher
