| Message ID | 20220919140149.4018927-6-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | powerpc: first hack at pcrel addressing | expand |
Hi! On Tue, Sep 20, 2022 at 12:01:47AM +1000, Nicholas Piggin wrote: > Update the 64s GENERIC_CPU option. POWER4 support has been dropped, so > make that clear in the option name. AFAIR the minimum now is POWER4+ (ISA 2.01), not POWER5 (ISA 2.02). > -mtune= before power8 is dropped because the minimum gcc version > supports power8, and tuning is made consistent between big and little > endian. Tuning for p8 on e.g. 970 gives quite bad results. No idea if anyone cares, but this is a serious regression if so. > Big endian drops -mcpu=power4 in favour of power5. Effectively the > minimum compiler version means power5 was always being selected here, > so this should not change anything. 970 / G5 code generation does not > seem to have been a problem with -mcpu=power5, but it's possible we > should go back to power4 to be really safe. Yes, -mcpu=power5 code does *not* run on 970, if you are unlucky enough that the compiler does something smart with popcntb (the sole non-float insn new on p5, not counting hrfid). > +# -mcpu=power5 should generate 970 compatible kernel code It doesn't. Even if it did, it would need more explanation! Segher
On Wed Sep 21, 2022 at 8:16 AM AEST, Segher Boessenkool wrote: > Hi! > > On Tue, Sep 20, 2022 at 12:01:47AM +1000, Nicholas Piggin wrote: > > Update the 64s GENERIC_CPU option. POWER4 support has been dropped, so > > make that clear in the option name. > > AFAIR the minimum now is POWER4+ (ISA 2.01), not POWER5 (ISA 2.02). It's POWER5 now, because of commit 471d7ff8b5 ("powerpc/64s: Remove POWER4 support"), which is misguided about POWER4+ and also introduced the -mcpu=power5 bug on 970 builds :\ Not sure it's worth adding POWER4+ support back but if someone has a POWER4+ or adds it to QEMU TCG, I will do the patch. > > -mtune= before power8 is dropped because the minimum gcc version > > supports power8, and tuning is made consistent between big and little > > endian. > > Tuning for p8 on e.g. 970 gives quite bad results. No idea if anyone > cares, but this is a serious regression if so. It's for "generic" kernel so we set low minimum but higher tune, assuming that people would usually have newer, so it was already doing -mtune=power7. We could make a specific 970/G5 entry though, since those still have users. > > Big endian drops -mcpu=power4 in favour of power5. Effectively the > > minimum compiler version means power5 was always being selected here, > > so this should not change anything. 970 / G5 code generation does not > > seem to have been a problem with -mcpu=power5, but it's possible we > > should go back to power4 to be really safe. > > Yes, -mcpu=power5 code does *not* run on 970, if you are unlucky enough > that the compiler does something smart with popcntb (the sole non-float > insn new on p5, not counting hrfid). > > > +# -mcpu=power5 should generate 970 compatible kernel code > > It doesn't. Even if it did, it would need more explanation! Okay, sounds like we should go back to power4. Thanks, Nick
Hi! On Wed, Sep 21, 2022 at 11:01:18AM +1000, Nicholas Piggin wrote: > On Wed Sep 21, 2022 at 8:16 AM AEST, Segher Boessenkool wrote: > > On Tue, Sep 20, 2022 at 12:01:47AM +1000, Nicholas Piggin wrote: > > > Update the 64s GENERIC_CPU option. POWER4 support has been dropped, so > > > make that clear in the option name. > > > > AFAIR the minimum now is POWER4+ (ISA 2.01), not POWER5 (ISA 2.02). > > It's POWER5 now, because of commit 471d7ff8b5 ("powerpc/64s: Remove > POWER4 support"), which is misguided about POWER4+ and also introduced > the -mcpu=power5 bug on 970 builds :\ ISA 2.01 added just a few things (LPES[0], HDEC, some PM things, but crucially also anything that sets MSR[PR] also sets MSR[EE] since then). > Not sure it's worth adding POWER4+ support back but if someone has a > POWER4+ or adds it to QEMU TCG, I will do the patch. 970 is 2.01 -- pretending it is 2.02 is a ticking time bomb: the popcntb insn will be generated for popcount and parity intrinsics, which can be generated by generic code! > > > -mtune= before power8 is dropped because the minimum gcc version > > > supports power8, and tuning is made consistent between big and little > > > endian. > > > > Tuning for p8 on e.g. 970 gives quite bad results. No idea if anyone > > cares, but this is a serious regression if so. > > It's for "generic" kernel so we set low minimum but higher tune, > assuming that people would usually have newer, so it was already > doing -mtune=power7. > > We could make a specific 970/G5 entry though, since those still > have users. If that uses -mcpu=power4 (which means ISA 2.01 btw!) all is fine already? (Or -mcpu=970, same thing really, it just allows VMX as well). Thanks for taking care of this Nick, much appreciated! Segher
On Thu Sep 22, 2022 at 1:22 AM AEST, Segher Boessenkool wrote: > Hi! > > On Wed, Sep 21, 2022 at 11:01:18AM +1000, Nicholas Piggin wrote: > > On Wed Sep 21, 2022 at 8:16 AM AEST, Segher Boessenkool wrote: > > > On Tue, Sep 20, 2022 at 12:01:47AM +1000, Nicholas Piggin wrote: > > > > Update the 64s GENERIC_CPU option. POWER4 support has been dropped, so > > > > make that clear in the option name. > > > > > > AFAIR the minimum now is POWER4+ (ISA 2.01), not POWER5 (ISA 2.02). > > > > It's POWER5 now, because of commit 471d7ff8b5 ("powerpc/64s: Remove > > POWER4 support"), which is misguided about POWER4+ and also introduced > > the -mcpu=power5 bug on 970 builds :\ > > ISA 2.01 added just a few things (LPES[0], HDEC, some PM things, but > crucially also anything that sets MSR[PR] also sets MSR[EE] since then). Ah, right. Some Book3 cleanups mainly. > > Not sure it's worth adding POWER4+ support back but if someone has a > > POWER4+ or adds it to QEMU TCG, I will do the patch. > > 970 is 2.01 -- pretending it is 2.02 is a ticking time bomb: the popcntb > insn will be generated for popcount and parity intrinsics, which can be > generated by generic code! Yeah agreed, it was an error on my part with that original patch. > > > > -mtune= before power8 is dropped because the minimum gcc version > > > > supports power8, and tuning is made consistent between big and little > > > > endian. > > > > > > Tuning for p8 on e.g. 970 gives quite bad results. No idea if anyone > > > cares, but this is a serious regression if so. > > > > It's for "generic" kernel so we set low minimum but higher tune, > > assuming that people would usually have newer, so it was already > > doing -mtune=power7. > > > > We could make a specific 970/G5 entry though, since those still > > have users. > > If that uses -mcpu=power4 (which means ISA 2.01 btw!) all is fine > already? (Or -mcpu=970, same thing really, it just allows VMX as well). Well it does -mcpu=power4 but the "generic" CPU option also does the -mtune=power7 or 8. I added a -mcpu=970 version, even though we don't let the compiler generate VMX in the kernel so I guess it's not functionally different. > Thanks for taking care of this Nick, much appreciated! No problem, thanks for reviewing and finding my errors :P Thanks, Nick
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile index 02742facf895..471ef14f8574 100644 --- a/arch/powerpc/Makefile +++ b/arch/powerpc/Makefile @@ -149,11 +149,13 @@ CFLAGS-$(CONFIG_PPC32) += $(call cc-option,-mno-readonly-in-sdata) ifdef CONFIG_PPC_BOOK3S_64 ifdef CONFIG_CPU_LITTLE_ENDIAN CFLAGS-$(CONFIG_GENERIC_CPU) += -mcpu=power8 -CFLAGS-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mtune=power9,-mtune=power8) else -CFLAGS-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mtune=power7,$(call cc-option,-mtune=power5)) -CFLAGS-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mcpu=power5,-mcpu=power4) +# -mcpu=power5 should generate 970 compatible kernel code +CFLAGS-$(CONFIG_GENERIC_CPU) += -mcpu=power5 endif +CFLAGS-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mtune=power10, \ + $(call cc-option,-mtune=power9, \ + $(call cc-option,-mtune=power8))) else ifdef CONFIG_PPC_BOOK3E_64 CFLAGS-$(CONFIG_GENERIC_CPU) += -mcpu=powerpc64 endif diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype index 5185d942b455..4bf9af6a6eb5 100644 --- a/arch/powerpc/platforms/Kconfig.cputype +++ b/arch/powerpc/platforms/Kconfig.cputype @@ -125,7 +125,7 @@ choice If unsure, select Generic. config GENERIC_CPU - bool "Generic (POWER4 and above)" + bool "Generic (POWER5 / PPC970 and above)" depends on PPC_BOOK3S_64 && !CPU_LITTLE_ENDIAN select PPC_64S_HASH_MMU
Update the 64s GENERIC_CPU option. POWER4 support has been dropped, so make that clear in the option name. -mtune= before power8 is dropped because the minimum gcc version supports power8, and tuning is made consistent between big and little endian. Big endian drops -mcpu=power4 in favour of power5. Effectively the minimum compiler version means power5 was always being selected here, so this should not change anything. 970 / G5 code generation does not seem to have been a problem with -mcpu=power5, but it's possible we should go back to power4 to be really safe. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- arch/powerpc/Makefile | 8 +++++--- arch/powerpc/platforms/Kconfig.cputype | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-)