From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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-Archive: List-Post: List-Help: Sender: gcc-patches-owner@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, 04 Nov 2015 17:52:00 -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 Content-Type: multipart/mixed; boundary=001a113d67445d8eb30523baabe0 X-IsSubscribed: yes X-SW-Source: 2015-11/txt/msg00387.txt.bz2 --001a113d67445d8eb30523baabe0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Content-length: 14218 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=3D61551 >> >> 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 tak= en 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 =3D 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 wo= uld be appropriate for the AArch64 backend. Further more I think Alan recen= tly 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 th= e 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 =3D 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 vecto= r 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_NEO= N 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 t= he 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 ab= stract 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 w= ith 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-c= ost-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 =3D > I'm curious as to the numbers here - The costs should reflect the relativ= e costs of the addressing modes not the costs of the loads and stores - thu= s 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 sat= isfactorily 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, e= num rtx_code outer_code, >> case MEM: >> /* A memory access costs 1 insn if the mode is small, or the addr= ess is >> a single register, otherwise it costs one insn per word. */ >> - if (REG_P (XEXP (x, 0))) >> - *cost =3D COSTS_N_INSNS (1); >> - else if (flag_pic >> - && GET_CODE (XEXP (x, 0)) =3D=3D 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 =3D COSTS_N_INSNS (2); >> - else >> - *cost =3D 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 =3D COSTS_N_INSNS (1); >> + else if (flag_pic >> + && GET_CODE (XEXP (x, 0)) =3D=3D 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 =3D COSTS_N_INSNS (2); >> + else >> + cost_old =3D COSTS_N_INSNS (ARM_NUM_REGS (mode)); >> + switch (GET_CODE (XEXP (x, 0))) >> + { >> + case REG: >> + op =3D cbmem_cost_table::REG; >> + break; >> + case POST_INC: >> + case POST_DEC: >> + op =3D cbmem_cost_table::POST_INCDEC; >> + break; >> + case PRE_INC: >> + case PRE_DEC: >> + op =3D cbmem_cost_table::PRE_INCDEC; >> + break; >> + case POST_MODIFY: >> + op =3D cbmem_cost_table::POST_MODIFY; >> + break; >> + default: >> + case PLUS: >> + op =3D cbmem_cost_table::PLUS; >> + break; >> + } >> + if (flag_pic >> + && GET_CODE (XEXP (x, 0)) =3D=3D PLUS >> + && will_be_in_index_register (XEXP (XEXP (x, 0), 1))) >> + cost_new =3D COSTS_N_INSNS (2); >> + else >> + { >> + cost_new =3D COSTS_N_INSNS (1); >> + if (arm_vector_mode_p (mode)) >> + { >> + cost_new +=3D >> + (ARM_NUM_REGS (mode) <=3D 2 ? extra_cost->addr.vec64[op] >> + : ARM_NUM_REGS (mode) <=3D 4 ? extra_cost->addr.vec128[o= p] >> + : ARM_NUM_REGS (mode) <=3D 6 ? extra_cost->addr.vec192[o= p] >> + : ARM_NUM_REGS (mode) <=3D 8 ? extra_cost->addr.vec256[o= p] >> + : ARM_NUM_REGS (mode) <=3D 12 ? extra_cost->addr.vec384[= op] >> + : extra_cost->addr.vec512[op]); >> + } >> + else if (FLOAT_MODE_P (mode)) >> + { >> + cost_new +=3D >> + (ARM_NUM_REGS (mode) <=3D 1 ? extra_cost->addr.sf[op] >> + : ARM_NUM_REGS (mode) <=3D 2 ? extra_cost->addr.df[op] >> + : extra_cost->addr.cdf[op]); >> + } >> + else if (mode =3D=3D BLKmode) >> + cost_new +=3D extra_cost->addr.blk[op]; >> + else >> + { /* integer modes */ >> + cost_new +=3D >> + (ARM_NUM_REGS (mode) <=3D 1 ? extra_cost->addr.si[op] >> + : ARM_NUM_REGS (mode) <=3D 2 ? extra_cost->addr.di[op] >> + : extra_cost->addr.cdi[op]); >> + } >> + } >> + *cost =3D cost_old; >> + if (cost_old !=3D cost_new) >> + { >> + debug_rtx(x); >> + fprintf(stderr,"old(%d) new(%d)\n", cost_old, cost_new); >> + gcc_assert (cost_old =3D=3D 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 a= nd 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 =3D COSTS_N_INSNS (1); - else if (flag_pic - && GET_CODE (XEXP (x, 0)) =3D=3D 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 =3D COSTS_N_INSNS (2); - else - *cost =3D 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 +=3D (extra_cost->ldst.load + arm_address_cost (XEXP (x, 0), mode, ADDR_SPACE_GENERIC, speed_p)); #else *cost +=3D 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) --001a113d67445d8eb30523baabe0 Content-Type: text/x-patch; charset=US-ASCII; name="0001-Simple-patch.patch" Content-Disposition: attachment; filename="0001-Simple-patch.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_igl36nf91 Content-length: 2079 RnJvbSBjMjc0NDQ2YWJjZTQxY2M4NDZiZjY3MjdmOWJlMjUzZmY5ODI3YTgy IE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBDaGFybGVzIEJheWxp cyA8Y2hhcmxlcy5iYXlsaXNAbGluYXJvLm9yZz4KRGF0ZTogV2VkLCA0IE5v diAyMDE1IDE3OjM0OjA1ICswMDAwClN1YmplY3Q6IFtQQVRDSF0gU2ltcGxl IHBhdGNoCgpDaGFuZ2UtSWQ6IEk0ZGM1ZDQzNzBlYTljNTM4NmUyYmNhZTY1 MWVjZjNmYzZjMjE1MjdlCi0tLQogZ2NjL2NvbmZpZy9hcm0vYXJtLmMgfCAx MiArKy0tLS0tLS0tLS0KIDEgZmlsZSBjaGFuZ2VkLCAyIGluc2VydGlvbnMo KyksIDEwIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL2djYy9jb25maWcv YXJtL2FybS5jIGIvZ2NjL2NvbmZpZy9hcm0vYXJtLmMKaW5kZXggYTU5OGM4 NC4uMGViMGU2ZiAxMDA2NDQKLS0tIGEvZ2NjL2NvbmZpZy9hcm0vYXJtLmMK KysrIGIvZ2NjL2NvbmZpZy9hcm0vYXJtLmMKQEAgLTk1MjIsMjkgKzk1MjIs MjEgQEAgYXJtX25ld19ydHhfY29zdHMgKHJ0eCB4LCBlbnVtIHJ0eF9jb2Rl IGNvZGUsIGVudW0gcnR4X2NvZGUgb3V0ZXJfY29kZSwKICAgICAgIHJldHVy biBmYWxzZTsKIAogICAgIGNhc2UgTUVNOgotICAgICAgLyogQSBtZW1vcnkg YWNjZXNzIGNvc3RzIDEgaW5zbiBpZiB0aGUgbW9kZSBpcyBzbWFsbCwgb3Ig dGhlIGFkZHJlc3MgaXMKLQkgYSBzaW5nbGUgcmVnaXN0ZXIsIG90aGVyd2lz ZSBpdCBjb3N0cyBvbmUgaW5zbiBwZXIgd29yZC4gICovCi0gICAgICBpZiAo UkVHX1AgKFhFWFAgKHgsIDApKSkKLQkqY29zdCA9IENPU1RTX05fSU5TTlMg KDEpOwotICAgICAgZWxzZSBpZiAoZmxhZ19waWMKKyAgICAgIGlmIChmbGFn X3BpYwogCSAgICAgICAmJiBHRVRfQ09ERSAoWEVYUCAoeCwgMCkpID09IFBM VVMKIAkgICAgICAgJiYgd2lsbF9iZV9pbl9pbmRleF9yZWdpc3RlciAoWEVY UCAoWEVYUCAoeCwgMCksIDEpKSkKIAkvKiBUaGlzIHdpbGwgYmUgc3BsaXQg aW50byB0d28gaW5zdHJ1Y3Rpb25zLgogCSAgIFNlZSBhcm0ubWQ6Y2FsY3Vs YXRlX3BpY19hZGRyZXNzLiAgKi8KIAkqY29zdCA9IENPU1RTX05fSU5TTlMg KDIpOwogICAgICAgZWxzZQotCSpjb3N0ID0gQ09TVFNfTl9JTlNOUyAoQVJN X05VTV9SRUdTIChtb2RlKSk7CisJKmNvc3QgPSBDT1NUU19OX0lOU05TICgx KTsKIAogICAgICAgLyogRm9yIHNwZWVkIG9wdGltaXphdGlvbnMsIGFkZCB0 aGUgY29zdHMgb2YgdGhlIGFkZHJlc3MgYW5kCiAJIGFjY2Vzc2luZyBtZW1v cnkuICAqLwogICAgICAgaWYgKHNwZWVkX3ApCi0jaWZkZWYgTk9UX1lFVAog CSpjb3N0ICs9IChleHRyYV9jb3N0LT5sZHN0LmxvYWQKIAkJICArIGFybV9h ZGRyZXNzX2Nvc3QgKFhFWFAgKHgsIDApLCBtb2RlLAogCQkJCSAgICAgIEFE RFJfU1BBQ0VfR0VORVJJQywgc3BlZWRfcCkpOwotI2Vsc2UKLSAgICAgICAg KmNvc3QgKz0gZXh0cmFfY29zdC0+bGRzdC5sb2FkOwotI2VuZGlmCiAgICAg ICByZXR1cm4gdHJ1ZTsKIAogICAgIGNhc2UgUEFSQUxMRUw6Ci0tIAoxLjku MQoK --001a113d67445d8eb30523baabe0--