Re[4]: [Gc] test_stack on powerpc (power7)

 Hi Will,

I don't think current definition of AO_load is incorrect. (The definition has never been changed and all other supported platforms have the same 
no-barrier atomic load definition.)

Probably the barrier should be added to stack implementation.
Is it really correct to have "first = AO_load(list)" (in AO_stack_pop_explicit_aux_acquire) instead of "first = AO_load_acquire(list)" ?


Monday,  3 Feb 2014, 14:30 -06:00 from Will Schmidt <[email protected]>:
>On Sat, 2014-02-01 at 12:52 +0400, Ivan Maidanski wrote:
>> 
>> Hello Will,
>
>Hi, 
>
>> Your current patch is just a workaround to make the test pass. It
>> would be good to develop a proper patch.
>
>Yes, no doubt.  As I mentioned, I leave that to someone who understands
>the intended include hierarchy here better than I do.
>
>> > The AO_load() function is being mapped to AO_load() in
>> > sysdeps/loadstore/atomic_load.h, rather than the AO_load_acquire()
>> in
>> > powerpc.h like I had initialy thought, per the #defines I was
>> looking
>> > at in generalize-small.h. The critical detail in that is the lack of
>> an
>> > isync in the atomic_load.h version.
You could also check the GCC implementation of no-barrier atomic load for ppc/ppc64 by producing assembly for the following code:

char char_load(const volatile char *addr) {  return __atomic_load_n(addr, __ATOMIC_RELAXED); }

void char_store(volatile char *addr, char value) {  __atomic_store_n(addr, value, __ATOMIC_RELAXED); }

short short_load(const volatile short *addr) {  return __atomic_load_n(addr, __ATOMIC_RELAXED); }

void short_store(volatile short *addr, short value) {  __atomic_store_n(addr, value, __ATOMIC_RELAXED); }

int int_load(const volatile int *addr) {  return __atomic_load_n(addr, __ATOMIC_RELAXED); }

void int_store(volatile int *addr, int value) {  __atomic_store_n(addr, value, __ATOMIC_RELAXED); }

>
>> 
>> 1. So, you say that aligned accesses are not guaranteed to be atomic,
>> right?
>
>There may be a nit between being atomic versus the accesses having the
>same view of storage.  that said, and in this context, I'll go with
>"correct"
I don't understand. AO_load does not require any barrier.

Regards,
Ivan

