Disallow minus in mem {+,-,&,|,^}= x; mem != 0 peepholes (PR target/85756)

Message ID 20180514124817.GF13963@laptop.zalov.cz
State New
Headers show
Series
  • Disallow minus in mem {+,-,&,|,^}= x; mem != 0 peepholes (PR target/85756)
Related show

Commit Message

Jakub Jelinek May 14, 2018, 12:48 p.m.
Hi!

The last peephole I've recently added is as the testcase shows fundamentally
incompatible with non-commutative operations, because we need to swap the
operands.

The pattern right before this one already is:
(define_peephole2
  [(parallel [(set (match_operand:SWI 0 "register_operand")
		   (match_operator:SWI 2 "plusminuslogic_operator"
		     [(match_dup 0)
		      (match_operand:SWI 1 "memory_operand")]))
	      (clobber (reg:CC FLAGS_REG))])
   (set (match_dup 1) (match_dup 0))
   (set (reg FLAGS_REG) (compare (match_dup 0) (const_int 0)))]
  "(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())
   && GET_CODE (operands[2]) != MINUS
^^^^^^^^ disallow non-commutative plusminuslogic_operator
   && peep2_reg_dead_p (3, operands[0])
   && !reg_overlap_mentioned_p (operands[0], operands[1])
   && ix86_match_ccmode (peep2_next_insn (2),
			 GET_CODE (operands[2]) == PLUS
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no need to check for MINUS here
			 ? CCGOCmode : CCNOmode)"
  [(parallel [(set (match_dup 3) (match_dup 5))
	      (set (match_dup 1) (match_dup 4))])]
{
  operands[3] = SET_DEST (PATTERN (peep2_next_insn (2)));
  operands[4]
    = gen_rtx_fmt_ee (GET_CODE (operands[2]), GET_MODE (operands[2]),
		      copy_rtx (operands[1]),
		      operands[0]);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ and here is where it swaps the operands,
^^^^^^^^^^^^^^ the memory originally in the last input is here as the first input
  operands[5]
    = gen_rtx_COMPARE (GET_MODE (operands[3]),
		       copy_rtx (operands[4]),
		       const0_rtx);
})

The following patch handles the cmpelim peephole the same.  Ok for trunk?

Unfortunately, I'm travelling and can't test it right now, Marek, do you
think you could bootstrap/regtest it for me?  Thanks.

2018-05-14  Jakub Jelinek  <jakub@redhat.com>

	PR target/85756
	* config/i386/i386.md: Disallow MINUS in last peephole for
	mem {+,-,&,|,^}= x; mem != 0 after cmpelim optimization.

	* gcc.c-torture/execute/pr85756.c: New test.


	Jakub

Comments

