* [PATCH] fix PowerPC < 7 w/ Altivec not to default to power7
@ 2024-03-08 11:33 Rene Rebe
2024-03-11 3:09 ` Kewen.Lin
0 siblings, 1 reply; 4+ messages in thread
From: Rene Rebe @ 2024-03-08 11:33 UTC (permalink / raw)
To: gcc-patches; +Cc: sjames
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 <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);
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fix PowerPC < 7 w/ Altivec not to default to power7
2024-03-08 11:33 [PATCH] fix PowerPC < 7 w/ Altivec not to default to power7 Rene Rebe
@ 2024-03-11 3:09 ` Kewen.Lin
2024-05-31 14:57 ` Rene Rebe
0 siblings, 1 reply; 4+ messages in thread
From: Kewen.Lin @ 2024-03-11 3:09 UTC (permalink / raw)
To: Rene Rebe
Cc: sjames, gcc-patches, Peter Bergner, Segher Boessenkool, David Edelsohn
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";
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fix PowerPC < 7 w/ Altivec not to default to power7
2024-03-11 3:09 ` Kewen.Lin
@ 2024-05-31 14:57 ` Rene Rebe
2024-06-04 5:57 ` Kewen.Lin
0 siblings, 1 reply; 4+ messages in thread
From: Rene Rebe @ 2024-05-31 14:57 UTC (permalink / raw)
To: linkw; +Cc: sjames, gcc-patches, bergner, segher, dje.gcc
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é
--
René Rebe, ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin
https://exactcode.com | https://t2sde.org | https://rene.rebe.de
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fix PowerPC < 7 w/ Altivec not to default to power7
2024-05-31 14:57 ` Rene Rebe
@ 2024-06-04 5:57 ` Kewen.Lin
0 siblings, 0 replies; 4+ messages in thread
From: Kewen.Lin @ 2024-06-04 5:57 UTC (permalink / raw)
To: Rene Rebe; +Cc: sjames, gcc-patches, bergner, segher, dje.gcc
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é
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-06-04 5:58 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-08 11:33 [PATCH] fix PowerPC < 7 w/ Altivec not to default to power7 Rene Rebe
2024-03-11 3:09 ` Kewen.Lin
2024-05-31 14:57 ` Rene Rebe
2024-06-04 5:57 ` Kewen.Lin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).