From patchwork Wed Aug 27 16:32:15 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 383490 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 484AE1400A3 for ; Thu, 28 Aug 2014 02:36:00 +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:subject:from:to:cc:date:in-reply-to:references :content-type:mime-version; q=dns; s=default; b=kUKuRosyDczmh1CQ 1F++3hFE/xR0mFZXDKpcuLaVd86y45wevTaz2cC3zLzn8ue9oiidpRrqSjnf2yt7 yg5zkzbCMJKwTs9IfYZbMIJQIrCfp8PyEgZxk7K9RdpGtkvTroEUaLCXgoHz3UKs iRR4iMCaCflynzX7oXxZpljgUiY= 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:subject:from:to:cc:date:in-reply-to:references :content-type:mime-version; s=default; bh=1hC6+MV4qMo5Se1d3OcPow kruKo=; b=P9PhyISDYx/vfTtFOHVeoKJw2LWiYCP9+HqyjPK0kyCjFTjko5mZRG huvk6lp7/v4xPezFYZ7yJMsn5L9cpL/iv9VQeaRrctLRJQ2fLp5Cbmhdgj7RitMF RjHsHKyvcNPCvM/ckMN3yY/dJn6ERBGKLHIkCElsvOdoctAJNQIPY= Received: (qmail 29169 invoked by alias); 27 Aug 2014 16:35:34 -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 29159 invoked by uid 89); 27 Aug 2014 16:35:33 -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, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 27 Aug 2014 16:35:32 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s7RGZVTl009937 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 27 Aug 2014 12:35:31 -0400 Received: from [10.3.226.167] (vpn-226-167.phx2.redhat.com [10.3.226.167]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s7RGZUOL028157; Wed, 27 Aug 2014 12:35:30 -0400 Message-ID: <1409157135.24896.77.camel@surprise> Subject: Re: [PATCH 003/236] config/mn10300: Fix missing PATTERN in PARALLEL handling From: David Malcolm To: Richard Henderson Cc: law@redhat.com, gcc-patches@gcc.gnu.org Date: Wed, 27 Aug 2014 12:32:15 -0400 In-Reply-To: <53FE0321.3080004@redhat.com> References: <1407345815-14551-1-git-send-email-dmalcolm@redhat.com> <1407345815-14551-4-git-send-email-dmalcolm@redhat.com> <53F3912D.6080403@redhat.com> <1409154506.24896.75.camel@surprise> <53FE0321.3080004@redhat.com> Mime-Version: 1.0 X-IsSubscribed: yes On Wed, 2014-08-27 at 09:11 -0700, Richard Henderson wrote: > On 08/27/2014 08:48 AM, David Malcolm wrote: > > Alternatively, should this simply use "single_set"? > > Yes. > > > (though I think that's a more invasive change, especially since some of > > the logic is for non-SETs). > > I don't think that's the case. Take the tests in order: > > if (mn10300_tune_cpu == PROCESSOR_AM34 > && is_load_insn (dep) > && is_store_insn (insn)) > cost += 1; > > Requires sets for both. > > else if (mn10300_tune_cpu == PROCESSOR_AM34 > && ! is_store_insn (insn) > && ! JUMP_P (insn) > && GET_CODE (PATTERN (dep)) == SET > && GET_CODE (PATTERN (insn)) == SET > > Duh. > > if (GET_CODE (PATTERN (dep)) != SET) > return cost; > > Filtering out non-sets from dep. > > /* Now check to see if the previous instruction is a load or store. */ > if (! is_load_insn (insn) && ! is_store_insn (insn)) > return cost; > > Filtering out non-sets from insn. > > Thus in no case do we return anything but the original "cost" when either the > dep or insn pattern is not a set. > > Oh, and while you're massaging this function... > > mn10300_adjust_sched_cost (rtx insn, rtx link, rtx dep, int cost) > { > int timings = get_attr_timings (insn); > ... > /* Extract the latency value from the timings attribute. */ > return timings < 100 ? (timings % 10) : (timings % 100); > } > > > Will you please move the (expensive) get_attr_timings call to the end, after > we've discarded all of the cases in which it isn't used? Fair enough. Update version of patch attached; again, only lightly tested so far. * gcc/config/mn10300/mn10300.c (is_load_insn): Rename to... (set_is_load_p): ...this, updating to work on a SET pattern rather than an insn. (is_store_insn): Rename to... (set_is_store_p): ...this, updating to work on a SET pattern rather than an insn. (mn10300_adjust_sched_cost): Move call to get_attr_timings from top of function to where it is needed. Rewrite the bogus condition that checks for "insn" and "dep" being PARALLEL to instead use single_set, introducing locals "insn_set" and "dep_set". Given that we only ever returned "cost" for a non-pair of SETs, bail out early if we don't have a pair of SET. Rewrite all uses of PATTERN (dep) and PATTERN (insn) to instead use the new locals "insn_set" and "dep_set", and update calls to is_load_insn and is_store_insn to be calls to set_is_load_p and set_is_store_p. Index: gcc/config/mn10300/mn10300.c =================================================================== --- gcc/config/mn10300/mn10300.c (revision 214575) +++ gcc/config/mn10300/mn10300.c (working copy) @@ -2742,21 +2742,15 @@ } static inline bool -is_load_insn (rtx insn) +set_is_load_p (rtx set) { - if (GET_CODE (PATTERN (insn)) != SET) - return false; - - return MEM_P (SET_SRC (PATTERN (insn))); + return MEM_P (SET_SRC (set)); } static inline bool -is_store_insn (rtx insn) +set_is_store_p (rtx set) { - if (GET_CODE (PATTERN (insn)) != SET) - return false; - - return MEM_P (SET_DEST (PATTERN (insn))); + return MEM_P (SET_DEST (set)); } /* Update scheduling costs for situations that cannot be @@ -2768,33 +2762,39 @@ static int mn10300_adjust_sched_cost (rtx insn, rtx link, rtx dep, int cost) { - int timings = get_attr_timings (insn); + rtx insn_set; + rtx dep_set; + int timings; if (!TARGET_AM33) return 1; - if (GET_CODE (insn) == PARALLEL) - insn = XVECEXP (insn, 0, 0); + /* We are only interested in pairs of SET. */ + insn_set = single_set (insn); + if (!insn_set) + return cost; - if (GET_CODE (dep) == PARALLEL) - dep = XVECEXP (dep, 0, 0); + dep_set = single_set (dep); + if (!dep_set) + return cost; + gcc_assert (GET_CODE (insn_set) == SET); + gcc_assert (GET_CODE (dep_set) == SET); + /* For the AM34 a load instruction that follows a store instruction incurs an extra cycle of delay. */ if (mn10300_tune_cpu == PROCESSOR_AM34 - && is_load_insn (dep) - && is_store_insn (insn)) + && set_is_load_p (dep_set) + && set_is_store_p (insn_set)) cost += 1; /* For the AM34 a non-store, non-branch FPU insn that follows another FPU insn incurs a one cycle throughput increase. */ else if (mn10300_tune_cpu == PROCESSOR_AM34 - && ! is_store_insn (insn) + && ! set_is_store_p (insn_set) && ! JUMP_P (insn) - && GET_CODE (PATTERN (dep)) == SET - && GET_CODE (PATTERN (insn)) == SET - && GET_MODE_CLASS (GET_MODE (SET_SRC (PATTERN (dep)))) == MODE_FLOAT - && GET_MODE_CLASS (GET_MODE (SET_SRC (PATTERN (insn)))) == MODE_FLOAT) + && GET_MODE_CLASS (GET_MODE (SET_SRC (dep_set))) == MODE_FLOAT + && GET_MODE_CLASS (GET_MODE (SET_SRC (insn_set))) == MODE_FLOAT) cost += 1; /* Resolve the conflict described in section 1-7-4 of @@ -2816,23 +2816,21 @@ return cost; /* Check that the instruction about to scheduled is an FPU instruction. */ - if (GET_CODE (PATTERN (dep)) != SET) + if (GET_MODE_CLASS (GET_MODE (SET_SRC (dep_set))) != MODE_FLOAT) return cost; - if (GET_MODE_CLASS (GET_MODE (SET_SRC (PATTERN (dep)))) != MODE_FLOAT) - return cost; - /* Now check to see if the previous instruction is a load or store. */ - if (! is_load_insn (insn) && ! is_store_insn (insn)) + if (! set_is_load_p (insn_set) && ! set_is_store_p (insn_set)) return cost; /* XXX: Verify: The text of 1-7-4 implies that the restriction only applies when an INTEGER load/store precedes an FPU instruction, but is this true ? For now we assume that it is. */ - if (GET_MODE_CLASS (GET_MODE (SET_SRC (PATTERN (insn)))) != MODE_INT) + if (GET_MODE_CLASS (GET_MODE (SET_SRC (insn_set))) != MODE_INT) return cost; /* Extract the latency value from the timings attribute. */ + timings = get_attr_timings (insn); return timings < 100 ? (timings % 10) : (timings % 100); }