RE: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

[Abhishek Deb <adeb at nvidia dot com>]

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.