i386: Fix wrong codegen for -mrelax-cmpxchg-loop

Message ID 20211118073726.21865-1-hongyu.wang@intel.com
State Committed
Commit 15f5e70cbb33b40c97325ef9d55557747a148d39
Headers
Series i386: Fix wrong codegen for -mrelax-cmpxchg-loop |

Commit Message

Hongyu Wang Nov. 18, 2021, 7:37 a.m. UTC
  Hi Uros,

For -mrelax-cmpxchg-loop introduced by PR 103069/r12-5265, it would
produce infinite loop. The correct code should be

.L84:
        movl    (%rdi), %ecx
        movl    %eax, %edx
        orl     %esi, %edx
        cmpl    %eax, %ecx
        jne     .L82
        lock cmpxchgl   %edx, (%rdi)
        jne     .L84
	movl    %r8d, %eax  <<< retval is missing in previous impl
	ret
.L82:
        rep nop
        jmp     .L84

Adjust corresponding expander to fix such issue, and fix runtime test
so the problem would be exposed.

Bootstrapped/regtested on x86_64-pc-linux-gnu{-m32,}.
Ok for master?

gcc/ChangeLog:

	* config/i386/i386-expand.c (ix86_expand_atomic_fetch_op_loop):
	Adjust generated cfg to avoid infinite loop.

gcc/testsuite/ChangeLog:

	* gcc.target/i386/pr103069-2.c: Adjust.
---
 gcc/config/i386/i386-expand.c              |  7 ++++++-
 gcc/testsuite/gcc.target/i386/pr103069-2.c | 11 ++++++-----
 2 files changed, 12 insertions(+), 6 deletions(-)
  

Comments

Uros Bizjak Nov. 18, 2021, 8:21 a.m. UTC | #1
On Thu, Nov 18, 2021 at 8:37 AM Hongyu Wang <hongyu.wang@intel.com> wrote:
>
> Hi Uros,
>
> For -mrelax-cmpxchg-loop introduced by PR 103069/r12-5265, it would
> produce infinite loop. The correct code should be
>
> .L84:
>         movl    (%rdi), %ecx
>         movl    %eax, %edx
>         orl     %esi, %edx
>         cmpl    %eax, %ecx
>         jne     .L82
>         lock cmpxchgl   %edx, (%rdi)
>         jne     .L84
>         movl    %r8d, %eax  <<< retval is missing in previous impl
>         ret
> .L82:
>         rep nop
>         jmp     .L84
>
> Adjust corresponding expander to fix such issue, and fix runtime test
> so the problem would be exposed.
>
> Bootstrapped/regtested on x86_64-pc-linux-gnu{-m32,}.
> Ok for master?
>
> gcc/ChangeLog:
>
>         * config/i386/i386-expand.c (ix86_expand_atomic_fetch_op_loop):
>         Adjust generated cfg to avoid infinite loop.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/i386/pr103069-2.c: Adjust.

LGTM.

Thanks,
Uros.

> ---
>  gcc/config/i386/i386-expand.c              |  7 ++++++-
>  gcc/testsuite/gcc.target/i386/pr103069-2.c | 11 ++++++-----
>  2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/config/i386/i386-expand.c b/gcc/config/i386/i386-expand.c
> index 3e4de64ec24..0d5d1a0e205 100644
> --- a/gcc/config/i386/i386-expand.c
> +++ b/gcc/config/i386/i386-expand.c
> @@ -23143,13 +23143,14 @@ void ix86_expand_atomic_fetch_op_loop (rtx target, rtx mem, rtx val,
>                                        bool doubleword)
>  {
>    rtx old_reg, new_reg, old_mem, success, oldval, new_mem;
> -  rtx_code_label *loop_label, *pause_label;
> +  rtx_code_label *loop_label, *pause_label, *done_label;
>    machine_mode mode = GET_MODE (target);
>
>    old_reg = gen_reg_rtx (mode);
>    new_reg = old_reg;
>    loop_label = gen_label_rtx ();
>    pause_label = gen_label_rtx ();
> +  done_label = gen_label_rtx ();
>    old_mem = copy_to_reg (mem);
>    emit_label (loop_label);
>    emit_move_insn (old_reg, old_mem);
> @@ -23207,11 +23208,15 @@ void ix86_expand_atomic_fetch_op_loop (rtx target, rtx mem, rtx val,
>                            GET_MODE (success), 1, loop_label,
>                            profile_probability::guessed_never ());
>
> +  emit_jump_insn (gen_jump (done_label));
> +  emit_barrier ();
> +
>    /* If mem is not expected, pause and loop back.  */
>    emit_label (pause_label);
>    emit_insn (gen_pause ());
>    emit_jump_insn (gen_jump (loop_label));
>    emit_barrier ();
> +  emit_label (done_label);
>  }
>
>  #include "gt-i386-expand.h"
> diff --git a/gcc/testsuite/gcc.target/i386/pr103069-2.c b/gcc/testsuite/gcc.target/i386/pr103069-2.c
> index 8ac824cc8e8..b3f2235fd55 100644
> --- a/gcc/testsuite/gcc.target/i386/pr103069-2.c
> +++ b/gcc/testsuite/gcc.target/i386/pr103069-2.c
> @@ -1,5 +1,5 @@
> -/* PR target/103068 */
> -/* { dg-do compile } */
> +/* PR target/103069 */
> +/* { dg-do run } */
>  /* { dg-additional-options "-O2 -march=x86-64 -mtune=generic" } */
>
>  #include <stdlib.h>
> @@ -37,13 +37,14 @@ FUNC_ATOMIC_RELAX (char, xor)
>  #define TEST_ATOMIC_FETCH_LOGIC(TYPE, OP) \
>  { \
>    TYPE a = 11, b = 101, res, exp; \
> +  TYPE c = 11, d = 101;        \
>    res = relax_##TYPE##_##OP##_fetch (&a, b); \
> -  exp = f_##TYPE##_##OP##_fetch (&a, b);  \
> +  exp = f_##TYPE##_##OP##_fetch (&c, d);  \
>    if (res != exp) \
>      abort (); \
> -  a = 21, b = 92; \
> +  a = c = 21, b = d = 92; \
>    res = relax_##TYPE##_fetch_##OP (&a, b); \
> -  exp = f_##TYPE##_fetch_##OP (&a, b);  \
> +  exp = f_##TYPE##_fetch_##OP (&c, d);  \
>    if (res != exp) \
>      abort (); \
>  }
> --
> 2.18.1
>
  

