Richard Sandiford <richard.sandif...@arm.com> writes:
> 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.
Or local_cpu_detect might be able to use -mcpu=generic-armv8-a+... instead
of -march=armv8-a+... If so, that would be even better :)
> (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. */
