Replace sync builtins with atomic builtins

Message ID 20181112140508.15215-1-blomqvist.janne@gmail.com
State New
Headers show
Series
  • Replace sync builtins with atomic builtins
Related show

Commit Message

Janne Blomqvist Nov. 12, 2018, 2:05 p.m.
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  <jb@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(-)

-- 
2.17.1

Comments

Janne Blomqvist Nov. 19, 2018, 7:37 a.m. | #1
PING!

On Mon, Nov 12, 2018 at 4:05 PM Janne Blomqvist <blomqvist.janne@gmail.com>
wrote:

> 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  <jb@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--;

> --

> 2.17.1

>

>


-- 
Janne Blomqvist
Thomas Koenig Nov. 21, 2018, 7:30 p.m. | #2
Hi Janne,

> PING!


OK.

Thanks for the patch!

Regards

	Thomas

Patch

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--;