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

[Kyrill Tkachov <kyrylo dot tkachov at arm dot com>]

On 27/04/15 14:53, Kyrill Tkachov wrote:
> 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.

Never mind, I misread the code. The case is indeed
missing the costing of operand 1, so your patch is
a superset of mine. I think it's correct, but I can't
approve. If this goes in, then I withdraw my patch at:

https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01617.html


Sorry for the confusion,
Kyrill


> 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