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 <[email protected]>
>>>
>>> [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é
>