Uros Bizjak May 14, 2018, 12:54 p.m. | #1
On Mon, May 14, 2018 at 2:48 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> The last peephole I've recently added is as the testcase shows fundamentally
> incompatible with non-commutative operations, because we need to swap the
> operands.
>
> The pattern right before this one already is:
> (define_peephole2
>   [(parallel [(set (match_operand:SWI 0 "register_operand")
>                    (match_operator:SWI 2 "plusminuslogic_operator"
>                      [(match_dup 0)
>                       (match_operand:SWI 1 "memory_operand")]))
>               (clobber (reg:CC FLAGS_REG))])
>    (set (match_dup 1) (match_dup 0))
>    (set (reg FLAGS_REG) (compare (match_dup 0) (const_int 0)))]
>   "(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())
>    && GET_CODE (operands[2]) != MINUS
> ^^^^^^^^ disallow non-commutative plusminuslogic_operator
>    && peep2_reg_dead_p (3, operands[0])
>    && !reg_overlap_mentioned_p (operands[0], operands[1])
>    && ix86_match_ccmode (peep2_next_insn (2),
>                          GET_CODE (operands[2]) == PLUS
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no need to check for MINUS here
>                          ? CCGOCmode : CCNOmode)"
>   [(parallel [(set (match_dup 3) (match_dup 5))
>               (set (match_dup 1) (match_dup 4))])]
> {
>   operands[3] = SET_DEST (PATTERN (peep2_next_insn (2)));
>   operands[4]
>     = gen_rtx_fmt_ee (GET_CODE (operands[2]), GET_MODE (operands[2]),
>                       copy_rtx (operands[1]),
>                       operands[0]);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ and here is where it swaps the operands,
> ^^^^^^^^^^^^^^ the memory originally in the last input is here as the first input
>   operands[5]
>     = gen_rtx_COMPARE (GET_MODE (operands[3]),
>                        copy_rtx (operands[4]),
>                        const0_rtx);
> })
>
> The following patch handles the cmpelim peephole the same.  Ok for trunk?
>
> Unfortunately, I'm travelling and can't test it right now, Marek, do you
> think you could bootstrap/regtest it for me?  Thanks.
>
> 2018-05-14  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/85756
>         * config/i386/i386.md: Disallow MINUS in last peephole for
>         mem {+,-,&,|,^}= x; mem != 0 after cmpelim optimization.
>
>         * gcc.c-torture/execute/pr85756.c: New test.
>
> --- gcc/config/i386/i386.md.jj  2018-05-11 18:44:32.000000000 +0200
> +++ gcc/config/i386/i386.md     2018-05-14 13:50:28.100482520 +0200
> @@ -19397,11 +19397,11 @@
>               (set (match_dup 0) (match_dup 2))])
>     (set (match_dup 1) (match_dup 0))]
>    "(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())
> +   && GET_CODE (operands[2]) != MINUS

&& COMMUTATIVE_ARITH_P (operands[2])

Uros.
Uros Bizjak May 14, 2018, 1:06 p.m. | #2
On Mon, May 14, 2018 at 2:48 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> The last peephole I've recently added is as the testcase shows fundamentally
> incompatible with non-commutative operations, because we need to swap the
> operands.
>
> The pattern right before this one already is:
> (define_peephole2
>   [(parallel [(set (match_operand:SWI 0 "register_operand")
>                    (match_operator:SWI 2 "plusminuslogic_operator"
>                      [(match_dup 0)
>                       (match_operand:SWI 1 "memory_operand")]))
>               (clobber (reg:CC FLAGS_REG))])
>    (set (match_dup 1) (match_dup 0))
>    (set (reg FLAGS_REG) (compare (match_dup 0) (const_int 0)))]
>   "(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())
>    && GET_CODE (operands[2]) != MINUS
> ^^^^^^^^ disallow non-commutative plusminuslogic_operator
>    && peep2_reg_dead_p (3, operands[0])
>    && !reg_overlap_mentioned_p (operands[0], operands[1])
>    && ix86_match_ccmode (peep2_next_insn (2),
>                          GET_CODE (operands[2]) == PLUS
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no need to check for MINUS here
>                          ? CCGOCmode : CCNOmode)"
>   [(parallel [(set (match_dup 3) (match_dup 5))
>               (set (match_dup 1) (match_dup 4))])]
> {
>   operands[3] = SET_DEST (PATTERN (peep2_next_insn (2)));
>   operands[4]
>     = gen_rtx_fmt_ee (GET_CODE (operands[2]), GET_MODE (operands[2]),
>                       copy_rtx (operands[1]),
>                       operands[0]);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ and here is where it swaps the operands,
> ^^^^^^^^^^^^^^ the memory originally in the last input is here as the first input
>   operands[5]
>     = gen_rtx_COMPARE (GET_MODE (operands[3]),
>                        copy_rtx (operands[4]),
>                        const0_rtx);
> })
>
> The following patch handles the cmpelim peephole the same.  Ok for trunk?
>
> Unfortunately, I'm travelling and can't test it right now, Marek, do you
> think you could bootstrap/regtest it for me?  Thanks.

I'll take care of this.

