[PATCH, libgfortran] Replace sync builtins with atomic builtins

[Janne Blomqvist]

The old __sync builtins have been deprecated for a long time now in
favor of the __atomic builtins following the C++11/C11 memory model.
This patch converts libgfortran to use the modern __atomic builtins.

At the same time I weakened the consistency to acquire-release as that
should be enough for this particular case. Jakub, as the original
author of the algorithm, do you concur?

Regtested on x86_64-pc-linux-gnu, Ok for trunk?

libgfortran/ChangeLog:

2018-11-09 Janne Blomqvist <***@gcc.gnu.org>

* acinclude.m4 (LIBGFOR_CHECK_ATOMIC_FETCH_ADD): Rename and test
presence of atomic builtins instead of sync builtins.
* configure.ac (LIBGFOR_CHECK_ATOMIC_FETCH_ADD): Call new test.
* io/io.h (inc_waiting_locked): Use __atomic_fetch_add.
(predec_waiting_locked): Use __atomic_add_fetch.
(dec_waiting_unlocked): Use __atomic_fetch_add.
* config.h.in: Regenerated.
* configure: Regenerated.
---
libgfortran/acinclude.m4 | 20 ++++++++++----------
libgfortran/configure.ac | 4 ++--
libgfortran/io/io.h | 12 ++++++------
3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/libgfortran/acinclude.m4 b/libgfortran/acinclude.m4
index dd5429ac0d2..5b0c094e716 100644
--- a/libgfortran/acinclude.m4
+++ b/libgfortran/acinclude.m4
@@ -59,17 +59,17 @@ extern void bar(void) __attribute__((alias("foo")));]],
[Define to 1 if the target supports __attribute__((alias(...))).])
fi])

-dnl Check whether the target supports __sync_fetch_and_add.
-AC_DEFUN([LIBGFOR_CHECK_SYNC_FETCH_AND_ADD], [
- AC_CACHE_CHECK([whether the target supports __sync_fetch_and_add],
- libgfor_cv_have_sync_fetch_and_add, [
+dnl Check whether the target supports __atomic_fetch_add.
+AC_DEFUN([LIBGFOR_CHECK_ATOMIC_FETCH_ADD], [
+ AC_CACHE_CHECK([whether the target supports __atomic_fetch_add],
+ libgfor_cv_have_atomic_fetch_add, [
AC_LINK_IFELSE([AC_LANG_PROGRAM([[int foovar = 0;]], [[
-if (foovar <= 0) return __sync_fetch_and_add (&foovar, 1);
-if (foovar > 10) return __sync_add_and_fetch (&foovar, -1);]])],
- libgfor_cv_have_sync_fetch_and_add=yes, libgfor_cv_have_sync_fetch_and_add=no)])
- if test $libgfor_cv_have_sync_fetch_and_add = yes; then
- AC_DEFINE(HAVE_SYNC_FETCH_AND_ADD, 1,
- [Define to 1 if the target supports __sync_fetch_and_add])
+if (foovar <= 0) return __atomic_fetch_add (&foovar, 1, __ATOMIC_ACQ_REL);
+if (foovar > 10) return __atomic_add_fetch (&foovar, -1, __ATOMIC_ACQ_REL);]])],
+ libgfor_cv_have_atomic_fetch_add=yes, libgfor_cv_have_atomic_fetch_add=no)])
+ if test $libgfor_cv_have_atomic_fetch_add = yes; then
+ AC_DEFINE(HAVE_ATOMIC_FETCH_ADD, 1,
+ [Define to 1 if the target supports __atomic_fetch_add])
fi])

dnl Check for pragma weak.
diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac
index 76007d38f6f..30ff8734760 100644
--- a/libgfortran/configure.ac
+++ b/libgfortran/configure.ac
@@ -608,8 +608,8 @@ fi
LIBGFOR_CHECK_ATTRIBUTE_VISIBILITY
LIBGFOR_CHECK_ATTRIBUTE_ALIAS

