Tamar Christina <tamar.christ...@arm.com> writes:
>> -----Original Message-----
>> From: Richard Sandiford <richard.sandif...@arm.com>
>> Sent: Monday, January 13, 2025 6:35 PM
>> To: Tamar Christina <tamar.christ...@arm.com>
>> Cc: gcc-patches@gcc.gnu.org; nd <n...@arm.com>; Richard Earnshaw
>> <richard.earns...@arm.com>; ktkac...@gcc.gnu.org
>> Subject: Re: [PATCH]AArch64: have -mcpu=native detect architecture extensions
>> for unknown non-homogenous systems [PR113257]
>>
>> Tamar Christina <tamar.christ...@arm.com> writes:
>> > Hi All,
>> >
>> > in g:e91a17fe39c39e98cebe6e1cbc8064ee6846a3a7 we added the ability for
>> > -mcpu=native on unknown CPUs to still enable architecture extensions.
>> >
>> > This has worked great but was only added for homogenous systems.
>> >
>> > However the same thing works for big.LITTLE as in such system the cores
>> > must
>> > have the same extensions otherwise it doesn't fundamentally work.
>> >
>> > i.e. task migration from one core to the other wouldn't work.
>> >
>> > This extends the same handling to non-homogenous systems.
>> >
>> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>> >
>> > Ok for master?
>> >
>> > Thanks,
>> > Tamar
>> >
>> > gcc/ChangeLog:
>> >
>> > PR target/113257
>> > * config/aarch64/driver-aarch64.cc (host_detect_local_cpu):
>> >
>> > gcc/testsuite/ChangeLog:
>> >
>> > PR target/113257
>> > * gcc.target/aarch64/cpunative/info_34: New test.
>> > * gcc.target/aarch64/cpunative/native_cpu_34.c: New test.
>> >
>> > ---
>> >
>> > diff --git a/gcc/config/aarch64/driver-aarch64.cc
>> > b/gcc/config/aarch64/driver-
>> aarch64.cc
>> > index
>> 45fce67a646351b848b7cd7d0fd35d343731c0d1..2a454daf031aa3ac81a9a2c0
>> 3b15c09731e4f56e 100644
>> > --- a/gcc/config/aarch64/driver-aarch64.cc
>> > +++ b/gcc/config/aarch64/driver-aarch64.cc
>> > @@ -449,6 +449,20 @@ host_detect_local_cpu (int argc, const char **argv)
>> > break;
>> > }
>> > }
>> > +
>> > + /* On big.LITTLE if we find any unknown CPUs we can still pick arch
>> > + features as the cores should have the same features. So just pick
>> > + the feature flags from any of the cpus. */
>> > + if (aarch64_cpu_data[i].name == NULL)
>> > + {
>> > + auto arch_info = get_arch_from_id (DEFAULT_ARCH);
>> > +
>> > + gcc_assert (arch_info);
>> > +
>> > + res = concat ("-march=", arch_info->name, NULL);
>> > + default_flags = arch_info->flags;
>> > + }
>> > +
>>
>> Currently, if gcc recognises the host cpu, and if one-thing is more
>> restrictive than that cpu, gcc will warn on:
>>
>> gcc -march=one-thing -mcpu=native
>>
>> and choose one-thing. It looks like one consequence of this patch
>> is that, for unrecognised big.LITTLE, the command line would get
>> converted to:
>>
>> gcc -march=one-thing -march=above-replacement
>>
>> and so -mcpu=native would silently "win" over one-thing. Is that right?
>>
>> Perhaps we should adjust:
>>
>> " %{mcpu=native:%<mcpu=native %:local_cpu_detect(cpu)}" \
>>
>> to pass something like "cpu/arch" rather than "cpu" when -march
>> is not specified, so that the routine knows that it has the choice
>> of using either -mcpu or -march. We wouldn't get the warning, but we
>> would get predictable preemption of -march over -mcpu.
>>
>> Admittedly, it looks like we already have this problem with:
>>
>> if (aarch64_cpu_data[i].name == NULL)
>> {
>> auto arch_info = get_arch_from_id (DEFAULT_ARCH);
>>
>> gcc_assert (arch_info);
>>
>> res = concat ("-march=", arch_info->name, NULL);
>> default_flags = arch_info->flags;
>> }
>>
>> so I guess this is pre-existing.
>
> Yes, it looks like this was a deliberate choice that mcpu=native
> overrides any march, this is the case for all previous GCCs as well.
>
> i.e. GCC 12-15 (ones I had at hand) convert
>
> -march=armv8.8-a+sve -mcpu=native on an Neoverse-V1 system to
>
> '-march=armv8.8-a+sve' '-c' '-mlittle-endian' '-mabi=lp64'
> '-mcpu=neoverse-v1+sm4+crc+aes+sha3+nossbs'
>
> In both the calls to CC1 and to AS
>
> "-march=armv8.8-a+sve"
> "-march=armv8.4-a+rng+sm4+crc+aes+sha3+i8mm+bf16+sve+profile"
>
> Your request would change this long standing behavior but I don't know If
> that's correct or not.
>
> I'll admit it's inconsistent with the warning given by CC1 for the flags.
>
> The warnings are coming from CC1, and the question is if it's up to GCC to
> enforce the CC1 constraint onto binutils or not.
I was talking about what DRIVER_SELF_SPECS does, and therefore what cc1
sees. My point was that, like you say, if you use:
-march=armv8.8-a+sve -mcpu=native
on a host that GCC recognises, cc1 will see:
-march=armv8.8-a+sve -mcpu=...
cc1 will then warn and compile for the -march.
But with this patch, if you run gcc on a big.LITTLE host that GCC
doesn't recognise, cc1 will see:
-march=armv8.8-a+sve -march=...
The second -march will then silently override the first -march,
so cc1 won't warn, and cc1 will compile for the second -march.
Like I say, that also seems to be the existing behaviour for
homogeneous systems that GCC doesn't recognise.
That is, there are two stages:
(1) DRIVER_SELF_SPECS converts -mcpu=native to -mcpu=something-specific,
via local_cpu_detect. This happens on the driver command line before
the driver runs any subcommands. And this is the part that is being
changed by the patch.
(2) ASM_SPEC converts -mcpu to -march, regardless of where the -mcpu
came from. This part isn't changed by the patch (but is by the
other patch that you sent).
I'm not questioning (2), which like you say is a deliberate choice.
But (1) means that the driver replaces -mcpu=native with -mcpu=...
if the driver recognises the CPU, but replaces -mcpu=native with
-march=... if it doesn't recognise the CPU. This means that cc1
sees -mcpu=native as an -march=... rather than an -mcpu=... on
CPUs that GCC doesn't recognise.
So if GCC recognises the CPU, cc1 will consistently honour an
explicit -march over whatever replaces -mcpu=native. But if GCC doesn't
recognise the CPU, cc1 won't consistently honour an explicit -march over
whatever replaces -mcpu=native.
So the idea was that "cpu/arch" (or whatever) would tell
local_cpu_detect that it can replace -mcpu with -march (because
there's no existing -march that it would conflict with).
On the other hand, "cpu" would tell local_cpu_detect that there is
already an -march option, and so local_cpu_detect should either
replace -mcpu=native with another -mcpu or drop the option.
(Sorry for over-laboured explanation -- just trying to make it
clearer than my first try.)
Thanks,
Richard
> So up to you, but I'm a bit hesitant to change this at this point without
> being able to tell what the impact is on downstream tools.
>
> Thanks,
> Tamar
>
>>
>> Thanks,
>> Richard
>>
>> > if (!res)
>> > goto not_found;
>> > }
>> > diff --git a/gcc/testsuite/gcc.target/aarch64/cpunative/info_34
>> b/gcc/testsuite/gcc.target/aarch64/cpunative/info_34
>> > new file mode 100644
>> > index
>> 0000000000000000000000000000000000000000..61cb254785a4b9ec19ebe3
>> 88402223c9a82af7ed
>> > --- /dev/null
>> > +++ b/gcc/testsuite/gcc.target/aarch64/cpunative/info_34
>> > @@ -0,0 +1,18 @@
>> > +processor : 0
>> > +BogoMIPS : 100.00
>> > +Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 asimddp sve sve2
>> fphp asimdhp fcma
>> > +CPU implementer : 0xfe
>> > +CPU architecture: 8
>> > +CPU variant : 0x0
>> > +CPU part : 0xd08
>> > +CPU revision : 2
>> > +
>> > +processor : 0
>> > +BogoMIPS : 100.00
>> > +Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 asimddp sve sve2
>> fphp asimdhp fcma
>> > +CPU implementer : 0xfe
>> > +CPU architecture: 8
>> > +CPU variant : 0x0
>> > +CPU part : 0xd09
>> > +CPU revision : 2
>> > +
>> > diff --git a/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_34.c
>> b/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_34.c
>> > new file mode 100644
>> > index
>> 0000000000000000000000000000000000000000..168140002a0f0205c0f552
>> de0cce9b2d356e09e2
>> > --- /dev/null
>> > +++ b/gcc/testsuite/gcc.target/aarch64/cpunative/native_cpu_34.c
>> > @@ -0,0 +1,12 @@
>> > +/* { dg-do compile { target { { aarch64*-*-linux*} && native } } } */
>> > +/* { dg-set-compiler-env-var GCC_CPUINFO
>> "$srcdir/gcc.target/aarch64/cpunative/info_34" } */
>> > +/* { dg-additional-options "-mcpu=native" } */
>> > +
>> > +int main()
>> > +{
>> > + return 0;
>> > +}
>> > +
>> > +/* { dg-final { scan-assembler {\.arch
>> > armv8-a\+dotprod\+crc\+crypto\+sve2\n}
>> } } */
>> > +
>> > +/* Test a normal looking procinfo. */
