Bug 68868 - atomic_init emits an unnecessary fence
Summary: atomic_init emits an unnecessary fence
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 6.0
: P3 normal
Target Milestone: ---
Assignee: Martin Sebor
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-12 00:49 UTC by Martin Sebor
Modified: 2015-12-17 01:34 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 4.9.2, 5.1.0, 6.0
Last reconfirmed: 2015-12-14 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Sebor 2015-12-12 00:49:08 UTC
This is similar but not the same as as bug 68622.  The following test case that uses atomic_init() and shows that while libstdc++ handles the C++ case efficiently, C does not and emits an unnecessary fence:

#if __cplusplus
#  include <atomic>

using std::atomic_int;
using std::atomic_init;

#else
#  include <stdatomic.h>
#endif

typedef atomic_int AI;

void foo (AI *ai)
{
    atomic_init (ai, 123);
}

Gcc (in C mode) emits the following with -O2 on x86_64:

foo:
	movl	$123, (%rdi)
	mfence
	ret

G++, on the other hand, and Clang in both modes, emit the following efficient code at -O2:

_Z3fooPSt6atomicIiE:
	movl	$123, (%rdi)
	ret

The C code is worse because the C <stdatomic.h> header doesn't distinguish initialization from assignment and implements the atomic_init() macro as plain assignment:

  #define atomic_init(PTR, VAL)                   \
    do                                            \
      {                                           \
        *(PTR) = (VAL);                           \
      }                                           \
    while (0)

Defining atomic_init() as in the otherwise untested patch below gets rid of the unnecessary fence:

--- ginclude/stdatomic.h	(revision 231532)
+++ ginclude/stdatomic.h	(working copy)
@@ -77,13 +77,13 @@
 
 
 #define ATOMIC_VAR_INIT(VALUE)	(VALUE)
-#define atomic_init(PTR, VAL)			\
-  do						\
-    {						\
-      *(PTR) = (VAL);				\
-    }						\
-  while (0)
 
+/* Initialize an atomic object pointed to by PTR with VALUE.  */
+#define atomic_init(PTR, VALUE) __extension__ ({                  \
+      __typeof__ (VALUE) __tmp = (VALUE);                         \
+      __atomic_store ((PTR), &__tmp, __ATOMIC_RELAXED);           \
+    })
+
 #define kill_dependency(Y)			\
   __extension__					\
   ({						\
Comment 1 Martin Sebor 2015-12-14 02:43:56 UTC
Unfortunately, the simple patch in the Description doesn't work for char literals.  I.e., this 

  atomic_char a;
  atomic_init (a, 'x');

doesn't compile because __typeof__ ('x') is int and calling __atomic_store with pointers to types of mismatching size produces:

  error: size mismatch in argument 2 of ‘__atomic_store’
     atomic_init (a, 'x');
     ^

This is likely why Clang implements atomic_init in terms of its __c11_atomic_init intrinsic.

Assigning to self to come up with a better patch.
Comment 2 jsm-csl@polyomino.org.uk 2015-12-15 00:00:10 UTC
On Sat, 12 Dec 2015, msebor at gcc dot gnu.org wrote:

> +      __typeof__ (VALUE) __tmp = (VALUE);                         \

Using typeof like this is incorrect; it results in multiple evaluation if 
VALUE has a variably modified type.  You have to use __auto_type in such a 
case, as elsewhere in stdatomic.h.
Comment 3 Martin Sebor 2015-12-15 00:55:44 UTC
A working patch posted for review:
https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01449.html
Comment 4 Martin Sebor 2015-12-17 01:34:07 UTC
Fixed in r231733.
Comment 5 Martin Sebor 2015-12-17 01:34:12 UTC
Author: msebor
Date: Thu Dec 17 01:33:41 2015
New Revision: 231733

URL: https://gcc.gnu.org/viewcvs?rev=231733&root=gcc&view=rev
Log:
PR c/68868 - atomic_init emits an unnecessary fence

gcc/ChangeLog
	* ginclude/stdatomic.h (atomic_init): Use atomic_store instead
	of plain assignment.
gcc/testsuite/ChangeLog
	* testsuite/gcc.dg/atomic/stdatomic-init.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/atomic/stdatomic-init.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ginclude/stdatomic.h
    trunk/gcc/testsuite/ChangeLog