From patchwork Thu Nov 13 14:09:39 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrylo Tkachov X-Patchwork-Id: 410428 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 A47EB1400D2 for ; Fri, 14 Nov 2014 01:09:55 +1100 (AEDT) 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:subject:references :in-reply-to:content-type; q=dns; s=default; b=ww3Y6i3DouvEw0pzm MkcbBJiqqGefd+DOFm6fKXsPCiX8lGtk779wjf4b/sbVrtwc1VLpRFJ7JGZ1r4qm X44mMORKyZg+9x8Y4/U9RFZbVImGV0oGaHvIMzX+Xo5Bon+OmzuPezkEItuEpt7P dtgb4VsCMQdBljd6bK5N3M4vbo= 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:subject:references :in-reply-to:content-type; s=default; bh=42/OXo2kKeevNHHxzCiK+2Q oYnA=; b=DCbzzO8fqBmVWXFSxRExf01YN44IsdX1oBfuoQ/VSoVJfE8CRWmbRbJ pEBitk3WmaOd9H/07NJFcTxMZbnStKt0dUtaGQt7/okjXQNUdynqqymNSPQdLuxa dhzb02QxSDWNPP1IusdTU6PngpPTb0mldlCl5xTGu43eaY1Iraks= Received: (qmail 12857 invoked by alias); 13 Nov 2014 14:09:48 -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 12846 invoked by uid 89); 13 Nov 2014 14:09:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL, BAYES_00, 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, 13 Nov 2014 14:09:45 +0000 Received: from cam-owa1.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.21]) by service87.mimecast.com; Thu, 13 Nov 2014 14:09:41 +0000 Received: from [10.1.207.43] ([10.1.255.212]) by cam-owa1.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Thu, 13 Nov 2014 14:09:40 +0000 Message-ID: <5464BBA3.1050503@arm.com> Date: Thu, 13 Nov 2014 14:09:39 +0000 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Jeff Law , GCC Patches Subject: Re: [PATCH][sched-deps] Remove needless check for modified_in_p when trying to fuse two non-conditional jump insns References: <54638967.7070409@arm.com> <5463EACF.9080504@redhat.com> In-Reply-To: <5463EACF.9080504@redhat.com> X-MC-Unique: 114111314094109501 X-IsSubscribed: yes On 12/11/14 23:18, Jeff Law wrote: > On 11/12/14 09:23, Kyrill Tkachov wrote: >> Hi all, >> >> I noticed that the check for modified_in_p in sched-deps is not needed >> and needlessly restricts the insn pairs that we can check for macro >> fusion in the backend hooks. >> This patch removes the check. Currently that execution path is only used >> in arm and aarch64. >> This enables the backend hooks to catch more cases that I'm working on. >> >> Bootstrapped on aarch64, x86, arm. Tested on aarch64. >> >> Ok for trunk? >> >> Thanks, >> Kyrill >> >> 2014-11-12 Kyrylo Tkachov >> >> * sched-deps.c (sched_macro_fuse_insns): Do not check modified_in_p >> in the not condtional jump case. > Typo in ChangeLog "condtional" > > Can you please include a testcase which shows fusion occurring after > this patch where it does not occur before this patch. > > I think you probably need to include some kind of doc change around this > change since you're effectively pushing responsibility for checking this > into the ports. > > With the testcase and doc change, this will probably be OK. Please take > care of those, repost for a review of the test & doc changes. > > jeff Hi Jeff, thanks for looking at this. I've updated the documentation for the hook. The testcase I was looking at involves fusing the AArch64 adrp+add instructions and depends on the backend implementation of the matching code, under review currently at https://gcc.gnu.org/ml/gcc-patches/2014-11/msg01263.html. I'm attaching a reduced testcase that generates an adrp and an add and due to the restriction addressed in this patch it doesn't call the backend hook for a pair of adrp and add instructions, causing them to be scheduled apart. I don't think it's a good candidate for the testsuite though because it's not easy to scan for consequent assembly instructions from Dejagnu and the instruction pair may end up scheduled together for some tuning parameters even though the macro fusion hook does not trigger for them as it should. What do you think of this patch? Kyrill 2014-11-13 Kyrylo Tkachov * sched-deps.c (sched_macro_fuse_insns): Do not check modified_in_p in the not conditional jump case. * doc/tm.texi (TARGET_SCHED_MACRO_FUSION_PAIR_P): Update description. * target.def (TARGET_SCHED_MACRO_FUSION_PAIR_P): Update description. commit a1bf782123e461726fd016d440a8d81679ee9dc0 Author: Kyrylo Tkachov Date: Thu Nov 6 12:05:03 2014 +0000 [sched-deps] remove overly strict check on macro fusion diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi index 33a5a97..bc7f657 100644 --- a/gcc/doc/tm.texi +++ b/gcc/doc/tm.texi @@ -6482,11 +6482,13 @@ cycle. These other insns can then be taken into account properly. This hook is used to check whether target platform supports macro fusion. @end deftypefn -@deftypefn {Target Hook} bool TARGET_SCHED_MACRO_FUSION_PAIR_P (rtx_insn *@var{condgen}, rtx_insn *@var{condjmp}) -This hook is used to check whether two insns could be macro fused for -target microarchitecture. If this hook returns true for the given insn pair -(@var{condgen} and @var{condjmp}), scheduler will put them into a sched -group, and they will not be scheduled apart. +@deftypefn {Target Hook} bool TARGET_SCHED_MACRO_FUSION_PAIR_P (rtx_insn *@var{prev}, rtx_insn *@var{curr}) +This hook is used to check whether two insns should be macro fused for +a target microarchitecture. If this hook returns true for the given insn pair +(@var{prev} and @var{curr}), the scheduler will put them into a sched +group, and they will not be scheduled apart. The two insns will be either +two SET insns or a compare and a conditional jump and this hook should +validate any dependencies needed to fuse the two insns together. @end deftypefn @deftypefn {Target Hook} void TARGET_SCHED_DEPENDENCIES_EVALUATION_HOOK (rtx_insn *@var{head}, rtx_insn *@var{tail}) diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c index a4ea836..ee534b0 100644 --- a/gcc/sched-deps.c +++ b/gcc/sched-deps.c @@ -2877,8 +2877,7 @@ sched_macro_fuse_insns (rtx_insn *insn) prev = prev_nonnote_nondebug_insn (insn); if (!prev || !insn_set - || !single_set (prev) - || !modified_in_p (SET_DEST (insn_set), prev)) + || !single_set (prev)) return; } diff --git a/gcc/target.def b/gcc/target.def index b2fe47b..4ada2ae 100644 --- a/gcc/target.def +++ b/gcc/target.def @@ -1067,11 +1067,13 @@ DEFHOOK DEFHOOK (macro_fusion_pair_p, - "This hook is used to check whether two insns could be macro fused for\n\ -target microarchitecture. If this hook returns true for the given insn pair\n\ -(@var{condgen} and @var{condjmp}), scheduler will put them into a sched\n\ -group, and they will not be scheduled apart.", - bool, (rtx_insn *condgen, rtx_insn *condjmp), NULL) + "This hook is used to check whether two insns should be macro fused for\n\ +a target microarchitecture. If this hook returns true for the given insn pair\n\ +(@var{prev} and @var{curr}), the scheduler will put them into a sched\n\ +group, and they will not be scheduled apart. The two insns will be either\n\ +two SET insns or a compare and a conditional jump and this hook should\n\ +validate any dependencies needed to fuse the two insns together.", + bool, (rtx_insn *prev, rtx_insn *curr), NULL) /* The following member value is a pointer to a function called after evaluation forward dependencies of insns in chain given