Uros.
Jakub Jelinek May 14, 2018, 1:07 p.m. | #3
On Mon, May 14, 2018 at 02:54:18PM +0200, Uros Bizjak wrote:
> > --- gcc/config/i386/i386.md.jj  2018-05-11 18:44:32.000000000 +0200
> > +++ gcc/config/i386/i386.md     2018-05-14 13:50:28.100482520 +0200
> > @@ -19397,11 +19397,11 @@
> >               (set (match_dup 0) (match_dup 2))])
> >     (set (match_dup 1) (match_dup 0))]
> >    "(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())
> > +   && GET_CODE (operands[2]) != MINUS
> 
> && COMMUTATIVE_ARITH_P (operands[2])

That works, but then we should change the peephole2 above it as well.
MINUS is the only non-commutative plusminuslogic_operator, so it doesn't
change any behavior.

BTW, wonder if we shouldn't allow giving peephole2's a name, it is too hard
to refer to them...

2018-05-14  Jakub Jelinek  <jakub@redhat.com>

	PR target/85756
	* config/i386/i386.md: Disallow non-commutative arithmetics in
	last twpeephole for mem {+,-,&,|,^}= x; mem != 0 after cmpelim
	optimization.  Use COMMUTATIVE_ARITH_P test rather than != MINUS
	in the peephole2 before it.

	* gcc.c-torture/execute/pr85756.c: New test.

--- gcc/config/i386/i386.md.jj	2018-05-11 18:44:32.000000000 +0200
+++ gcc/config/i386/i386.md	2018-05-14 15:02:48.363662960 +0200
@@ -19367,7 +19367,7 @@ (define_peephole2
    (set (match_dup 1) (match_dup 0))
    (set (reg FLAGS_REG) (compare (match_dup 0) (const_int 0)))]
   "(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())
-   && GET_CODE (operands[2]) != MINUS
+   && COMMUTATIVE_ARITH_P (operands[2])
    && peep2_reg_dead_p (3, operands[0])
    && !reg_overlap_mentioned_p (operands[0], operands[1])
    && ix86_match_ccmode (peep2_next_insn (2),
@@ -19397,11 +19397,11 @@ (define_peephole2
 	      (set (match_dup 0) (match_dup 2))])
    (set (match_dup 1) (match_dup 0))]
   "(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())
+   && COMMUTATIVE_ARITH_P (operands[2])
    && peep2_reg_dead_p (2, operands[0])
    && !reg_overlap_mentioned_p (operands[0], operands[1])
    && ix86_match_ccmode (peep2_next_insn (0),
-			 (GET_CODE (operands[2]) == PLUS
-			  || GET_CODE (operands[2]) == MINUS)
+			 GET_CODE (operands[2]) == PLUS
 			 ? CCGOCmode : CCNOmode)"
   [(parallel [(set (match_dup 3) (match_dup 5))
 	      (set (match_dup 1) (match_dup 4))])]
--- gcc/testsuite/gcc.c-torture/execute/pr85756.c.jj	2018-05-14 13:50:44.384307289 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr85756.c	2018-05-14 13:48:17.000000000 +0200
@@ -0,0 +1,50 @@
+/* PR target/85756 */
+
+#if __CHAR_BIT__ == 8 && __SIZEOF_SHORT__ == 2 && __SIZEOF_INT__ == 4
+int a, c, *e, f, h = 10;
+short b;
+unsigned int p;
+
+__attribute__((noipa)) void
+bar (int a)
+{
+  asm volatile ("" : : "r" (a) : "memory");
+}
+
+void
+foo ()
+{
+  unsigned j = 1, m = 430523;
+  int k, n = 1, *l = &h;
+lab:
+  p = m;
+  m = -((~65535U | j) - n);
+  f = b << ~(n - 8);
+  n = (m || b) ^ f;
+  j = p;
+  if (p < m)
+    *l = k < 3;
+  if (!n)
+    l = &k;
+  if (c)
+    {
+      bar (a);
+      goto lab;
+    }
+  if (!*l)
+    *e = 1;
+}
+
+int
+main ()
+{
+  foo ();
+  return 0;
+}
+#else
+int
+main ()
+{
+  return 0;
+}
+#endif


	Jakub
