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