public inbox for libc-ports@sourceware.org
 help / color / mirror / Atom feed
* [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
@ 2013-08-13 17:11 Dinar Temirbulatov
  2013-08-15 21:46 ` Abhishek Deb
  2013-08-18 20:29 ` Joseph S. Myers
  0 siblings, 2 replies; 21+ messages in thread
From: Dinar Temirbulatov @ 2013-08-13 17:11 UTC (permalink / raw)
  To: libc-ports; +Cc: joseph, adeb

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

Hello,
Following patch redefines atomic_exchange_acq/atomic_exchange_rel to
__atomic_exchange_n for ARM, that allows for example to reduce number
of instruction sequence for lll_unlock
from:
ldex, cmp, bne, stex, cmp, bne
to
ldex, stex, cmp, bne
, more on the issue here:
http://sourceware.org/bugzilla/show_bug.cgi?id=15640

This patch was tested on ARM a9 with glibc testsuite with no new
regressions. OK to commit?

                       Thanks, Dinar.

[-- Attachment #2: arm_atomic.patch --]
[-- Type: application/octet-stream, Size: 1131 bytes --]

diff --git a/ports/sysdeps/arm/bits/atomic.h b/ports/sysdeps/arm/bits/atomic.h
index 6e20df7..78d4a6e 100644
--- a/ports/sysdeps/arm/bits/atomic.h
+++ b/ports/sysdeps/arm/bits/atomic.h
@@ -79,3 +79,26 @@ void __arm_link_error (void);
 # define __arm_assisted_compare_and_exchange_val_32_acq(mem, newval, oldval) \
   ({ __arm_link_error (); oldval; })
 #endif
+
+#if __GNUC_PREREQ (4, 7)
+# define atomic_exchange_acq(mem, value)                                \
+  __atomic_val_bysize (__arch_exchange, int, mem, value, __ATOMIC_ACQUIRE)
+
+# define atomic_exchange_rel(mem, value)                                \
+  __atomic_val_bysize (__arch_exchange, int, mem, value, __ATOMIC_RELEASE)
+
+/* Atomic exchange (without compare).  */
+
+# define __arch_exchange_8_int(mem, newval, model)      \
+  (abort (), (typeof(*mem)) 0)
+
+# define __arch_exchange_16_int(mem, newval, model)     \
+  (abort (), (typeof(*mem)) 0)
+
+# define __arch_exchange_32_int(mem, newval, model)     \
+  __atomic_exchange_n (mem, newval, model)
+
+# define __arch_exchange_64_int(mem, newval, model)     \
+  (abort (), (typeof(*mem)) 0)
+
+#endif

^ permalink raw reply	[flat|nested] 21+ messages in thread

* RE: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-08-13 17:11 [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n Dinar Temirbulatov
@ 2013-08-15 21:46 ` Abhishek Deb
  2013-08-16 22:57   ` Dinar Temirbulatov
  2013-08-18 20:29 ` Joseph S. Myers
  1 sibling, 1 reply; 21+ messages in thread
From: Abhishek Deb @ 2013-08-15 21:46 UTC (permalink / raw)
  To: Dinar Temirbulatov, libc-ports; +Cc: joseph

Dinar,

Apologies for delayed response, as I was on vacation. Your changes look in line with the MIPS changes. Since GCC 4.7. onwards _atomic builtins are provided, it's the right thing to do. I have not tested your changes, but I don't see anything obviously wrong. 

Following is how the compiled code looks like, when compiled with gcc 4.8

Atomic_exchange_rel : 

  c8:   f57ff05f        dmb     sy
  cc:   e1952f9f        ldrex   r2, [r5]
  d0:   e1851f93        strex   r1, r3, [r5]
  d4:   e3510000        cmp     r1, #0
  d8:   1afffffb        bne     cc 

Atomic_exchange_acq :

  94:   e1953f9f        ldrex   r3, [r5]
  98:   e1852f97        strex   r2, r7, [r5]
  9c:   e3520000        cmp     r2, #0
  a0:   1afffffb        bne     94 
  a4:   e3530000        cmp     r3, #0
  a8:   130f6c18        movwne  r6, #64536      ; 0xfc18
  ac:   f57ff05f        dmb     sy

So these changes look correct.

However, taking a deeper look into atomic_compare_and_exchange_acq/rel, following is how the compiled code looks like

  34:   f57ff05f        dmb     sy
  38:   e1903f9f        ldrex   r3, [r0]
  3c:   e3530000        cmp     r3, #0
  40:   1a000002        bne     50 
  44:   e1801f92        strex   r1, r2, [r0]
  48:   e3510000        cmp     r1, #0
  4c:   1afffff9        bne     38 
  50:   e3530000        cmp     r3, #0
  54:   f57ff05f        dmb     sy
  58:   1afffff5        bne     34 

As you can see it contains two memory barrier one in the beginning and one in the end. Is it because gcc's _sync_* builtin was used? The _atomic_* builtins provides users to specify memodel for each of the builtin functions. Hence, shouldn't _atomic_ builtin with appropriate memmodel be used to do an atomic_compare_and_exchange_acq/rel?

In other words for a acquire semantic the first dmb is redundant and for a release semantic the second one is redundant. I don't know what do you think.

Abhishek


-----Original Message-----
From: Dinar Temirbulatov [mailto:dinar@kugelworks.com] 
Sent: Tuesday, August 13, 2013 10:11 AM
To: libc-ports@sourceware.org
Cc: joseph@codesourcery.com; Abhishek Deb
Subject: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Hello,
Following patch redefines atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n for ARM, that allows for example to reduce number of instruction sequence for lll_unlock
from:
ldex, cmp, bne, stex, cmp, bne
to
ldex, stex, cmp, bne
, more on the issue here:
http://sourceware.org/bugzilla/show_bug.cgi?id=15640

This patch was tested on ARM a9 with glibc testsuite with no new regressions. OK to commit?

                       Thanks, Dinar.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-08-15 21:46 ` Abhishek Deb
@ 2013-08-16 22:57   ` Dinar Temirbulatov
  2013-08-17  2:42     ` Abhishek Deb
  0 siblings, 1 reply; 21+ messages in thread
From: Dinar Temirbulatov @ 2013-08-16 22:57 UTC (permalink / raw)
  To: Abhishek Deb; +Cc: libc-ports, joseph

Hello Abhishek,

>However, taking a deeper look into atomic_compare_and_exchange_acq/rel, following is how the compiled code looks like
>
>  34:   f57ff05f        dmb     sy
>  38:   e1903f9f        ldrex   r3, [r0]
>  3c:   e3530000        cmp     r3, #0
>  40:   1a000002        bne     50
>  44:   e1801f92        strex   r1, r2, [r0]
>  48:   e3510000        cmp     r1, #0
>  4c:   1afffff9        bne     38
>  50:   e3530000        cmp     r3, #0
>  54:   f57ff05f        dmb     sy
>  58:   1afffff5        bne     34
oh, I see that lll_unlock looks correct on my side:
.LBB12:
        .loc 1 42 0
        dmb     sy
.L21:
        ldrex   r2, [r4]
        strex   r1, r3, [r4]
        cmp     r1, #0
        bne     .L21
.LVL10:

and for example __lll_cond_lock/__lll_timedlock also look right.

But, I see that for  lll_lock defined as :
#define __lll_lock(futex, private)                                            \
  ((void) ({                                                                  \
    int *__futex = (futex);                                                   \
    if (__builtin_expect (atomic_compare_and_exchange_val_acq (__futex,       \
                                                                1, 0), 0))    \
      {                                                                       \
        if (__builtin_constant_p (private) && (private) == LLL_PRIVATE)       \
          __lll_lock_wait_private (__futex);                                  \
        else                                                                  \
          __lll_lock_wait (__futex, private);                                 \
      }                                                                       \
  }))

and we could see that on instruction level it looks like this:
        dmb     sy
.L91:
        ldrex   r0, [r4]
        cmp     r0, r3
        bne     .L92
        strex   r1, r7, [r4]
        cmp     r1, #0
        bne     .L91
.L92:
.LBE10:
        .loc 1 204 0
        cmp     r3, r0
.LBB11:
        .loc 1 202 0
        dmb     sy

, so nothing wrong on my side.
              thanks, Dinar.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* RE: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-08-16 22:57   ` Dinar Temirbulatov
@ 2013-08-17  2:42     ` Abhishek Deb
  2013-08-19 19:21       ` Dinar Temirbulatov
  0 siblings, 1 reply; 21+ messages in thread
From: Abhishek Deb @ 2013-08-17  2:42 UTC (permalink / raw)
  To: Dinar Temirbulatov; +Cc: libc-ports, joseph

Dinar,

As I said currently __arch_compare_and_exchange_val_32_acq is defined to __sync_val_compare_and_swap ((mem), (oldval), (newval)), which probably uses two dmb. 
Can't __atomic_compare_exchange_n be used and appropriate memodel be specified for acquire and release variants?

-----Original Message-----
From: Dinar Temirbulatov [mailto:dinar@kugelworks.com] 
Sent: Friday, August 16, 2013 3:58 PM
To: Abhishek Deb
Cc: libc-ports@sourceware.org; joseph@codesourcery.com
Subject: Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Hello Abhishek,

>However, taking a deeper look into atomic_compare_and_exchange_acq/rel, following is how the compiled code looks like
>
>  34:   f57ff05f        dmb     sy
>  38:   e1903f9f        ldrex   r3, [r0]
>  3c:   e3530000        cmp     r3, #0
>  40:   1a000002        bne     50
>  44:   e1801f92        strex   r1, r2, [r0]
>  48:   e3510000        cmp     r1, #0
>  4c:   1afffff9        bne     38
>  50:   e3530000        cmp     r3, #0
>  54:   f57ff05f        dmb     sy
>  58:   1afffff5        bne     34
oh, I see that lll_unlock looks correct on my side:
.LBB12:
        .loc 1 42 0
        dmb     sy
.L21:
        ldrex   r2, [r4]
        strex   r1, r3, [r4]
        cmp     r1, #0
        bne     .L21
.LVL10:

and for example __lll_cond_lock/__lll_timedlock also look right.

But, I see that for  lll_lock defined as :
#define __lll_lock(futex, private)                                            \
  ((void) ({                                                                  \
    int *__futex = (futex);                                                   \
    if (__builtin_expect (atomic_compare_and_exchange_val_acq (__futex,       \
                                                                1, 0), 0))    \
      {                                                                       \
        if (__builtin_constant_p (private) && (private) == LLL_PRIVATE)       \
          __lll_lock_wait_private (__futex);                                  \
        else                                                                  \
          __lll_lock_wait (__futex, private);                                 \
      }                                                                       \
  }))

and we could see that on instruction level it looks like this:
        dmb     sy
.L91:
        ldrex   r0, [r4]
        cmp     r0, r3
        bne     .L92
        strex   r1, r7, [r4]
        cmp     r1, #0
        bne     .L91
.L92:
.LBE10:
        .loc 1 204 0
        cmp     r3, r0
.LBB11:
        .loc 1 202 0
        dmb     sy

, so nothing wrong on my side.
              thanks, Dinar.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-08-13 17:11 [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n Dinar Temirbulatov
  2013-08-15 21:46 ` Abhishek Deb
@ 2013-08-18 20:29 ` Joseph S. Myers
  1 sibling, 0 replies; 21+ messages in thread
From: Joseph S. Myers @ 2013-08-18 20:29 UTC (permalink / raw)
  To: Dinar Temirbulatov; +Cc: libc-ports, adeb

On Tue, 13 Aug 2013, Dinar Temirbulatov wrote:

> Hello,
> Following patch redefines atomic_exchange_acq/atomic_exchange_rel to
> __atomic_exchange_n for ARM, that allows for example to reduce number
> of instruction sequence for lll_unlock
> from:
> ldex, cmp, bne, stex, cmp, bne
> to
> ldex, stex, cmp, bne
> , more on the issue here:
> http://sourceware.org/bugzilla/show_bug.cgi?id=15640
> 
> This patch was tested on ARM a9 with glibc testsuite with no new
> regressions. OK to commit?

What does this do when building for an ARM architecture version where the 
kernel helpers need to be used for atomicity?

I think what's desired in such a case is for the operations to continue to 
be expanded with inline calls to the kernel helpers.  Generating 
out-of-line calls to libgcc helpers would be less desirable (and 
out-of-line calls to anything not in libgcc.a would break the build).

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-08-17  2:42     ` Abhishek Deb
@ 2013-08-19 19:21       ` Dinar Temirbulatov
  0 siblings, 0 replies; 21+ messages in thread
From: Dinar Temirbulatov @ 2013-08-19 19:21 UTC (permalink / raw)
  To: Abhishek Deb; +Cc: libc-ports, joseph

Abhishek,

>As I said currently __arch_compare_and_exchange_val_32_acq is defined to __sync_val_compare_and_swap ((mem), (oldval), (newval)), which probably uses two dmb.
>Can't __atomic_compare_exchange_n be used and appropriate memodel be specified for acquire and release variants?
Yes, Looks like that makes sense.
                   thanks, Dinar.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-09-19  6:53                 ` Maxim Kuvyrkov
@ 2013-09-19 16:29                   ` Joseph S. Myers
  0 siblings, 0 replies; 21+ messages in thread
From: Joseph S. Myers @ 2013-09-19 16:29 UTC (permalink / raw)
  To: Maxim Kuvyrkov; +Cc: Dinar Temirbulatov, Abhishek Deb, libc-ports

Note for future reference to use git commit --author= when committing 
patches for someone else.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-09-18 17:16               ` Joseph S. Myers
@ 2013-09-19  6:53                 ` Maxim Kuvyrkov
  2013-09-19 16:29                   ` Joseph S. Myers
  0 siblings, 1 reply; 21+ messages in thread
From: Maxim Kuvyrkov @ 2013-09-19  6:53 UTC (permalink / raw)
  To: Joseph S. Myers; +Cc: Dinar Temirbulatov, Abhishek Deb, libc-ports

On 19/09/2013, at 5:16 AM, Joseph S. Myers <joseph@codesourcery.com> wrote:

> On Sat, 14 Sep 2013, Dinar Temirbulatov wrote:
> 
>> Hello Joseph,
>> Oh, I don't have any write permissions to the project.
> 
> Please provide a GNU-format ChangeLog entry (as detailed in the 
> contribution checklist) for the patch (probably using [BZ #15640], as I 
> understand it).

I've checked in Dinar's patch with appropriate ChangeLog and closed the bug.

Thanks,

--
Maxim Kuvyrkov
www.kugelworks.com

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-09-14  3:29             ` Dinar Temirbulatov
@ 2013-09-18 17:16               ` Joseph S. Myers
  2013-09-19  6:53                 ` Maxim Kuvyrkov
  0 siblings, 1 reply; 21+ messages in thread
From: Joseph S. Myers @ 2013-09-18 17:16 UTC (permalink / raw)
  To: Dinar Temirbulatov; +Cc: Abhishek Deb, libc-ports, Maxim Kuvyrkov

On Sat, 14 Sep 2013, Dinar Temirbulatov wrote:

> Hello Joseph,
> Oh, I don't have any write permissions to the project.

Please provide a GNU-format ChangeLog entry (as detailed in the 
contribution checklist) for the patch (probably using [BZ #15640], as I 
understand it).

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-09-13 17:14           ` Joseph S. Myers
@ 2013-09-14  3:29             ` Dinar Temirbulatov
  2013-09-18 17:16               ` Joseph S. Myers
  0 siblings, 1 reply; 21+ messages in thread
From: Dinar Temirbulatov @ 2013-09-14  3:29 UTC (permalink / raw)
  To: Joseph S. Myers; +Cc: Abhishek Deb, libc-ports, Maxim Kuvyrkov

Hello Joseph,
Oh, I don't have any write permissions to the project.
               thanks, Dinar.

On Fri, Sep 13, 2013 at 9:14 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Wed, 11 Sep 2013, Dinar Temirbulatov wrote:
>
>> Hi,
>> Another updated version of the change with just eliminated $. Tested
>> glibc internal testsuite with: 1) arm v4t gcc-4.5.3 with
>> __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 undefined (kernel helpers only)
>> with no new regrssions, 2) arm with FSF gcc-4.6.4 on  armv7-a with
>> __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 defined with no new regrssions, 3)
>> arm FSF gcc-4.7.3 on armv7-a with __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
>> with no new regressions. Ok to commit?
>
> This version is OK.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-09-11 19:27         ` Dinar Temirbulatov
  2013-09-11 20:40           ` Abhishek Deb
@ 2013-09-13 17:14           ` Joseph S. Myers
  2013-09-14  3:29             ` Dinar Temirbulatov
  1 sibling, 1 reply; 21+ messages in thread
From: Joseph S. Myers @ 2013-09-13 17:14 UTC (permalink / raw)
  To: Dinar Temirbulatov; +Cc: Abhishek Deb, libc-ports, Maxim Kuvyrkov

On Wed, 11 Sep 2013, Dinar Temirbulatov wrote:

> Hi,
> Another updated version of the change with just eliminated $. Tested
> glibc internal testsuite with: 1) arm v4t gcc-4.5.3 with
> __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 undefined (kernel helpers only)
> with no new regrssions, 2) arm with FSF gcc-4.6.4 on  armv7-a with
> __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 defined with no new regrssions, 3)
> arm FSF gcc-4.7.3 on armv7-a with __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
> with no new regressions. Ok to commit?

