Re: [PATCH][AArch64] Fix aarch64_rtx_costs of PLUS/MINUS

[Kyrill Tkachov <kyrylo.tkachov@arm.com>]

Hi Wilco,

On 27/04/15 14:43, Wilco Dijkstra wrote:
> ping
>
>> -----Original Message-----
>> From: Wilco Dijkstra [mailto:wdijkstr@arm.com]
>> Sent: 04 March 2015 15:38
>> To: GCC Patches
>> Subject: [PATCH][AArch64] Fix aarch64_rtx_costs of PLUS/MINUS
>>
>> Include the cost of op0 and op1 in all cases in PLUS and MINUS in aarch64_rtx_costs.
>> Bootstrap & regression OK.
>>
>> ChangeLog:
>> 2015-03-04  Wilco Dijkstra  <wdijkstr@arm.com>
>>
>>          * gcc/config/aarch64/aarch64.c (aarch64_rtx_costs):
>>          Calculate cost of op0 and op1 in PLUS and MINUS cases.
>>
>> ---
>>   gcc/config/aarch64/aarch64.c | 12 +++++-------
>>   1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> index 39921a7..e22d72e 100644
>> --- a/gcc/config/aarch64/aarch64.c
>> +++ b/gcc/config/aarch64/aarch64.c
>> @@ -5794,6 +5794,8 @@ aarch64_rtx_costs (rtx x, int code, int outer ATTRIBUTE_UNUSED,
>>   	op1 = XEXP (x, 1);
>>
>>   cost_minus:
>> +	*cost += rtx_cost (op0, MINUS, 0, speed);
>> +
>>   	/* Detect valid immediates.  */
>>   	if ((GET_MODE_CLASS (mode) == MODE_INT
>>   	     || (GET_MODE_CLASS (mode) == MODE_CC
>> @@ -5801,13 +5803,10 @@ cost_minus:
>>   	    && CONST_INT_P (op1)
>>   	    && aarch64_uimm12_shift (INTVAL (op1)))
>>   	  {
>> -	    *cost += rtx_cost (op0, MINUS, 0, speed);
>> -
>>   	    if (speed)
>>   	      /* SUB(S) (immediate).  */
>>   	      *cost += extra_cost->alu.arith;
>>   	    return true;
>> -
>>   	  }
>>
>>   	/* Look for SUB (extended register).  */
>> @@ -5832,7 +5831,6 @@ cost_minus:
>>   	    *cost += aarch64_rtx_mult_cost (new_op1, MULT,
>>   					    (enum rtx_code) code,
>>   					    speed);
>> -	    *cost += rtx_cost (op0, MINUS, 0, speed);
>>   	    return true;
>>   	  }
>>
>> @@ -5879,6 +5877,8 @@ cost_plus:
>>   	    return true;
>>   	  }
>>
>> +	*cost += rtx_cost (op1, PLUS, 1, speed);
>> +

I don't think this is correct. In the code directly below
(when the aarch64_rtx_arith_op_extract_p condition is true)
we have a shift/extend operation by a constant, so we don't
want to take into account the cost of operand 1 (which is
the extend+shift rtx).

So, I looked at this code myself recently and found that the
wrong logic is addressed by my patch at:
https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01617.html
which fixes the case where we forget to cost operand0 in the
MINUS case.

What do you think?
Kyrill

>>   	/* Look for ADD (extended register).  */
>>           if (aarch64_rtx_arith_op_extract_p (op0, mode))
>>   	  {
>> @@ -5900,12 +5900,10 @@ cost_plus:
>>   	  {
>>   	    *cost += aarch64_rtx_mult_cost (new_op0, MULT, PLUS,
>>   					    speed);
>> -	    *cost += rtx_cost (op1, PLUS, 1, speed);
>>   	    return true;
>>   	  }
>>
>> -	*cost += (rtx_cost (new_op0, PLUS, 0, speed)
>> -		  + rtx_cost (op1, PLUS, 1, speed));
>> +	*cost += rtx_cost (new_op0, PLUS, 0, speed);
>>
>>   	if (speed)
>>   	  {
>> --
>> 1.9.1
>