From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25181 invoked by alias); 6 Dec 2018 11:36:03 -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 25168 invoked by uid 89); 6 Dec 2018 11:36:02 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS,TIME_LIMIT_EXCEEDED autolearn=unavailable version=3.3.2 spammy=contrary, machine_mode, sk:remove_ X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 06 Dec 2018 11:35:52 +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 2C81880D; Thu, 6 Dec 2018 03:35:51 -0800 (PST) Received: from localhost (unknown [10.32.99.101]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7F1783F5AF; Thu, 6 Dec 2018 03:35:50 -0800 (PST) From: Richard Sandiford To: Steve Ellcey Mail-Followup-To: Steve Ellcey ,gcc-patches , richard.sandiford@arm.com Cc: gcc-patches Subject: Re: [Patch 3/4][Aarch64] v2: Implement Aarch64 SIMD ABI References: <1541699683.12016.8.camel@cavium.com> Date: Thu, 06 Dec 2018 11:36:00 -0000 In-Reply-To: <1541699683.12016.8.camel@cavium.com> (Steve Ellcey's message of "Thu, 8 Nov 2018 17:54:44 +0000") Message-ID: <87efau51m3.fsf@arm.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2018-12/txt/msg00357.txt.bz2 LGTM, just minor stuff. Steve Ellcey writes: > +/* Return true if the instruction is a call to a SIMD function, false > + if it is not a SIMD function or if we do not know anything about > + the function. */ > + > +static bool > +aarch64_simd_call_p (rtx_insn *insn) > +{ > + rtx symbol; > + rtx call; > + tree fndecl; > + > + if (!insn) > + return false; Better to arrange it so that the hook never sees null insns, since there's nothing the hook can do in that case. The global sets should be correct when no other information is available. > +/* Possibly remove some registers from register set if we know they > + are preserved by this call, even though they are marked as not > + being callee saved in CALL_USED_REGISTERS. */ > + > +void > +aarch64_remove_extra_call_preserved_regs (rtx_insn *insn, > + HARD_REG_SET *return_set) s/from register set/from RETURN_SET/. But it would be better to avoid duplicating the description of the hook so much. Maybe: /* Implement TARGET_REMOVE_EXTRA_CALL_PRESERVED_REGS. If INSN calls a function that uses the SIMD ABI, take advantage of the extra call-preserved registers that the ABI provides. */ > +{ > + int regno; > + > + if (aarch64_simd_call_p (insn)) > + { > + for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++) > + if (FP_SIMD_SAVED_REGNUM_P (regno)) > + CLEAR_HARD_REG_BIT (*return_set, regno); > + } Might as well use: for (int regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++) > diff --git a/gcc/target.def b/gcc/target.def > index 4b166d1..25be927 100644 > --- a/gcc/target.def > +++ b/gcc/target.def > @@ -5757,6 +5757,12 @@ for targets that don't have partly call-clobbered registers.", > bool, (unsigned int regno, machine_mode mode), > hook_bool_uint_mode_false) > > +DEFHOOK > +(remove_extra_call_preserved_regs, > + "This hook removes some registers from the callee used register set.", Think a bit more detail would be useful here. E.g.: "This hook removes registers from the set of call-clobbered registers\n\ in @var{used_regs} if, contrary to the default rules, something guarantees\n\ that @samp{insn} preserves those registers. For example, some targets\n\ support variant ABIs in which functions preserve more registers than\n\ normal functions would. Removing those extra registers from @var{used_regs}\n\ can lead to better register allocation.\n\ \n\ The default implementation does nothing, which is always safe.\n\ Defining the hook is purely an optimization." > + void, (rtx_insn *insn, HARD_REG_SET *used_regs), > + default_remove_extra_call_preserved_regs) You need to declare this in targhooks.h. Please sanity-test on something like x86 that doesn't override the hook. > diff --git a/gcc/targhooks.c b/gcc/targhooks.c > index 3d8b3b9..a9fb101 100644 > --- a/gcc/targhooks.c > +++ b/gcc/targhooks.c > @@ -2372,4 +2372,11 @@ default_speculation_safe_value (machine_mode mode ATTRIBUTE_UNUSED, > return result; > } > > +void > +default_remove_extra_call_preserved_regs (rtx_insn *insn ATTRIBUTE_UNUSED, > + HARD_REG_SET *used_regs > + ATTRIBUTE_UNUSED) > +{ > +} Seems easier to leave out the parameter names and drop the ATTRIBUTE_UNUSED. The formatting wouldn't be as awkward that way. Thanks, Richard