Re: [PATCH] fix PowerPC < 7 w/ Altivec not to default to power7
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
>>>
>>> [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é
>
Re: [PATCH] fix PowerPC < 7 w/ Altivec not to default to power7
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.vanilla2024-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
> >
> > [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é
--
René Rebe, ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin
https://exactcode.com | https://t2sde.org | https://rene.rebe.de
Re: [PATCH] fix PowerPC < 7 w/ Altivec not to default to power7
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 > > [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.cc2021-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"; >
Re: [PATCH] fix PowerPC < 7 w/ Altivec not to default to power7
On 3/8/24 5:30 AM, Jonathan Wakely via Gcc wrote: > Patches should be sent to the gcc-patches list instead of this one, > and should be against trunk not an old gcc-11 RC. See > https://gcc.gnu.org/contribute.html#patches for more details - thanks! And you need to CC the rs6000/powerpc port maintainers which you can find along with their preferred email addresses in the MAINTAINERS file. If you don't CC them, they may miss seeing the patch. Peter
[PATCH] fix PowerPC < 7 w/ Altivec not to default to power7
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. 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 We ship this in T2/Linux [2] since 2020 and it is tested on G5, Cell and Power8. Signed-of-by: René Rebe [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); if ((flags & (ISA_3_1_MASKS_SERVER & ~ISA_3_0_MASKS_SERVER)) != 0) return "power10"; -- René Rebe, ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin https://exactcode.com | https://t2sde.org | https://rene.rebe.de
Re: [PATCH] fix PowerPC < 7 w/ Altivec not to default to power7
On Fri, 8 Mar 2024 at 11:27, 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. > > 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 > > We ship this in T2/Linux [2] since 2020 and it is tested on G5, Cell > and Power8. > > Signed-of-by: René Rebe > > [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); > >if ((flags & (ISA_3_1_MASKS_SERVER & ~ISA_3_0_MASKS_SERVER)) != 0) > return "power10"; Patches should be sent to the gcc-patches list instead of this one, and should be against trunk not an old gcc-11 RC. See https://gcc.gnu.org/contribute.html#patches for more details - thanks!
[PATCH] fix PowerPC < 7 w/ Altivec not to default to power7
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. 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 We ship this in T2/Linux [2] since 2020 and it is tested on G5, Cell and Power8. Signed-of-by: René Rebe [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); if ((flags & (ISA_3_1_MASKS_SERVER & ~ISA_3_0_MASKS_SERVER)) != 0) return "power10"; -- René Rebe, ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin https://exactcode.com | https://t2sde.org | https://rene.rebe.de
