Re: should sync builtins be full optimization barriers?

[Andrew MacLeod <amacleod at redhat dot com>]

On 09/09/2011 04:17 AM, Jakub Jelinek wrote:
> On Fri, Sep 09, 2011 at 10:07:30AM +0200, Paolo Bonzini wrote:
> > sync builtins are described in the documentations as being full
> > memory barriers, with the possible exception of
> > __sync_lock_test_and_set. However, GCC is not enforcing the fact
> > that they are also full _optimization_ barriers.  The RTL produced
> > by builtins does not in general include a memory optimization
> > barrier such as a set of (mem/v:BLK (scratch:P)).
> >
> > This can cause problems with lock-free algorithms, for example this:
> >
> > http://libdispatch.macosforge.org/trac/ticket/35
> >
> > This can be solved either in generic code, by wrapping sync builtins
> > (before and after) with an asm("":::"memory"), or in the single
> > machine descriptions by adding a memory barrier in parallel to the
> > locked instructions or with the ll/sc instructions.
> >
> > Is the above analysis correct?  Or should the users put explicit
> > compiler barriers?
> I'd say they should be optimization barriers too (and at the tree level
> they I think work that way, being represented as function calls), so if
> they don't act as memory barriers in RTL, the *.md patterns should be
> fixed.  The only exception should be IMHO the __SYNC_MEM_RELAXED
> variants - if the CPU can reorder memory accesses across them at will,
> why shouldn't the compiler be able to do the same as well?
> 	
Yeah, some of this is part of the ongoing C++0x work... the memory model 
parameter is going to allow certain types of code movement in optimizers 
based on whether its an acquire operation, a release operation, neither, 
or both.    It is ongoing and hopefully we will eventually have proper 
consistency.  The older __sync builtins are eventually going to invoke 
the new__sync_mem routines and their new patterns, but will fall back to 
the old ones if new patterns aren't specified.

In the case of your program, this would in fact be a valid 
transformation I believe...  __sync_lock_test_and_set is documented to 
only have ACQUIRE semantics. This does not guarantee that a store BEFORE 
the operation will be visible in another thread, which means it possible 
to reorder it.  (A summary of the different modes can be found at : 
http://gcc.gnu.org/wiki/Atomic/GCCMM/Optimizations     So you would 
require a  barrier before this code anyway for the behaviour you are 
looking for.    Once the new routines are available and implemented, you 
could simply specify the SEQ_CST model and then it should, in theory, 
work properly with a barrier being emitted for you.

I don't see anything in this pattern however that would enforce acquire 
mode and prevent the reverse operation.. moving something from after to 
before it... so there may be a bug there anyway.

And I suspect most people actually expect all the old __sync routines to 
be full optimization barriers all the time...  maybe we should consider 
just doing that...

Andrew