[Bug target/107713] New: Wrong implementation atomic_exchange<short> on LoongArch

[Bug target/107713] New: Wrong implementation atomic_exchange<short> on LoongArch

[hejinyang at loongson dot cn]

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

            Bug ID: 107713
           Summary: Wrong implementation atomic_exchange<short> on
                    LoongArch
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: hejinyang at loongson dot cn
  Target Milestone: ---

Created attachment 53905
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53905&action=edit
preprocessed file

Target: loongarch64-unknown-linux-gnu
Configured with: ../configure --prefix=/usr --libdir=/usr/lib64
--build=x86_64-cross-linux-gnu --host=loongarch64-unknown-linux-gnu
--target=loongarch64-unknown-linux-gnu --enable-__cxa_atexit
--enable-threads=posix --with-system-zlib --enable-libstdcxx-time
--enable-checking=release --with-build-sysroot=/opt/mylaos/sysroot
--enable-default-pie --enable-languages=c,c++,fortran,objc,obj-c++,lto
gcc version 13.0.0 20220801 (experimental) (GCC) 

test_of_sync_lock_test_and_set.c:
char lock;
char hello()
{
  return __sync_lock_test_and_set (&lock, 1);
}

Cmdline:
gcc test_of_sync_lock_test_and_set.c -S --save-temps

Output Fragment:
        1:
        ll.w    $r12,$r16,0
        and     $r19,$r12,$r14
        bne     $r19,$r15,2f          <--- Wrong
        and     $r19,$r12,$r17
        or      $r19,$r19,$r18
        sc.w    $r19,$r16,0
        beq     $zero,$r19,1b
        b       3f
        2:
        dbar    0x700
        3:

[Bug target/107713] Wrong implementation atomic_exchange<short> on LoongArch

[hejinyang at loongson dot cn]

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

--- Comment #1 from Jinyang He <hejinyang at loongson dot cn> ---
Created attachment 53906
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53906&action=edit
a simple test

[Bug target/107713] Wrong implementation atomic_exchange<short> on LoongArch

[xry111 at gcc dot gnu.org]

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

Xi Ruoyao <xry111 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2022-11-16
             Target|                            |loongarch64-linux-gnu
      Known to fail|                            |12.2.0, 13.0

--- Comment #2 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
Confirmed.

[Bug target/107713] Wrong implementation atomic_exchange<short> on LoongArch

[xry111 at gcc dot gnu.org]

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

--- Comment #3 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
Created attachment 53907
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53907&action=edit
testcase

A more straightforward test (in C++).

[Bug target/107713] Wrong implementation atomic_exchange<short> on LoongArch

[xry111 at gcc dot gnu.org]

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

Xi Ruoyao <xry111 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #53907|0                           |1
        is obsolete|                            |

--- Comment #4 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
Created attachment 53908
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53908&action=edit
testcase

Ouch, uploaded a wrong file.

[Bug target/107713] Wrong implementation atomic_exchange<short> on LoongArch

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by LuluCheng <chenglulu@gcc.gnu.org>:

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

commit r13-4136-gf0024bfb228f94e60e06dc32a4983e40a9b90be5
Author: Jinyang He <hejinyang@loongson.cn>
Date:   Thu Nov 17 14:38:52 2022 +0800

    LoongArch: Fix atomic_exchange expanding [PR107713]

    We used to expand atomic_exchange_n(ptr, new, mem_order) for subword types
    into something like:

        {
          __typeof__(*ptr) t = atomic_load_n(ptr, mem_order);
          atomic_compare_exchange_n(ptr, &t, new, true, mem_order, mem_order);
          return t;
        }

    It's incorrect because another thread may store a different value into *ptr
    after atomic_load_n.  Then atomic_compare_exchange_n will not store into
    *ptr, but atomic_exchange_n should always perform the store.

    gcc/ChangeLog:

            PR target/107713
            * config/loongarch/sync.md
            (atomic_cas_value_exchange_7_<mode>): New define_insn.
            (atomic_exchange): Use atomic_cas_value_exchange_7_si instead of
            atomic_cas_value_cmp_and_7_si.

    gcc/testsuite/ChangeLog:

            PR target/107713
            * gcc.target/loongarch/pr107713-1.c: New test.
            * gcc.target/loongarch/pr107713-2.c: New test.

[Bug target/107713] Wrong implementation atomic_exchange<short> on LoongArch

[xry111 at gcc dot gnu.org]

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

--- Comment #6 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
Fixed for trunk.  Should we backport it to gcc-12 branch too?

[Bug target/107713] Wrong implementation atomic_exchange<short> on LoongArch

[chenglulu at loongson dot cn]

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

--- Comment #7 from chenglulu <chenglulu at loongson dot cn> ---
(In reply to Xi Ruoyao from comment #6)
> Fixed for trunk.  Should we backport it to gcc-12 branch too?

I don't know what the problem is, I always fail when I backport.
If it is convenient for you, could you help me backport?

[Bug target/107713] Wrong implementation atomic_exchange<short> on LoongArch

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Xi Ruoyao <xry111@gcc.gnu.org>:

https://gcc.gnu.org/g:2adcbcc69a1d5d9554042f09ec35e72bf39fb56f

commit r12-8918-g2adcbcc69a1d5d9554042f09ec35e72bf39fb56f
Author: Jinyang He <hejinyang@loongson.cn>
Date:   Thu Nov 17 14:38:52 2022 +0800

    LoongArch: Fix atomic_exchange expanding [PR107713]

    We used to expand atomic_exchange_n(ptr, new, mem_order) for subword types
    into something like:

        {
          __typeof__(*ptr) t = atomic_load_n(ptr, mem_order);
          atomic_compare_exchange_n(ptr, &t, new, true, mem_order, mem_order);
          return t;
        }

    It's incorrect because another thread may store a different value into *ptr
    after atomic_load_n.  Then atomic_compare_exchange_n will not store into
    *ptr, but atomic_exchange_n should always perform the store.

    gcc/ChangeLog:

            PR target/107713
            * config/loongarch/sync.md
            (atomic_cas_value_exchange_7_<mode>): New define_insn.
            (atomic_exchange): Use atomic_cas_value_exchange_7_si instead of
            atomic_cas_value_cmp_and_7_si.

    gcc/testsuite/ChangeLog:

            PR target/107713
            * gcc.target/loongarch/pr107713-1.c: New test.
            * gcc.target/loongarch/pr107713-2.c: New test.

    (cherry picked from commit f0024bfb228f94e60e06dc32a4983e40a9b90be5)

[Bug target/107713] Wrong implementation atomic_exchange<short> on LoongArch

[xry111 at gcc dot gnu.org]

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

Xi Ruoyao <xry111 at gcc dot gnu.org> changed:

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

--- Comment #9 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
Fixed for gcc-12 too.

[Bug target/107713] Wrong implementation atomic_exchange<short> on LoongArch

[xry111 at gcc dot gnu.org]

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

Xi Ruoyao <xry111 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.3
            Version|13.0                        |10.4.1

[Bug target/107713] Wrong implementation atomic_exchange<short> on LoongArch

[chenglulu at loongson dot cn]

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

--- Comment #10 from chenglulu <chenglulu at loongson dot cn> ---
(In reply to Xi Ruoyao from comment #9)
> Fixed for gcc-12 too.

Thanks! ^v^