diff mbox series

[v4,rs6000] Add a combine pattern for CA minus one [PR95737]

Message ID 243c7199-e6a5-f04b-568c-64024590695f@linux.ibm.com
State New
Headers show
Series [v4,rs6000] Add a combine pattern for CA minus one [PR95737] | expand

Commit Message

HAO CHEN GUI May 13, 2022, 1:07 a.m. UTC 
Hi,
   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.

   Bootstrapped and tested on powerpc64-linux BE and LE with no regressions.
Is this okay for trunk? Any recommendations? Thanks a lot.

ChangeLog
2022-05-13 Haochen Gui <guihaoc@linux.ibm.com>

gcc/
	PR target/95737
	* config/rs6000/rs6000.md (extenddi_ca_minus_one): Define.

gcc/testsuite/
	PR target/95737
	* gcc.target/powerpc/pr95737.c: New.

patch.diff

Comments

Kewen.Lin May 13, 2022, 2:40 a.m. UTC | #1 
Hi Haochen,

on 2022/5/13 09:07, HAO CHEN GUI wrote:
> Hi,
>    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.
> 
>    Bootstrapped and tested on powerpc64-linux BE and LE with no regressions.
> Is this okay for trunk? Any recommendations? Thanks a lot.
> 
> ChangeLog
> 2022-05-13 Haochen Gui <guihaoc@linux.ibm.com>
> 
> gcc/
> 	PR target/95737
> 	* config/rs6000/rs6000.md (extenddi_ca_minus_one): Define.
> 

Nit: (*extenddi_ca_minus_one): New define_insn_and_split.

> gcc/testsuite/
> 	PR target/95737
> 	* gcc.target/powerpc/pr95737.c: New.
> 
> patch.diff
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 64049a6e521..483a93956f8 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -2353,6 +2353,19 @@ (define_insn "subf<mode>3_carry_in_xx"
>    "subfe %0,%0,%0"
>    [(set_attr "type" "add")])
> 
> +(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))])]
> +  ""
> +)
> 
>  (define_insn "@neg<mode>2"
>    [(set (match_operand:GPR 0 "gpc_reg_operand" "=r")
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr95737.c b/gcc/testsuite/gcc.target/powerpc/pr95737.c
> new file mode 100644
> index 00000000000..d4d6a4198cf
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr95737.c
> @@ -0,0 +1,10 @@
> +/* PR target/95737 */
> +/* { dg-do compile { target lp64 } } */> +/* { dg-options "-O2 -mno-isel" } */

Nit: It seems good to put one comment for the reason why we need this
special -mno-isel, like something: Power9 leverages isel for this case,
force -mno-isel to keep the test point valid on Power9 and later."

> +/* { dg-final { scan-assembler-not {\mextsw\M} } } */
> +
> +
> +unsigned long negativeLessThan (unsigned long a, unsigned long b)
> +{
> +   return -(a < b);
> +}

OK with those nits fixed, but please wait for a few days, just in case
Segher/David have more comments. 

BR,
Kewen
Segher Boessenkool May 13, 2022, 1:21 p.m. UTC | #2 
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
Segher Boessenkool May 13, 2022, 1:32 p.m. UTC | #3 
Hi!

On Fri, May 13, 2022 at 10:40:22AM +0800, Kewen.Lin wrote:
> on 2022/5/13 09:07, HAO CHEN GUI wrote:
> > 	* config/rs6000/rs6000.md (extenddi_ca_minus_one): Define.
> 
> Nit: (*extenddi_ca_minus_one): New define_insn_and_split.

Or just "New." even :-)  It's boring, yes, but boring is good.  It helps
to be more verbose sometimes, certainly, but it isn't required here.

Changelog entries are free-form in any case, after the colons that is :-)

> > +/* { dg-options "-O2 -mno-isel" } */
> 
> Nit: It seems good to put one comment for the reason why we need this
> special -mno-isel, like something: Power9 leverages isel for this case,
> force -mno-isel to keep the test point valid on Power9 and later."

But in fewer words if you can :-)  Even "/* for p9 and later */" would
be very helpful already.  It's not necessary to explain everything, but
giving some leads to whoever gets to investigate this testcase next is
useful :-)


Segher
diff mbox series

Patch

diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index 64049a6e521..483a93956f8 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -2353,6 +2353,19 @@  (define_insn "subf<mode>3_carry_in_xx"
   "subfe %0,%0,%0"
   [(set_attr "type" "add")])

+(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))])]
+  ""
+)

 (define_insn "@neg<mode>2"
   [(set (match_operand:GPR 0 "gpc_reg_operand" "=r")
diff --git a/gcc/testsuite/gcc.target/powerpc/pr95737.c b/gcc/testsuite/gcc.target/powerpc/pr95737.c
new file mode 100644
index 00000000000..d4d6a4198cf
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr95737.c
@@ -0,0 +1,10 @@ 
+/* PR target/95737 */
+/* { dg-do compile { target lp64 } } */
+/* { dg-options "-O2 -mno-isel" } */
+/* { dg-final { scan-assembler-not {\mextsw\M} } } */
+
+
+unsigned long negativeLessThan (unsigned long a, unsigned long b)
+{
+   return -(a < b);
+}