986745 - Add atomic primitives for Linux ppc64

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Patch by Ulrich Weigand

Ulrich Weigand of IBM sent me a patch to implement the NSPR
atomic functions using GCC built-in functions.

Comment 1

[Wan-Teh Chang]

Comment on attachment 8395119 [details] [diff] [review]
Patch by Ulrich Weigand

Review of attachment 8395119 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Thanks for the patch.

::: pr/include/md/_linux.h
@@ +136,5 @@
>  #define _MD_ATOMIC_SET                _PR_ppc_AtomicSet
>  #endif
>  
> +#if defined(__powerpc64__)
> +#if defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)

Ulrich: can we assume that for ppc64, __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
is always defined? That'll simply the patch a bit.

Comment 2

[Ulrich Weigand]

(In reply to Wan-Teh Chang from comment #1)
> Ulrich: can we assume that for ppc64, __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
> is always defined? That'll simply the patch a bit.

No, the atomic builtins (including __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4) have only been available since 2007 (about GCC 4.3), while ppc64 suppport has been present at least since 2001 (about GCC 3.2) as far as I can see.

If at some point you decide to drop support for GCC < 4.3, then this check can go away (probably for other platforms too).

Comment 3

[Wan-Teh Chang]

Attachment: Patch by Ulrich Weigand, v2

I edited Ulrich's patch and checked it in:
https://hg.mozilla.org/projects/nspr/rev/c7eb4da67836

I will annotate my changes in the next comment.

Comment 4

[Wan-Teh Chang]

Comment on attachment 8398257 [details] [diff] [review]
Patch by Ulrich Weigand, v2

Review of attachment 8398257 [details] [diff] [review]:
-----------------------------------------------------------------

Ulrich: please let me know if my changes are reasonable.

::: pr/include/md/_linux.h
@@ +136,5 @@
>  #define _MD_ATOMIC_SET                _PR_ppc_AtomicSet
>  #endif
>  
> +#if defined(__powerpc64__)
> +#if (__GNUC__ > 4) || (__GNUC__ == 4 && __GNUC_MINOR__ >= 1)

I decided to test GCC version instead of the
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 feature test
macro. The reason is that clang didn't define
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 until last
year, and it might only define that macro for
x86 and ARM:
http://llvm.org/bugs/show_bug.cgi?id=11174#c6
http://llvm.org/bugs/show_bug.cgi?id=12730#c19

GCC manuals show that these built-in functions
were added in GCC 4.1. My testing showed that
clang claims to be GCC version 4.2.1, so testing
GCC version >= 4.1 should work for both GCC and
clang.

::: pr/include/pratom.h
@@ +106,5 @@
>         (defined(__linux__) && \
>             ((defined(__i386__) && \
>             defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)) || \
>             defined(__ia64__) || defined(__x86_64__) || \
> +           defined(__powerpc__) || \

Here we can simply test defined(__powerpc__) to
cover both 32-bit and 64-bit PowerPC.

Comment 5

[Ulrich Weigand]

(In reply to Wan-Teh Chang from comment #4)

> Ulrich: please let me know if my changes are reasonable.

Looks good to me.

Comment 6

[Wan-Teh Chang]

Thank you. If you could verify that the new code in pratom.h
and _linux.h is activated on Linux ppc64, that would be great.

Another Linux ppc optimization we can work on is PRStack, a
LIFO singly-linked list:
http://mxr.mozilla.org/mozilla-central/ident?i=PRStack

PRStack is not used as widely as the PR_AtomicXXX functions,
so optimizing PRStack is not as critical. I think we can use
__sync_val_compare_and_swap on Linux ppc because that built-in
function should not have the ABA problem on PowerPC.

I tried to optimize PRStack for Mac OS X ppc in bug 194339
by writing assembly code, but for some reason didn't finish
that work.

Comment 7

[Ulrich Weigand]

(In reply to Wan-Teh Chang from comment #6)
> Thank you. If you could verify that the new code in pratom.h
> and _linux.h is activated on Linux ppc64, that would be great.

Yes, with current tip I see the atomic sequences both in
pr/src/misc/pratom.o and in pr/tests/atomic (for the inline
case).