[PATCH] Modify combine pattern by anding a pseudo with its nonzero bits

[Segher Boessenkool]

Hi!

On Tue, Nov 30, 2021 at 04:46:34PM +0800, HAO CHEN GUI wrote:
>     This patch modifies the combine pattern with a helper - change_pseudo_and_mask when recog fails. The helper converts a single pseudo to the pseudo and with a mask if the outer operator is IOR/XOR/PLUS and the inner operator is ASHIFT/LSHIFTRT/AND. The conversion helps match shift + ior pattern.
> 
>     Bootstrapped and tested on powerpc64-linux BE and LE with no regressions. Is this okay for trunk? Any recommendations? Thanks a lot.

(Please make shorter lines in email.  70 chars is usual).

> gcc/
>         * combine.c (change_pseudo_and_mask): New.
>         (recog_for_combine): If recog fails, try again with the pattern
>         modified by change_pseudo_and_mask.
> 
> gcc/testsuite/
>         * gcc.target/powerpc/20050603-3.c: Modify the dump check conditions.
>         * gcc.target/powerpc/rlwimi-2.c: Likewise.

> +/* When the outer code of set_src is IOR/XOR/PLUS and the inner code is
> +   ASHIFT/LSHIFTRT/AND, convert a psuedo to psuedo AND with a mask if its
> +   nonzero_bits is less than its mode mask.  */

Please add some words *why* we do this (namely, because you cannot use
nonzero_bits in combine as well as after combine and expect the same
answer).

> +static bool
> +change_pseudo_and_mask (rtx pat)
> +{
> +  bool changed = false;
> +
> +  rtx src = SET_SRC (pat);
> +  if ((GET_CODE (src) == IOR
> +       || GET_CODE (src) == XOR
> +       || GET_CODE (src) == PLUS)
> +      && (((GET_CODE (XEXP (src, 0)) == ASHIFT
> +           || GET_CODE (XEXP (src, 0)) == LSHIFTRT
> +           || GET_CODE (XEXP (src, 0)) == AND)
> +          && REG_P (XEXP (src, 1)))
> +         || ((GET_CODE (XEXP (src, 1)) == ASHIFT
> +              || GET_CODE (XEXP (src, 1)) == LSHIFTRT
> +              || GET_CODE (XEXP (src, 1)) == AND)
> +             && REG_P (XEXP (src, 0)))))

If one arm is a pseudo and the other is compound, the compound one is
first always.  This is one of those canonicalisations that simplifies a
lot of code -- including this new code :-)

> +    {
> +      rtx *reg = REG_P (XEXP (src, 0))
> +                ? &XEXP (SET_SRC (pat), 0)
> +                : &XEXP (SET_SRC (pat), 1);

This is indented wrong.  But, in fact, all tabs are changed to spaces in
your patch?

> @@ -11586,7 +11622,14 @@ recog_for_combine (rtx *pnewpat, rtx_insn *insn, rtx *pnotes)
>             }
>         }
>        else
> -       changed = change_zero_ext (pat);
> +       {
> +         if (change_pseudo_and_mask (pat))
> +           {
> +             maybe_swap_commutative_operands (SET_SRC (pat));
> +             changed = true;
> +           }
> +         changed |= change_zero_ext (pat);
> +       }
>      }
>    else if (GET_CODE (pat) == PARALLEL)
>      {


  changed = change_zero_ext (pat);
  if (!changed)
    changed = change_pseudo_and_mask (pat);

  if (changed)
    maybe_swap_commutative_operands (SET_SRC (pat));


> --- a/gcc/testsuite/gcc.target/powerpc/20050603-3.c
> +++ b/gcc/testsuite/gcc.target/powerpc/20050603-3.c
> @@ -12,7 +12,7 @@ void rotins (unsigned int x)
>    b.y = (x<<12) | (x>>20);
>  }
> 
> -/* { dg-final { scan-assembler-not {\mrlwinm} } } */
> +/* { dg-final { scan-assembler-not {\mrlwinm} { target ilp32 } } } */
>  /* { dg-final { scan-assembler-not {\mrldic} } } */
>  /* { dg-final { scan-assembler-not {\mrot[lr]} } } */
>  /* { dg-final { scan-assembler-not {\ms[lr][wd]} } } */

Please show the -m32 code before and after the change?  Why is it okay
to get an rlwinm there?

> diff --git a/gcc/testsuite/gcc.target/powerpc/rlwimi-2.c b/gcc/testsuite/gcc.target/powerpc/rlwimi-2.c
> index bafa371db73..ffb5f9e450f 100644
> --- a/gcc/testsuite/gcc.target/powerpc/rlwimi-2.c
> +++ b/gcc/testsuite/gcc.target/powerpc/rlwimi-2.c
> @@ -2,14 +2,14 @@
>  /* { dg-options "-O2" } */
> 
>  /* { dg-final { scan-assembler-times {(?n)^\s+[a-z]} 14121 { target ilp32 } } } */
> -/* { dg-final { scan-assembler-times {(?n)^\s+[a-z]} 20217 { target lp64 } } } */
> +/* { dg-final { scan-assembler-times {(?n)^\s+[a-z]} 21279 { target lp64 } } } */

No, it is not okay to generate worse code.  In what cases do you see
more insns now, and why?

>  /* { dg-final { scan-assembler-times {(?n)^\s+blr} 6750 } } */
>  /* { dg-final { scan-assembler-times {(?n)^\s+mr} 643 { target ilp32 } } } */
>  /* { dg-final { scan-assembler-times {(?n)^\s+mr} 11 { target lp64 } } } */
>  /* { dg-final { scan-assembler-times {(?n)^\s+rldicl} 7790 { target lp64 } } } */
> 
>  /* { dg-final { scan-assembler-times {(?n)^\s+rlwimi} 1692 { target ilp32 } } } */
> -/* { dg-final { scan-assembler-times {(?n)^\s+rlwimi} 1666 { target lp64 } } } */
> +/* { dg-final { scan-assembler-times {(?n)^\s+rlwimi} 1692 { target lp64 } } } */
> 
>  /* { dg-final { scan-assembler-times {(?n)^\s+mulli} 5036 } } */

Are the new rlwimi's good to have, or can we do those with simpler or
fewer insns?


Segher