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 B24C43858295 for ; Thu, 16 Nov 2023 09:41:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B24C43858295 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 B24C43858295 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=1700127699; cv=none; b=RERZj31saq/eNrH/gtKkoWU4F43UQU9VUzZAPjmtrP+jVeSf5gEr3BF+lCPTU9XbakJUOc7ReibhpE1UBso1fonXiXhIx7eDxoOyT83TrGOEDpZaXaStCTph5dIp9NtAKA8aQSHXRq/hU7tbhQMisrlXE+BibDKJeOB2VYUiyO8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700127699; c=relaxed/simple; bh=oCDpn2QlEJOSTlY1NPln05lhmtHuxyfOHr3sCzRsLaU=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=V9ynnPhCi88BgiuntfKZMqQNh4E03/QlkanQUo6r8BN1mhad80TaOICExGtFx7eFKfgJQemLeJ7fPOY8JGXyd2f3RX8CNSZzMkRbrHQntvr3RB9kRlgw2H40+GtKT68fvM5gWiwVRlccdsdrCsM6FE7C6JUttyrFDLvz3nq7cFE= 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 56CFF1595; Thu, 16 Nov 2023 01:42:23 -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 26F043F6C4; Thu, 16 Nov 2023 01:41:36 -0800 (PST) Message-ID: Date: Thu, 16 Nov 2023 09:41:34 +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 , Richard Earnshaw , Marcus Shawcroft , Kyrylo Tkachov , Richard Sandiford References: <3fbce99a-3aee-413e-8ed2-fed34af864df@foss.arm.com> 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,KAM_SHORT,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 16/11/2023 09:33, Tamar Christina wrote: >> -----Original Message----- >> From: Richard Earnshaw >> Sent: Thursday, November 16, 2023 9:27 AM >> To: Tamar Christina ; gcc-patches@gcc.gnu.org >> Cc: nd ; Richard Earnshaw ; >> Marcus Shawcroft ; Kyrylo Tkachov >> ; Richard Sandiford >> >> 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 >>> >>> >>> 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; >>> >>> >>> >>>