[Bug target/96898] New: [nvptx] libatomic support

[Bug target/96898] New: [nvptx] libatomic support

[vries at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96898

            Bug ID: 96898
           Summary: [nvptx] libatomic support
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: vries at gcc dot gnu.org
  Target Milestone: ---

When building gcc for nvptx, we get:
...
checking for libatomic support... no
...

As mentioned here (
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553142.html  ), could
be useful.

[Bug target/96898] [nvptx] libatomic support

[vries at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96898

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at redhat dot com

--- Comment #1 from Tom de Vries <vries at gcc dot gnu.org> ---
Hmm, so libatomic needs to fall back onto protect_start and protect_end.

It would make sense for the openmp/openacc programs to have that map onto
GOMP_start/GOMP_stop.

But this introduces a dependency of libatomic on libgomp.  Not ideal, I
suppose.

Maybe we could (at least for the nvptx case) move the global lock out of
libgomp into libatomic, and have a dependency of libgomp on libatomic instead.

[Bug target/96898] [nvptx] libatomic support

[vries at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96898

--- Comment #2 from Tom de Vries <vries at gcc dot gnu.org> ---
Hmm, I found this difference: 
- AFAIU, GOMP_atomic_start/end have barrier semantics
- libatomics protect_start/end are always paired with explicit barriers, so
  presumably these don't have barrier semantics

So, using GOMP_atomic_start for protect_start in libatomics will have the
effect of issuing the barrier twice, which might be a performance problem.

[Bug target/96898] [nvptx] libatomic support

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96898

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
For OpenMP reductions, we really don't care what kind of mutex protects the
updates, as long as it is the same for all updates of the same reduction.
I believe we don't rely on any other synchronization effects.
So, I think we should change omp-low.c so that it emits __atomic_* calls with
__ATOMIC_RELAXED rather than __sync_* calls.  And could just use libatomic with
its own locking if we didn't go the GOMP_atomic_{start,end} route (that one is
done if there are multiple reductions or the atomics aren't available or there
are user defined reductions we don't understand (or all?), perhaps we should
consider also using atomics perhaps even for two simple reductions or similar.
And nvptx certainly could just use libatomic...

[Bug target/96898] [nvptx] libatomic support

[vries at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96898

--- Comment #4 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #3)
> For OpenMP reductions, we really don't care what kind of mutex protects the
> updates, as long as it is the same for all updates of the same reduction.
> I believe we don't rely on any other synchronization effects.
> So, I think we should change omp-low.c so that it emits __atomic_* calls
> with __ATOMIC_RELAXED rather than __sync_* calls.

That sounds like a good idea.

> And could just use
> libatomic with its own locking if we didn't go the GOMP_atomic_{start,end}
> route (that one is done if there are multiple reductions or the atomics
> aren't available or there are user defined reductions we don't understand
> (or all?), perhaps we should consider also using atomics perhaps even for
> two simple reductions or similar.
> And nvptx certainly could just use libatomic...

If we use libatomic as fallback for openmp, shouldn't we then use the same lock
in both?

[Bug target/96898] [nvptx] libatomic support

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96898

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
It wouldn't be a fallback.  omp-low.c just decides if it is going to use
GOMP_atomic_{start,end} synchronization, __atomic_* or __sync_* to perform the
reduction.  And whether that uses the same or different lock doesn't matter,
because for one reduction omp-low.c will only use one way.

[Bug target/96898] [nvptx] libatomic support

[vries at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96898

--- Comment #6 from Tom de Vries <vries at gcc dot gnu.org> ---
Created attachment 49195
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49195&action=edit
Tentative patch

Introduces an option -fatomic-libcalls (analogous to -fsync-libcalls) such that
__atomic_test_and_set maps onto libatomic function __atomic_test_and_set_1.

[Bug target/96898] [nvptx] libatomic support

[vries at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96898

--- Comment #7 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Tom de Vries from comment #6)
> Created attachment 49195 [details]
> Tentative patch
> 
> Introduces an option -fatomic-libcalls (analogous to -fsync-libcalls) such
> that __atomic_test_and_set maps onto libatomic function
> __atomic_test_and_set_1.

I've now achieved the same in the target, not relying on a new option:
...
diff --git a/gcc/config/nvptx/nvptx.md b/gcc/config/nvptx/nvptx.md
index 4168190fa42..6178e6a0f77 100644
--- a/gcc/config/nvptx/nvptx.md
+++ b/gcc/config/nvptx/nvptx.md
@@ -1667,6 +1667,22 @@
   "%.\\tatom%A1.b%T0.<logic>\\t%0, %1, %2;"
   [(set_attr "atomic" "true")])

+(define_expand "atomic_test_and_set"
+  [(match_operand:SI 0 "nvptx_register_operand")       ;; output
+   (match_operand:QI 1 "memory_operand")               ;; memory
+   (match_operand:SI 2 "const_int_operand")]           ;; model
+  ""
+{
+  rtx libfunc;
+  rtx addr;
+  libfunc = init_one_libfunc ("__atomic_test_and_set_1");
+  addr = convert_memory_address (ptr_mode, XEXP (operands[1], 0));
+  emit_library_call_value (libfunc, operands[0], LCT_NORMAL, SImode,
+                         addr, ptr_mode,
+                         operands[2], SImode);
+  DONE;
+})
+
 (define_insn "nvptx_barsync"
   [(unspec_volatile [(match_operand:SI 0 "nvptx_nonmemory_operand" "Ri")
                     (match_operand:SI 1 "const_int_operand")]
...

[Bug target/96898] [nvptx] libatomic support

[vries at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96898

--- Comment #8 from Tom de Vries <vries at gcc dot gnu.org> ---
Patch submitted:
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553393.html

[Bug target/96898] [nvptx] libatomic support

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96898

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tom de Vries <vries@gcc.gnu.org>:

https://gcc.gnu.org/g:15545563128f0240192c263522d4a36b7f86250f

commit r11-3145-g15545563128f0240192c263522d4a36b7f86250f
Author: Tom de Vries <tdevries@suse.de>
Date:   Mon Sep 7 10:47:25 2020 +0200

    [libatomic] Add nvptx support

    Add nvptx support to libatomic.

    Given that atomic_test_and_set is not implemented for nvptx (PR96964), the
    compiler translates __atomic_test_and_set falling back onto the "Failing
all
    else, assume a single threaded environment and simply perform the
operation"
    case in expand_atomic_test_and_set, so it doesn't map onto an actual atomic
    operation.

    Still, that counts as supported for the configure test of libatomic, so we
    end up with HAVE_ATOMIC_TAS_1/2/4/8/16 == 1, and the corresponding
    __atomic_test_and_set_1/2/4/8/16 in libatomic all using that non-atomic
    implementation.

    Fix this by adding an atomic_test_and_set expansion for nvptx, that uses
    libatomics __atomic_test_and_set_1.

    This again makes the configure tests for HAVE_ATOMIC_TAS_1/2/4/8/16 fail,
so
    instead we use this case in tas_n.c:
    ...
    /* If this type is smaller than word-sized, fall back to a word-sized
       compare-and-swap loop.  */
    bool
    SIZE(libat_test_and_set) (UTYPE *mptr, int smodel)
    ...
    which for __atomic_test_and_set_8 uses INVERT_MASK_8.

    Add INVERT_MASK_8 in libatomic_i.h, as well as MASK_8.

    Tested libatomic testsuite on nvptx.

    gcc/ChangeLog:

            PR target/96964
            * config/nvptx/nvptx.md (define_expand "atomic_test_and_set"): New
            expansion.

    libatomic/ChangeLog:

            PR target/96898
            * configure.tgt: Add nvptx.
            * libatomic_i.h (MASK_8, INVERT_MASK_8): New macro definition.
            * config/nvptx/host-config.h: New file.
            * config/nvptx/lock.c: New file.

[Bug target/96898] [nvptx] libatomic support

[vries at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96898

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|---                         |11.0

--- Comment #10 from Tom de Vries <vries at gcc dot gnu.org> ---
Patch committed, marking resolved-fixed.