Re: [PATCH 1/2] [ARM] Refactor costs calculation for MEM.

["Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com>]

On 21/02/17 16:54, charles.baylis@linaro.org wrote:
> From: Charles Baylis <charles.baylis@linaro.org>
> 
> This patch moves the calculation of costs for MEM into a
> separate function, and reforms the calculation into two
> parts. Firstly any additional cost of the addressing mode
> is calculated, and then the cost of the memory access itself
> is added.
> 
> In this patch, the calculation of the cost of the addressing
> mode is left as a placeholder, to be added in a subsequent
> patch.
> 
> gcc/ChangeLog:
> 
> <date>  Charles Baylis  <charles.baylis@linaro.org>
> 
>         * config/arm/arm.c (arm_mem_costs): New function.
>         (arm_rtx_costs_internal): Use arm_mem_costs.

I like the idea of this patch, but it needs further work...

Comments inline.

R.

> 
> Change-Id: I99e93406ea39ee31f71c7bf428ad3e127b7a618e
> ---
>  gcc/config/arm/arm.c | 66 +++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 42 insertions(+), 24 deletions(-)
> 
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 6cae178..7f002f1 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -9072,6 +9072,47 @@ arm_unspec_cost (rtx x, enum rtx_code /* outer_code */, bool speed_p, int *cost)
>  	  }								\
>  	while (0);
>  
> +/* Helper function for arm_rtx_costs_internal. Calculates the cost of a MEM,
> +   considering the costs of the addressing mode and memory access
> +   separately.  */
> +static bool
> +arm_mem_costs (rtx x, const struct cpu_cost_table *extra_cost,
> +	       int *cost, bool speed_p)
> +{
> +  machine_mode mode = GET_MODE (x);
> +  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.  Add the cost of the
> +       additional instruction here.  The cost of the memory access is computed
> +       below.  See arm.md:calculate_pic_address.  */
> +    *cost = COSTS_N_INSNS (1);
> +  else
> +    *cost = 0;
> +
> +  /* Calculate cost of the addressing mode.  */
> +  if (speed_p)
> +  {

This patch needs to be reformatted in the GNU style (indentation of
braces, braces and else clauses on separate lines etc).

> +    /* TODO: Add table-driven costs for addressing modes.  */

You need to sort out the comment.  What's missing here?

> +  }
> +
> +  /* cost of memory access */
> +  if (speed_p)
> +  {
> +    /* data transfer is transfer size divided by bus width.  */
> +    int bus_width = arm_arch7 ? 8 : 4;

Basing bus width on the architecture is a bit too simplistic.  Instead
this should be a parameter that comes from the CPU cost tables, based on
the current tune target.

> +    *cost += COSTS_N_INSNS((GET_MODE_SIZE (mode) + bus_width - 1) / bus_width);

Use CEIL (from system.h)

> +    *cost += extra_cost->ldst.load;
> +  } else {
> +    *cost += COSTS_N_INSNS (1);
> +  }
> +
> +  return true;
> +}
> +/* Convert fron bytes to ints.  */
> +#define ARM_NUM_INTS(X) (((X) + UNITS_PER_WORD - 1) / UNITS_PER_WORD)
> +
> +
>  /* 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,
> @@ -9152,30 +9193,7 @@ arm_rtx_costs_internal (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
> -	       && 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
> -      return true;
> +      return arm_mem_costs (x, extra_cost, cost, speed_p);
>  
>      case PARALLEL:
>      {
>