Patch

diff --git a/gcc/config/i386/i386-expand.c b/gcc/config/i386/i386-expand.c
index 3e4de64ec24..0d5d1a0e205 100644
--- a/gcc/config/i386/i386-expand.c
+++ b/gcc/config/i386/i386-expand.c
@@ -23143,13 +23143,14 @@  void ix86_expand_atomic_fetch_op_loop (rtx target, rtx mem, rtx val,
 				       bool doubleword)
 {
   rtx old_reg, new_reg, old_mem, success, oldval, new_mem;
-  rtx_code_label *loop_label, *pause_label;
+  rtx_code_label *loop_label, *pause_label, *done_label;
   machine_mode mode = GET_MODE (target);
 
   old_reg = gen_reg_rtx (mode);
   new_reg = old_reg;
   loop_label = gen_label_rtx ();
   pause_label = gen_label_rtx ();
+  done_label = gen_label_rtx ();
   old_mem = copy_to_reg (mem);
   emit_label (loop_label);
   emit_move_insn (old_reg, old_mem);
@@ -23207,11 +23208,15 @@  void ix86_expand_atomic_fetch_op_loop (rtx target, rtx mem, rtx val,
 			   GET_MODE (success), 1, loop_label,
 			   profile_probability::guessed_never ());
 
+  emit_jump_insn (gen_jump (done_label));
+  emit_barrier ();
+
   /* If mem is not expected, pause and loop back.  */
   emit_label (pause_label);
   emit_insn (gen_pause ());
   emit_jump_insn (gen_jump (loop_label));
   emit_barrier ();
+  emit_label (done_label);
 }
 
 #include "gt-i386-expand.h"
diff --git a/gcc/testsuite/gcc.target/i386/pr103069-2.c b/gcc/testsuite/gcc.target/i386/pr103069-2.c
index 8ac824cc8e8..b3f2235fd55 100644
--- a/gcc/testsuite/gcc.target/i386/pr103069-2.c
+++ b/gcc/testsuite/gcc.target/i386/pr103069-2.c
@@ -1,5 +1,5 @@ 
-/* PR target/103068 */
-/* { dg-do compile } */
+/* PR target/103069 */
+/* { dg-do run } */
 /* { dg-additional-options "-O2 -march=x86-64 -mtune=generic" } */ 
 
 #include <stdlib.h>
@@ -37,13 +37,14 @@  FUNC_ATOMIC_RELAX (char, xor)
 #define TEST_ATOMIC_FETCH_LOGIC(TYPE, OP) \
 { \
   TYPE a = 11, b = 101, res, exp; \
+  TYPE c = 11, d = 101;	\
   res = relax_##TYPE##_##OP##_fetch (&a, b); \
-  exp = f_##TYPE##_##OP##_fetch (&a, b);  \
+  exp = f_##TYPE##_##OP##_fetch (&c, d);  \
   if (res != exp) \
     abort (); \
-  a = 21, b = 92; \
+  a = c = 21, b = d = 92; \
   res = relax_##TYPE##_fetch_##OP (&a, b); \
-  exp = f_##TYPE##_fetch_##OP (&a, b);  \
+  exp = f_##TYPE##_fetch_##OP (&c, d);  \
   if (res != exp) \
     abort (); \
 }