Marek Polacek May 14, 2018, 3:08 p.m. | #4
On Mon, May 14, 2018 at 03:07:54PM +0200, Jakub Jelinek wrote:
> On Mon, May 14, 2018 at 02:54:18PM +0200, Uros Bizjak wrote:
> > > --- gcc/config/i386/i386.md.jj  2018-05-11 18:44:32.000000000 +0200
> > > +++ gcc/config/i386/i386.md     2018-05-14 13:50:28.100482520 +0200
> > > @@ -19397,11 +19397,11 @@
> > >               (set (match_dup 0) (match_dup 2))])
> > >     (set (match_dup 1) (match_dup 0))]
> > >    "(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())
> > > +   && GET_CODE (operands[2]) != MINUS
> > 
> > && COMMUTATIVE_ARITH_P (operands[2])
> 
> That works, but then we should change the peephole2 above it as well.
> MINUS is the only non-commutative plusminuslogic_operator, so it doesn't
> change any behavior.
> 
> BTW, wonder if we shouldn't allow giving peephole2's a name, it is too hard
> to refer to them...
 
It might be too late but I've regtested/bootstrapped this patch on x86_64-linux.

> 2018-05-14  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/85756
> 	* config/i386/i386.md: Disallow non-commutative arithmetics in
> 	last twpeephole for mem {+,-,&,|,^}= x; mem != 0 after cmpelim
> 	optimization.  Use COMMUTATIVE_ARITH_P test rather than != MINUS
> 	in the peephole2 before it.
> 
> 	* gcc.c-torture/execute/pr85756.c: New test.
> 
> --- gcc/config/i386/i386.md.jj	2018-05-11 18:44:32.000000000 +0200
> +++ gcc/config/i386/i386.md	2018-05-14 15:02:48.363662960 +0200
> @@ -19367,7 +19367,7 @@ (define_peephole2
>     (set (match_dup 1) (match_dup 0))
>     (set (reg FLAGS_REG) (compare (match_dup 0) (const_int 0)))]
>    "(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())
> -   && GET_CODE (operands[2]) != MINUS
> +   && COMMUTATIVE_ARITH_P (operands[2])
>     && peep2_reg_dead_p (3, operands[0])
>     && !reg_overlap_mentioned_p (operands[0], operands[1])
>     && ix86_match_ccmode (peep2_next_insn (2),
> @@ -19397,11 +19397,11 @@ (define_peephole2
>  	      (set (match_dup 0) (match_dup 2))])
>     (set (match_dup 1) (match_dup 0))]
>    "(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())
> +   && COMMUTATIVE_ARITH_P (operands[2])
>     && peep2_reg_dead_p (2, operands[0])
>     && !reg_overlap_mentioned_p (operands[0], operands[1])
>     && ix86_match_ccmode (peep2_next_insn (0),
> -			 (GET_CODE (operands[2]) == PLUS
> -			  || GET_CODE (operands[2]) == MINUS)
> +			 GET_CODE (operands[2]) == PLUS
>  			 ? CCGOCmode : CCNOmode)"
>    [(parallel [(set (match_dup 3) (match_dup 5))
>  	      (set (match_dup 1) (match_dup 4))])]
> --- gcc/testsuite/gcc.c-torture/execute/pr85756.c.jj	2018-05-14 13:50:44.384307289 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr85756.c	2018-05-14 13:48:17.000000000 +0200
> @@ -0,0 +1,50 @@
> +/* PR target/85756 */
> +
> +#if __CHAR_BIT__ == 8 && __SIZEOF_SHORT__ == 2 && __SIZEOF_INT__ == 4
> +int a, c, *e, f, h = 10;
> +short b;
> +unsigned int p;
> +
> +__attribute__((noipa)) void
> +bar (int a)
> +{
> +  asm volatile ("" : : "r" (a) : "memory");
> +}
> +
> +void
> +foo ()
> +{
> +  unsigned j = 1, m = 430523;
> +  int k, n = 1, *l = &h;
> +lab:
> +  p = m;
> +  m = -((~65535U | j) - n);
> +  f = b << ~(n - 8);
> +  n = (m || b) ^ f;
> +  j = p;
> +  if (p < m)
> +    *l = k < 3;
> +  if (!n)
> +    l = &k;
> +  if (c)
> +    {
> +      bar (a);
> +      goto lab;
> +    }
> +  if (!*l)
> +    *e = 1;
> +}
> +
> +int
> +main ()
> +{
> +  foo ();
> +  return 0;
> +}
> +#else
> +int
> +main ()
> +{
> +  return 0;
> +}
> +#endif
> 
> 
> 	Jakub

	Marek
