Re: [PATCH] Replace sync builtins with atomic builtins

[Janne Blomqvist <blomqvist.janne@gmail.com>]

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