From patchwork Wed Nov 4 17:52:09 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Charles Baylis X-Patchwork-Id: 56001 Delivered-To: patch@linaro.org Received: by 10.112.61.134 with SMTP id p6csp2551634lbr; Wed, 4 Nov 2015 09:52:29 -0800 (PST) X-Received: by 10.107.166.196 with SMTP id p187mr5250887ioe.140.1446659549250; Wed, 04 Nov 2015 09:52:29 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id l67si3154905iod.90.2015.11.04.09.52.28 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 04 Nov 2015 09:52:29 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-412671-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; spf=pass (google.com: domain of gcc-patches-return-412671-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-412671-patch=linaro.org@gcc.gnu.org; dkim=pass header.i=@gcc.gnu.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=FjyvqFWid3uPqikhoj STiRX5ca6Quzuu9A7NlYulqvOpFZVhaEheWkzTjX+/2ZrEnf9h93M9paw2DCuYP+ IfGuSYsAyyja3Kb7Nh2kFQl3l0Svyumzjg6JH9btNqiRMTaLKcOR714VC3G0cg5S 45miryPvdtY3tzUK5ihPniZIk= 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 :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=C4j1iDguCki8gs2mf0AhFmDp gBU=; b=JONvt8zyldDzJF+GOdv1WvaYoNyGtXse/QrZEICVMhtLchd3RA6bwLWM iTK9SN6mwh0V1YKLGnpyIxQ9lCFX/aviWPMZyisxiJPb87ij9fhp0c8sbAnrmXMs 1TyqwUc/BkeCk1eNMIsmzkxK2TC3fnQeqyhtmvczZiyGislfaXc= Received: (qmail 120081 invoked by alias); 4 Nov 2015 17:52:15 -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 120071 invoked by uid 89); 4 Nov 2015 17:52:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.3 required=5.0 tests=AWL, BAYES_99, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=no version=3.3.2 X-HELO: mail-oi0-f52.google.com Received: from mail-oi0-f52.google.com (HELO mail-oi0-f52.google.com) (209.85.218.52) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 04 Nov 2015 17:52:11 +0000 Received: by oifu63 with SMTP id u63so32711159oif.2 for ; Wed, 04 Nov 2015 09:52:09 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=5bmW9ZQ/gKRnQ/OHFDpomp92ubm2M89PkFrWg1/JQF4=; b=kWxV4dC8kLP7rfLEP/1o0UjvrC63mGF8K9kaPvFUlPCnap9vGukAKBnVMgi4CKikNY XwOqoU1rMTlnyJqhppyLx7ru/9ozvPnH+tcgaHVk0VDX6adwXCZT0MF+/+Hx+EdYspZA fQA5yLAcU3HfvsXO6OGrVpVOvZtuG/LLA0m6h6vlni0e0WkWhyXfwuQnYW8/A/ghJVcT Nuw1SPAHAglYpJNNc4ETdR5tWM6K1Ymw5tF26dmne0+vU4gkepZdDw70w/OZ86H7hiv2 n2WubixldNacoYorpYPBerZ4cjfJysssUqaYl2tO8TC1yOizFJXo3Xnr3UwetIgnaCwS vV3Q== X-Gm-Message-State: ALoCoQktw10KJhU5G5ltC1OPGN96WjMf6xQXyGevOYP0qfSSu8BFVQV/6oVoj+XhCfKgjBhb18Qz MIME-Version: 1.0 X-Received: by 10.202.68.8 with SMTP id r8mr1632073oia.116.1446659529459; Wed, 04 Nov 2015 09:52:09 -0800 (PST) Received: by 10.202.215.215 with HTTP; Wed, 4 Nov 2015 09:52:09 -0800 (PST) In-Reply-To: <5639BC2C.9020308@foss.arm.com> References: <5639BC2C.9020308@foss.arm.com> Date: Wed, 4 Nov 2015 17:52:09 +0000 Message-ID: Subject: Re: [PATCH] [ARM] PR61551 RFC: Improve costs for NEON addressing modes From: Charles Baylis To: Ramana Radhakrishnan Cc: GCC Patches , Kyrylo Tkachov , Richard Earnshaw X-IsSubscribed: yes On 4 November 2015 at 08:05, Ramana Radhakrishnan wrote: > Hi Charles, > > Sorry I missed this completely in my inbox. > > On 31/10/15 03:34, Charles Baylis wrote: >> Hi Ramana, >> >> [revisiting https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01593.html] >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61551 >> >> This patch is an initial attempt to rework the ARM rtx costs to better >> handle the costs of various addressing modes, in particular to remove >> the incorrect large costs associated with post-indexed addressing in >> NEON memory operations. >> >> This patch introduces per-core tables for the costs of using different >> addressing modes for different access modes. I have retained the >> original code so that the calculated costs can be compared. Currently, >> the tables replicate the costs calculated by the original code, and a >> debug assert is left in place. >> >> Obviously, a fair amount of clean up is needed before this can be >> applied, but I would like a quick comment on the general approach to >> check that I haven't completely missed the point before continuing. > > No you haven't missed the point - this is the direction I wanted this taken in though not expecting this degree of detail. OK, Thanks :) >> +struct cbmem_cost_table >> +{ >> + enum access_type >> + { >> + REG, >> + POST_INCDEC, >> + PRE_INCDEC, >> + /*PRE_MODIFY,*/ >> + POST_MODIFY, >> + PLUS, >> + ACCESS_TYPE_LAST = PLUS >> + }; >> + const int si[ACCESS_TYPE_LAST + 1]; >> + const int di[ACCESS_TYPE_LAST + 1]; >> + const int cdi[ACCESS_TYPE_LAST + 1]; >> + const int sf[ACCESS_TYPE_LAST + 1]; >> + const int df[ACCESS_TYPE_LAST + 1]; >> + const int cdf[ACCESS_TYPE_LAST + 1]; >> + const int blk[ACCESS_TYPE_LAST + 1]; >> + const int vec64[ACCESS_TYPE_LAST + 1]; >> + const int vec128[ACCESS_TYPE_LAST + 1]; >> + const int vec192[ACCESS_TYPE_LAST + 1]; >> + const int vec256[ACCESS_TYPE_LAST + 1]; >> + const int vec384[ACCESS_TYPE_LAST + 1]; >> + const int vec512[ACCESS_TYPE_LAST + 1]; >> +}; >> + >> >> After that, I will clean up the coding style, check for impact on the >> AArch64 backend, remove the debug code and in a separate patch improve >> the tuning for the vector modes. > > I think adding additional costs for zero / sign extension of registers would be appropriate for the AArch64 backend. Further more I think Alan recently had patches to change the use of vector modes to BLKmode in the AArch64 backend, so some of the vector costing might become interesting. The aarch64 already has a mechanism for doing costs for those operations in aarch64_address_cost(). Using BLKmode will certainly make this difficult. > If you can start turning this around quickly I'd like to keep the review momentum going but it will need time and effort from a number of parties to get this working. This is however likely to be a high impact change on the backends as this is an invasive change and I'm not sure if it will meet the Stage3 cutoff point. I'll see what I can do. In the short term, the only part of the cost model I want changed is the excessive costs for the pre/post-indexed addressing on vector modes. >> From b10c6dd7af1f5b9821946783ba9d96b08c751f2b Mon Sep 17 00:00:00 2001 >> From: Charles Baylis >> Date: Wed, 28 Oct 2015 18:48:16 +0000 >> Subject: [PATCH] WIP >> >> Change-Id: If349ffd7dbbe13a814be4a0d022382ddc8270973 >> --- >> gcc/config/arm/aarch-common-protos.h | 28 ++ >> gcc/config/arm/aarch-cost-tables.h | 328 +++++++++++++++++ >> gcc/config/arm/arm.c | 677 ++++++++++++++++++++++++++++++++++- >> 3 files changed, 1023 insertions(+), 10 deletions(-) >> >> diff --git a/gcc/config/arm/aarch-common-protos.h b/gcc/config/arm/aarch-common-protos.h >> index 348ae74..dae42d7 100644 >> --- a/gcc/config/arm/aarch-common-protos.h >> +++ b/gcc/config/arm/aarch-common-protos.h >> @@ -130,6 +130,33 @@ struct vector_cost_table >> const int alu; >> }; >> >> +struct cbmem_cost_table >> +{ >> + enum access_type >> + { >> + REG, >> + POST_INCDEC, >> + PRE_INCDEC, >> + /*PRE_MODIFY,*/ >> + POST_MODIFY, >> + PLUS, >> + ACCESS_TYPE_LAST = PLUS >> + }; >> + const int si[ACCESS_TYPE_LAST + 1]; >> + const int di[ACCESS_TYPE_LAST + 1]; >> + const int cdi[ACCESS_TYPE_LAST + 1]; >> + const int sf[ACCESS_TYPE_LAST + 1]; >> + const int df[ACCESS_TYPE_LAST + 1]; >> + const int cdf[ACCESS_TYPE_LAST + 1]; >> + const int blk[ACCESS_TYPE_LAST + 1]; >> + const int vec64[ACCESS_TYPE_LAST + 1]; >> + const int vec128[ACCESS_TYPE_LAST + 1]; >> + const int vec192[ACCESS_TYPE_LAST + 1]; >> + const int vec256[ACCESS_TYPE_LAST + 1]; >> + const int vec384[ACCESS_TYPE_LAST + 1]; >> + const int vec512[ACCESS_TYPE_LAST + 1]; >> +}; > > > > > I was considering a single table for scalar integer , scalar fp and vector modes mapping scalar fp and vector modes down to scalar integer modes in case of soft float mode or in the absence of a vector unit (i.e. TARGET_NEON was false.) I also wasn't sure what the impact would be by adding address_cost in with the computation of rtx_cost for MEM expressions and whether the 2 needed to be added or not. This needs plenty of analysis and tweaking over a range of benchmarks and mcpu options. I hadn't considered softfloat. AFAIK we don't see NEON types if TARGET_NEON was false (arm_neon.h errors out in that case). >> struct cpu_cost_table >> { >> const struct alu_cost_table alu; >> @@ -137,6 +164,7 @@ struct cpu_cost_table >> const struct mem_cost_table ldst; >> const struct fp_cost_table fp[2]; /* SFmode and DFmode. */ >> const struct vector_cost_table vect; >> + const struct cbmem_cost_table addr; >> }; >> > > Can we make this a pointer instead and have simple tables that sort of abstract the same meaning - I would like to see if we can share the data here between multiple cores rather than creating 20 copies for the same thing. Initially atleast it would make life much easier if we only played around with 1 cost model on one core and had everything else map to the same thing. That would certainly be easier. >> >> diff --git a/gcc/config/arm/aarch-cost-tables.h b/gcc/config/arm/aarch-cost-tables.h >> index 66e09a8..c5ecdcf 100644 >> --- a/gcc/config/arm/aarch-cost-tables.h >> +++ b/gcc/config/arm/aarch-cost-tables.h >> @@ -122,6 +122,88 @@ const struct cpu_cost_table generic_extra_costs = > I'm curious as to the numbers here - The costs should reflect the relative costs of the addressing modes not the costs of the loads and stores - thus having high numbers here for vector modes may just prevent this from even triggering in auto-inc-dec code ? In my experience with GCC I've never satisfactorily answered the question whether these should be comparable to rtx_costs or not. In an ideal world they should be but I'm never sure. IOW I'm not sure if using COSTS_N_INSNS or plain numbers here is appropriate. The aim here was to replicate the costs calculated by the existing code. AFAICS the costs for the vector modes have evolved by accident. They do prevent the auto-inc-dec phase from triggering on the vector modes, which is what motivated this work in the first place. My plan was to demonstrate that the new tables captured the existing heuristics accurately, so that actual changes to the cost model could be reviewed independently from the change to the code structure. Once the table/code structure is agreed, the next patch would remove the old code and the assert, then further patches can be applied to incrementally improve the tuning numbers. This can be done in steps, it should be easy to guess some better numbers for the vector modes, while changing the integer costs is likely to require more careful benchmarking. Realistically, I think the correct costs are {0, 0, 0, 0, 0} so the units probably don't matter... >> /* RTX costs. Make an estimate of the cost of executing the operation >> X, which is contained with an operation with code OUTER_CODE. >> SPEED_P indicates whether the cost desired is the performance cost, >> @@ -9524,16 +10114,83 @@ arm_new_rtx_costs (rtx x, enum rtx_code code, enum rtx_code outer_code, >> case MEM: >> /* A memory access costs 1 insn if the mode is small, or the address is >> a single register, otherwise it costs one insn per word. */ >> - if (REG_P (XEXP (x, 0))) >> - *cost = COSTS_N_INSNS (1); >> - else if (flag_pic >> - && GET_CODE (XEXP (x, 0)) == PLUS >> - && will_be_in_index_register (XEXP (XEXP (x, 0), 1))) >> - /* This will be split into two instructions. >> - See arm.md:calculate_pic_address. */ >> - *cost = COSTS_N_INSNS (2); >> - else >> - *cost = COSTS_N_INSNS (ARM_NUM_REGS (mode)); >> + { >> + int cost_old; >> + int cost_new; >> + cbmem_cost_table::access_type op; >> + if (REG_P (XEXP (x, 0))) >> + cost_old = COSTS_N_INSNS (1); >> + else if (flag_pic >> + && GET_CODE (XEXP (x, 0)) == PLUS >> + && will_be_in_index_register (XEXP (XEXP (x, 0), 1))) >> + /* This will be split into two instructions. >> + See arm.md:calculate_pic_address. */ >> + cost_old = COSTS_N_INSNS (2); >> + else >> + cost_old = COSTS_N_INSNS (ARM_NUM_REGS (mode)); >> + switch (GET_CODE (XEXP (x, 0))) >> + { >> + case REG: >> + op = cbmem_cost_table::REG; >> + break; >> + case POST_INC: >> + case POST_DEC: >> + op = cbmem_cost_table::POST_INCDEC; >> + break; >> + case PRE_INC: >> + case PRE_DEC: >> + op = cbmem_cost_table::PRE_INCDEC; >> + break; >> + case POST_MODIFY: >> + op = cbmem_cost_table::POST_MODIFY; >> + break; >> + default: >> + case PLUS: >> + op = cbmem_cost_table::PLUS; >> + break; >> + } >> + if (flag_pic >> + && GET_CODE (XEXP (x, 0)) == PLUS >> + && will_be_in_index_register (XEXP (XEXP (x, 0), 1))) >> + cost_new = COSTS_N_INSNS (2); >> + else >> + { >> + cost_new = COSTS_N_INSNS (1); >> + if (arm_vector_mode_p (mode)) >> + { >> + cost_new += >> + (ARM_NUM_REGS (mode) <= 2 ? extra_cost->addr.vec64[op] >> + : ARM_NUM_REGS (mode) <= 4 ? extra_cost->addr.vec128[op] >> + : ARM_NUM_REGS (mode) <= 6 ? extra_cost->addr.vec192[op] >> + : ARM_NUM_REGS (mode) <= 8 ? extra_cost->addr.vec256[op] >> + : ARM_NUM_REGS (mode) <= 12 ? extra_cost->addr.vec384[op] >> + : extra_cost->addr.vec512[op]); >> + } >> + else if (FLOAT_MODE_P (mode)) >> + { >> + cost_new += >> + (ARM_NUM_REGS (mode) <= 1 ? extra_cost->addr.sf[op] >> + : ARM_NUM_REGS (mode) <= 2 ? extra_cost->addr.df[op] >> + : extra_cost->addr.cdf[op]); >> + } >> + else if (mode == BLKmode) >> + cost_new += extra_cost->addr.blk[op]; >> + else >> + { /* integer modes */ >> + cost_new += >> + (ARM_NUM_REGS (mode) <= 1 ? extra_cost->addr.si[op] >> + : ARM_NUM_REGS (mode) <= 2 ? extra_cost->addr.di[op] >> + : extra_cost->addr.cdi[op]); >> + } >> + } >> + *cost = cost_old; >> + if (cost_old != cost_new) >> + { >> + debug_rtx(x); >> + fprintf(stderr,"old(%d) new(%d)\n", cost_old, cost_new); >> + gcc_assert (cost_old == cost_new); >> + } >> + } > > Right, but this does not change arm_address_costs - so how is this going to work ? I would like this moved into a new function aarch_address_costs and that replacing arm_address_costs only to be called from here. arm_address_costs seems to do something reasonable already, but is only used for the TARGET_ADDRESS_COST hook. I haven't looked into the use of that hook at all, but it doesn't seem particularly connected to the part of the code I am trying to fix. This patch is addressing the excessive costs for RTXs which use MEM, as computed by arm_new_rtx_costs(). For clarity, I'm talking about this code, which becomes the calculation of cost_old in the patch. The NOT_YET guard means that arm_address_cost isn't used in this calculation. @@ -9524,16 +10114,83 @@ arm_new_rtx_costs (rtx x, enum rtx_code code, enum rtx_ code outer_code, case MEM: /* A memory access costs 1 insn if the mode is small, or the address is a single register, otherwise it costs one insn per word. */ - if (REG_P (XEXP (x, 0))) - *cost = COSTS_N_INSNS (1); - else if (flag_pic - && GET_CODE (XEXP (x, 0)) == PLUS - && will_be_in_index_register (XEXP (XEXP (x, 0), 1))) - /* This will be split into two instructions. - See arm.md:calculate_pic_address. */ - *cost = COSTS_N_INSNS (2); - else - *cost = COSTS_N_INSNS (ARM_NUM_REGS (mode)); /* For speed optimizations, add the costs of the address and accessing memory. */ if (speed_p) #ifdef NOT_YET *cost += (extra_cost->ldst.load + arm_address_cost (XEXP (x, 0), mode, ADDR_SPACE_GENERIC, speed_p)); #else *cost += extra_cost->ldst.load; #endif On reflection, I observe that the heuristics implemented by this code are completely bogus (as seen in the tables in my patch) and it would actually be better to remove most of this code. After all, if there is a single ARM instruction to do MEM (REG...), then there is also a single ARM instruction to do the other addressing modes. Example patch attached. (This gets the costs for CDImode wrong, since there's no 128 bit integer load instruction, but so does the original code) >From c274446abce41cc846bf6727f9be253ff9827a82 Mon Sep 17 00:00:00 2001 From: Charles Baylis Date: Wed, 4 Nov 2015 17:34:05 +0000 Subject: [PATCH] Simple patch Change-Id: I4dc5d4370ea9c5386e2bcae651ecf3fc6c21527e --- gcc/config/arm/arm.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index a598c84..0eb0e6f 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -9522,29 +9522,21 @@ arm_new_rtx_costs (rtx x, enum rtx_code code, enum rtx_code outer_code, return false; case MEM: - /* A memory access costs 1 insn if the mode is small, or the address is - a single register, otherwise it costs one insn per word. */ - if (REG_P (XEXP (x, 0))) - *cost = COSTS_N_INSNS (1); - else if (flag_pic + if (flag_pic && GET_CODE (XEXP (x, 0)) == PLUS && will_be_in_index_register (XEXP (XEXP (x, 0), 1))) /* This will be split into two instructions. See arm.md:calculate_pic_address. */ *cost = COSTS_N_INSNS (2); else - *cost = COSTS_N_INSNS (ARM_NUM_REGS (mode)); + *cost = COSTS_N_INSNS (1); /* For speed optimizations, add the costs of the address and accessing memory. */ if (speed_p) -#ifdef NOT_YET *cost += (extra_cost->ldst.load + arm_address_cost (XEXP (x, 0), mode, ADDR_SPACE_GENERIC, speed_p)); -#else - *cost += extra_cost->ldst.load; -#endif return true; case PARALLEL: -- 1.9.1