Uros Bizjak May 14, 2018, 3:55 p.m. | #5
On Mon, May 14, 2018 at 5:08 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Mon, May 14, 2018 at 03:07:54PM +0200, Jakub Jelinek wrote:
>> On Mon, May 14, 2018 at 02:54:18PM +0200, Uros Bizjak wrote:
>> > > --- gcc/config/i386/i386.md.jj  2018-05-11 18:44:32.000000000 +0200
>> > > +++ gcc/config/i386/i386.md     2018-05-14 13:50:28.100482520 +0200
>> > > @@ -19397,11 +19397,11 @@
>> > >               (set (match_dup 0) (match_dup 2))])
>> > >     (set (match_dup 1) (match_dup 0))]
>> > >    "(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())
>> > > +   && GET_CODE (operands[2]) != MINUS
>> >
>> > && COMMUTATIVE_ARITH_P (operands[2])
>>
>> That works, but then we should change the peephole2 above it as well.
>> MINUS is the only non-commutative plusminuslogic_operator, so it doesn't
>> change any behavior.
>>
>> BTW, wonder if we shouldn't allow giving peephole2's a name, it is too hard
>> to refer to them...
>
> It might be too late but I've regtested/bootstrapped this patch on x86_64-linux.

Also works for me.

Committed to mainline SVN.

Uros.

Patch

--- gcc/config/i386/i386.md.jj	2018-05-11 18:44:32.000000000 +0200
+++ gcc/config/i386/i386.md	2018-05-14 13:50:28.100482520 +0200
@@ -19397,11 +19397,11 @@ 
 	      (set (match_dup 0) (match_dup 2))])
    (set (match_dup 1) (match_dup 0))]
   "(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())
+   && GET_CODE (operands[2]) != MINUS
    && peep2_reg_dead_p (2, operands[0])
    && !reg_overlap_mentioned_p (operands[0], operands[1])
    && ix86_match_ccmode (peep2_next_insn (0),
-			 (GET_CODE (operands[2]) == PLUS
-			  || GET_CODE (operands[2]) == MINUS)
+			 GET_CODE (operands[2]) == PLUS
 			 ? CCGOCmode : CCNOmode)"
   [(parallel [(set (match_dup 3) (match_dup 5))
 	      (set (match_dup 1) (match_dup 4))])]
--- gcc/testsuite/gcc.c-torture/execute/pr85756.c.jj	2018-05-14 13:50:44.384307289 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr85756.c	2018-05-14 13:48:17.000000000 +0200
@@ -0,0 +1,50 @@ 
+/* PR target/85756 */
+
+#if __CHAR_BIT__ == 8 && __SIZEOF_SHORT__ == 2 && __SIZEOF_INT__ == 4
+int a, c, *e, f, h = 10;
+short b;
+unsigned int p;
+
+__attribute__((noipa)) void
+bar (int a)
+{
+  asm volatile ("" : : "r" (a) : "memory");
+}
+
+void
+foo ()
+{
+  unsigned j = 1, m = 430523;
+  int k, n = 1, *l = &h;
+lab:
+  p = m;
+  m = -((~65535U | j) - n);
+  f = b << ~(n - 8);
+  n = (m || b) ^ f;
+  j = p;
+  if (p < m)
+    *l = k < 3;
+  if (!n)
+    l = &k;
+  if (c)
+    {
+      bar (a);
+      goto lab;
+    }
+  if (!*l)
+    *e = 1;
+}
+
+int
+main ()
+{
+  foo ();
+  return 0;
+}
+#else
+int
+main ()
+{
+  return 0;
+}
+#endif