Re: Re[2]: test_stack on powerpc (power7)

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.
> 
> 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"

>  (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"
> <lsorense-1wCw9BSqJbv44Nm34jS7GywD8/[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 <will_schmidt-DhXWPJtHtuFWk0Htik3J/[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!
>         
>         
>