Fix x86 setcc + movzbl peephole2s (PR target/86314)

Message ID 20180626105223.GO7166@tucnak
State New
Headers show
Series
  • Fix x86 setcc + movzbl peephole2s (PR target/86314)
Related show

Commit Message

Jakub Jelinek June 26, 2018, 10:52 a.m.
Hi!

These peephole2s assume that when matching insns like:
  [(parallel [(set (reg FLAGS_REG) (match_operand 0))
              (match_operand 4)])
that operands[4] must be a set of a register with operands[0]
as SET_SRC, but that isn't the case e.g. for:
(insn 9 8 10 2 (parallel [
            (set (reg:CCC 17 flags)
                (compare:CCC (unspec_volatile:DI [
                            (mem/v:DI (reg/v/f:DI 5 di [orig:94 p ] [94]) [-1  S8 A64])
                            (const_int 5 [0x5])
                        ] UNSPECV_XCHG)
                    (const_int 0 [0])))
            (set (zero_extract:DI (mem/v:DI (reg/v/f:DI 5 di [orig:94 p ] [94]) [-1  S8 A64])
                    (const_int 1 [0x1])
                    (reg:DI 0 ax [96]))
                (const_int 1 [0x1]))
        ]) "pr86314.C":7 5268 {atomic_bit_test_and_setdi_1}
     (expr_list:REG_DEAD (reg/v/f:DI 5 di [orig:94 p ] [94])
        (expr_list:REG_DEAD (reg:DI 0 ax [96])
            (nil))))

As we want to clear operands[3] before this instruction, we need to
verify that it isn't used in operands[0] (we check that) and that
operands[4] does not set it (also checked), but also need to check that
operands[4] doesn't use it (which we don't do).  In the usual case
when SET_DEST of operands[4] is a REG and SET_SRC is operands[0] it
makes no difference, but if SET_DEST sets something that uses operands[3]
in it, or if SET_SRC is different from operands[0] and uses operands[3],
then this results in a wrong peephole2 transformation.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk and release branches?

2018-06-26  Jakub Jelinek  <jakub@redhat.com>

	PR target/86314
	* config/i386/i386.md (setcc + movzbl to xor + setcc peephole2s):
	Check reg_overlap_mentioned_p in addition to reg_set_p with the same
	operands.

	* gcc.dg/pr86314.c: New test.


	Jakub

Comments

Uros Bizjak June 26, 2018, 11:03 a.m. | #1
On Tue, Jun 26, 2018 at 12:52 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!

>

> These peephole2s assume that when matching insns like:

>   [(parallel [(set (reg FLAGS_REG) (match_operand 0))

>               (match_operand 4)])

> that operands[4] must be a set of a register with operands[0]

> as SET_SRC, but that isn't the case e.g. for:

> (insn 9 8 10 2 (parallel [

>             (set (reg:CCC 17 flags)

>                 (compare:CCC (unspec_volatile:DI [

>                             (mem/v:DI (reg/v/f:DI 5 di [orig:94 p ] [94]) [-1  S8 A64])

>                             (const_int 5 [0x5])

>                         ] UNSPECV_XCHG)

>                     (const_int 0 [0])))

>             (set (zero_extract:DI (mem/v:DI (reg/v/f:DI 5 di [orig:94 p ] [94]) [-1  S8 A64])

>                     (const_int 1 [0x1])

>                     (reg:DI 0 ax [96]))

>                 (const_int 1 [0x1]))

>         ]) "pr86314.C":7 5268 {atomic_bit_test_and_setdi_1}

>      (expr_list:REG_DEAD (reg/v/f:DI 5 di [orig:94 p ] [94])

>         (expr_list:REG_DEAD (reg:DI 0 ax [96])

>             (nil))))

>

> As we want to clear operands[3] before this instruction, we need to

> verify that it isn't used in operands[0] (we check that) and that

> operands[4] does not set it (also checked), but also need to check that

> operands[4] doesn't use it (which we don't do).  In the usual case

> when SET_DEST of operands[4] is a REG and SET_SRC is operands[0] it

> makes no difference, but if SET_DEST sets something that uses operands[3]

> in it, or if SET_SRC is different from operands[0] and uses operands[3],

> then this results in a wrong peephole2 transformation.

>

> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for

> trunk and release branches?

>

> 2018-06-26  Jakub Jelinek  <jakub@redhat.com>

>

>         PR target/86314

>         * config/i386/i386.md (setcc + movzbl to xor + setcc peephole2s):

>         Check reg_overlap_mentioned_p in addition to reg_set_p with the same

>         operands.

>

>         * gcc.dg/pr86314.c: New test.


OK everywhere.

Thanks,
Uros.

> --- gcc/config/i386/i386.md.jj  2018-06-25 14:51:24.878990827 +0200

> +++ gcc/config/i386/i386.md     2018-06-26 10:01:43.042907384 +0200

> @@ -12761,6 +12761,7 @@ (define_peephole2

>    "(peep2_reg_dead_p (3, operands[1])

>      || operands_match_p (operands[1], operands[3]))

>     && ! reg_overlap_mentioned_p (operands[3], operands[0])

> +   && ! reg_overlap_mentioned_p (operands[3], operands[4])

>     && ! reg_set_p (operands[3], operands[4])

>     && peep2_regno_dead_p (0, FLAGS_REG)"

