Re: [AArch64][PR65375] Fix RTX cost for vector SET

[James Greenhalgh <james dot greenhalgh at arm dot com>]

On Sat, Apr 25, 2015 at 12:26:16AM +0100, Kugan wrote:
> 
> 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  <kuganv@linaro.org>
> 	    Jim Wilson  <jim.wilson@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.

Hi Kugan,

Just a few syle comments, regarding the placements of comments in single-line
if statements. I know the current code does not neccesarily always follow the
comments below, I'll write a patch cleaning that up at some point when I'm back
at my desk.

Thanks,
James

> @@ -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

Personally, I find commented if statements without braces hard to
quickly parse. Something like this is much faster for me:

	  if (speed)
	    {
	      /* FNEG.  */
	      *cost += extra_cost->vect.alu;
	    }

> @@ -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)

As above.

> @@ -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);

Drop this whitespace change.

> @@ -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)

As above.

> @@ -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.  */

As above.

> @@ -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;
>  
>      ca§se SIGN_EXTEND:

And again :)

> @@ -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

Again, the comment here makes it very difficult to spot the form of
the if/else statement.

> @@ -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.  */
>          }
>  

Likewise here.