> -----Original Message-----
> From: Richard Earnshaw <richard.earns...@foss.arm.com>
> Sent: Thursday, November 16, 2023 9:42 AM
> To: Tamar Christina <tamar.christ...@arm.com>; gcc-patches@gcc.gnu.org
> Cc: nd <n...@arm.com>; Richard Earnshaw <richard.earns...@arm.com>;
> Marcus Shawcroft <marcus.shawcr...@arm.com>; Kyrylo Tkachov
> <kyrylo.tkac...@arm.com>; Richard Sandiford
> <richard.sandif...@arm.com>
> Subject: Re: [PATCH 6/6]AArch64: only emit mismatch error when features
> would be disabled.
>
>
>
> On 16/11/2023 09:33, Tamar Christina wrote:
> >> -----Original Message-----
> >> From: Richard Earnshaw <richard.earns...@foss.arm.com>
> >> Sent: Thursday, November 16, 2023 9:27 AM
> >> To: Tamar Christina <tamar.christ...@arm.com>;
> >> gcc-patches@gcc.gnu.org
> >> Cc: nd <n...@arm.com>; Richard Earnshaw <richard.earns...@arm.com>;
> >> Marcus Shawcroft <marcus.shawcr...@arm.com>; Kyrylo Tkachov
> >> <kyrylo.tkac...@arm.com>; Richard Sandiford
> >> <richard.sandif...@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?
>
AArch64 in general does not validate extensions to architectures. So basically
we would allow it.
e.g.
> aarch64-none-elf-gcc -O3 ./gcc/testsuite/gcc.dg/tree-ssa/slsr-20.c -S -o -
> -march=armv8.2-a+sve -mcpu=cortex-r82
cc1: warning: switch '-mcpu=cortex-r82' conflicts with '-march=armv8.2-a+sve'
switch and would result in options
+sve+norcpc+nodotprod+nofp16fml+noflagm+nopauth being added
.arch armv8.2-a+crc+sve
The new warning only tells you exactly what the compiler will be doing to your
options, but doesn't change the behavior
the compiler exhibits today since we always take -march over -mcpu.
The difference is today we just say "there's a conflict" and don't specify what
the conflict it.
Regards,
Tamar
> 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;
> >>>
> >>>
> >>>
> >>>