This version is OK.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 21+ messages in thread

* RE: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-09-11 19:27         ` Dinar Temirbulatov
@ 2013-09-11 20:40           ` Abhishek Deb
  2013-09-13 17:14           ` Joseph S. Myers
  1 sibling, 0 replies; 21+ messages in thread
From: Abhishek Deb @ 2013-09-11 20:40 UTC (permalink / raw)
  To: Dinar Temirbulatov; +Cc: Joseph Myers, libc-ports, Maxim Kuvyrkov

Compilation OK and the generated code OK.

OK from my side.

Abhishek

-----Original Message-----
From: Dinar Temirbulatov [mailto:dtemirbulatov@gmail.com] 
Sent: Wednesday, September 11, 2013 12:28 PM
To: Abhishek Deb
Cc: Joseph Myers; libc-ports@sourceware.org; Maxim Kuvyrkov
Subject: Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Hi,
Another updated version of the change with just eliminated $. Tested glibc internal testsuite with: 1) arm v4t gcc-4.5.3 with
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 undefined (kernel helpers only) with no new regrssions, 2) arm with FSF gcc-4.6.4 on  armv7-a with
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 defined with no new regrssions, 3) arm FSF gcc-4.7.3 on armv7-a with __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
with no new regressions. Ok to commit?
                          thanks, Dinar.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-09-05 17:09       ` Abhishek Deb
  2013-09-11 19:25         ` Dinar Temirbulatov
@ 2013-09-11 19:27         ` Dinar Temirbulatov
  2013-09-11 20:40           ` Abhishek Deb
  2013-09-13 17:14           ` Joseph S. Myers
  1 sibling, 2 replies; 21+ messages in thread