-# Check out sync builtins support.
-LIBGFOR_CHECK_SYNC_FETCH_AND_ADD
+# Check out atomic builtins support.
+LIBGFOR_CHECK_ATOMIC_FETCH_ADD

# Check out #pragma weak.
LIBGFOR_GTHREAD_WEAK
diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
index 902eb412848..36245cbff1f 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -961,8 +961,8 @@ internal_proto(free_ionml);
static inline void
inc_waiting_locked (gfc_unit *u)
{
-#ifdef HAVE_SYNC_FETCH_AND_ADD
- (void) __sync_fetch_and_add (&u->waiting, 1);
+#ifdef HAVE_ATOMIC_FETCH_ADD
+ (void) __atomic_fetch_add (&u->waiting, 1, __ATOMIC_ACQ_REL);
#else
u->waiting++;
#endif
@@ -971,8 +971,8 @@ inc_waiting_locked (gfc_unit *u)
static inline int
predec_waiting_locked (gfc_unit *u)
{
-#ifdef HAVE_SYNC_FETCH_AND_ADD
- return __sync_add_and_fetch (&u->waiting, -1);
+#ifdef HAVE_ATOMIC_FETCH_ADD
+ return __atomic_add_fetch (&u->waiting, -1, __ATOMIC_ACQ_REL);
#else
return --u->waiting;
#endif
@@ -981,8 +981,8 @@ predec_waiting_locked (gfc_unit *u)
static inline void
dec_waiting_unlocked (gfc_unit *u)
{
-#ifdef HAVE_SYNC_FETCH_AND_ADD
- (void) __sync_fetch_and_add (&u->waiting, -1);
+#ifdef HAVE_ATOMIC_FETCH_ADD
+ (void) __atomic_fetch_add (&u->waiting, -1, __ATOMIC_ACQ_REL);
#else
__gthread_mutex_lock (&unit_lock);
u->waiting--;

[Jakub Jelinek]

Post by Janne Blomqvist
The old __sync builtins have been deprecated for a long time now in
favor of the __atomic builtins following the C++11/C11 memory model.
This patch converts libgfortran to use the modern __atomic builtins.
At the same time I weakened the consistency to acquire-release as that
should be enough for this particular case. Jakub, as the original
author of the algorithm, do you concur?

As mentioned on IRC, I think this could use just __ATOMIC_RELAXED,
but am not 100% sure.

Post by Janne Blomqvist
* acinclude.m4 (LIBGFOR_CHECK_ATOMIC_FETCH_ADD): Rename and test
presence of atomic builtins instead of sync builtins.
* configure.ac (LIBGFOR_CHECK_ATOMIC_FETCH_ADD): Call new test.
* io/io.h (inc_waiting_locked): Use __atomic_fetch_add.
(predec_waiting_locked): Use __atomic_add_fetch.
(dec_waiting_unlocked): Use __atomic_fetch_add.
* config.h.in: Regenerated.
* configure: Regenerated.

Jakub

[Janne Blomqvist]

The old __sync builtins have been deprecated for a long time now in
favor of the __atomic builtins following the C++11/C11 memory model.
This patch converts libgfortran to use the modern __atomic builtins.

At the same time I weakened the consistency to relaxed for
incrementing and decrementing the counter, and acquire-release when
decrementing to check whether the counter is 0 and the unit can be
freed. This is similar to e.g. std::shared_ptr in C++. Jakub, as the
original author of the algorithm, do you concur?

Regtested on x86_64-pc-linux-gnu, Ok for trunk?

libgfortran/ChangeLog:

2018-11-12 Janne Blomqvist <***@gcc.gnu.org>

* acinclude.m4 (LIBGFOR_CHECK_ATOMIC_FETCH_ADD): Rename and test
presence of atomic builtins instead of sync builtins.
* configure.ac (LIBGFOR_CHECK_ATOMIC_FETCH_ADD): Call new test.
* io/io.h (inc_waiting_locked): Use __atomic_fetch_add.
(predec_waiting_locked): Use __atomic_add_fetch.
(dec_waiting_unlocked): Use __atomic_fetch_add.
* config.h.in: Regenerated.
* configure: Regenerated.
---
libgfortran/acinclude.m4 | 20 ++++++++++----------
libgfortran/configure.ac | 4 ++--
libgfortran/io/io.h | 24 ++++++++++++++++++------
3 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/libgfortran/acinclude.m4 b/libgfortran/acinclude.m4
index dd5429ac0d2..5b0c094e716 100644
--- a/libgfortran/acinclude.m4
+++ b/libgfortran/acinclude.m4
@@ -59,17 +59,17 @@ extern void bar(void) __attribute__((alias("foo")));]],
[Define to 1 if the target supports __attribute__((alias(...))).])
fi])

