From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31468 invoked by alias); 27 Aug 2014 15:51: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 31445 invoked by uid 89); 27 Aug 2014 15:51:45 -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 15:51:44 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s7RFpgqs018203 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 27 Aug 2014 11:51:42 -0400 Received: from [10.3.226.167] (vpn-226-167.phx2.redhat.com [10.3.226.167]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s7RFpfoK015449; Wed, 27 Aug 2014 11:51:41 -0400 Message-ID: <1409154506.24896.75.camel@surprise> Subject: Re: [PATCH 003/236] config/mn10300: Fix missing PATTERN in PARALLEL handling From: David Malcolm To: Richard Henderson , law@redhat.com Cc: gcc-patches@gcc.gnu.org Date: Wed, 27 Aug 2014 15:51:00 -0000 In-Reply-To: <53F3912D.6080403@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> Content-Type: multipart/mixed; boundary="=-1PYEQB8zHuxSq7AcH3u1" Mime-Version: 1.0 X-IsSubscribed: yes X-SW-Source: 2014-08/txt/msg02493.txt.bz2 --=-1PYEQB8zHuxSq7AcH3u1 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Content-length: 2906 On Tue, 2014-08-19 at 11:02 -0700, Richard Henderson wrote: > On 08/06/2014 10:19 AM, David Malcolm wrote: > > @@ -2772,11 +2772,11 @@ mn10300_adjust_sched_cost (rtx insn, rtx link, rtx dep, int cost) > > if (!TARGET_AM33) > > return 1; > > > > - if (GET_CODE (insn) == PARALLEL) > > - insn = XVECEXP (insn, 0, 0); > > + if (GET_CODE (PATTERN (insn)) == PARALLEL) > > + insn = XVECEXP (PATTERN (insn), 0, 0); > > > > - if (GET_CODE (dep) == PARALLEL) > > - dep = XVECEXP (dep, 0, 0); > > + if (GET_CODE (PATTERN (dep)) == PARALLEL) > > + dep = XVECEXP (PATTERN (dep), 0, 0); > > I think these tests are simply wrong and should be removed. > > Certainly one can't expect to extract the first element of an insn's pattern > and then a few lines later test the pattern vs JUMP_P. You're correct: both old and new versions of the code are confusing insns with patterns. I was able to trigger these lines of code by running with optimization enabled with -fschedule-insns -mam33 (mn10300_processor >= PROCESSOR_AM33 is needed to avoid mn10300_option_override from turning off -fschedule-insns). It does encounter insns with PARALLEL patterns e.g.: (gdb) call debug(dep) (insn 90 4 75 2 (parallel [ (set (reg:SI 0 d0 [orig:58 D.1504 ] [58]) (const_int 0 [0])) (clobber (reg:CC 51 EPSW)) ]) /home/david/coding/gcc-python/smoketest/smoketest.c:30 7 {*movsi_clr} (expr_list:REG_UNUSED (reg:CC 51 EPSW) (nil))) and my patch updates dep to the inner SET within the PARALLEL: (gdb) call debug(dep) (set (reg:SI 0 d0 [orig:58 D.1504 ] [58]) (const_int 0 [0])) which is a pattern, not an insn. I think I was confusing PARALLEL with SEQUENCE, where the elements of the latter are insns, but the elements of the former are patterns. I had a go at reworking the logic here to (I hope) properly handle a SET within a PARALLEL (assuming some clobbers); patch attached. Appears to work (in light smoketesting); am working on running the full testsuite with this build. Alternatively, should this simply use "single_set"? (though I think that's a more invasive change, especially since some of the logic is for non-SETs). * gcc/config/mn10300/mn10300.c (is_load_insn): Rename to... (is_load_pat): ...this, updating to work on a pattern rather than an insn. (is_store_insn): Rename to... (is_store_pat): ...this, updating to work on a pattern rather than an insn. (mn10300_adjust_sched_cost): Rewrite the bogus condition that checks for "insn" and "dep" being PARALLEL to work on their patterns instead, introducing locals "insn_pat" and "dep_pat" to track these, updating if needed to the initial pattern within any such PARALLEL. Rewrite all uses of PATTERN (dep) and PATTERN (insn) to instead use these new locals, and update calls to is_load_insn/is_store_insn to be calls to is_load_pat/is_store_pat. --=-1PYEQB8zHuxSq7AcH3u1 Content-Disposition: attachment; filename="mn10300-sched-cost-rewrite.patch" Content-Type: text/x-patch; name="mn10300-sched-cost-rewrite.patch"; charset="UTF-8" Content-Transfer-Encoding: 7bit Content-length: 3489 Index: gcc/config/mn10300/mn10300.c =================================================================== --- gcc/config/mn10300/mn10300.c (revision 214575) +++ gcc/config/mn10300/mn10300.c (working copy) @@ -2742,21 +2742,21 @@ } static inline bool -is_load_insn (rtx insn) +is_load_pat (rtx pat) { - if (GET_CODE (PATTERN (insn)) != SET) + if (GET_CODE (pat) != SET) return false; - return MEM_P (SET_SRC (PATTERN (insn))); + return MEM_P (SET_SRC (pat)); } static inline bool -is_store_insn (rtx insn) +is_store_pat (rtx pat) { - if (GET_CODE (PATTERN (insn)) != SET) + if (GET_CODE (pat) != SET) return false; - return MEM_P (SET_DEST (PATTERN (insn))); + return MEM_P (SET_DEST (pat)); } /* Update scheduling costs for situations that cannot be @@ -2768,33 +2768,38 @@ static int mn10300_adjust_sched_cost (rtx insn, rtx link, rtx dep, int cost) { + rtx insn_pat; + rtx dep_pat; + int timings = get_attr_timings (insn); if (!TARGET_AM33) return 1; - if (GET_CODE (insn) == PARALLEL) - insn = XVECEXP (insn, 0, 0); + insn_pat = PATTERN (insn); + if (GET_CODE (insn_pat) == PARALLEL) + insn_pat = XVECEXP (insn_pat, 0, 0); - if (GET_CODE (dep) == PARALLEL) - dep = XVECEXP (dep, 0, 0); + dep_pat = PATTERN (dep); + if (GET_CODE (dep_pat) == PARALLEL) + dep_pat = XVECEXP (dep_pat, 0, 0); /* 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)) + && is_load_pat (dep_pat) + && is_store_pat (insn_pat)) 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) + && ! is_store_pat (insn_pat) && ! 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_CODE (dep_pat) == SET + && GET_CODE (insn_pat) == SET + && GET_MODE_CLASS (GET_MODE (SET_SRC (dep_pat))) == MODE_FLOAT + && GET_MODE_CLASS (GET_MODE (SET_SRC (insn_pat))) == MODE_FLOAT) cost += 1; /* Resolve the conflict described in section 1-7-4 of @@ -2816,20 +2821,20 @@ return cost; /* Check that the instruction about to scheduled is an FPU instruction. */ - if (GET_CODE (PATTERN (dep)) != SET) + if (GET_CODE (dep_pat) != SET) return cost; - if (GET_MODE_CLASS (GET_MODE (SET_SRC (PATTERN (dep)))) != MODE_FLOAT) + if (GET_MODE_CLASS (GET_MODE (SET_SRC (dep_pat))) != 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 (! is_load_pat (insn_pat) && ! is_store_pat (insn_pat)) 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_pat))) != MODE_INT) return cost; /* Extract the latency value from the timings attribute. */ --=-1PYEQB8zHuxSq7AcH3u1--