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