Re[2]: [Gc] test_stack on powerpc (power7)
Ivan Maidanski <ivmai-JGs/[email protected]>
| Newsgroups | gmane.comp.programming.garbage-collection.boehmgc |
|---|---|
| Message-ID | <[email protected]> |
Hello Will, Your current patch is just a workaround to make the test pass. It would be good to develop a proper patch. > 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. 1. So, you say that aligned accesses are not guaranteed to be atomic, right? (This means that inclusion of all_aligned_atomic_load_store.h is incorrect.) 2. What about stores? What's about char/short/int wide loads? 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! > >-- >Len Sorensen >_______________________________________________ >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/