-dnl Check whether the target supports __sync_fetch_and_add.
-AC_DEFUN([LIBGFOR_CHECK_SYNC_FETCH_AND_ADD], [
- AC_CACHE_CHECK([whether the target supports __sync_fetch_and_add],
- libgfor_cv_have_sync_fetch_and_add, [
+dnl Check whether the target supports __atomic_fetch_add.
+AC_DEFUN([LIBGFOR_CHECK_ATOMIC_FETCH_ADD], [
+ AC_CACHE_CHECK([whether the target supports __atomic_fetch_add],
+ libgfor_cv_have_atomic_fetch_add, [
AC_LINK_IFELSE([AC_LANG_PROGRAM([[int foovar = 0;]], [[
-if (foovar <= 0) return __sync_fetch_and_add (&foovar, 1);
-if (foovar > 10) return __sync_add_and_fetch (&foovar, -1);]])],
- libgfor_cv_have_sync_fetch_and_add=yes, libgfor_cv_have_sync_fetch_and_add=no)])
- if test $libgfor_cv_have_sync_fetch_and_add = yes; then
- AC_DEFINE(HAVE_SYNC_FETCH_AND_ADD, 1,
- [Define to 1 if the target supports __sync_fetch_and_add])
+if (foovar <= 0) return __atomic_fetch_add (&foovar, 1, __ATOMIC_ACQ_REL);
+if (foovar > 10) return __atomic_add_fetch (&foovar, -1, __ATOMIC_ACQ_REL);]])],
+ libgfor_cv_have_atomic_fetch_add=yes, libgfor_cv_have_atomic_fetch_add=no)])
+ if test $libgfor_cv_have_atomic_fetch_add = yes; then
+ AC_DEFINE(HAVE_ATOMIC_FETCH_ADD, 1,
+ [Define to 1 if the target supports __atomic_fetch_add])
fi])

dnl Check for pragma weak.
diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac
index 76007d38f6f..30ff8734760 100644
--- a/libgfortran/configure.ac
+++ b/libgfortran/configure.ac
@@ -608,8 +608,8 @@ fi
LIBGFOR_CHECK_ATTRIBUTE_VISIBILITY
LIBGFOR_CHECK_ATTRIBUTE_ALIAS

-# Check out sync builtins support.
-LIBGFOR_CHECK_SYNC_FETCH_AND_ADD
+# Check out atomic builtins support.
+LIBGFOR_CHECK_ATOMIC_FETCH_ADD