>
>
>>  (This means that inclusion of all_aligned_atomic_load_store.h is
>> incorrect.)
>
>I'm not certain what the intent is with regards to the header files.  At
>least some of the compare_and_swap function was coming out of powerpc.h.
>This may be an #ifdef/#endif issue somewhere within.
>
>> 2. What about stores? What's about char/short/int wide loads?
>
>(I'll paraphrase a bit from the bookII.)  "The lwarx/stwcx instructions
>together permit atomic update of a storage location."   Access for data
>types smaller than wordsize would need to round up, so
>shifting/masking/inserting may apply. 
>
>
>
>> 3. What about other operations? If there is a gcc version produces
>> correct code for atomic builtin, could please check assembly generated
>> by tests/list_atomic.c (I'm referring here to master branch, i.e
>> v7.4.0) of current implementation of primitives with the gcc/generic.h
>> implementation (this could be done by commenting out entire content of
>> gcc/powerpc.h including generic.h instead).

>> Thank you
>> 
>> Regards,
>> Ivan
>> 
>> Thu, 30 Jan 2014, 15:09 -05:00 from "Lennart Sorensen"
>> < [email protected] >:
>>         On Thu, Jan 30, 2014 at 12:56:03PM -0600, Will Schmidt wrote:
>>         > On Wed, 2014-01-29 at 07:05 +0000, Boehm, Hans wrote:
>>         > > Interesting.
>>         > <...snip...>
>>         > 
>>         > > I would also check that the recheck of first against
>>         AO_load(list) appears in the assembly code where it should.
>>         > 
>>         > Thanks for the suggestions. :-)
>>         > 
>>         > I looked closer at the code that does the x_bits twiddling,
>>         and after a
>>         > bit of quality time single-stepping within gdb, have found
>>         an issue.
>>         > The AO_load() function is being mapped to AO_load() in
>>         > sysdeps/loadstore/atomic_load.h, rather than the
>>         AO_load_acquire() in
>>         > powerpc.h like I had initialy thought, per the #defines I
>>         was lookging
>>         > at in generalize-small.h. The critical detail in that is the
>>         lack of an
>>         > isync in the atomic_load.h version.
>>         > 
>>         > I'm admittedly unclear of how the path through the header
>>         file includes
>>         > should be.
>>         > 
>>         > The patch below (inline and attached) seems a bit hackish to
>>         me, but is
>>         > also sufficient to allow test_stack to run to completion on
>>         the P7 here.
>>         > (test_stack running in a loop, ~ 1000 successful runs so
>>         far). For
>>         > inclusion as-is, or as inspiration to whomever better
>>         understands the
>>         > include hierarchy.
>>         > 
>>         > Thanks, 
>>         > -Will
>>         > 
>>         > --
>>         > 
>>         > Force AO_load() to map to AO_load_acquire() for powerpc. The
>>         > AO_load_acquire() function includes isync instructions that
>>         > are critical for proper behavior on power system.
>>         > 
>>         > 
>>         > Signed-Off-By: Will Schmidt < [email protected] >
>>         > 
>>         > 
>>         > diff -aur --exclude='*.Plo' --exclude='*.Po'
>>         libatomic_ops-7.4.0/src/atomic_ops/sysdeps/gcc/powerpc.h
>>         libatomic_ops-7.4.0.new/src/atomic_ops/sysdeps/gcc/powerpc.h
>>         > --- libatomic_ops-7.4.0/src/atomic_ops/sysdeps/gcc/powerpc.h
>>         2013-11-10 03:57:17.000000000 -0600
>>         > +++
>>         libatomic_ops-7.4.0.new/src/atomic_ops/sysdeps/gcc/powerpc.h
>>         2014-01-30 12:17:20.819984940 -0600
>>         > @@ -29,6 +29,8 @@
>>         > 
>>         > #include "../all_aligned_atomic_load_store.h"
>>         > 
>>         > +#define AO_load(addr) AO_load_acquire(addr)
>>         > +
>>         > #include "../test_and_set_t_is_ao_t.h"
>>         > /* There seems to be no byte equivalent of lwarx, so this */
>>         > /* may really be what we want, at least in the 32-bit case.
>>         */
>>         > 
>>         > 
>>         > 
>> 
>>         > diff -aur --exclude='*.Plo' --exclude='*.Po'
>>         libatomic_ops-7.4.0/src/atomic_ops/sysdeps/gcc/powerpc.h
>>         libatomic_ops-7.4.0.new/src/atomic_ops/sysdeps/gcc/powerpc.h
>>         > --- libatomic_ops-7.4.0/src/atomic_ops/sysdeps/gcc/powerpc.h
>>         2013-11-10 03:57:17.000000000 -0600
>>         > +++
>>         libatomic_ops-7.4.0.new/src/atomic_ops/sysdeps/gcc/powerpc.h
>>         2014-01-30 12:17:20.819984940 -0600
>>         > @@ -29,6 +29,8 @@
>>         > 
>>         > #include "../all_aligned_atomic_load_store.h"
>>         > 
>>         > +#define AO_load(addr) AO_load_acquire(addr)
>>         > +
>>         > #include "../test_and_set_t_is_ao_t.h"
>>         > /* There seems to be no byte equivalent of lwarx, so this */
>>         > /* may really be what we want, at least in the 32-bit case.
>>         */
>> 
>>         Works for me on the version in Debian Wheezy
>>         (libatomic-ops-7.2~alpha5+cvs20100601).
>> 
>>         Yayyyy!
>> 
>> 
>> 
>
>_______________________________________________
>Gc mailing list
>[email protected]
>http://www.hpl.hp.com/hosted/linux/mail-archives/gc/

_______________________________________________
Gc mailing list
Gc-V9/[email protected]
http://www.hpl.hp.com/hosted/linux/mail-archives/gc/