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

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

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.]

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..3afd222ad3bdcfb922cc010dcc0b138db29caf7f 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;
> 
> 
> 
>