Bug 36793 - x86-64 does not get __sync_synchronize right
x86-64 does not get __sync_synchronize right
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: target
4.3.1
: P3 normal
: 4.4.0
Assigned To: Uroš Bizjak
http://gcc.gnu.org/ml/gcc-patches/200...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-10 15:10 UTC by John F. Carr
Modified: 2009-08-03 08:51 UTC (History)
5 users (show)

See Also:
Host: x86_64-linux-gnu
Target: x86_64-linux-gnu
Build: x86_64-linux-gnu
Known to work:
Known to fail:
Last reconfirmed: 2008-11-22 18:29:20


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description John F. Carr 2008-07-10 15:10:14 UTC
As I understand __sync_synchronize, the intent is to emit a memory barrier instruction, at least on multiprocessor systems.  Currently on x86 __sync_synchronize inhibits explicit code motion across the builtin function call but not processor reordering of memory operations across the builtin function call.

I think mfence is the right instruction on x86-64, and this or a similar pattern should be added to sync.md:

(define_insn "memory_barrier"
  [(unspec_volatile [(const_int 0)] UNSPEC_MFENCE)]
  "TARGET_SSE2"
  "mfence")
Comment 1 Samuel Thibault 2008-11-21 11:16:44 UTC
Just to confirm the bug: the gcc doc says it follows the Intel itanium binary interface. The Intel documentation says « Associated with each instrinsic are certain memory barrier properties that restrict the movement of memory references to visible data across the intrinsic operation (by either the compiler or the processor). » Not including the mfence instruction would let the processor move references across the instruction, so it is mandatory.  And that is not only for x86_64, but also x86, on which you can use e.g. a locked nop if you don't know the arch, or a mfence when using -march= (IIRC it appeared with SSE2) 
Comment 2 Uroš Bizjak 2008-11-21 17:22:49 UTC
H.J. can probably confirm this.
Comment 3 H.J. Lu 2008-11-21 17:36:26 UTC
__sync_synchronize isn't specified for IA32/Intel64. You can check
out Intel Memory Ordering White Paper:

www.intel.com/products/processor/manuals/318147.pdf

to see what is the most appropriate.
Comment 4 H.J. Lu 2008-11-21 17:37:18 UTC
The Intel Memory Ordering White Paper is at

http://www.intel.com/products/processor/manuals/318147.pdf
Comment 5 Samuel Thibault 2008-11-21 23:20:46 UTC
We do already know which x86 memory barrier instruction we need, that's not the problem, no need to give us pointers to documentations. The problem is that we'd like to not use explicit x86 instructions but rather rely on the "portability" of gcc's __sync_synchronize.  If your answer is "sync_foobar is not specified for baz", all these __sync_foobar become useless, since the user can't assume what their are supposed to achieve.  From what the documentation says, I had assumed that gcc would implement for other archs what Intel documents for ia64, but it happens not to be the case at least for sync_synchronize.  So in short, if that's your answer to the bug, then I'll have to tell people _not_ to use gcc's __sync_* at all except on ia64, since that would be the only arch on which we would have any semantic guarantee... (and then the documentation needs to be fixed).  Of course, that's not an option I'd like, and actually I believe there may be SMP bugs in libgc & such that use it... 
Comment 6 H.J. Lu 2008-11-21 23:38:39 UTC
I think it is a bug.
Comment 7 Uroš Bizjak 2008-11-22 18:29:20 UTC
Patch that implements "memory_barrier" for x86 at [1].

[1] http://gcc.gnu.org/ml/gcc-patches/2008-11/msg01181.html
Comment 8 Samuel Thibault 2008-11-22 19:41:33 UTC
Ah, well, by "nop", I was thinking about things like what Linux does: lock; addl $0,0(%%esp) 
Comment 9 uros 2008-11-24 16:57:00 UTC
Subject: Bug 36793

Author: uros
Date: Mon Nov 24 16:55:49 2008
New Revision: 142160

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=142160
Log:
	* config/i386/i386.md (UNSPECV_CMPXCHG): Rename from
	UNSPECV_CMPXCHG_[12].
	* config/i386/sync.md: Use UNSPECV_CMPXCHG instead of
	UNSPECV_CMPXCHG_[12].

	PR target/36793
	* config/i386/sync.md (memory_barrier): New expander.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.md
    trunk/gcc/config/i386/sync.md

Comment 10 Uroš Bizjak 2008-11-24 16:59:36 UTC
Fixed.
Comment 11 Uroš Bizjak 2008-11-25 09:15:06 UTC
Should we fix __sync_synchronize in 4.3 too?