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

[Kugan <kugan dot vivekanandarajah at linaro dot org>]

On 05/05/15 16:17, James Greenhalgh wrote:
> 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.
> 
> 

Thanks James for the review. Attached patch changes this. Is this OK ?


Thanks,
Kugan

Attachment: cost3.txt
Description: Text document