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 07B073858294 for ; Wed, 20 Jul 2022 08:24:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 07B073858294 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 0F6581576; Wed, 20 Jul 2022 01:24:27 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.37]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 123AC3F70D; Wed, 20 Jul 2022 01:24:25 -0700 (PDT) From: Richard Sandiford To: Andrew Carlotti Mail-Followup-To: Andrew Carlotti , gcc-patches@gcc.gnu.org, richard.sandiford@arm.com Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH v2.1 3/4] aarch64: Consolidate simd type lookup functions References: Date: Wed, 20 Jul 2022 09:24:24 +0100 In-Reply-To: (Andrew Carlotti's message of "Tue, 19 Jul 2022 19:22:23 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-53.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Jul 2022 08:24:28 -0000 Andrew Carlotti writes: > On Wed, Jul 13, 2022 at 05:36:04PM +0100, Richard Sandiford wrote: >> I like the part about getting rid of: >> >> static tree >> aarch64_simd_builtin_type (machine_mode mode, >> bool unsigned_p, bool poly_p) >> >> and the flow of the new function. However, I think it's still >> slightly more readable if we keep the switch and lookup routines >> separate, partly to keep down the size of the main routine and >> partly to avoid the goto. > > I agree. > >> So how about: >> >> - aarch64_simd_builtin_std_type becomes aarch64_int_or_fp_element_type >> but otherwise stays as-is >> >> ... > > I've called it aarch64_int_or_fp_type, because it's sometimes used for > an operand that doesn't represent an element of a vector. > > > Updated patch below. > > --- > > There were several similarly-named functions, which each built or looked up an > operand type using a different subset of valid modes or qualifiers. > > This change provides a single function to return operand types, which can > additionally handle const and pointer qualifiers. For clarity, the existing > functionality is kept in separate helper functions. > > gcc/ChangeLog: > > * config/aarch64/aarch64-builtins.cc > (aarch64_simd_builtin_std_type): Rename to... > (aarch64_int_or_fp_type): ...this, and allow irrelevant qualifiers. > (aarch64_lookup_simd_builtin_type): Rename to... > (aarch64_simd_builtin_type): ...this. Add const/pointer > support, and extract table lookup to... > (aarch64_lookup_simd_type_in_table): ...this function. > (aarch64_init_crc32_builtins): Update to use aarch64_simd_builtin_type. > (aarch64_init_fcmla_laneq_builtins): Ditto. > (aarch64_init_simd_builtin_functions): Ditto. LGTM, thanks. OK for trunk with a couple of minor formatting fixes: > diff --git a/gcc/config/aarch64/aarch64-builtins.cc b/gcc/config/aarch64/aarch64-builtins.cc > index 55ad2e8b6831d6cc2b039270c8656d429347092d..cd7c2a79d9b4d67adf1d9de1f9b56eb3a0d1ee2b 100644 > --- a/gcc/config/aarch64/aarch64-builtins.cc > +++ b/gcc/config/aarch64/aarch64-builtins.cc > @@ -788,12 +788,13 @@ aarch64_general_mangle_builtin_type (const_tree type) > return NULL; > } > > +/* Helper function for aarch64_simd_builtin_type. */ > static tree > -aarch64_simd_builtin_std_type (machine_mode mode, > - enum aarch64_type_qualifiers q) > +aarch64_int_or_fp_type (machine_mode mode, > + enum aarch64_type_qualifiers qualifiers) This line should be reindented so that the arguments continue to line up. > { > -#define QUAL_TYPE(M) \ > - ((q == qualifier_none) ? int##M##_type_node : unsigned_int##M##_type_node); > +#define QUAL_TYPE(M) ((qualifiers & qualifier_unsigned) \ > + ? unsigned_int##M##_type_node : int##M##_type_node); > switch (mode) > { > case E_QImode: > [...] > @@ -1383,13 +1381,13 @@ aarch64_init_simd_builtins (void) > static void > aarch64_init_crc32_builtins () > { > - tree usi_type = aarch64_simd_builtin_std_type (SImode, qualifier_unsigned); > + tree usi_type = aarch64_simd_builtin_type (SImode, qualifier_unsigned); > unsigned int i = 0; > > for (i = 0; i < ARRAY_SIZE (aarch64_crc_builtin_data); ++i) > { > aarch64_crc_builtin_datum* d = &aarch64_crc_builtin_data[i]; > - tree argtype = aarch64_simd_builtin_std_type (d->mode, > + tree argtype = aarch64_simd_builtin_type (d->mode, > qualifier_unsigned); Same here. Richard > tree ftype = build_function_type_list (usi_type, usi_type, argtype, NULL_TREE); > tree attrs = aarch64_get_attributes (FLAG_NONE, d->mode);