>    [(parallel [(set (match_dup 5) (match_dup 0))

> @@ -12786,6 +12787,7 @@ (define_peephole2

>      || operands_match_p (operands[2], operands[4]))

>     && ! reg_overlap_mentioned_p (operands[4], operands[0])

>     && ! reg_overlap_mentioned_p (operands[4], operands[1])

> +   && ! reg_overlap_mentioned_p (operands[4], operands[5])

>     && ! reg_set_p (operands[4], operands[5])

>     && refers_to_regno_p (FLAGS_REG, operands[1], (rtx *)NULL)

>     && peep2_regno_dead_p (0, FLAGS_REG)"

> @@ -12835,6 +12837,7 @@ (define_peephole2

>    "(peep2_reg_dead_p (3, operands[1])

>      || operands_match_p (operands[1], operands[3]))

>     && ! reg_overlap_mentioned_p (operands[3], operands[0])

> +   && ! reg_overlap_mentioned_p (operands[3], operands[4])

>     && ! reg_set_p (operands[3], operands[4])

>     && peep2_regno_dead_p (0, FLAGS_REG)"

>    [(parallel [(set (match_dup 5) (match_dup 0))

> @@ -12861,6 +12864,7 @@ (define_peephole2

>      || operands_match_p (operands[2], operands[4]))

>     && ! reg_overlap_mentioned_p (operands[4], operands[0])

>     && ! reg_overlap_mentioned_p (operands[4], operands[1])

> +   && ! reg_overlap_mentioned_p (operands[4], operands[5])

>     && ! reg_set_p (operands[4], operands[5])

>     && refers_to_regno_p (FLAGS_REG, operands[1], (rtx *)NULL)

>     && peep2_regno_dead_p (0, FLAGS_REG)"

> --- gcc/testsuite/gcc.dg/pr86314.c.jj   2018-06-26 10:25:35.190160477 +0200

> +++ gcc/testsuite/gcc.dg/pr86314.c      2018-06-26 10:24:42.310077331 +0200

> @@ -0,0 +1,20 @@

> +// PR target/86314

> +// { dg-do run { target sync_int_long } }

> +// { dg-options "-O2" }

> +

> +__attribute__((noinline, noclone)) unsigned long

> +foo (unsigned long *p)

> +{

> +  unsigned long m = 1UL << ((*p & 1) ? 1 : 0);

> +  unsigned long n = __atomic_fetch_or (p, m, __ATOMIC_SEQ_CST);

> +  return (n & m) == 0;

> +}

> +

> +int

> +main ()

> +{

> +  unsigned long v = 1;

> +  if (foo (&v) != 1)

> +    __builtin_abort ();

> +  return 0;

> +}

>

>         Jakub

Patch

--- gcc/config/i386/i386.md.jj	2018-06-25 14:51:24.878990827 +0200
+++ gcc/config/i386/i386.md	2018-06-26 10:01:43.042907384 +0200
@@ -12761,6 +12761,7 @@  (define_peephole2
   "(peep2_reg_dead_p (3, operands[1])
     || operands_match_p (operands[1], operands[3]))
    && ! reg_overlap_mentioned_p (operands[3], operands[0])
+   && ! reg_overlap_mentioned_p (operands[3], operands[4])
    && ! reg_set_p (operands[3], operands[4])
    && peep2_regno_dead_p (0, FLAGS_REG)"
   [(parallel [(set (match_dup 5) (match_dup 0))
@@ -12786,6 +12787,7 @@  (define_peephole2
     || operands_match_p (operands[2], operands[4]))
    && ! reg_overlap_mentioned_p (operands[4], operands[0])
    && ! reg_overlap_mentioned_p (operands[4], operands[1])
+   && ! reg_overlap_mentioned_p (operands[4], operands[5])
    && ! reg_set_p (operands[4], operands[5])
    && refers_to_regno_p (FLAGS_REG, operands[1], (rtx *)NULL)
    && peep2_regno_dead_p (0, FLAGS_REG)"
@@ -12835,6 +12837,7 @@  (define_peephole2
   "(peep2_reg_dead_p (3, operands[1])
     || operands_match_p (operands[1], operands[3]))
    && ! reg_overlap_mentioned_p (operands[3], operands[0])
+   && ! reg_overlap_mentioned_p (operands[3], operands[4])
    && ! reg_set_p (operands[3], operands[4])
    && peep2_regno_dead_p (0, FLAGS_REG)"
   [(parallel [(set (match_dup 5) (match_dup 0))
@@ -12861,6 +12864,7 @@  (define_peephole2
     || operands_match_p (operands[2], operands[4]))
    && ! reg_overlap_mentioned_p (operands[4], operands[0])
    && ! reg_overlap_mentioned_p (operands[4], operands[1])
+   && ! reg_overlap_mentioned_p (operands[4], operands[5])
    && ! reg_set_p (operands[4], operands[5])
    && refers_to_regno_p (FLAGS_REG, operands[1], (rtx *)NULL)
    && peep2_regno_dead_p (0, FLAGS_REG)"
--- gcc/testsuite/gcc.dg/pr86314.c.jj	2018-06-26 10:25:35.190160477 +0200
+++ gcc/testsuite/gcc.dg/pr86314.c	2018-06-26 10:24:42.310077331 +0200
@@ -0,0 +1,20 @@ 
+// PR target/86314
+// { dg-do run { target sync_int_long } }
+// { dg-options "-O2" }
+
+__attribute__((noinline, noclone)) unsigned long
+foo (unsigned long *p)
+{
+  unsigned long m = 1UL << ((*p & 1) ? 1 : 0);
+  unsigned long n = __atomic_fetch_or (p, m, __ATOMIC_SEQ_CST);
+  return (n & m) == 0;
+}
+
+int
+main ()
+{
+  unsigned long v = 1;
+  if (foo (&v) != 1)
+    __builtin_abort ();
+  return 0;
+}