From: Dinar Temirbulatov @ 2013-09-11 19:27 UTC (permalink / raw)
  To: Abhishek Deb; +Cc: Joseph Myers, libc-ports, Maxim Kuvyrkov

[-- Attachment #1: Type: text/plain, Size: 483 bytes --]

Hi,
Another updated version of the change with just eliminated $. Tested
glibc internal testsuite with: 1) arm v4t gcc-4.5.3 with
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 undefined (kernel helpers only)
with no new regrssions, 2) arm with FSF gcc-4.6.4 on  armv7-a with
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 defined with no new regrssions, 3)
arm FSF gcc-4.7.3 on armv7-a with __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
with no new regressions. Ok to commit?
                          thanks, Dinar.

[-- Attachment #2: arm_atomic8.patch --]
[-- Type: application/octet-stream, Size: 5430 bytes --]

--- glibc-orig/ports/sysdeps/arm/bits/atomic.h	2013-08-19 21:46:44.592731210 +0400
+++ glibc/ports/sysdeps/arm/bits/atomic.h	2013-09-05 23:16:18.336235291 +0400
@@ -35,9 +35,6 @@ typedef uintmax_t uatomic_max_t;
 
 void __arm_link_error (void);
 
-/* Use the atomic builtins provided by GCC in case the backend provides
-   a pattern to do this efficiently.  */
-
 #ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
 # define atomic_full_barrier() __sync_synchronize ()
 #else
@@ -51,9 +48,88 @@ void __arm_link_error (void);
 # define __arm_assisted_full_barrier()  __arm_link_error()
 #endif
 
-/* Atomic compare and exchange.  */
+/* Use the atomic builtins provided by GCC in case the backend provides
+   a pattern to do this efficiently.  */
+#if __GNUC_PREREQ (4, 7) && defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
 
-#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
+# define atomic_exchange_acq(mem, value)                                \
+  __atomic_val_bysize (__arch_exchange, int, mem, value, __ATOMIC_ACQUIRE)
+
+# define atomic_exchange_rel(mem, value)                                \
+  __atomic_val_bysize (__arch_exchange, int, mem, value, __ATOMIC_RELEASE)
+
+/* Atomic exchange (without compare).  */
+
+# define __arch_exchange_8_int(mem, newval, model)      \
+  (__arm_link_error (), (typeof (*mem)) 0)
+
+# define __arch_exchange_16_int(mem, newval, model)     \
+  (__arm_link_error (), (typeof (*mem)) 0)
+
+# define __arch_exchange_32_int(mem, newval, model)     \
+  __atomic_exchange_n (mem, newval, model)
+
+# define __arch_exchange_64_int(mem, newval, model)     \
+  (__arm_link_error (), (typeof (*mem)) 0)
+
+/* Compare and exchange with "acquire" semantics, ie barrier after.  */
+
+# define atomic_compare_and_exchange_bool_acq(mem, new, old)    \
+  __atomic_bool_bysize (__arch_compare_and_exchange_bool, int,  \
+                        mem, new, old, __ATOMIC_ACQUIRE)
+
+# define atomic_compare_and_exchange_val_acq(mem, new, old)     \
+  __atomic_val_bysize (__arch_compare_and_exchange_val, int,    \
+                       mem, new, old, __ATOMIC_ACQUIRE)
+
+/* Compare and exchange with "release" semantics, ie barrier before.  */
+
+# define atomic_compare_and_exchange_bool_rel(mem, new, old)    \
+  __atomic_bool_bysize (__arch_compare_and_exchange_bool, int,  \
+                        mem, new, old, __ATOMIC_RELEASE)
+
+# define atomic_compare_and_exchange_val_rel(mem, new, old)      \
+  __atomic_val_bysize (__arch_compare_and_exchange_val, int,    \
+                       mem, new, old, __ATOMIC_RELEASE)
+
+/* Compare and exchange.
+   For all "bool" routines, we return FALSE if exchange succesful.  */
+
+# define __arch_compare_and_exchange_bool_8_int(mem, newval, oldval, model) \
+  ({__arm_link_error (); oldval; })
+
+# define __arch_compare_and_exchange_bool_16_int(mem, newval, oldval, model) \
+  ({__arm_link_error (); oldval; })
+
+# define __arch_compare_and_exchange_bool_32_int(mem, newval, oldval, model) \
+  ({                                                                    \
+    typeof (*mem) __oldval = (oldval);                                  \
+    !__atomic_compare_exchange_n (mem, (void *) &__oldval, newval, 0,   \
+                                  model, __ATOMIC_RELAXED);             \
+  })
+
+# define __arch_compare_and_exchange_bool_64_int(mem, newval, oldval, model) \
+  ({__arm_link_error (); oldval; })
+
+# define __arch_compare_and_exchange_val_8_int(mem, newval, oldval, model) \
+  ({__arm_link_error (); oldval; })
+
+# define __arch_compare_and_exchange_val_16_int(mem, newval, oldval, model) \
+  ({__arm_link_error (); oldval; })
+
+# define __arch_compare_and_exchange_val_32_int(mem, newval, oldval, model) \
+  ({                                                                    \
+    typeof (*mem) __oldval = (oldval);                                  \
+    __atomic_compare_exchange_n (mem, (void *) &__oldval, newval, 0,    \
+                                 model, __ATOMIC_RELAXED);              \
+    __oldval;                                                           \
+  })
+
+# define __arch_compare_and_exchange_val_64_int(mem, newval, oldval, model) \
+  ({__arm_link_error (); oldval; })
+
+#elif defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
+/* Atomic compare and exchange.  */
 # define __arch_compare_and_exchange_val_32_acq(mem, newval, oldval) \
   __sync_val_compare_and_swap ((mem), (oldval), (newval))
 #else
@@ -61,16 +137,18 @@ void __arm_link_error (void);
   __arm_assisted_compare_and_exchange_val_32_acq ((mem), (newval), (oldval))
 #endif
 
+#if !__GNUC_PREREQ (4, 7) || !defined (__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)
 /* We don't support atomic operations on any non-word types.
    So make them link errors.  */
-#define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \
+# define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \
   ({ __arm_link_error (); oldval; })
 
-#define __arch_compare_and_exchange_val_16_acq(mem, newval, oldval) \
+# define __arch_compare_and_exchange_val_16_acq(mem, newval, oldval) \
   ({ __arm_link_error (); oldval; })
 
-#define __arch_compare_and_exchange_val_64_acq(mem, newval, oldval) \
+# define __arch_compare_and_exchange_val_64_acq(mem, newval, oldval) \
   ({ __arm_link_error (); oldval; })
+#endif
 
 /* An OS-specific bits/atomic.h file will define this macro if
    the OS can provide something.  If not, we'll fail to build

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-09-05 17:09       ` Abhishek Deb
@ 2013-09-11 19:25         ` Dinar Temirbulatov
  2013-09-11 19:27         ` Dinar Temirbulatov
  1 sibling, 0 replies; 21+ messages in thread
From: Dinar Temirbulatov @ 2013-09-11 19:25 UTC (permalink / raw)
  To: Abhishek Deb; +Cc: Joseph Myers, libc-ports, Maxim Kuvyrkov

Hi,
Another updated version of the change with just eliminated $. Tested
glibc internal testsuite with: 1) arm v4t gcc-4.5.3 with
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 undefined (kernel helpers only)
with no new regrssions, 2) arm with FSF gcc-4.6.4 on  armv7-a with
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 defined with no new regrssions, 3)
arm FSF gcc-4.7.3 on armv7-a with __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
with no new regressions. Ok to commit?
                          thanks, Dinar.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* RE: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-09-05 15:48     ` Joseph S. Myers
@ 2013-09-05 17:09       ` Abhishek Deb
  2013-09-11 19:25         ` Dinar Temirbulatov
  2013-09-11 19:27         ` Dinar Temirbulatov
  0 siblings, 2 replies; 21+ messages in thread
From: Abhishek Deb @ 2013-09-05 17:09 UTC (permalink / raw)
  To: Joseph Myers, Dinar Temirbulatov; +Cc: libc-ports, Maxim Kuvyrkov

Compilation with 4.8.0 succeeded, whereas compilation with 4.6.2 failed due to the trailing $ as pointed by Joseph.

Abhishek

-----Original Message-----
From: Joseph Myers [mailto:joseph@codesourcery.com] 
Sent: Thursday, September 05, 2013 8:48 AM
To: Dinar Temirbulatov
Cc: libc-ports@sourceware.org; Abhishek Deb; Maxim Kuvyrkov
Subject: Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

On Thu, 5 Sep 2013, Dinar Temirbulatov wrote:

> Hi,
> Here is updated version of change. Ok to commit?

You appear to insert a '$' after '\' on one line - that can't be right.  
Please try to test all three cases in the code.

Where you have "typeof(*mem)", there should be a space before the '('.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-09-05 12:56   ` Dinar Temirbulatov
@ 2013-09-05 15:48     ` Joseph S. Myers
  2013-09-05 17:09       ` Abhishek Deb
  0 siblings, 1 reply; 21+ messages in thread
From: Joseph S. Myers @ 2013-09-05 15:48 UTC (permalink / raw)
  To: Dinar Temirbulatov; +Cc: libc-ports, adeb, Maxim Kuvyrkov

On Thu, 5 Sep 2013, Dinar Temirbulatov wrote:

> Hi,
> Here is updated version of change. Ok to commit?

You appear to insert a '$' after '\' on one line - that can't be right.  
Please try to test all three cases in the code.

Where you have "typeof(*mem)", there should be a space before the '('.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-09-02 15:08 ` Joseph S. Myers
  2013-09-03 17:31   ` Abhishek Deb
@ 2013-09-05 12:56   ` Dinar Temirbulatov
  2013-09-05 15:48     ` Joseph S. Myers
  1 sibling, 1 reply; 21+ messages in thread
From: Dinar Temirbulatov @ 2013-09-05 12:56 UTC (permalink / raw)
  To: Joseph S. Myers; +Cc: libc-ports, adeb, Maxim Kuvyrkov

[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]

Hi,
Here is updated version of change. Ok to commit?
                thanks, Dinar.

On Mon, Sep 2, 2013 at 7:08 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Mon, 2 Sep 2013, Dinar Temirbulatov wrote:
>
>> Hi,
>> Another version of change, I added
>> atomic_compare_and_exchange_val_acq/atomic_compare_and_exchange_val_rel
>> and atomic_compare_and_exchange_bool_acq/atomic_compare_and_exchange_boot_rel
>> defenitions and also for gcc-4.7 and higher in the case of unsupported
>> atomic compare and swap operation, it uses the kernel helper inlines.
>> Tested on arm a9 with no new regressions. Ok to commit?
>> Oh, sorry. I missed to attach the change. Here it is.
>
> For subsequent patch revisions, please note there should be an extra space
> after "#" for preprocessor directives inside #if conditionals, one per
> level of #if nesting (other than toplevel multiple-inclusion guards) -
> which means that if conditioning existing code, you need to adjust
> directives inside that code.
>
> This patch appears to have too much duplication.  For example, you
> duplicate the definition of __arm_assisted_compare_and_exchange_val_32_acq
> - but that should not need any extra conditionals at all (beyond the
> existing #ifndef __arm_assisted_compare_and_exchange_val_32_acq), there's
> no reason ever not to define it.  Similarly, you duplicate
> __arch_compare_and_exchange_val_64_acq, but with proper #if structure
> there should only need to be one copy of the version that uses
> __arm_link_error.
>
> What I think you should aim for is that each definition, or small group of
> definitions, uses conditionals in the form
>
> #if __GNUC_PREREQ (4, 7) && defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
> /* Version using __atomic_*.  */
> #elif defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
> /* Version using __sync_*.  */
> #else
> /* Version using __arm_assisted_*.  */
> #endif
>
> with cases omitted if not useful for that particular macro (this may
> include some macros not being defined at all in some cases).  So don't
> insert any global conditionals affecting all the existing definitions at
> all - look at each block of conditionals and add a third case as needed,
> along with any new macros (again with conditionals in that form) that are
> appropriate.
>
> Where you use abort () in some definitions, use __arm_link_error ()
> instead, like for the existing definitions.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

[-- Attachment #2: arm_atomic7.patch --]
[-- Type: application/octet-stream, Size: 5558 bytes --]

--- glibc-orig/ports/sysdeps/arm/bits/atomic.h	2013-08-19 21:46:44.592731210 +0400
+++ glibc/ports/sysdeps/arm/bits/atomic.h	2013-09-05 15:53:52.752801514 +0400
@@ -35,9 +35,6 @@ typedef uintmax_t uatomic_max_t;
 
 void __arm_link_error (void);
 
-/* Use the atomic builtins provided by GCC in case the backend provides
-   a pattern to do this efficiently.  */
-
 #ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
 # define atomic_full_barrier() __sync_synchronize ()
 #else
@@ -51,26 +48,107 @@ void __arm_link_error (void);
 # define __arm_assisted_full_barrier()  __arm_link_error()
 #endif
 
-/* Atomic compare and exchange.  */
+/* Use the atomic builtins provided by GCC in case the backend provides
+   a pattern to do this efficiently.  */
+#if __GNUC_PREREQ (4, 7) && defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
 
-#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
-# define __arch_compare_and_exchange_val_32_acq(mem, newval, oldval) \
+# define atomic_exchange_acq(mem, value)                                \
+  __atomic_val_bysize (__arch_exchange, int, mem, value, __ATOMIC_ACQUIRE)
+
+# define atomic_exchange_rel(mem, value)                                \
+  __atomic_val_bysize (__arch_exchange, int, mem, value, __ATOMIC_RELEASE)
+
+/* Atomic exchange (without compare).  */
+
+# define __arch_exchange_8_int(mem, newval, model)      \
+  (__arm_link_error (), (typeof(*mem)) 0)
+
+# define __arch_exchange_16_int(mem, newval, model)     \
+  (__arm_link_error (), (typeof(*mem)) 0)
+
+# define __arch_exchange_32_int(mem, newval, model)     \
+  __atomic_exchange_n (mem, newval, model)
+
+# define __arch_exchange_64_int(mem, newval, model)     \
+  (__arm_link_error (), (typeof(*mem)) 0)
+
+/* Compare and exchange with "acquire" semantics, ie barrier after.  */
+
+# define atomic_compare_and_exchange_bool_acq(mem, new, old)    \
+  __atomic_bool_bysize (__arch_compare_and_exchange_bool, int,  \
+                        mem, new, old, __ATOMIC_ACQUIRE)
+
+# define atomic_compare_and_exchange_val_acq(mem, new, old)     \
+  __atomic_val_bysize (__arch_compare_and_exchange_val, int,    \
+                       mem, new, old, __ATOMIC_ACQUIRE)
+
+/* Compare and exchange with "release" semantics, ie barrier before.  */
+
+# define atomic_compare_and_exchange_bool_rel(mem, new, old)    \
+  __atomic_bool_bysize (__arch_compare_and_exchange_bool, int,  \
+                        mem, new, old, __ATOMIC_RELEASE)
+
+# define atomic_compare_and_exchange_val_rel(mem, new, old)      \
+  __atomic_val_bysize (__arch_compare_and_exchange_val, int,    \
+                       mem, new, old, __ATOMIC_RELEASE)
+
+/* Compare and exchange.
+   For all "bool" routines, we return FALSE if exchange succesful.  */
+
+# define __arch_compare_and_exchange_bool_8_int(mem, newval, oldval, model) \
+  ({__arm_link_error (); oldval; })
+
+# define __arch_compare_and_exchange_bool_16_int(mem, newval, oldval, model) \
+  ({__arm_link_error (); oldval; })
+
+# define __arch_compare_and_exchange_bool_32_int(mem, newval, oldval, model) \
+  ({                                                                    \
+    typeof (*mem) __oldval = (oldval);                                  \
+    !__atomic_compare_exchange_n (mem, (void *) &__oldval, newval, 0,   \
+                                  model, __ATOMIC_RELAXED);             \
+  })
+
+# define __arch_compare_and_exchange_bool_64_int(mem, newval, oldval, model) \
+  ({__arm_link_error (); oldval; })
+
+# define __arch_compare_and_exchange_val_8_int(mem, newval, oldval, model) \
+  ({__arm_link_error (); oldval; })
+
+# define __arch_compare_and_exchange_val_16_int(mem, newval, oldval, model) \
+  ({__arm_link_error (); oldval; })
+
+# define __arch_compare_and_exchange_val_32_int(mem, newval, oldval, model) \
+  ({                                                                    \
+    typeof (*mem) __oldval = (oldval);                                  \
+    __atomic_compare_exchange_n (mem, (void *) &__oldval, newval, 0,    \
+                                 model, __ATOMIC_RELAXED);              \
+    __oldval;                                                           \
+  })
+
+# define __arch_compare_and_exchange_val_64_int(mem, newval, oldval, model) \
+  ({__arm_link_error (); oldval; })
+
+#elif defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
+/* Atomic compare and exchange.  */
+# define __arch_compare_and_exchange_val_32_acq(mem, newval, oldval) \$                                   
   __sync_val_compare_and_swap ((mem), (oldval), (newval))
 #else
 # define __arch_compare_and_exchange_val_32_acq(mem, newval, oldval) \
   __arm_assisted_compare_and_exchange_val_32_acq ((mem), (newval), (oldval))
 #endif
 
+#if !__GNUC_PREREQ (4, 7) || !defined (__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)
 /* We don't support atomic operations on any non-word types.
    So make them link errors.  */
-#define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \
+# define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \
   ({ __arm_link_error (); oldval; })
 
-#define __arch_compare_and_exchange_val_16_acq(mem, newval, oldval) \
+# define __arch_compare_and_exchange_val_16_acq(mem, newval, oldval) \
   ({ __arm_link_error (); oldval; })
 
-#define __arch_compare_and_exchange_val_64_acq(mem, newval, oldval) \
+# define __arch_compare_and_exchange_val_64_acq(mem, newval, oldval) \
   ({ __arm_link_error (); oldval; })
+#endif
 
 /* An OS-specific bits/atomic.h file will define this macro if
    the OS can provide something.  If not, we'll fail to build

^ permalink raw reply	[flat|nested] 21+ messages in thread

* RE: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-09-02 15:08 ` Joseph S. Myers
@ 2013-09-03 17:31   ` Abhishek Deb
  2013-09-05 12:56   ` Dinar Temirbulatov
  1 sibling, 0 replies; 21+ messages in thread
From: Abhishek Deb @ 2013-09-03 17:31 UTC (permalink / raw)
  To: Joseph Myers, Dinar Temirbulatov; +Cc: libc-ports

From the functionality point of view, the changes look ok.

An objdump of the atomic_compare_and_exchange_bool_acq shows the following:

  60:   e1903f9f        ldrex   r3, [r0]
  64:   e1530002        cmp     r3, r2
  68:   1a000003        bne     7c <local__lll_lock_wait+0x34>
  6c:   e1804f91        strex   r4, r1, [r0]
  70:   e3540000        cmp     r4, #0
  74:   1afffff9        bne     60 <local__lll_lock_wait+0x18>
  78:   f57ff05f        dmb     sy

As you can see it contains only one dmb in the end and not in the beginning, which is in-line with acquire semantics to not allow any code hoisting.

Abhishek

-----Original Message-----
From: Joseph Myers [mailto:joseph@codesourcery.com] 
Sent: Monday, September 02, 2013 8:08 AM
To: Dinar Temirbulatov
Cc: libc-ports@sourceware.org; Abhishek Deb
Subject: Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

On Mon, 2 Sep 2013, Dinar Temirbulatov wrote:

> Hi,
> Another version of change, I added
> atomic_compare_and_exchange_val_acq/atomic_compare_and_exchange_val_re
> l and 
> atomic_compare_and_exchange_bool_acq/atomic_compare_and_exchange_boot_
> rel defenitions and also for gcc-4.7 and higher in the case of 
> unsupported atomic compare and swap operation, it uses the kernel 
> helper inlines.
> Tested on arm a9 with no new regressions. Ok to commit?
> Oh, sorry. I missed to attach the change. Here it is.

For subsequent patch revisions, please note there should be an extra space after "#" for preprocessor directives inside #if conditionals, one per level of #if nesting (other than toplevel multiple-inclusion guards) - which means that if conditioning existing code, you need to adjust directives inside that code.

This patch appears to have too much duplication.  For example, you duplicate the definition of __arm_assisted_compare_and_exchange_val_32_acq
- but that should not need any extra conditionals at all (beyond the existing #ifndef __arm_assisted_compare_and_exchange_val_32_acq), there's no reason ever not to define it.  Similarly, you duplicate __arch_compare_and_exchange_val_64_acq, but with proper #if structure there should only need to be one copy of the version that uses __arm_link_error.

What I think you should aim for is that each definition, or small group of definitions, uses conditionals in the form

#if __GNUC_PREREQ (4, 7) && defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
/* Version using __atomic_*.  */
#elif defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
/* Version using __sync_*.  */
#else
/* Version using __arm_assisted_*.  */
#endif

with cases omitted if not useful for that particular macro (this may include some macros not being defined at all in some cases).  So don't insert any global conditionals affecting all the existing definitions at all - look at each block of conditionals and add a third case as needed, along with any new macros (again with conditionals in that form) that are appropriate.

Where you use abort () in some definitions, use __arm_link_error () instead, like for the existing definitions.

--
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
  2013-09-01 20:43 Dinar Temirbulatov
@ 2013-09-02 15:08 ` Joseph S. Myers
  2013-09-03 17:31   ` Abhishek Deb
  2013-09-05 12:56   ` Dinar Temirbulatov
  0 siblings, 2 replies; 21+ messages in thread
From: Joseph S. Myers @ 2013-09-02 15:08 UTC (permalink / raw)
  To: Dinar Temirbulatov; +Cc: libc-ports, adeb

On Mon, 2 Sep 2013, Dinar Temirbulatov wrote:

> Hi,
> Another version of change, I added
> atomic_compare_and_exchange_val_acq/atomic_compare_and_exchange_val_rel
> and atomic_compare_and_exchange_bool_acq/atomic_compare_and_exchange_boot_rel
> defenitions and also for gcc-4.7 and higher in the case of unsupported
> atomic compare and swap operation, it uses the kernel helper inlines.
> Tested on arm a9 with no new regressions. Ok to commit?
> Oh, sorry. I missed to attach the change. Here it is.

For subsequent patch revisions, please note there should be an extra space 
after "#" for preprocessor directives inside #if conditionals, one per 
level of #if nesting (other than toplevel multiple-inclusion guards) - 
which means that if conditioning existing code, you need to adjust 
directives inside that code.

This patch appears to have too much duplication.  For example, you 
duplicate the definition of __arm_assisted_compare_and_exchange_val_32_acq 
- but that should not need any extra conditionals at all (beyond the 
existing #ifndef __arm_assisted_compare_and_exchange_val_32_acq), there's 
no reason ever not to define it.  Similarly, you duplicate 
__arch_compare_and_exchange_val_64_acq, but with proper #if structure 
there should only need to be one copy of the version that uses 
__arm_link_error.

What I think you should aim for is that each definition, or small group of 
definitions, uses conditionals in the form

#if __GNUC_PREREQ (4, 7) && defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
/* Version using __atomic_*.  */
#elif defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
/* Version using __sync_*.  */
#else
/* Version using __arm_assisted_*.  */
#endif

with cases omitted if not useful for that particular macro (this may 
include some macros not being defined at all in some cases).  So don't 
insert any global conditionals affecting all the existing definitions at 
all - look at each block of conditionals and add a third case as needed, 
along with any new macros (again with conditionals in that form) that are 
appropriate.

Where you use abort () in some definitions, use __arm_link_error () 
instead, like for the existing definitions.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
@ 2013-09-01 20:43 Dinar Temirbulatov
  2013-09-02 15:08 ` Joseph S. Myers
  0 siblings, 1 reply; 21+ messages in thread
From: Dinar Temirbulatov @ 2013-09-01 20:43 UTC (permalink / raw)
  To: libc-ports; +Cc: joseph, adeb

[-- Attachment #1: Type: text/plain, Size: 475 bytes --]

Hi,
Another version of change, I added
atomic_compare_and_exchange_val_acq/atomic_compare_and_exchange_val_rel
and atomic_compare_and_exchange_bool_acq/atomic_compare_and_exchange_boot_rel
defenitions and also for gcc-4.7 and higher in the case of unsupported
atomic compare and swap operation, it uses the kernel helper inlines.
Tested on arm a9 with no new regressions. Ok to commit?
Oh, sorry. I missed to attach the change. Here it is.
                    thanks, Dinar.

[-- Attachment #2: arm_atomic.patch --]
[-- Type: application/octet-stream, Size: 4864 bytes --]

--- glibc-orig/ports/sysdeps/arm/bits/atomic.h	2013-08-19 21:46:44.592731210 +0400
+++ glibc/ports/sysdeps/arm/bits/atomic.h	2013-09-01 07:03:05.691924824 +0400
@@ -51,6 +51,113 @@ void __arm_link_error (void);
 # define __arm_assisted_full_barrier()  __arm_link_error()
 #endif
 
+#if __GNUC_PREREQ (4, 7)
+
+#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
+
+#define atomic_exchange_acq(mem, value)                                \
+  __atomic_val_bysize (__arch_exchange, int, mem, value, __ATOMIC_ACQUIRE)
+
+#define atomic_exchange_rel(mem, value)                                \
+  __atomic_val_bysize (__arch_exchange, int, mem, value, __ATOMIC_RELEASE)
+
+/* Atomic exchange (without compare).  */
+
+#define __arch_exchange_8_int(mem, newval, model)      \
+  (abort (), (typeof(*mem)) 0)
+
+#define __arch_exchange_16_int(mem, newval, model)     \
+  (abort (), (typeof(*mem)) 0)
+
+#define __arch_exchange_32_int(mem, newval, model)     \
+  __atomic_exchange_n (mem, newval, model)
+
+#define __arch_exchange_64_int(mem, newval, model)     \
+  (abort (), (typeof(*mem)) 0)
+
+/* Compare and exchange with "acquire" semantics, ie barrier after.  */
+
+# define atomic_compare_and_exchange_bool_acq(mem, new, old)    \
+  __atomic_bool_bysize (__arch_compare_and_exchange_bool, int,  \
+                        mem, new, old, __ATOMIC_ACQUIRE)
+
+# define atomic_compare_and_exchange_val_acq(mem, new, old)     \
+  __atomic_val_bysize (__arch_compare_and_exchange_val, int,    \
+                       mem, new, old, __ATOMIC_ACQUIRE)
+
+/* Compare and exchange with "release" semantics, ie barrier before.  */
+
+# define atomic_compare_and_exchange_bool_rel(mem, new, old)    \
+  __atomic_bool_bysize (__arch_compare_and_exchange_bool, int,  \
+                        mem, new, old, __ATOMIC_RELEASE)
+
+# define atomic_compare_and_exchange_val_rel(mem, new, old)      \
+  __atomic_val_bysize (__arch_compare_and_exchange_val, int,    \
+                       mem, new, old, __ATOMIC_RELEASE)
+
+/* Compare and exchange.
+   For all "bool" routines, we return FALSE if exchange succesful.  */
+
+# define __arch_compare_and_exchange_bool_8_int(mem, newval, oldval, model) \
+  (abort (), 0)
+
+# define __arch_compare_and_exchange_bool_16_int(mem, newval, oldval, model) \
+  (abort (), 0)
+
+# define __arch_compare_and_exchange_bool_32_int(mem, newval, oldval, model) \
+  ({                                                                    \
+    typeof (*mem) __oldval = (oldval);                                  \
+    !__atomic_compare_exchange_n (mem, (void *) &__oldval, newval, 0,   \
+                                  model, __ATOMIC_RELAXED);             \
+  })
+
+# define __arch_compare_and_exchange_bool_64_int(mem, newval, oldval, model) \
+  (abort (), 0)
+
+# define __arch_compare_and_exchange_val_8_int(mem, newval, oldval, model) \
+  (abort (), (typeof(*mem)) 0)
+
+# define __arch_compare_and_exchange_val_16_int(mem, newval, oldval, model) \
+  (abort (), (typeof(*mem)) 0)
+
+# define __arch_compare_and_exchange_val_32_int(mem, newval, oldval, model) \
+  ({                                                                    \
+    typeof (*mem) __oldval = (oldval);                                  \
+    __atomic_compare_exchange_n (mem, (void *) &__oldval, newval, 0,    \
+                                 model, __ATOMIC_RELAXED);              \
+    __oldval;                                                           \
+  })
+
+# define __arch_compare_and_exchange_val_64_int(mem, newval, oldval, model) \
+  (abort (), (typeof(*mem)) 0)
+
+#else
+
+# define __arch_compare_and_exchange_val_32_acq(mem, newval, oldval) \
+  __arm_assisted_compare_and_exchange_val_32_acq ((mem), (newval), (oldval))
+
+/* We don't support atomic operations on any non-word types.
+   So make them link errors.  */
+#define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \
+  ({ __arm_link_error (); oldval; })
+
+#define __arch_compare_and_exchange_val_16_acq(mem, newval, oldval) \
+  ({ __arm_link_error (); oldval; })
+
+#define __arch_compare_and_exchange_val_64_acq(mem, newval, oldval) \
+  ({ __arm_link_error (); oldval; })
+
+/* An OS-specific bits/atomic.h file will define this macro if
+   the OS can provide something.  If not, we'll fail to build
+   with a compiler that doesn't supply the operation.  */
+#ifndef __arm_assisted_compare_and_exchange_val_32_acq
+# define __arm_assisted_compare_and_exchange_val_32_acq(mem, newval, oldval) \
+  ({ __arm_link_error (); oldval; })
+#endif
+
+#endif
+#else /* !__GNUC_PREREQ (4, 7) */
+
 /* Atomic compare and exchange.  */
 
 #ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
@@ -79,3 +186,5 @@ void __arm_link_error (void);
 # define __arm_assisted_compare_and_exchange_val_32_acq(mem, newval, oldval) \
   ({ __arm_link_error (); oldval; })
 #endif
+
+#endif

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n
@ 2013-09-01 20:42 Dinar Temirbulatov
  0 siblings, 0 replies; 21+ messages in thread
From: Dinar Temirbulatov @ 2013-09-01 20:42 UTC (permalink / raw)
  To: libc-ports; +Cc: joseph, adeb, Maxim Kuvyrkov

Hi,
Another version of change, I added
atomic_compare_and_exchange_val_acq/atomic_compare_and_exchange_val_rel
and atomic_compare_and_exchange_bool_acq/atomic_compare_and_exchange_boot_rel
defenitions and also for gcc-4.7 and higher in the case of unsupported
atomic compare and swap operation, it uses the kernel helper inlines.
Tested on arm a9 with no new regressions. Ok to commit?
                    thanks, Dinar.

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2013-09-19 16:29 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-13 17:11 [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n Dinar Temirbulatov
2013-08-15 21:46 ` Abhishek Deb
2013-08-16 22:57   ` Dinar Temirbulatov
2013-08-17  2:42     ` Abhishek Deb
2013-08-19 19:21       ` Dinar Temirbulatov
2013-08-18 20:29 ` Joseph S. Myers
2013-09-01 20:42 Dinar Temirbulatov
2013-09-01 20:43 Dinar Temirbulatov
2013-09-02 15:08 ` Joseph S. Myers
2013-09-03 17:31   ` Abhishek Deb
2013-09-05 12:56   ` Dinar Temirbulatov
2013-09-05 15:48     ` Joseph S. Myers
2013-09-05 17:09       ` Abhishek Deb
2013-09-11 19:25         ` Dinar Temirbulatov
2013-09-11 19:27         ` Dinar Temirbulatov
2013-09-11 20:40           ` Abhishek Deb
2013-09-13 17:14           ` Joseph S. Myers
2013-09-14  3:29             ` Dinar Temirbulatov
2013-09-18 17:16               ` Joseph S. Myers
2013-09-19  6:53                 ` Maxim Kuvyrkov
2013-09-19 16:29                   ` Joseph S. Myers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).