From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 629CD3858297 for ; Thu, 16 Nov 2023 09:26:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 629CD3858297 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=foss.arm.com Authentication-Results: sourceware.org; spf=none smtp.mailfrom=foss.arm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 629CD3858297 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=217.140.110.172 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700126804; cv=none; b=fif9MaumegRtUeKpaIuMcZrFqsr9FJwYxnmfnypJENcXVEf3xkQu+qhHgA16Hpb3wV50i4kB4bubqOx5wBEXkbt+l35VDH7UwhRYF459I1rxaNJ/lD4WDvtkSHjq6B3+OLC6/4NEARyE261E1hJMk/4/DItg8uQXVU82D/bBZU4= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700126804; c=relaxed/simple; bh=BMnrprwNgWjsI3KPlPLJQzX+fiBFxk3pXgdofWEnCqg=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=I9Vpgo0N7KcuAfIlYr1BgCKpOscYthnPy7QbK2F9507nu/1VmoWtwOTIl5y8I05RzsP8YBhp9ScDH4MLKDeNz8m4DDwdPdlWFI+WTlFlqcVqRwISjgMO25UyfkxZogxnqQRJD4epKJMvhL3C9uYIfGx4N2bmvUFUXNRn3d3OF8Y= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 1DECB1595; Thu, 16 Nov 2023 01:27:29 -0800 (PST) Received: from [10.57.41.187] (unknown [10.57.41.187]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D777C3F6C4; Thu, 16 Nov 2023 01:26:41 -0800 (PST) Message-ID: <3fbce99a-3aee-413e-8ed2-fed34af864df@foss.arm.com> Date: Thu, 16 Nov 2023 09:26:40 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 6/6]AArch64: only emit mismatch error when features would be disabled. Content-Language: en-GB To: Tamar Christina , gcc-patches@gcc.gnu.org Cc: nd@arm.com, Richard.Earnshaw@arm.com, Marcus.Shawcroft@arm.com, Kyrylo.Tkachov@arm.com, richard.sandiford@arm.com References: From: Richard Earnshaw In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3495.3 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 > > > 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; > > > >