On 21/04/15 06:22, James Greenhalgh wrote: > On Fri, Apr 17, 2015 at 12:19:14PM +0100, Kugan wrote: >>>> My point is that adding your patch while keeping the logic at the top >>>> which claims to catch ALL vector operations makes for less readable >>>> code. >>>> >>>> At the very least you'll need to update this comment: >>>> >>>> /* TODO: The cost infrastructure currently does not handle >>>> vector operations. Assume that all vector operations >>>> are equally expensive. */ >>>> >>>> to make it clear that this doesn't catch vector set operations. >>>> >>>> But fixing the comment doesn't improve the messy code so I'd certainly >>>> prefer to see one of the other approaches which have been discussed. >>> >>> I see your point. Let me work on this based on your suggestions above. >> >> Hi James, >> >> Here is an attempt along this line. Is this what you have in mind? >> Trying to keep functionality as before so that we can tune the >> parameters later. Not fully tested yet. > > Hi Kugan, > > Sorry to have dropped out of the thread for a while, I'm currently > travelling in the US. > > This is along the lines of what I had in mind, thanks for digging through > and doing it. It needs a little polishing, just neaten up the rough edges > of comments and where they sit next to the new if conditionals, and of course, > testing, and I have a few comments below. > > Thanks, > James > >> diff --git a/gcc/config/aarch64/aarch64-cost-tables.h >> b/gcc/config/aarch64/aarch64-cost-tables.h >> index ae2b547..ed9432e 100644 >> --- a/gcc/config/aarch64/aarch64-cost-tables.h >> +++ b/gcc/config/aarch64/aarch64-cost-tables.h >> @@ -121,7 +121,9 @@ const struct cpu_cost_table thunderx_extra_costs = >> }, >> /* Vector */ >> { >> - COSTS_N_INSNS (1) /* Alu. */ >> + COSTS_N_INSNS (1), /* Alu. */ >> + COSTS_N_INSNS (1), /* Load. */ >> + COSTS_N_INSNS (1) /* Store. */ >> } >> }; > > Can you push the Load/Stores in to the LD/ST section above and give > them a name like loadv/storev. > >> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c >> index cba3c1a..c2d4a53 100644 >> --- a/gcc/config/aarch64/aarch64.c >> +++ b/gcc/config/aarch64/aarch64.c > > <snip> > >> @@ -5570,6 +5569,7 @@ aarch64_rtx_costs (rtx x, int code, int outer >> ATTRIBUTE_UNUSED, >> && (GET_MODE_BITSIZE (GET_MODE (XEXP (op1, 0))) >> >= INTVAL (XEXP (op0, 1)))) >> op1 = XEXP (op1, 0); >> + gcc_assert (!VECTOR_MODE_P (mode)); > > As Kyrill asked, please drop this.
Thanks for the review. I have updated the patch based on the comments with some other minor changes. Bootstrapped and regression tested on aarch64-none-linux-gnu with no-new regressions. Is this OK for trunk? Thanks, Kugan gcc/ChangeLog: 2015-04-24 Kugan Vivekanandarajah <kug...@linaro.org> Jim Wilson <jim.wil...@linaro.org> * config/arm/aarch-common-protos.h (struct mem_cost_table): Added new fields loadv and storev. * config/aarch64/aarch64-cost-tables.h (thunderx_extra_costs): Initialize loadv and storev. * config/arm/aarch-cost-tables.h (generic_extra_costs): Likewise. (cortexa53_extra_costs): Likewise. (cortexa57_extra_costs): Likewise. (xgene1_extra_costs): Likewise. * config/aarch64/aarch64.c (aarch64_rtx_costs): Update vector rtx_costs.
diff --git a/gcc/config/aarch64/aarch64-cost-tables.h b/gcc/config/aarch64/aarch64-cost-tables.h index ae2b547..939125c 100644 --- a/gcc/config/aarch64/aarch64-cost-tables.h +++ b/gcc/config/aarch64/aarch64-cost-tables.h @@ -83,7 +83,9 @@ const struct cpu_cost_table thunderx_extra_costs = 0, /* N/A: Stm_regs_per_insn_subsequent. */ 0, /* Storef. */ 0, /* Stored. */ - COSTS_N_INSNS (1) /* Store_unaligned. */ + COSTS_N_INSNS (1), /* Store_unaligned. */ + COSTS_N_INSNS (1), /* Loadv. */ + COSTS_N_INSNS (1) /* Storev. */ }, { /* FP SFmode */ diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index cba3c1a..13425fc 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -5499,16 +5499,6 @@ aarch64_rtx_costs (rtx x, int code, int outer ATTRIBUTE_UNUSED, above this default. */ *cost = COSTS_N_INSNS (1); - /* 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; - } - switch (code) { case SET: @@ -5523,7 +5513,9 @@ aarch64_rtx_costs (rtx x, int code, int outer ATTRIBUTE_UNUSED, if (speed) { rtx address = XEXP (op0, 0); - if (GET_MODE_CLASS (mode) == MODE_INT) + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->ldst.storev; + else if (GET_MODE_CLASS (mode) == MODE_INT) *cost += extra_cost->ldst.store; else if (mode == SFmode) *cost += extra_cost->ldst.storef; @@ -5544,15 +5536,22 @@ aarch64_rtx_costs (rtx x, int code, int outer ATTRIBUTE_UNUSED, /* Fall through. */ case REG: + /* The cost is one per vector-register copied. */ + if (VECTOR_MODE_P (GET_MODE (op0)) && REG_P (op1)) + { + 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) - { - /* The cost is 1 per register copied. */ - int n_minus_1 = (GET_MODE_SIZE (GET_MODE (op0)) - 1) + 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; - *cost = COSTS_N_INSNS (n_minus_1 + 1); - } + *cost = COSTS_N_INSNS (n_minus_1 + 1); + } else /* Cost is just the cost of the RHS of the set. */ *cost += rtx_cost (op1, SET, 1, speed); @@ -5650,7 +5649,9 @@ aarch64_rtx_costs (rtx x, int code, int outer ATTRIBUTE_UNUSED, approximation for the additional cost of the addressing mode. */ rtx address = XEXP (x, 0); - if (GET_MODE_CLASS (mode) == MODE_INT) + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->ldst.loadv; + else if (GET_MODE_CLASS (mode) == MODE_INT) *cost += extra_cost->ldst.load; else if (mode == SFmode) *cost += extra_cost->ldst.loadf; @@ -5667,6 +5668,14 @@ aarch64_rtx_costs (rtx x, int code, int outer ATTRIBUTE_UNUSED, case NEG: op0 = XEXP (x, 0); + if (VECTOR_MODE_P (mode)) + { + if (speed) + /* FNEG. */ + *cost += extra_cost->vect.alu; + return false; + } + if (GET_MODE_CLASS (GET_MODE (x)) == MODE_INT) { if (GET_RTX_CLASS (GET_CODE (op0)) == RTX_COMPARE @@ -5705,7 +5714,12 @@ aarch64_rtx_costs (rtx x, int code, int outer ATTRIBUTE_UNUSED, case CLRSB: case CLZ: if (speed) - *cost += extra_cost->alu.clz; + { + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->vect.alu; + else + *cost += extra_cost->alu.clz; + } return false; @@ -5790,6 +5804,20 @@ aarch64_rtx_costs (rtx x, int code, int outer ATTRIBUTE_UNUSED, return false; } + if (VECTOR_MODE_P (mode)) + { + /* Vector compare. */ + if (speed) + *cost += extra_cost->vect.alu; + + if (aarch64_float_const_zero_rtx_p (op1)) + { + /* Vector cm (eq|ge|gt|lt|le) supports constant 0.0 for no extra + cost. */ + return true; + } + return false; + } return false; case MINUS: @@ -5844,7 +5872,10 @@ cost_minus: if (speed) { - if (GET_MODE_CLASS (mode) == MODE_INT) + if (VECTOR_MODE_P (mode)) + /* Vector SUB. */ + *cost += extra_cost->vect.alu; + else if (GET_MODE_CLASS (mode) == MODE_INT) /* SUB(S). */ *cost += extra_cost->alu.arith; else if (GET_MODE_CLASS (mode) == MODE_FLOAT) @@ -5888,7 +5919,6 @@ cost_plus: { if (speed) *cost += extra_cost->alu.arith_shift; - *cost += rtx_cost (XEXP (XEXP (op0, 0), 0), (enum rtx_code) GET_CODE (op0), 0, speed); @@ -5913,7 +5943,10 @@ cost_plus: if (speed) { - if (GET_MODE_CLASS (mode) == MODE_INT) + if (VECTOR_MODE_P (mode)) + /* Vector ADD. */ + *cost += extra_cost->vect.alu; + else if (GET_MODE_CLASS (mode) == MODE_INT) /* ADD. */ *cost += extra_cost->alu.arith; else if (GET_MODE_CLASS (mode) == MODE_FLOAT) @@ -5927,8 +5960,12 @@ cost_plus: *cost = COSTS_N_INSNS (1); if (speed) - *cost += extra_cost->alu.rev; - + { + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->vect.alu; + else + *cost += extra_cost->alu.rev; + } return false; case IOR: @@ -5936,10 +5973,14 @@ cost_plus: { *cost = COSTS_N_INSNS (1); - if (speed) - *cost += extra_cost->alu.rev; - - return true; + if (speed) + { + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->vect.alu; + else + *cost += extra_cost->alu.rev; + } + return true; } /* Fall through. */ case XOR: @@ -5948,6 +5989,13 @@ cost_plus: op0 = XEXP (x, 0); op1 = XEXP (x, 1); + if (VECTOR_MODE_P (mode)) + { + if (speed) + *cost += extra_cost->vect.alu; + return true; + } + if (code == AND && GET_CODE (op0) == MULT && CONST_INT_P (XEXP (op0, 1)) @@ -6013,10 +6061,15 @@ cost_plus: return false; case NOT: - /* MVN. */ if (speed) - *cost += extra_cost->alu.logical; - + { + /* Vector NOT. */ + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->vect.alu; + /* MVN. */ + else + *cost += extra_cost->alu.logical; + } /* The logical instruction could have the shifted register form, but the cost is the same if the shift is processed as a separate instruction, so we don't bother with it here. */ @@ -6055,10 +6108,15 @@ cost_plus: return true; } - /* UXTB/UXTH. */ if (speed) - *cost += extra_cost->alu.extend; - + { + if (VECTOR_MODE_P (mode)) + /* UMOV. */ + *cost += extra_cost->vect.alu; + else + /* UXTB/UXTH. */ + *cost += extra_cost->alu.extend; + } return false; case SIGN_EXTEND: @@ -6078,7 +6136,12 @@ cost_plus: } if (speed) - *cost += extra_cost->alu.extend; + { + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->vect.alu; + else + *cost += extra_cost->alu.extend; + } return false; case ASHIFT: @@ -6087,10 +6150,16 @@ cost_plus: if (CONST_INT_P (op1)) { - /* LSL (immediate), UBMF, UBFIZ and friends. These are all - aliases. */ if (speed) - *cost += extra_cost->alu.shift; + { + /* Vector shift (immediate). */ + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->vect.alu; + /* LSL (immediate), UBMF, UBFIZ and friends. These are all + aliases. */ + else + *cost += extra_cost->alu.shift; + } /* We can incorporate zero/sign extend for free. */ if (GET_CODE (op0) == ZERO_EXTEND @@ -6102,10 +6171,15 @@ cost_plus: } else { - /* LSLV. */ if (speed) - *cost += extra_cost->alu.shift_reg; - + { + /* Vector shift (register). */ + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->vect.alu; + /* LSLV. */ + else + *cost += extra_cost->alu.shift_reg; + } return false; /* All arguments need to be in registers. */ } @@ -6120,7 +6194,12 @@ cost_plus: { /* ASR (immediate) and friends. */ if (speed) - *cost += extra_cost->alu.shift; + { + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->vect.alu; + else + *cost += extra_cost->alu.shift; + } *cost += rtx_cost (op0, (enum rtx_code) code, 0, speed); return true; @@ -6130,8 +6209,12 @@ cost_plus: /* ASR (register) and friends. */ if (speed) - *cost += extra_cost->alu.shift_reg; - + { + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->vect.alu; + else + *cost += extra_cost->alu.shift_reg; + } return false; /* All arguments need to be in registers. */ } @@ -6179,7 +6262,12 @@ cost_plus: case SIGN_EXTRACT: /* UBFX/SBFX. */ if (speed) - *cost += extra_cost->alu.bfx; + { + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->vect.alu; + else + *cost += extra_cost->alu.bfx; + } /* We can trust that the immediates used will be correct (there are no by-register forms), so we need only cost op0. */ @@ -6196,7 +6284,9 @@ cost_plus: case UMOD: if (speed) { - if (GET_MODE_CLASS (GET_MODE (x)) == MODE_INT) + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->vect.alu; + else if (GET_MODE_CLASS (GET_MODE (x)) == MODE_INT) *cost += (extra_cost->mult[GET_MODE (x) == DImode].add + extra_cost->mult[GET_MODE (x) == DImode].idiv); else if (GET_MODE (x) == DFmode) @@ -6213,7 +6303,9 @@ cost_plus: case SQRT: if (speed) { - if (GET_MODE_CLASS (mode) == MODE_INT) + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->vect.alu; + else if (GET_MODE_CLASS (mode) == MODE_INT) /* There is no integer SQRT, so only DIV and UDIV can get here. */ *cost += extra_cost->mult[mode == DImode].idiv; @@ -6245,7 +6337,12 @@ cost_plus: op2 = XEXP (x, 2); if (speed) - *cost += extra_cost->fp[mode == DFmode].fma; + { + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->vect.alu; + else + *cost += extra_cost->fp[mode == DFmode].fma; + } /* FMSUB, FNMADD, and FNMSUB are free. */ if (GET_CODE (op0) == NEG) @@ -6285,12 +6382,24 @@ cost_plus: case FLOAT_EXTEND: if (speed) - *cost += extra_cost->fp[mode == DFmode].widen; + { + if (VECTOR_MODE_P (mode)) + /*Vector truncate. */ + *cost += extra_cost->vect.alu; + else + *cost += extra_cost->fp[mode == DFmode].widen; + } return false; case FLOAT_TRUNCATE: if (speed) - *cost += extra_cost->fp[mode == DFmode].narrow; + { + if (VECTOR_MODE_P (mode)) + /*Vector conversion. */ + *cost += extra_cost->vect.alu; + else + *cost += extra_cost->fp[mode == DFmode].narrow; + } return false; case FIX: @@ -6311,13 +6420,23 @@ cost_plus: } if (speed) - *cost += extra_cost->fp[GET_MODE (x) == DFmode].toint; - + { + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->vect.alu; + else + *cost += extra_cost->fp[GET_MODE (x) == DFmode].toint; + } *cost += rtx_cost (x, (enum rtx_code) code, 0, speed); return true; case ABS: - if (GET_MODE_CLASS (mode) == MODE_FLOAT) + if (VECTOR_MODE_P (mode)) + { + /* ABS (vector). */ + if (speed) + *cost += extra_cost->vect.alu; + } + else if (GET_MODE_CLASS (mode) == MODE_FLOAT) { /* FABS and FNEG are analogous. */ if (speed) @@ -6338,10 +6457,13 @@ cost_plus: case SMIN: if (speed) { + if (VECTOR_MODE_P (mode)) + *cost += extra_cost->vect.alu; /* FMAXNM/FMINNM/FMAX/FMIN. TODO: This may not be accurate for all implementations, but we do not model this in the cost tables. */ - *cost += extra_cost->fp[mode == DFmode].addsub; + else + *cost += extra_cost->fp[mode == DFmode].addsub; } return false; diff --git a/gcc/config/arm/aarch-common-protos.h b/gcc/config/arm/aarch-common-protos.h index 3ee7ebf..29f7c99 100644 --- a/gcc/config/arm/aarch-common-protos.h +++ b/gcc/config/arm/aarch-common-protos.h @@ -102,6 +102,8 @@ struct mem_cost_table const int storef; /* SFmode. */ const int stored; /* DFmode. */ const int store_unaligned; /* Extra for unaligned stores. */ + const int loadv; /* Vector load. */ + const int storev; /* Vector store. */ }; struct fp_cost_table diff --git a/gcc/config/arm/aarch-cost-tables.h b/gcc/config/arm/aarch-cost-tables.h index 05e96a9..809feb8 100644 --- a/gcc/config/arm/aarch-cost-tables.h +++ b/gcc/config/arm/aarch-cost-tables.h @@ -81,7 +81,9 @@ const struct cpu_cost_table generic_extra_costs = 1, /* stm_regs_per_insn_subsequent. */ COSTS_N_INSNS (2), /* storef. */ COSTS_N_INSNS (3), /* stored. */ - COSTS_N_INSNS (1) /* store_unaligned. */ + COSTS_N_INSNS (1), /* store_unaligned. */ + COSTS_N_INSNS (1), /* loadv. */ + COSTS_N_INSNS (1) /* storev. */ }, { /* FP SFmode */ @@ -182,7 +184,9 @@ const struct cpu_cost_table cortexa53_extra_costs = 2, /* stm_regs_per_insn_subsequent. */ 0, /* storef. */ 0, /* stored. */ - COSTS_N_INSNS (1) /* store_unaligned. */ + COSTS_N_INSNS (1), /* store_unaligned. */ + COSTS_N_INSNS (1), /* loadv. */ + COSTS_N_INSNS (1) /* storev. */ }, { /* FP SFmode */ @@ -283,7 +287,9 @@ const struct cpu_cost_table cortexa57_extra_costs = 2, /* stm_regs_per_insn_subsequent. */ 0, /* storef. */ 0, /* stored. */ - COSTS_N_INSNS (1) /* store_unaligned. */ + COSTS_N_INSNS (1), /* store_unaligned. */ + COSTS_N_INSNS (1), /* loadv. */ + COSTS_N_INSNS (1) /* storev. */ }, { /* FP SFmode */ @@ -385,6 +391,8 @@ const struct cpu_cost_table xgene1_extra_costs = 0, /* storef. */ 0, /* stored. */ 0, /* store_unaligned. */ + COSTS_N_INSNS (1), /* loadv. */ + COSTS_N_INSNS (1) /* storev. */ }, { /* FP SFmode */