From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 128329 invoked by alias); 10 Jan 2019 09:16:46 -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 128314 invoked by uid 89); 10 Jan 2019 09:16:46 -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 autolearn=ham version=3.3.2 spammy=HTo:D*marvell.com 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, 10 Jan 2019 09:16:44 +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 9648580D; Thu, 10 Jan 2019 01:16:42 -0800 (PST) Received: from localhost (unknown [10.32.98.35]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C825E3F694; Thu, 10 Jan 2019 01:16:41 -0800 (PST) From: Richard Sandiford To: Steve Ellcey Mail-Followup-To: Steve Ellcey ,"sellcey\@cavium.com" , "gcc-patches\@gcc.gnu.org" , richard.sandiford@arm.com Cc: "sellcey\@cavium.com" , "gcc-patches\@gcc.gnu.org" Subject: Re: [Patch 4/4][Aarch64] v2: Implement Aarch64 SIMD ABI References: <1541699749.12016.9.camel@cavium.com> <87tvjq3kqs.fsf@arm.com> <875zv0l64x.fsf@arm.com> <7bdc455d8d0b38778e151ad8ce8cfe6b69c580c5.camel@marvell.com> <87d0p6gng4.fsf@arm.com> <175d9f25425f2aa634e2bd88f7ee6eefabae97ed.camel@marvell.com> Date: Thu, 10 Jan 2019 09:16:00 -0000 In-Reply-To: <175d9f25425f2aa634e2bd88f7ee6eefabae97ed.camel@marvell.com> (Steve Ellcey's message of "Thu, 10 Jan 2019 00:19:11 +0000") Message-ID: <87bm4o98jr.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: 2019-01/txt/msg00518.txt.bz2 Steve Ellcey writes: > On Wed, 2019-01-09 at 10:00 +0000, Richard Sandiford wrote: > > Thanks for the quick turnaround on the comments Richard. Here is a new > version where I tried to address all the issues you raised. One thing > I noticed is that I think your calls_have_same_clobbers_p function only > works if, when return_call_with_max_clobbers is called with two calls > that clobber the same set of registers, it always returns the first > call. > > I don't think my original function had that guarantee but I changed it > so that it would and documented that requirement in target.def. I > couldn't see a better way to implement the calls_have_same_clobbers_p > function other than doing that. Yeah, I think that's a good guarantee to have. > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index 1c300af..d88be6c 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -1655,14 +1655,56 @@ aarch64_reg_save_mode (tree fndecl, unsigned regno) > /* Implement TARGET_HARD_REGNO_CALL_PART_CLOBBERED. The callee only saves > the lower 64 bits of a 128-bit register. Tell the compiler the callee > clobbers the top 64 bits when restoring the bottom 64 bits. */ > > static bool > -aarch64_hard_regno_call_part_clobbered (unsigned int regno, machine_mode mode) > +aarch64_hard_regno_call_part_clobbered (rtx_insn *insn, unsigned int regno, > + machine_mode mode) > { > - return FP_REGNUM_P (regno) && maybe_gt (GET_MODE_SIZE (mode), 8); > + bool simd_p = insn && CALL_P (insn) && aarch64_simd_call_p (insn); > + return FP_REGNUM_P (regno) > + && maybe_gt (GET_MODE_SIZE (mode), simd_p ? 16 : 8); > +} > + > +/* Implement TARGET_RETURN_CALL_WITH_MAX_CLOBBERS. */ > + > +rtx_insn * > +aarch64_return_call_with_max_clobbers (rtx_insn *call_1, rtx_insn *call_2) > +{ > + gcc_assert (CALL_P (call_1) && CALL_P (call_2)); > + > + if (aarch64_simd_call_p (call_1) == aarch64_simd_call_p (call_2)) > + return call_1; > + > + if (aarch64_simd_call_p (call_2)) > + return call_1; > + else > + return call_2; Think this is simpler as: gcc_assert (CALL_P (call_1) && CALL_P (call_2)); if (!aarch64_simd_call_p (call_1) || aarch64_simd_call_p (call_2)) return call_1; else return call_2; > diff --git a/gcc/lra-lives.c b/gcc/lra-lives.c > index a00ec38..61149e1 100644 > --- a/gcc/lra-lives.c > +++ b/gcc/lra-lives.c > @@ -579,22 +579,32 @@ lra_setup_reload_pseudo_preferenced_hard_reg (int regno, > PSEUDOS_LIVE_THROUGH_CALLS and PSEUDOS_LIVE_THROUGH_SETJUMPS. */ > static inline void > check_pseudos_live_through_calls (int regno, > - HARD_REG_SET last_call_used_reg_set) > + HARD_REG_SET last_call_used_reg_set, > + rtx_insn *call_insn) Should document the new parameter. > @@ -906,17 +933,22 @@ process_bb_lives (basic_block bb, int &curr_point, bool dead_insn_p) > > bool flush = (! hard_reg_set_empty_p (last_call_used_reg_set) > && ! hard_reg_set_equal_p (last_call_used_reg_set, > - this_call_used_reg_set)); > + this_call_used_reg_set) > + && ! calls_have_same_clobbers_p (call_insn, > + last_call_insn)); This should be || with the current test, not &&. We need to check that last_call_insn is nonnull first. > EXECUTE_IF_SET_IN_SPARSESET (pseudos_live, j) > { > IOR_HARD_REG_SET (lra_reg_info[j].actual_call_used_reg_set, > this_call_used_reg_set); > + > if (flush) > - check_pseudos_live_through_calls > - (j, last_call_used_reg_set); > + check_pseudos_live_through_calls (j, > + last_call_used_reg_set, > + curr_insn); > } Should be last_call_insn rather than curr_insn. I.e. when we flush, we apply the properties of the previous call to pseudos live after the new call. Looks good otherwise. Thanks, Richard