From: Tamar Christina <Tamar.Christina@arm.com>
To: Richard Earnshaw <Richard.Earnshaw@foss.arm.com>,
"gcc-patches@gcc.gnu.org" <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.
Date: Thu, 16 Nov 2023 09:33:14 +0000 [thread overview]
Message-ID: <VI1PR08MB532522DC95F1A8B0F51277A3FFB0A@VI1PR08MB5325.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <3fbce99a-3aee-413e-8ed2-fed34af864df@foss.arm.com>
> -----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
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;
> >
> >
> >
> >
next prev parent reply other threads:[~2023-11-16 9:33 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-15 17:06 [PATCH 1/6]AArch64: Refactor costs models to different files Tamar Christina
2023-11-15 17:07 ` [PATCH 2/6]AArch64: Remove special handling of generic cpu Tamar Christina
2023-11-16 9:14 ` Richard Earnshaw
2023-11-15 17:07 ` [PATCH 3/6]AArch64: Add new generic-armv8-a CPU and make it the default Tamar Christina
2023-11-16 9:23 ` Richard Earnshaw
2023-11-15 17:08 ` [PATCH 4/6]AArch64: Add new generic-armv9-a CPU and make it the default for Armv9 Tamar Christina
2023-11-16 9:23 ` Richard Earnshaw
2023-11-15 17:08 ` [PATCH 6/6]AArch64: only emit mismatch error when features would be disabled Tamar Christina
2023-11-16 9:26 ` Richard Earnshaw
2023-11-16 9:33 ` Tamar Christina [this message]
2023-11-16 9:41 ` Richard Earnshaw
2023-11-16 9:50 ` Tamar Christina
2023-11-16 10:33 ` Richard Earnshaw
2023-11-16 9:13 ` [PATCH 1/6]AArch64: Refactor costs models to different files Richard Earnshaw
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=VI1PR08MB532522DC95F1A8B0F51277A3FFB0A@VI1PR08MB5325.eurprd08.prod.outlook.com \
--to=tamar.christina@arm.com \
--cc=Kyrylo.Tkachov@arm.com \
--cc=Marcus.Shawcroft@arm.com \
--cc=Richard.Earnshaw@arm.com \
--cc=Richard.Earnshaw@foss.arm.com \
--cc=Richard.Sandiford@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=nd@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).