[Bug target/96189] New: Failure to use eflags from cmpxchg on x86

[Bug target/96189] New: Failure to use eflags from cmpxchg on x86

[gabravier at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96189

            Bug ID: 96189
           Summary: Failure to use eflags from cmpxchg on x86
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: gabravier at gmail dot com
  Target Milestone: ---

bool f(unsigned char* addr, unsigned char old_val, unsigned char new_val)
{
    auto old_val_cpy = old_val;
    __atomic_compare_exchange_n(addr, &old_val, new_val, 0, 0, 0);
    return old_val == old_val_cpy;
}

With -O3, LLVM outputs this :

f(unsigned char*, unsigned char, unsigned char): # @f(unsigned char*, unsigned
char, unsigned char)
  mov eax, esi
  lock cmpxchg byte ptr [rdi], dl
  sete al
  ret

GCC outputs this :

f(unsigned char*, unsigned char, unsigned char):
  mov eax, esi
  movzx edx, dl
  lock cmpxchg BYTE PTR [rdi], dl
  cmp al, sil
  sete al
  ret

GCC could use the EFLAGS generated from cmpxchg, but it does not.

[Bug target/96189] Failure to use eflags from cmpxchg on x86

[gabravier at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96189

--- Comment #1 from Gabriel Ravier <gabravier at gmail dot com> ---
PS : The extraneous movzx is already reported at
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96176

[Bug target/96189] Failure to use eflags from cmpxchg on x86

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96189

Uroš Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2020-07-15
     Ever confirmed|0                           |1
           Assignee|unassigned at gcc dot gnu.org      |ubizjak at gmail dot com

--- Comment #2 from Uroš Bizjak <ubizjak at gmail dot com> ---
Created attachment 48877
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48877&action=edit
Prototype patch

Introduce peephole2 pattern that removes the comparison in certain cases.
Doubleword cmpxchg is not handled, the doubleword comparison sequence is just
too complicated in this late stage of compilation.

[Bug target/96189] Failure to use eflags from cmpxchg on x86

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96189

--- Comment #3 from Uroš Bizjak <ubizjak at gmail dot com> ---
The master branch has been updated by Uros Bizjak <uros@gcc.gnu.org>:

https://gcc.gnu.org/g:6c2848ad02feef5ac094d1158be3861819b3bb49

commit r11-2140-g6c2848ad02feef5ac094d1158be3861819b3bb49
Author: Uros Bizjak <ubizjak@gmail.com>
Date:   Wed Jul 15 21:27:00 2020 +0200

    i386: Introduce peephole2 to use flags from CMPXCHG more [PR96189]

    CMPXCHG instruction sets ZF flag if the values in the destination operand
    and EAX register are equal; otherwise the ZF flag is cleared and value
    from destination operand is loaded to EAX. Following assembly:

            movl    %esi, %eax
            lock cmpxchgl   %edx, (%rdi)
            cmpl    %esi, %eax
            sete    %al

    can be optimized by removing the unneeded comparison, since set ZF flag
    signals that no update to EAX happened.

    2020-15-07  Uroš Bizjak  <ubizjak@gmail.com>

    gcc/ChangeLog:
            PR target/95355
            * config/i386/sync.md
            (peephole2 to remove unneded compare after CMPXCHG): New pattern.

    gcc/testsuite/ChangeLog:
            PR target/95355
            * gcc.target/i386/pr96189.c: New test.

[Bug target/96189] Failure to use eflags from cmpxchg on x86

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96189

Uroš Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
   Target Milestone|---                         |11.0
             Status|ASSIGNED                    |RESOLVED

--- Comment #4 from Uroš Bizjak <ubizjak at gmail dot com> ---
Implemented for gcc-11.

[Bug target/96189] Failure to use eflags from cmpxchg on x86

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96189

Uroš Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org
             Status|RESOLVED                    |REOPENED
         Resolution|FIXED                       |---

--- Comment #5 from Uroš Bizjak <ubizjak at gmail dot com> ---
Hm...

Please note that peephole2 scanning require exact RTL sequences, and already
fails for e.g.:

_Bool
foo (unsigned int *x, unsigned int z)
{
  unsigned int y = 0;
  __atomic_compare_exchange_n (x, &y, z, 0, __ATOMIC_RELAXED,
__ATOMIC_RELAXED);
  return y == 0;
}

(which is used in a couple of places throughout glibc), due to early peephole2
optimization that converts:

(insn 7 4 8 2 (set (reg:SI 0 ax [90])
        (const_int 0 [0])) "cmpx0.c":5:3 75 {*movsi_internal}

to:

(insn 31 4 8 2 (parallel [
            (set (reg:DI 0 ax [90])
                (const_int 0 [0]))
            (clobber (reg:CC 17 flags))

Other than that, the required sequence is broken quite often by various
reloads, due to the complexity of CMPXCHG insn.

However, __atomic_compare_exchange_n returns a boolean value that is exactly
what the first function is testing, so the following two functions are
equivalent:

--cut here--
_Bool
foo (unsigned int *x, unsigned int y, unsigned int z)
{
  unsigned int old_y = y;
  __atomic_compare_exchange_n (x, &y, z, 0, __ATOMIC_RELAXED,
__ATOMIC_RELAXED);
  return y == old_y;
}

_Bool
bar (unsigned int *x, unsigned int y, unsigned int z)
{
  return __atomic_compare_exchange_n (x, &y, z, 0, __ATOMIC_RELAXED,
__ATOMIC_RELAXED);
}
--cut here--

I wonder, if the above transformation can happen on the tree level, so it would
apply universally for all targets, and would also handle CMPXCHG[8,16]B
doubleword instructions on x86 targets.

Let's ask experts.

[Bug target/96189] Failure to use eflags from cmpxchg on x86

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96189

--- Comment #6 from rguenther at suse dot de <rguenther at suse dot de> ---
On July 16, 2020 9:05:52 AM GMT+02:00, ubizjak at gmail dot com
<gcc-bugzilla@gcc.gnu.org> wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96189
>
>Uroš Bizjak <ubizjak at gmail dot com> changed:
>
>           What    |Removed                     |Added
>----------------------------------------------------------------------------
>              CC|                            |jakub at gcc dot gnu.org,
>               |                            |rguenth at gcc dot gnu.org
>             Status|RESOLVED                    |REOPENED
>         Resolution|FIXED                       |---
>
>--- Comment #5 from Uroš Bizjak <ubizjak at gmail dot com> ---
>Hm...
>
>Please note that peephole2 scanning require exact RTL sequences, and
>already
>fails for e.g.:
>
>_Bool
>foo (unsigned int *x, unsigned int z)
>{
>  unsigned int y = 0;
>  __atomic_compare_exchange_n (x, &y, z, 0, __ATOMIC_RELAXED,
>__ATOMIC_RELAXED);
>  return y == 0;
>}
>
>(which is used in a couple of places throughout glibc), due to early
>peephole2
>optimization that converts:
>
>(insn 7 4 8 2 (set (reg:SI 0 ax [90])
>        (const_int 0 [0])) "cmpx0.c":5:3 75 {*movsi_internal}
>
>to:
>
>(insn 31 4 8 2 (parallel [
>            (set (reg:DI 0 ax [90])
>                (const_int 0 [0]))
>            (clobber (reg:CC 17 flags))
>
>Other than that, the required sequence is broken quite often by various
>reloads, due to the complexity of CMPXCHG insn.
>
>However, __atomic_compare_exchange_n returns a boolean value that is
>exactly
>what the first function is testing, so the following two functions are
>equivalent:
>
>--cut here--
>_Bool
>foo (unsigned int *x, unsigned int y, unsigned int z)
>{
>  unsigned int old_y = y;
>  __atomic_compare_exchange_n (x, &y, z, 0, __ATOMIC_RELAXED,
>__ATOMIC_RELAXED);
>  return y == old_y;
>}
>
>_Bool
>bar (unsigned int *x, unsigned int y, unsigned int z)
>{
>  return __atomic_compare_exchange_n (x, &y, z, 0, __ATOMIC_RELAXED,
>__ATOMIC_RELAXED);
>}
>--cut here--
>
>I wonder, if the above transformation can happen on the tree level, so
>it would
>apply universally for all targets, and would also handle CMPXCHG[8,16]B
>doubleword instructions on x86 targets.
>
>Let's ask experts.

In principle value numbering can make the comparison available at the cmpxchg
and replace the later comparison. We've pondered with this trick for memcpy
results for example. 

Richard.

[Bug target/96189] Failure to use eflags from cmpxchg on x86

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96189

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Uros Bizjak <uros@gcc.gnu.org>:

https://gcc.gnu.org/g:cc1ef413a859433a8313fa9c15aaff41bdc837dc

commit r11-2181-gcc1ef413a859433a8313fa9c15aaff41bdc837dc
Author: Uros Bizjak <ubizjak@gmail.com>
Date:   Thu Jul 16 20:11:43 2020 +0200

    i386: Additional peephole2 to use flags from CMPXCHG more [PR96189]

    CMPXCHG instruction sets ZF flag if the values in the destination operand
    and EAX register are equal; otherwise the ZF flag is cleared and value
    from destination operand is loaded to EAX. Following assembly:

            xorl    %eax, %eax
            lock cmpxchgl   %edx, (%rdi)
            testl   %eax, %eax
            sete    %al

    can be optimized by removing the unneeded comparison, since set ZF flag
    signals that no update to EAX happened.  This patch adds peephole2
    pattern to also handle XOR zeroing and load of -1 by OR.

    2020-07-16  Uroš Bizjak  <ubizjak@gmail.com>

    gcc/ChangeLog:
            PR target/96189
            * config/i386/sync.md
            (peephole2 to remove unneded compare after CMPXCHG):
            New pattern, also handle XOR zeroing and load of -1 by OR.

    gcc/testsuite/ChangeLog:
            PR target/96189
            * gcc.target/i386/pr96189-1.c: New test.

[Bug target/96189] Failure to use eflags from cmpxchg on x86

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96189

Uroš Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Uroš Bizjak <ubizjak at gmail dot com> ---
Fixed again.