Re: [PATCH 01/15] rs6000: Split out rs6000_is_valid_and_mask_wide

[Segher Boessenkool <segher@kernel.crashing.org>]

On Tue, Aug 11, 2015 at 06:11:30PM -0700, Richard Henderson wrote:
> This allows testing for a mask without having to call GEN_INT.
> 
> Cc: David Edelsohn <dje.gcc@gmail.com>
> ---
> 	* config/rs6000/rs6000.c (rs6000_is_valid_mask_wide): Split out from...
> 	(rs6000_is_valid_mask): ... here.
> 	(rs6000_is_valid_and_mask_wide): Split out from...
> 	(rs6000_is_valid_and_mask): ... here.

I don't like these "_wide" names much.  You could overload the shorter
name, if you really think creating some garbage const_int's is too much
overhead (it might well be if you use it a lot more in later patches).

The original functions really want rtx's since they are used like
predicates (so should look and behave like one); rs6000_is_valid_mask
itself is different (and a lousy name; suggestions welcome).

> -bool
> -rs6000_is_valid_mask (rtx mask, int *b, int *e, machine_mode mode)
> +static bool
> +rs6000_is_valid_mask_wide (unsigned HOST_WIDE_INT val, int *b, int *e, int n)

But why change the mode parameter?  The code was clearer before.

>  {
> -  unsigned HOST_WIDE_INT val = INTVAL (mask);
>    unsigned HOST_WIDE_INT bit;
>    int nb, ne;
> -  int n = GET_MODE_PRECISION (mode);
>  
> -  if (mode != DImode && mode != SImode)
> -    return false;
> -
> -  if (INTVAL (mask) >= 0)
> +  if ((HOST_WIDE_INT)val >= 0)
                        ^ missing space

>      {
>        bit = val & -val;
>        ne = exact_log2 (bit);
> @@ -16430,27 +16427,54 @@ rs6000_is_valid_mask (rtx mask, int *b, int *e, machine_mode mode)
>    return true;
>  }
>  
> +bool
> +rs6000_is_valid_mask (rtx mask, int *b, int *e, machine_mode mode)
> +{
> +  int n;
> +
> +  if (mode == DImode)
> +    n = 64;
> +  else if (mode == SImode)
> +    n = 32;
> +  else
> +    return false;
> +
> +  unsigned HOST_WIDE_INT val = INTVAL (mask);
> +  return rs6000_is_valid_mask_wide (val, b, e, n);
> +}
> +
>  /* Return whether MASK (a CONST_INT) is a valid mask for any rlwinm, rldicl,
>     or rldicr instruction, to implement an AND with it in mode MODE.  */
>  
> -bool
> -rs6000_is_valid_and_mask (rtx mask, machine_mode mode)
> +static bool
> +rs6000_is_valid_and_mask_wide (unsigned HOST_WIDE_INT val, machine_mode mode)
>  {
>    int nb, ne;
>  
> -  if (!rs6000_is_valid_mask (mask, &nb, &ne, mode))
> -    return false;
> +  switch (mode)
> +    {
> +    case DImode:
> +      if (!rs6000_is_valid_mask_wide (val, &nb, &ne, 64))
> +	return false;
> +      /* For DImode, we need a rldicl, rldicr, or a rlwinm with
> +	 mask that does not wrap.  */
> +      return (ne == 0 || nb == 63 || (nb < 32 && ne <= nb));
>  
> -  /* For DImode, we need a rldicl, rldicr, or a rlwinm with mask that
> -     does not wrap.  */
> -  if (mode == DImode)
> -    return (ne == 0 || nb == 63 || (nb < 32 && ne <= nb));
> +    case SImode:
> +      if (!rs6000_is_valid_mask_wide (val, &nb, &ne, 32))
> +	return false;
> +      /* For SImode, rlwinm can do everything.  */
> +      return (nb < 32 && ne < 32);
>  
> -  /* For SImode, rlwinm can do everything.  */
> -  if (mode == SImode)
> -    return (nb < 32 && ne < 32);
> +    default:
> +      return false;
> +    }
> +}
>  
> -  return false;

You don't need any of these changes then, either.

> +bool
> +rs6000_is_valid_and_mask (rtx mask, machine_mode mode)
> +{
> +  return rs6000_is_valid_and_mask_wide (UINTVAL (mask), mode);
>  }
>  
>  /* Return the instruction template for an AND with mask in mode MODE, with
> @@ -16739,12 +16763,12 @@ rs6000_is_valid_2insn_and (rtx c, machine_mode mode)
>  
>    /* Otherwise, fill in the lowest "hole"; if we can do the result with
>       one insn, we can do the whole thing with two.  */
> -  unsigned HOST_WIDE_INT val = INTVAL (c);
> +  unsigned HOST_WIDE_INT val = UINTVAL (c);

Does it matter?


Segher