Hi!

On Fri, May 13, 2022 at 09:07:54AM +0800, HAO CHEN GUI wrote:
>    This patch adds a combine pattern for "CA minus one". As CA only has two
> values (0 or 1), we could convert following pattern
>       (sign_extend:DI (plus:SI (reg:SI 98 ca)
>                 (const_int -1 [0xffffffffffffffff]))))
> to
>        (plus:DI (reg:DI 98 ca)
>             (const_int -1 [0xffffffffffffffff])))
>    With this patch, one unnecessary sign extend is eliminated.

> +(define_insn_and_split "*extenddi_ca_minus_one"
> +  [(set (match_operand:DI 0 "gpc_reg_operand" "=r")
> +     (sign_extend:DI (plus:SI (reg:SI CA_REGNO)
> +                              (const_int -1))))]
> +  ""
> +  "#"
> +  ""
> +  [(parallel [(set (match_dup 0)
> +                (plus:DI (reg:DI CA_REGNO)
> +                         (const_int -1)))
> +           (clobber (reg:DI CA_REGNO))])]
> +  ""
> +)

This is the subf<mode>3_carry_in_xx pattern but with an extend, so a
better name (well, more like existing names :-) ) would be
*subfsi3_carry_in_xx_64.  You already put it right after the more basic
pattern, which would have been my next suggestion :-)

It needs TARGET_POWERPC64 in the insn condition.  Without it, DImode
does exist, but it stands for two registers then.  Very unlikely to
ever match the RTL of course, but it's much cleaner to not risk it even.

It would be better to teach simplify-rtx how to do this, but it will
have problems understanding that CA can only be 0 or 1.  Okay.

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr95737.c
> @@ -0,0 +1,10 @@
> +/* PR target/95737 */
> +/* { dg-do compile { target lp64 } } */

This testcase will work fine on 32 bit.  Of course there is no extsw
insn generated then no matter what, but it simplifies the testcase, and
it gives a bit more test coverage anyway.  In general, don't restrict
testcases to only be tested for some systems, unless there is a reason
for that (and if that reason isn't obvious, make a short note in the
testcase itself:
/* { dg-do compile { target lp64 } } */
/* This requires lp64 because of XYZ.  */
or similar).

So please add that TARGET_POWERPC64 and remove the lp64 from the
testcase.  Oh and the pattern name.  Looks perfect to me then.  Thanks,


Segher

Reply via email to