> -----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 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; > > > > > > > >
