[Bug libgomp/45025] New: Memory ordering issues with libgomp critical sections and __sync

[Bug libgomp/45025] New: Memory ordering issues with libgomp critical sections and __sync

[Hans dot Boehm at hp dot com]

This is really a continuation of bug 42869, but that title mentions Itanium,
and this is not Itanium-specific.

Gomp_mutex_lock and as a result OpenMP critical sections and locks do not
implement sane memory ordering semantics.  This is largely due to __sync
implementation problems.  I looked at Itanium and PowerPC, but other weakly
ordered architectures are probably similar.

I was trying to understand the memory ordering semantics of OpenMP critical
sections with gcc, since the 3.0 OpenMP spec requires full fence semantics,
which seems to be overkill.  I concluded that the current state of affairs in
4.4.4 doesn't make much sense, independent of the spec interpretation. 
Specific issues:

1. The Itanium version of gomp_mutex_lock still has the fence on the wrong side
of the cmpxchg4.rel.  This is a __sync_bool_compare_and_swap problem, and was
fixed in the trunk (bug 42869, thanks Steve!) but still appears to be wrong in
the 4.4 series.  I think this needs a backport to 4.4, since we may effectively
be generating critical sections that don't guarantee mutual exclusion.  (I
don't know whether current hardware provides stronger properties that mitigate
this, but ...)

2. The PowerPC & Itanium version of gomp_mutex_lock explicitly calls
__sync_bool_compare_and_swap.  Nonetheless the PowerPC code ends up generating
a code sequence along the lines of

lwsync; lwarx; stwcx.; bne; isync

As far as I can determine, this does not order a preceding store with respect
to a later load.  (It uses lwsync, not sync.) It's unclear whether this is a
problem with respect to the OpenMP spec.  (Technically it is for 3.0, but 3.1
was reworded, on my suggestion, to allow this, since it's a useless
restriction.)  But I think it clearly fails to satisfy the
__sync_bool_compare_and_swap spec, which claims this should be a "full
barrier".

More generally, gomp_mutex_lock() appears inconsistent about whether it
satisfies the OpenMP 3.0 and earlier rules that it include a flush or full
barrier.  Modulo the earlier bug, the Itanium versions do.  The PowerPC version
does for unlock, but not lock.  (And given that it doesn't, I'm not sure
there's a point behind the lwsync in the lock sequence either.)  Since this
choice has a significant performance impact on both architectures, this seems
to be a way to get the worst of all possible worlds; we get both weak
guarantees and, in many cases, bad performance.

I would recommend following the OpenMP3.1 draft spec, and insisting on only
acquire/release semantics for gomp_mutex_(un)lock.


-- 
           Summary: Memory ordering issues with libgomp critical sections
                    and  __sync
           Product: gcc
           Version: 4.4.4
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libgomp
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: Hans dot Boehm at hp dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45025

[Bug libgomp/45025] Memory ordering issues with libgomp critical sections and __sync

[sje at cup dot hp dot com]

------- Comment #1 from sje at cup dot hp dot com  2010-07-21 22:39 -------
I backported the patch for PR 42869 to the 4.4 branch to fix Itanium.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45025

[Bug libgomp/45025] Memory ordering issues with libgomp critical sections and __sync

[jakub at gcc dot gnu dot org]

------- Comment #2 from jakub at gcc dot gnu dot org  2010-07-22 07:59 -------
So, 1. is fixed.  If __sync_*compare_and_swap on PowerPC doesn't act as full
barrier, that would be a target bug, not libgomp bug.

Unless separate __sync_*_acq/__sync_*_rel builtins are added, I'm afraid the
only option for libgomp would be on the weakly ordered targets to use inline
asm instead, which wouldn't be very pretty.


-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rth at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45025

[Bug libgomp/45025] Memory ordering issues with libgomp critical sections and __sync

[Hans dot Boehm at hp dot com]

------- Comment #3 from Hans dot Boehm at hp dot com  2010-07-22 18:03 -------
Is there a plan for more complete C++0x/C1x atomics support?  Something
eventually needs to implement the memory_order_acquire/memory_order_release
versions of these primitives?  Nptl also seems to be in need of these.  I agree
that it would be unfortunate to use inline assembly code here at this point in
time.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45025

[Bug libgomp/45025] Memory ordering issues with libgomp critical sections and __sync

[jason at gcc dot gnu dot org]

------- Comment #4 from jason at gcc dot gnu dot org  2010-07-22 18:41 -------
(In reply to comment #3)
> Is there a plan for more complete C++0x/C1x atomics support?

http://gcc.gnu.org/wiki/Atomic


-- 

jason at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amacleod at redhat dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45025

[Bug libgomp/45025] Memory ordering issues with libgomp critical sections and __sync

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45025

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> 2012-02-03 02:49:44 UTC ---
I almost think this has been fixed now on the trunk.

[Bug libgomp/45025] Memory ordering issues with libgomp critical sections and __sync

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=45025

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED
   Target Milestone|---                         |4.7.0

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This was fixed by r0-113642-gb40c885f183cb7 which added the C11 atomics
barrier.