| Message ID | 20240308.123342.1112119677226246836.rene@exactcode.de |
|---|---|
| State | New |
| Headers | show |
| Series | fix PowerPC < 7 w/ Altivec not to default to power7 | expand |
Hi, on 2024/3/8 19:33, Rene Rebe wrote: > This might not be the best timing -short before a major release-, > however, Sam just commented on the bug I filled years ago [1], so here > we go: > > Glibc uses .machine to determine assembler optimizations to use. > However, since reworking the rs6000 .machine output selection in > commit e154242724b084380e3221df7c08fcdbd8460674 22 May 2019, G5 as > well as Cell, and even power4 w/ -maltivec currently resulted in > power7. Mask _ALTIVEC away as the .machine selection already did for > GFX and GPOPT. Thanks for fixing, this fix looks reasonable to me, OPTION_MASK_ALTIVEC is a part of POWERPC_7400_MASK so any specified cpu type which has this POWERPC_7400_MASK by default and isn't handled early in function rs6000_machine_from_flags can suffer from this issue. > > powerpc64-t2-linux-gnu-gcc test.c -S -o - -mcpu=G5 > .file "test.c" > .machine power7 > .abiversion 2 > .section ".text" > .ident "GCC: (GNU) 10.2.0" > .section .note.GNU-stack,"",@progbits > Nit: Could you also add one test case for this? btw, -mdejagnu-cpu=G5 can force the cpu type in dg-options. > We ship this in T2/Linux [2] since 2020 and it is tested on G5, Cell > and Power8. > > Signed-of-by: René Rebe <rene@exactcode.de> > > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97367 > [2] https://t2sde.org > > --- gcc-11.1.0-RC-20210423/gcc/config/rs6000/rs6000.cc.vanilla 2021-04-25 22:57:16.964223106 +0200 > +++ gcc-11.1.0-RC-20210423/gcc/config/rs6000/rs6000.cc 2021-04-25 22:57:27.193223841 +0200 > @@ -5765,7 +5765,7 @@ > HOST_WIDE_INT flags = rs6000_isa_flags; > > /* Disable the flags that should never influence the .machine selection. */ > - flags &= ~(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_PPC_GPOPT | OPTION_MASK_ISEL); > + flags &= ~(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_PPC_GPOPT | OPTION_MASK_ALTIVEC | OPTION_MASK_ISEL); Nit: This line is too long and needs re-format. BR, Kewen > > if ((flags & (ISA_3_1_MASKS_SERVER & ~ISA_3_0_MASKS_SERVER)) != 0) > return "power10"; >
Hi Kewen, thank you for your reply. > on 2024/3/8 19:33, Rene Rebe wrote: > > This might not be the best timing -short before a major release-, > > however, Sam just commented on the bug I filled years ago [1], so here > > we go: > > > > Glibc uses .machine to determine assembler optimizations to use. > > However, since reworking the rs6000 .machine output selection in > > commit e154242724b084380e3221df7c08fcdbd8460674 22 May 2019, G5 as > > well as Cell, and even power4 w/ -maltivec currently resulted in > > power7. Mask _ALTIVEC away as the .machine selection already did for > > GFX and GPOPT. > > Thanks for fixing, this fix looks reasonable to me, OPTION_MASK_ALTIVEC > is a part of POWERPC_7400_MASK so any specified cpu type which has this > POWERPC_7400_MASK by default and isn't handled early in function > rs6000_machine_from_flags can suffer from this issue. > > > > > powerpc64-t2-linux-gnu-gcc test.c -S -o - -mcpu=G5 > > .file "test.c" > > .machine power7 > > .abiversion 2 > > .section ".text" > > .ident "GCC: (GNU) 10.2.0" > > .section .note.GNU-stack,"",@progbits > > > > Nit: Could you also add one test case for this? > > btw, -mdejagnu-cpu=G5 can force the cpu type in dg-options. It took me a while to allocate enough time to study dejagnu and write a suitable test, I hope this suits your needs: --- ./gcc/testsuite/gcc.target/powerpc/pr97367.c.vanilla 2024-05-30 18:26:29.839784279 +0200 +++ ./gccc/testsuite/gcc.target/powerpc/pr97367.c 2024-05-30 18:20:34.873818482 +0200 @@ -0,0 +1,5 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target lp64 } */ +/* { dg-options "-S -mcpu=G5" } */ + +/* { dg-final { scan-assembler "power4" } } */ I double checked it works and fails as expected. > > We ship this in T2/Linux [2] since 2020 and it is tested on G5, Cell > > and Power8. > > > > Signed-of-by: René Rebe <rene@exactcode.de> > > > > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97367 > > [2] https://t2sde.org > > > > --- gcc-11.1.0-RC-20210423/gcc/config/rs6000/rs6000.cc.vanilla 2021-04-25 22:57:16.964223106 +0200 > > +++ gcc-11.1.0-RC-20210423/gcc/config/rs6000/rs6000.cc 2021-04-25 22:57:27.193223841 +0200 > > @@ -5765,7 +5765,7 @@ > > HOST_WIDE_INT flags = rs6000_isa_flags; > > > > /* Disable the flags that should never influence the .machine selection. */ > > - flags &= ~(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_PPC_GPOPT | OPTION_MASK_ISEL); > > + flags &= ~(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_PPC_GPOPT | OPTION_MASK_ALTIVEC | OPTION_MASK_ISEL); > > Nit: This line is too long and needs re-format. While I don't really find ~100 chars too long for modern standards, I'm happy to line break that for you once the above test is approved. Thank you so much, René
Hi Rene, on 2024/5/31 22:57, Rene Rebe wrote: > Hi Kewen, > > thank you for your reply. > >> on 2024/3/8 19:33, Rene Rebe wrote: >>> This might not be the best timing -short before a major release-, >>> however, Sam just commented on the bug I filled years ago [1], so here >>> we go: >>> >>> Glibc uses .machine to determine assembler optimizations to use. >>> However, since reworking the rs6000 .machine output selection in >>> commit e154242724b084380e3221df7c08fcdbd8460674 22 May 2019, G5 as >>> well as Cell, and even power4 w/ -maltivec currently resulted in >>> power7. Mask _ALTIVEC away as the .machine selection already did for >>> GFX and GPOPT. >> >> Thanks for fixing, this fix looks reasonable to me, OPTION_MASK_ALTIVEC >> is a part of POWERPC_7400_MASK so any specified cpu type which has this >> POWERPC_7400_MASK by default and isn't handled early in function >> rs6000_machine_from_flags can suffer from this issue. >> >>> >>> powerpc64-t2-linux-gnu-gcc test.c -S -o - -mcpu=G5 >>> .file "test.c" >>> .machine power7 >>> .abiversion 2 >>> .section ".text" >>> .ident "GCC: (GNU) 10.2.0" >>> .section .note.GNU-stack,"",@progbits >>> >> >> Nit: Could you also add one test case for this? >> >> btw, -mdejagnu-cpu=G5 can force the cpu type in dg-options. > > It took me a while to allocate enough time to study dejagnu and write > a suitable test, I hope this suits your needs: > > --- ./gcc/testsuite/gcc.target/powerpc/pr97367.c.vanilla 2024-05-30 18:26:29.839784279 +0200 > +++ ./gccc/testsuite/gcc.target/powerpc/pr97367.c 2024-05-30 18:20:34.873818482 +0200 > @@ -0,0 +1,5 @@ > +/* { dg-do compile } */ > +/* { dg-require-effective-target lp64 } */ Nit: I think this require-effective-target line isn't needed. > +/* { dg-options "-S -mcpu=G5" } */ "dg-do compile" ensures "Compile with ‘-S’ to produce an assembly code file.", so "-S" is redundant. As hinted before, we want -mdejagnu-cpu=G5 here rather than -mcpu=G5 because for some old versions of dejagnu the command line arguments you set in RUNTESTFLAGS will override the one set in dg-option, -mdejagnu-cpu= is one internal option only for test suite (also powerpc specific). Nit: I prefer to have one dummy function here to avoid some possible failure if users specify some options which forbids an empty translation unit. Maybe something like: int dummy () { return 0; } > + > +/* { dg-final { scan-assembler "power4" } } */ > > I double checked it works and fails as expected. > >>> We ship this in T2/Linux [2] since 2020 and it is tested on G5, Cell >>> and Power8. >>> >>> Signed-of-by: René Rebe <rene@exactcode.de> >>> >>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97367 >>> [2] https://t2sde.org >>> >>> --- gcc-11.1.0-RC-20210423/gcc/config/rs6000/rs6000.cc.vanilla 2021-04-25 22:57:16.964223106 +0200 >>> +++ gcc-11.1.0-RC-20210423/gcc/config/rs6000/rs6000.cc 2021-04-25 22:57:27.193223841 +0200 >>> @@ -5765,7 +5765,7 @@ >>> HOST_WIDE_INT flags = rs6000_isa_flags; >>> >>> /* Disable the flags that should never influence the .machine selection. */ >>> - flags &= ~(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_PPC_GPOPT | OPTION_MASK_ISEL); >>> + flags &= ~(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_PPC_GPOPT | OPTION_MASK_ALTIVEC | OPTION_MASK_ISEL); >> >> Nit: This line is too long and needs re-format. > > While I don't really find ~100 chars too long for modern standards, It's required by https://gcc.gnu.org/codingconventions.html#C_Formatting and there is one script for the check contrib/check_GNU_style.sh, as https://gcc.gnu.org/contribute.html#standards. BR, Kewen > I'm happy to line break that for you once the above test is approved. > > Thank you so much, > > René >
--- gcc-11.1.0-RC-20210423/gcc/config/rs6000/rs6000.cc.vanilla 2021-04-25 22:57:16.964223106 +0200 +++ gcc-11.1.0-RC-20210423/gcc/config/rs6000/rs6000.cc 2021-04-25 22:57:27.193223841 +0200 @@ -5765,7 +5765,7 @@ HOST_WIDE_INT flags = rs6000_isa_flags; /* Disable the flags that should never influence the .machine selection. */ - flags &= ~(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_PPC_GPOPT | OPTION_MASK_ISEL); + flags &= ~(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_PPC_GPOPT | OPTION_MASK_ALTIVEC | OPTION_MASK_ISEL); if ((flags & (ISA_3_1_MASKS_SERVER & ~ISA_3_0_MASKS_SERVER)) != 0) return "power10";