[3/8,RS6000] rs6000_rtx_costs tidy AND

* config/rs6000/rs6000.c (rs6000_rtx_costs): Tidy AND code.
	Don't avoid recursion on const_int shift count.

Ping.

On Tue, Jan 12, 2021 at 02:01:57PM +1030, Alan Modra wrote:
> Ping
> https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555754.html
> 
> On Thu, Oct 08, 2020 at 09:27:55AM +1030, Alan Modra wrote:
> > 	* config/rs6000/rs6000.c (rs6000_rtx_costs): Tidy AND code.
> > 	Don't avoid recursion on const_int shift count.
> > 
> > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> > index e870ba0039a..bc5e51aa5ce 100644
> > --- a/gcc/config/rs6000/rs6000.c
> > +++ b/gcc/config/rs6000/rs6000.c
> > @@ -21253,6 +21253,7 @@ static bool
> >  rs6000_rtx_costs (rtx x, machine_mode mode, int outer_code,
> >  		  int opno ATTRIBUTE_UNUSED, int *total, bool speed)
> >  {
> > +  rtx right;
> >    int code = GET_CODE (x);
> >  
> >    switch (code)
> > @@ -21430,7 +21431,9 @@ rs6000_rtx_costs (rtx x, machine_mode mode, int outer_code,
> >        return false;
> >  
> >      case AND:
> > -      if (CONST_INT_P (XEXP (x, 1)))
> > +      *total = COSTS_N_INSNS (1);
> > +      right = XEXP (x, 1);
> > +      if (CONST_INT_P (right))
> >  	{
> >  	  rtx left = XEXP (x, 0);
> >  	  rtx_code left_code = GET_CODE (left);
> > @@ -21439,17 +21442,13 @@ rs6000_rtx_costs (rtx x, machine_mode mode, int outer_code,
> >  	  if ((left_code == ROTATE
> >  	       || left_code == ASHIFT
> >  	       || left_code == LSHIFTRT)
> > -	      && rs6000_is_valid_shift_mask (XEXP (x, 1), left, mode))
> > +	      && rs6000_is_valid_shift_mask (right, left, mode))
> >  	    {
> > -	      *total = rtx_cost (XEXP (left, 0), mode, left_code, 0, speed);
> > -	      if (!CONST_INT_P (XEXP (left, 1)))
> > -		*total += rtx_cost (XEXP (left, 1), SImode, left_code, 1, speed);
> > -	      *total += COSTS_N_INSNS (1);
> > +	      *total += rtx_cost (XEXP (left, 0), mode, left_code, 0, speed);
> > +	      *total += rtx_cost (XEXP (left, 1), SImode, left_code, 1, speed);
> >  	      return true;
> >  	    }
> >  	}
> > -
> > -      *total = COSTS_N_INSNS (1);
> >        return false;
> >  
> >      case IOR:

Hi!

On Thu, Oct 08, 2020 at 09:27:55AM +1030, Alan Modra wrote:
> 	* config/rs6000/rs6000.c (rs6000_rtx_costs): Tidy AND code.
> 	Don't avoid recursion on const_int shift count.
> 
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index e870ba0039a..bc5e51aa5ce 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -21253,6 +21253,7 @@ static bool
>  rs6000_rtx_costs (rtx x, machine_mode mode, int outer_code,
>  		  int opno ATTRIBUTE_UNUSED, int *total, bool speed)
>  {
> +  rtx right;

Please declare things where you first use them.

>    int code = GET_CODE (x);
>  
>    switch (code)
> @@ -21430,7 +21431,9 @@ rs6000_rtx_costs (rtx x, machine_mode mode, int outer_code,
>        return false;
>  
>      case AND:
> -      if (CONST_INT_P (XEXP (x, 1)))
> +      *total = COSTS_N_INSNS (1);
> +      right = XEXP (x, 1);
> +      if (CONST_INT_P (right))
>  	{
>  	  rtx left = XEXP (x, 0);
>  	  rtx_code left_code = GET_CODE (left);
> @@ -21439,17 +21442,13 @@ rs6000_rtx_costs (rtx x, machine_mode mode, int outer_code,
>  	  if ((left_code == ROTATE
>  	       || left_code == ASHIFT
>  	       || left_code == LSHIFTRT)
> -	      && rs6000_is_valid_shift_mask (XEXP (x, 1), left, mode))
> +	      && rs6000_is_valid_shift_mask (right, left, mode))

You could have left all this intact, it is just distraction from the
rest of the patch (nothing changed here).  Except you set *total.

>  	    {
> -	      *total = rtx_cost (XEXP (left, 0), mode, left_code, 0, speed);
> -	      if (!CONST_INT_P (XEXP (left, 1)))
> -		*total += rtx_cost (XEXP (left, 1), SImode, left_code, 1, speed);
> -	      *total += COSTS_N_INSNS (1);
> +	      *total += rtx_cost (XEXP (left, 0), mode, left_code, 0, speed);
> +	      *total += rtx_cost (XEXP (left, 1), SImode, left_code, 1, speed);
>  	      return true;
>  	    }
>  	}
> -
> -      *total = COSTS_N_INSNS (1);
>        return false;

I still do not see what this improves, I only see possible obvious
regressions :-(


Segher

On Mon, Jan 25, 2021 at 04:37:21PM -0600, Segher Boessenkool wrote:
> I still do not see what this improves, I only see possible obvious
> regressions :-(

You asked me to break the patch series into small pieces, for ease of
review and to separate tidies from functional changes.  Well OK, fair
enough.  This is one of the tidies.  The idea being to make
rs6000_rtx_costs a little more self-consistent, to not have someone
look at this code in the future and wonder why AND was treated
differently to other operations.

The only part of this patch that I can imagine you see as a possible
regression is the "Don't avoid recursion on const_int shift count"
part.  That is there only because you wanted it that way in new code.
I think you said something about premature optimisation when I made
the new code special case const_int and reg to stop recursion, like
AND.  So for consistency I made the change in old code too.