Re: [PATCH 6/6]AArch64: only emit mismatch error when features would be disabled.

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

On 16/11/2023 09:33, Tamar Christina wrote:
>> -----Original Message-----
>> From: Richard Earnshaw <Richard.Earnshaw@foss.arm.com>
>> Sent: Thursday, November 16, 2023 9:27 AM
>> To: Tamar Christina <Tamar.Christina@arm.com>; gcc-patches@gcc.gnu.org
>> Cc: nd <nd@arm.com>; Richard Earnshaw <Richard.Earnshaw@arm.com>;
>> Marcus Shawcroft <Marcus.Shawcroft@arm.com>; Kyrylo Tkachov
>> <Kyrylo.Tkachov@arm.com>; Richard Sandiford
>> <Richard.Sandiford@arm.com>
>> Subject: Re: [PATCH 6/6]AArch64: only emit mismatch error when features
>> would be disabled.
>>
>>
>>
>> On 15/11/2023 17:08, Tamar Christina wrote:
>>> Hi All,
>>>
>>> At the moment we emit a warning whenever you specify both -march and
>>> -mcpu and the architecture of them differ.  The idea originally was
>>> that the user may not be aware of this change.
>>>
>>> However this has a few problems:
>>>
>>> 1.  Architecture revisions is not an observable part of the architecture,
>>>       extensions are.  Starting with GCC 14 we have therefore relaxed the rule
>> that
>>>       all extensions can be enabled at any architecture level.  Therefore it's
>>>       incorrect, or at least not useful to keep the check on architecture.
>>>
>>> 2.  It's problematic in Makefiles and other build systems, where you want to
>>>       for certain files enable CPU specific builds.  i.e. you may be by default
>>>       building for -march=armv8-a but for some file for -mcpu=neoverse-n1.
>> Since
>>>       there's no easy way to remove the earlier options we end up warning and
>>>       there's no way to disable just this warning.  Build systems compiling with
>>>       -Werror face an issue in this case that compiling with GCC is needlessly
>>>       hard.
>>>
>>> 3. It doesn't actually warn for cases that may lead to issues, so e.g.
>>>      -march=armv8.2-a+sve -mcpu=neoverse-n1 does not give a warning that
>> SVE would
>>>      be disabled.
>>>
>>> For this reason I have one of two proposals:
>>>
>>> 1.  Just remove this warning all together.
>>>
>>> 2.  Rework the warning based on extensions and only warn when features
>> would be
>>>       disabled by the presence of the -mcpu.  This is the approach this patch has
>>>       taken.
>>
>> There's a third option here, which is what I plan to add for the Arm port:
>>
>> 3. Add -mcpu=unset and -march=unset support in the driver, which has the
>> effect of suppressing any earlier option that sets that flag.
>>
>> [BTW: patch 5 seems to be missing so I'm holding off on approving this now.]
>>
> 
> Ah sorry, I should have re-numbered this series. Patch 5 was sent earlier to unblock
> an internal team. It was https://gcc.gnu.org/pipermail/gcc-patches/2023-October/632802.html

Ah, OK.

So going back to your option 2.  What should happen if the user 
specified -mcpu=cortex-r82 and then specifies an extension that doesn't 
exist in the R profile?

R.

> 
> Thanks,
> Tamar
>> R.
>>
>>>
>>> As examples:
>>>
>>>> aarch64-none-linux-gnu-gcc -march=armv8.2-a+sve -mcpu=neoverse-n1
>>> cc1: warning: switch ‘-mcpu=neoverse-n1’ conflicts with ‘-march=armv8.2-
>> a+sve’ switch and resulted in options +crc+sve+norcpc+nodotprod being
>> added
>> .arch armv8.2-a+crc+sve
>>>
>>>> aarch64-none-linux-gnu-gcc -march=armv8.2-a -mcpu=neoverse-n1
>>>> aarch64-none-linux-gnu-gcc -march=armv8.2-a+dotprod -mcpu=neoverse-
>> n1
>>>> aarch64-none-linux-gnu-gcc -march=armv8.2-a+dotprod -mcpu=neoverse-
>> n2
>>> <no warning>
>>>
>>> The one remaining issue here is that if both -march and -mcpu are
>>> specified we pick the -march.  This is not particularly obvious and
>>> for the use case to be more useful I think it makes sense to pick the CPU's
>> arch?
>>>
>>> I did not make that change in the patch as it changes semantics.
>>>
>>> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>>>
>>> Note that I can't write a test for this because dg-warning expects
>>> warnings to be at a particular line and doesn't support warnings at the
>> "global" level.
>>>
>>> Ok for master?
>>>
>>> Thanks,
>>> Tamar
>>>
>>> gcc/ChangeLog:
>>>
>>> 	* config/aarch64/aarch64.cc (aarch64_override_options): Rework
>> warnings.
>>>
>>> --- inline copy of patch --
>>> diff --git a/gcc/config/aarch64/aarch64.cc
>>> b/gcc/config/aarch64/aarch64.cc index
>>>
>> caf80d66b3a744cc93899645aa5f9374983cd3db..3afd222ad3bdcfb922cc01
>> 0dcc0b
>>> 138db29caf7f 100644
>>> --- a/gcc/config/aarch64/aarch64.cc
>>> +++ b/gcc/config/aarch64/aarch64.cc
>>> @@ -16388,12 +16388,22 @@ aarch64_override_options (void)
>>>      if (cpu && arch)
>>>        {
>>>          /* If both -mcpu and -march are specified, warn if they are not
>>> -	 architecturally compatible and prefer the -march ISA flags.  */
>>> -      if (arch->arch != cpu->arch)
>>> -	{
>>> -	  warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%>
>> switch",
>>> +	 feature compatible.  feature compatible means that the inclusion of
>> the
>>> +	 cpu features would end up disabling an achitecture feature.  In
>>> +	 otherwords the cpu features need to be a strict superset of the arch
>>> +	 features and if so prefer the -march ISA flags.  */
>>> +      auto full_arch_flags = arch->flags | arch_isa;
>>> +      auto full_cpu_flags = cpu->flags | cpu_isa;
>>> +      if (~full_cpu_flags & full_arch_flags)
>>> +	{
>>> +	  std::string ext_diff
>>> +	    = aarch64_get_extension_string_for_isa_flags (full_arch_flags,
>>> +							  full_cpu_flags);
>>> +	  warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%>
>> switch "
>>> +		      "and resulted in options %s being added",
>>>    		       aarch64_cpu_string,
>>> -		       aarch64_arch_string);
>>> +		       aarch64_arch_string,
>>> +		       ext_diff.c_str ());
>>>    	}
>>>
>>>          selected_arch = arch->arch;
>>>
>>>
>>>
>>>