From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 50204 invoked by alias); 21 Feb 2019 22:35:02 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 50177 invoked by uid 89); 21 Feb 2019 22:35:02 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_PASS autolearn=ham version=3.3.2 spammy=that'll, thatll, aes, retroactive X-HELO: foss.arm.com Received: from usa-sjc-mx-foss1.foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 21 Feb 2019 22:35:00 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 17745A78; Thu, 21 Feb 2019 14:34:59 -0800 (PST) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 039973F703; Thu, 21 Feb 2019 14:34:57 -0800 (PST) Date: Thu, 21 Feb 2019 22:42:00 -0000 From: James Greenhalgh To: Tamar Christina Cc: "gcc-patches@gcc.gnu.org" , nd , Richard Earnshaw , Marcus Shawcroft Subject: Re: [PATCH][GCC][AArch64] Fix command line options canonicalization version #2. (PR target/88530) Message-ID: <20190221223451.GA23595@arm.com> References: <20190220140034.GA32675@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190220140034.GA32675@arm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2019-02/txt/msg01776.txt.bz2 On Wed, Feb 20, 2019 at 08:00:38AM -0600, Tamar Christina wrote: > Hi All, > > Commandline options on AArch64 don't get canonicalized into the smallest > possible set before output to the assembler. This means that overlapping feature > sets are emitted with superfluous parts. > > Normally this isn't an issue, but in the case of crypto we have retro-actively > split it into aes and sha2. We need to emit only +crypto to the assembler > so old assemblers continue to work. > > Because of how -mcpu=native and -march=native work they end up enabling all > feature bits. Instead we need to get the smallest possible set, which would also > fix the problem with older the assemblers and the retro-active split. > > The function that handles this is called quite often. It is called for any > push/pop options or attribute that changes arch, cpu etc. In order to not make > this search for the smallest set too expensive we sort the options based on the > number of features (bits) they enable. This allows us to process the list > linearly instead of quadratically (Once we have enabled a feature, we know that > anything else that enables it can be ignored. By sorting we'll get the biggest > groups first and thus the smallest combination of commandline flags). > > The Option handling structures have been extended to have a boolean to indicate > whether the option is synthetic, with that I mean if the option flag itself > enables a new feature. > > e.g. +crypto isn't an actual feature, it just enables other features, but others > like +rdma enable multiple dependent features but is itself also a feature. > > There are two ways to solve this. > > 1) Either have the options that are feature bits also turn themselves on, e.g. > change rdma to turn on FP, SIMD and RDMA as dependency bits. > > 2) Make a distinction between these two different type of features and have the > framework handle it correctly. > > Even though it's more code I went for the second approach, as it's the one > that'll be less fragile (people can't forget it) and gives the least surprises. > > Effectively this patch changes the following: > > The values before the => are the old compiler and after the => the new code. > > -march=armv8.2-a+crypto+sha2 => -march=armv8.2-a+crypto > -march=armv8.2-a+sha2+aes => -march=armv8.2-a+crypto > > The remaining behaviors stay the same. > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Ok for trunk? OK, but I don't understand why the CRC special case is needed. My copy of the Arm Architecture Reference Manual suggests that all versions of the architceture from ARmv8.1-A are required to implement the CRC32 extension. Is there some old assembler that doesn't honour that? Whatever is driving that requirement could usefully be added to the comments. I find it very hard to believe that this code is what we need for correct behaviour on AArch64; this level of complexity implies we're doing something very wrong with either the definition or the implementation of these features bits which makes it hard for us to maintain, and hard for users and dependent tools (e.g. assemblers) to know what to expect from the compiler. I've seen a number of bugs in this code recently. While I appreciate your patch for fixing one of them, I find the cases and expectations so hard to reason about that I can't say I am sure we are now bug free. OK for trunk as an improvement over today, and to help us get towards a release; but I'm very unhappy with this corner of the compiler! Thanks, James > gcc/ChangeLog: > > 2019-02-20 Tamar Christina > > PR target/88530 > * common/config/aarch64/aarch64-common.c > (struct aarch64_option_extension): Add is_synthetic. > (all_extensions): Use it. > (TARGET_OPTION_INIT_STRUCT): Define hook. > (struct gcc_targetm_common): Moved to end. > (all_extensions_by_on): New. > (opt_ext_cmp, typedef opt_ext): New. > (aarch64_option_init_struct): New. > (aarch64_contains_opt): New. > (aarch64_get_extension_string_for_isa_flags): Output smallest set. > * config/aarch64/aarch64-option-extensions.def > (AARCH64_OPT_EXTENSION): Explicitly include AES and SHA2 in crypto. > (fp, simd, crc, lse, fp16, rcpc, rdma, dotprod, aes, sha2, sha3, > sm4, fp16fml, sve, profile, rng, memtag, sb, ssbs, predres): > Set is_synthetic to false. > (crypto): Set is_synthetic to true. > * config/aarch64/driver-aarch64.c (AARCH64_OPT_EXTENSION): Add > SYNTHETIC. > > gcc/testsuite/ChangeLog: > > 2019-02-20 Tamar Christina > > PR target/88530 > * gcc.target/aarch64/options_set_1.c: New test. > * gcc.target/aarch64/options_set_2.c: New test. > * gcc.target/aarch64/options_set_3.c: New test. > * gcc.target/aarch64/options_set_4.c: New test. > * gcc.target/aarch64/options_set_5.c: New test. > * gcc.target/aarch64/options_set_6.c: New test. > * gcc.target/aarch64/options_set_7.c: New test. > * gcc.target/aarch64/options_set_8.c: New test. > * gcc.target/aarch64/options_set_9.c: New test. > > --