# Check out #pragma weak.
LIBGFOR_GTHREAD_WEAK
diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
index 902eb412848..282c1455763 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -961,8 +961,8 @@ internal_proto(free_ionml);
static inline void
inc_waiting_locked (gfc_unit *u)
{
-#ifdef HAVE_SYNC_FETCH_AND_ADD
- (void) __sync_fetch_and_add (&u->waiting, 1);
+#ifdef HAVE_ATOMIC_FETCH_ADD
+ (void) __atomic_fetch_add (&u->waiting, 1, __ATOMIC_RELAXED);
#else
u->waiting++;
#endif
@@ -971,8 +971,20 @@ inc_waiting_locked (gfc_unit *u)
static inline int
predec_waiting_locked (gfc_unit *u)
{
-#ifdef HAVE_SYNC_FETCH_AND_ADD
- return __sync_add_and_fetch (&u->waiting, -1);
+#ifdef HAVE_ATOMIC_FETCH_ADD
+ /* Note that the pattern
+
+ if (predec_waiting_locked (u) == 0)
+ // destroy u
+
+ could be further optimized by making this be an __ATOMIC_RELEASE,
+ and then inserting a
+
+ __atomic_thread_fence (__ATOMIC_ACQUIRE);
+
+ inside the branch before destroying. But for now, lets keep it
+ simple. */
+ return __atomic_add_fetch (&u->waiting, -1, __ATOMIC_ACQ_REL);
#else
return --u->waiting;
#endif
@@ -981,8 +993,8 @@ predec_waiting_locked (gfc_unit *u)
static inline void
dec_waiting_unlocked (gfc_unit *u)
{
-#ifdef HAVE_SYNC_FETCH_AND_ADD
- (void) __sync_fetch_and_add (&u->waiting, -1);
+#ifdef HAVE_ATOMIC_FETCH_ADD
+ (void) __atomic_fetch_add (&u->waiting, -1, __ATOMIC_RELAXED);
#else
__gthread_mutex_lock (&unit_lock);
u->waiting--;

[Janne Blomqvist]

PING!

Post by Janne Blomqvist
The old __sync builtins have been deprecated for a long time now in
favor of the __atomic builtins following the C++11/C11 memory model.
This patch converts libgfortran to use the modern __atomic builtins.
At the same time I weakened the consistency to relaxed for
incrementing and decrementing the counter, and acquire-release when
decrementing to check whether the counter is 0 and the unit can be
freed. This is similar to e.g. std::shared_ptr in C++. Jakub, as the
original author of the algorithm, do you concur?
Regtested on x86_64-pc-linux-gnu, Ok for trunk?
* acinclude.m4 (LIBGFOR_CHECK_ATOMIC_FETCH_ADD): Rename and test
presence of atomic builtins instead of sync builtins.
* configure.ac (LIBGFOR_CHECK_ATOMIC_FETCH_ADD): Call new test.
* io/io.h (inc_waiting_locked): Use __atomic_fetch_add.
(predec_waiting_locked): Use __atomic_add_fetch.
(dec_waiting_unlocked): Use __atomic_fetch_add.
* config.h.in: Regenerated.
* configure: Regenerated.
---
libgfortran/acinclude.m4 | 20 ++++++++++----------
libgfortran/configure.ac | 4 ++--
libgfortran/io/io.h | 24 ++++++++++++++++++------
3 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/libgfortran/acinclude.m4 b/libgfortran/acinclude.m4
index dd5429ac0d2..5b0c094e716 100644
--- a/libgfortran/acinclude.m4
+++ b/libgfortran/acinclude.m4
@@ -59,17 +59,17 @@ extern void bar(void) __attribute__((alias("foo")));]],
[Define to 1 if the target supports __attribute__((alias(...))).])
fi])
-dnl Check whether the target supports __sync_fetch_and_add.
-AC_DEFUN([LIBGFOR_CHECK_SYNC_FETCH_AND_ADD], [
- AC_CACHE_CHECK([whether the target supports __sync_fetch_and_add],
- libgfor_cv_have_sync_fetch_and_add, [
+dnl Check whether the target supports __atomic_fetch_add.
+AC_DEFUN([LIBGFOR_CHECK_ATOMIC_FETCH_ADD], [
+ AC_CACHE_CHECK([whether the target supports __atomic_fetch_add],
+ libgfor_cv_have_atomic_fetch_add, [
AC_LINK_IFELSE([AC_LANG_PROGRAM([[int foovar = 0;]], [[
-if (foovar <= 0) return __sync_fetch_and_add (&foovar, 1);
-if (foovar > 10) return __sync_add_and_fetch (&foovar, -1);]])],
- libgfor_cv_have_sync_fetch_and_add=yes,
libgfor_cv_have_sync_fetch_and_add=no)])
- if test $libgfor_cv_have_sync_fetch_and_add = yes; then
- AC_DEFINE(HAVE_SYNC_FETCH_AND_ADD, 1,
- [Define to 1 if the target supports __sync_fetch_and_add])
+if (foovar <= 0) return __atomic_fetch_add (&foovar, 1, __ATOMIC_ACQ_REL);
+if (foovar > 10) return __atomic_add_fetch (&foovar, -1,
__ATOMIC_ACQ_REL);]])],
+ libgfor_cv_have_atomic_fetch_add=yes,
libgfor_cv_have_atomic_fetch_add=no)])
+ if test $libgfor_cv_have_atomic_fetch_add = yes; then
+ AC_DEFINE(HAVE_ATOMIC_FETCH_ADD, 1,
+ [Define to 1 if the target supports __atomic_fetch_add])
fi])
dnl Check for pragma weak.
diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac
index 76007d38f6f..30ff8734760 100644
--- a/libgfortran/configure.ac
+++ b/libgfortran/configure.ac
@@ -608,8 +608,8 @@ fi
LIBGFOR_CHECK_ATTRIBUTE_VISIBILITY
LIBGFOR_CHECK_ATTRIBUTE_ALIAS
-# Check out sync builtins support.
-LIBGFOR_CHECK_SYNC_FETCH_AND_ADD
+# Check out atomic builtins support.
+LIBGFOR_CHECK_ATOMIC_FETCH_ADD
# Check out #pragma weak.
LIBGFOR_GTHREAD_WEAK
diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
index 902eb412848..282c1455763 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -961,8 +961,8 @@ internal_proto(free_ionml);
static inline void
inc_waiting_locked (gfc_unit *u)
{
-#ifdef HAVE_SYNC_FETCH_AND_ADD
- (void) __sync_fetch_and_add (&u->waiting, 1);
+#ifdef HAVE_ATOMIC_FETCH_ADD
+ (void) __atomic_fetch_add (&u->waiting, 1, __ATOMIC_RELAXED);
#else
u->waiting++;
#endif
@@ -971,8 +971,20 @@ inc_waiting_locked (gfc_unit *u)
static inline int
predec_waiting_locked (gfc_unit *u)
{
-#ifdef HAVE_SYNC_FETCH_AND_ADD
- return __sync_add_and_fetch (&u->waiting, -1);
+#ifdef HAVE_ATOMIC_FETCH_ADD
+ /* Note that the pattern
+
+ if (predec_waiting_locked (u) == 0)
+ // destroy u
+
+ could be further optimized by making this be an __ATOMIC_RELEASE,
+ and then inserting a
+
+ __atomic_thread_fence (__ATOMIC_ACQUIRE);
+
+ inside the branch before destroying. But for now, lets keep it
+ simple. */
+ return __atomic_add_fetch (&u->waiting, -1, __ATOMIC_ACQ_REL);
#else
return --u->waiting;
#endif
@@ -981,8 +993,8 @@ predec_waiting_locked (gfc_unit *u)
static inline void
dec_waiting_unlocked (gfc_unit *u)
{
-#ifdef HAVE_SYNC_FETCH_AND_ADD
- (void) __sync_fetch_and_add (&u->waiting, -1);
+#ifdef HAVE_ATOMIC_FETCH_ADD
+ (void) __atomic_fetch_add (&u->waiting, -1, __ATOMIC_RELAXED);
#else
__gthread_mutex_lock (&unit_lock);
u->waiting--;
--
2.17.1

[Thomas Koenig]

Hi Janne,

PING!

OK.

Thanks for the patch!

Regards

Thomas