From patchwork Thu Jul 10 08:00:04 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrylo Tkachov X-Patchwork-Id: 368497 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 05F43140141 for ; Thu, 10 Jul 2014 18:00:26 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=mu+AKg+LBjBc59fXG 7O1lqH8D72aWeRwJ/dnwlyIaMqgqxWxd0QUenFCPz+V8tU8kPp+tRETSnFWiwIQ0 gxNv9Ee5c6xAjfjHGWPmQPxNFWIAxFLBFqmh/Vju4/NcfHHtAKrYBrOIHy/imO+9 NHYqD+7TctS3YFUhlCiaEa4GrI= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=utgBiPYwe57Urkxh9PtYLH4 BTFo=; b=N0XrkXK/ZyGgBoKLsjlji9JRXCQY1NuXIpMf8vc4vOOnGyr0sz1XWTt PMka4vzFsEy7EwcBGN2sjS10sNQbNoe9KtRvb5C7Pr8LalEwPqAn1Qben7diqXEl 29mRECHQZLGobHmThGLJf4jBII3GNpt8ihyeQzHj/h9jhur1xF58= Received: (qmail 30305 invoked by alias); 10 Jul 2014 08:00:12 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 30269 invoked by uid 89); 10 Jul 2014 08:00:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: service87.mimecast.com Received: from service87.mimecast.com (HELO service87.mimecast.com) (91.220.42.44) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 10 Jul 2014 08:00:08 +0000 Received: from cam-owa2.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.21]) by service87.mimecast.com; Thu, 10 Jul 2014 09:00:06 +0100 Received: from [10.1.208.24] ([10.1.255.212]) by cam-owa2.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Thu, 10 Jul 2014 09:00:04 +0100 Message-ID: <53BE4804.4010709@arm.com> Date: Thu, 10 Jul 2014 09:00:04 +0100 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: Jeff Law , GCC Patches CC: Ramana Radhakrishnan , Maxim Kuvyrkov Subject: Re: [PATCH][sched-deps] Generalise usage of macro fusion to work on any two insns References: <53AD2B65.6030406@arm.com> <53B1CB16.10206@redhat.com> In-Reply-To: <53B1CB16.10206@redhat.com> X-MC-Unique: 114071009000604301 X-IsSubscribed: yes On 30/06/14 21:39, Jeff Law wrote: > On 06/27/14 02:29, Kyrill Tkachov wrote: >> Hi all, >> >> This patch generalises the TARGET_MACRO_FUSION_PAIR_P hook usage to work >> on more than just >> compares and conditional branches for which it was initially designed >> for (for x86). >> >> There are some instructions in arm and aarch64 that can be fused >> together when they're back to back in the instruction stream and I'd >> like to use this hook to keep them together. >> >> I'll post an implementation of TARGET_MACRO_FUSION_PAIR_P for arm and >> aarch64 shortly... >> >> Bootstrapped and tested on x86, aarch64-none-linux-gnu and >> arm-none-linux-gnueabihf. >> >> Ok for trunk? >> >> 2014-06-27 Ramana Radhakrishnan >> Kyrylo Tkachov >> >> * sched-deps.c (try_group_insn): Generalise macro fusion hook usage >> to any two insns. Update comment. > Isn't this going to end up calling the x86 specific macro_fusion_pair_p > with a lot more insns than that function was previously prepared to handle? > > In particular I'm concerned that the 2nd argument is going to be a > non-jumping insn a lot more often. Of particular concern is this code: > > > test_if = SET_SRC (pc_set (condjmp)); > cond = XEXP (test_if, 0); > ccode = GET_CODE (cond); > > if CONDJMP is not a JUMP_INSN, pc_set is going to return NULL and XEXP > (test_if, 0) will then fault. > > I realize you bootstrapped on x86, but I suspect that whatever tuning > you need to enable to really exercise this code wasn't on. > > I think you can deal with this by putting > > if (!any_condjump_p (condjmp)) at the start of the x86 specific > macro_fusion_pair_p is sufficient to address this issue. It also > ensures that we don't do a lot of unnecessary work in that function. > > From a general code structure standpoint, can you avoid this kind of > structure: > > if (any_condjmp_p (insn)) > { > ... > goto succ; > } > else > { > ... > goto succ > } > return > > succ: > > Can you structure so that you return for all the cases where you don't > want to set SCHED_GROUP_P from each arm? Or go ahead and duplicate the > SCHED_GROUP_P setting in each arm of the conditional. Hi Jeff, Thanks for the pointers, I've reworked the patch and it does look cleaner. I've made sure to run the x86 bootstrap with Haswell tuning and instrumented the code to make sure that the x86 macro fusion code was being exercised and it passed that fine. How's that? 2014-07-10 Ramana Radhakrishnan Kyrylo Tkachov * sched-deps.c (try_group_insn): Generalise macro fusion hook usage to any two insns. Update comment. * config/i386/i386.c (ix86_macro_fusion_pair_p): Reject 2nd arguments that are not conditional jumps. > > jeff > > commit e36b8977738dbe3f63445199710ca627ab37e243 Author: Kyrylo Tkachov Date: Fri Jun 13 11:41:41 2014 +0100 [sched-deps] Generalise macro fusion hook usage diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 8046c67..7dd2ce5 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -25817,6 +25817,9 @@ ix86_macro_fusion_pair_p (rtx condgen, rtx condjmp) rtx compare_set = NULL_RTX, test_if, cond; rtx alu_set = NULL_RTX, addr = NULL_RTX; + if (!any_condjump_p (condjmp)) + return false; + if (get_attr_type (condgen) != TYPE_TEST && get_attr_type (condgen) != TYPE_ICMP && get_attr_type (condgen) != TYPE_INCDEC diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c index 7cafc8b..c01a8a6 100644 --- a/gcc/sched-deps.c +++ b/gcc/sched-deps.c @@ -2820,35 +2820,48 @@ sched_analyze_2 (struct deps_desc *deps, rtx x, rtx insn) sched_deps_info->finish_rhs (); } -/* Try to group comparison and the following conditional jump INSN if - they're already adjacent. This is to prevent scheduler from scheduling - them apart. */ +/* Try to group two fuseable insns together to prevent scheduler + from scheduling them apart. */ static void try_group_insn (rtx insn) { - unsigned int condreg1, condreg2; - rtx cc_reg_1; rtx prev; - if (!any_condjump_p (insn)) + if (!targetm.sched.macro_fusion_p ()) return; - targetm.fixed_condition_code_regs (&condreg1, &condreg2); - cc_reg_1 = gen_rtx_REG (CCmode, condreg1); - prev = prev_nonnote_nondebug_insn (insn); - if (!reg_referenced_p (cc_reg_1, PATTERN (insn)) - || !prev - || !modified_in_p (cc_reg_1, prev)) - return; + if (any_condjump_p (insn)) + { + unsigned int condreg1, condreg2; + rtx cc_reg_1; + targetm.fixed_condition_code_regs (&condreg1, &condreg2); + cc_reg_1 = gen_rtx_REG (CCmode, condreg1); + prev = prev_nonnote_nondebug_insn (insn); + if (!reg_referenced_p (cc_reg_1, PATTERN (insn)) + || !prev + || !modified_in_p (cc_reg_1, prev)) + return; - /* Different microarchitectures support macro fusions for different - combinations of insn pairs. */ - if (!targetm.sched.macro_fusion_pair_p - || !targetm.sched.macro_fusion_pair_p (prev, insn)) - return; + if (targetm.sched.macro_fusion_pair_p (prev, insn)) + SCHED_GROUP_P (insn) = 1; + } + else + { + rtx insn_set = single_set (insn); + + prev = prev_nonnote_nondebug_insn (insn); + if (prev + && insn_set + && single_set (prev) + && modified_in_p (SET_DEST (insn_set), prev) + && targetm.sched.macro_fusion_pair_p (prev, insn)) + { + SCHED_GROUP_P (insn) = 1; + } + + } - SCHED_GROUP_P (insn) = 1; } /* Analyze an INSN with pattern X to find all dependencies. */