From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 109911 invoked by alias); 15 Apr 2015 09:25:15 -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 109902 invoked by uid 89); 15 Apr 2015 09:25:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: cam-smtp0.cambridge.arm.com Received: from fw-tnat.cambridge.arm.com (HELO cam-smtp0.cambridge.arm.com) (217.140.96.140) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Wed, 15 Apr 2015 09:25:13 +0000 Received: from arm.com (e106375-lin.cambridge.arm.com [10.2.206.37]) by cam-smtp0.cambridge.arm.com (8.13.8/8.13.8) with ESMTP id t3F9P9qG029948; Wed, 15 Apr 2015 10:25:09 +0100 Date: Wed, 15 Apr 2015 09:25:00 -0000 From: James Greenhalgh To: Kugan Cc: Kyrylo Tkachov , "gcc-patches@gcc.gnu.org" , Marcus Shawcroft , Richard Earnshaw , Jim Wilson Subject: Re: [AArch64][PR65375] Fix RTX cost for vector SET Message-ID: <20150415092509.GA20852@arm.com> References: <55066BCC.4010900@linaro.org> <5506AA24.3050108@arm.com> <5506CD7A.7030109@linaro.org> <5506D77B.5060909@linaro.org> <55070972.3000800@arm.com> <5507813E.3060106@linaro.org> <5513B390.2030201@linaro.org> <552D8FF7.5000105@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <552D8FF7.5000105@linaro.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2015-04/txt/msg00734.txt.bz2 On Tue, Apr 14, 2015 at 11:08:55PM +0100, Kugan wrote: > Now that Stage1 is open, is this OK for trunk. Hi Kugan, > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index cba3c1a..d6ad0af 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -5544,10 +5544,17 @@ aarch64_rtx_costs (rtx x, int code, int outer ATTRIBUTE_UNUSED, > > /* Fall through. */ > case REG: > + if (VECTOR_MODE_P (GET_MODE (op0)) && REG_P (op1)) > + { > + /* The cost is 1 per vector-register copied. */ > + int n_minus_1 = (GET_MODE_SIZE (GET_MODE (op0)) - 1) > + / GET_MODE_SIZE (V4SImode); > + *cost = COSTS_N_INSNS (n_minus_1 + 1); > + } > /* const0_rtx is in general free, but we will use an > instruction to set a register to 0. */ > - if (REG_P (op1) || op1 == const0_rtx) > - { > + else if (REG_P (op1) || op1 == const0_rtx) > + { > /* The cost is 1 per register copied. */ > int n_minus_1 = (GET_MODE_SIZE (GET_MODE (op0)) - 1) > / UNITS_PER_WORD; I would not have expected control flow to reach this point, as we have: > /* TODO: The cost infrastructure currently does not handle > vector operations. Assume that all vector operations > are equally expensive. */ > if (VECTOR_MODE_P (mode)) > { > if (speed) > *cost += extra_cost->vect.alu; > return true; > } But, I see that this check is broken for a set RTX (which has no mode). So, your patch works, but only due to a bug in my original implementation. This leaves the code with quite a messy design. There are two ways I see that we could clean things up, both of which require some reworking of your patch. Either we remove my check above and teach the RTX costs how to properly cost vector operations, or we fix my check to catch all vector RTX and add the special cases for the small subset of things we understand up there. The correct approach in the long term is to fix the RTX costs to correctly understand vector operations, so I'd much prefer to see a patch along these lines, though I appreciate that is a substantially more invasive piece of work. Thanks, James