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

[Kyrill Tkachov <kyrylo dot tkachov at arm dot 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