]> git.ipfire.org Git - thirdparty/gcc.git/commitdiff
Fix OpenACC "ephemeral" asynchronous host-to-device copies
authorJulian Brown <julian@codesourcery.com>
Tue, 29 Jun 2021 23:42:03 +0000 (16:42 -0700)
committerThomas Schwinge <thomas@codesourcery.com>
Tue, 27 Jul 2021 09:16:27 +0000 (11:16 +0200)
This patch fixes several places in libgomp/target.c where "ephemeral" data
(on the stack or in temporary heap locations) may be used as the source of
an asynchronous host-to-device copy that may not complete before the host
data disappears.

An existing, but flawed, workaround for this problem in the AMD GCN
libgomp offloading plugin is currently present on mainline, and was
posted for the og9 branch here:

  https://gcc.gnu.org/legacy-ml/gcc-patches/2019-08/msg00901.html

and previous versions of this patch were posted here (for mainline/og9):

  https://gcc.gnu.org/legacy-ml/gcc-patches/2019-11/msg01482.html
  https://gcc.gnu.org/legacy-ml/gcc-patches/2019-09/msg01026.html

libgomp/
* libgomp.h (gomp_copy_host2dev): Update prototype.
* oacc-mem.c (memcpy_tofrom_device, update_dev_host): Add new
argument to gomp_copy_host2dev (false).
* plugin/plugin-gcn.c (struct copy_data): Remove free_src field.
(copy_data): Don't free src.
(queue_push_copy): Remove free_src handling.
(GOMP_OFFLOAD_dev2dev): Update call to queue_push_copy.
(GOMP_OFFLOAD_openacc_async_host2dev): Remove source-data
snapshotting.
(GOMP_OFFLOAD_openacc_async_dev2host): Update call to
queue_push_copy.
* target.c (goacc_device_copy_async): Add SRCADDR_ORIG parameter.
(gomp_copy_host2dev): Add EPHEMERAL parameter.  Snapshot source
data when true, and set up deferred freeing of temporary buffer.
(gomp_copy_dev2host): Update call to goacc_device_copy_async.
(gomp_map_vars_existing, gomp_map_pointer, gomp_attach_pointer)
(gomp_detach_pointer, gomp_map_vars_internal, gomp_update): Update
calls to gomp_copy_host2dev with appropriate ephemeral argument.
* testsuite/libgomp.oacc-c-c++-common/async-data-1-1.c: Remove
XFAIL.

Co-Authored-By: Thomas Schwinge <thomas@codesourcery.com>
libgomp/libgomp.h
libgomp/oacc-mem.c
libgomp/plugin/plugin-gcn.c
libgomp/target.c
libgomp/testsuite/libgomp.oacc-c-c++-common/async-data-1-1.c

index 8d25dc8e2a8518421bacb091690e28ac0c8fc8a5..e8901da1069737cfd8403e1899be5486eec0b853 100644 (file)
@@ -1226,7 +1226,7 @@ extern void gomp_acc_declare_allocate (bool, size_t, void **, size_t *,
 struct gomp_coalesce_buf;
 extern void gomp_copy_host2dev (struct gomp_device_descr *,
                                struct goacc_asyncqueue *, void *, const void *,
-                               size_t, struct gomp_coalesce_buf *);
+                               size_t, bool, struct gomp_coalesce_buf *);
 extern void gomp_copy_dev2host (struct gomp_device_descr *,
                                struct goacc_asyncqueue *, void *, const void *,
                                size_t);
index c21508f3739986631946c4952502e359ccc51421..5988db0b8867d4a97eabc18906718ab6806930aa 100644 (file)
@@ -202,7 +202,7 @@ memcpy_tofrom_device (bool from, void *d, void *h, size_t s, int async,
   if (from)
     gomp_copy_dev2host (thr->dev, aq, h, d, s);
   else
-    gomp_copy_host2dev (thr->dev, aq, d, h, s, /* TODO: cbuf? */ NULL);
+    gomp_copy_host2dev (thr->dev, aq, d, h, s, false, /* TODO: cbuf? */ NULL);
 
   if (profiling_p)
     {
@@ -874,7 +874,7 @@ update_dev_host (int is_dev, void *h, size_t s, int async)
   goacc_aq aq = get_goacc_asyncqueue (async);
 
   if (is_dev)
-    gomp_copy_host2dev (acc_dev, aq, d, h, s, /* TODO: cbuf? */ NULL);
+    gomp_copy_host2dev (acc_dev, aq, d, h, s, false, /* TODO: cbuf? */ NULL);
   else
     gomp_copy_dev2host (acc_dev, aq, h, d, s);
 
index cfed42a2d4d89e7a7f2885117e64c56aa4fac77e..2548614a2e58cc7b2b2117c8a434ffda5810b229 100644 (file)
@@ -292,7 +292,6 @@ struct copy_data
   void *dst;
   const void *src;
   size_t len;
-  bool free_src;
   struct goacc_asyncqueue *aq;
 };
 
@@ -2914,8 +2913,6 @@ copy_data (void *data_)
             data->aq->agent->device_id, data->aq->id, data->len, data->src,
             data->dst);
   hsa_memory_copy_wrapper (data->dst, data->src, data->len);
-  if (data->free_src)
-    free ((void *) data->src);
   free (data);
 }
 
@@ -2929,12 +2926,11 @@ gomp_offload_free (void *ptr)
 }
 
 /* Request an asynchronous data copy, to or from a device, on a given queue.
-   The event will be registered as a callback.  If FREE_SRC is true
-   then the source data will be freed following the copy.  */
+   The event will be registered as a callback.  */
 
 static void
 queue_push_copy (struct goacc_asyncqueue *aq, void *dst, const void *src,
-                size_t len, bool free_src)
+                size_t len)
 {
   if (DEBUG_QUEUES)
     GCN_DEBUG ("queue_push_copy %d:%d: %zu bytes from (%p) to (%p)\n",
@@ -2944,7 +2940,6 @@ queue_push_copy (struct goacc_asyncqueue *aq, void *dst, const void *src,
   data->dst = dst;
   data->src = src;
   data->len = len;
-  data->free_src = free_src;
   data->aq = aq;
   queue_push_callback (aq, copy_data, data);
 }
@@ -3646,7 +3641,7 @@ GOMP_OFFLOAD_dev2dev (int device, void *dst, const void *src, size_t n)
     {
       struct agent_info *agent = get_agent_info (device);
       maybe_init_omp_async (agent);
-      queue_push_copy (agent->omp_async_queue, dst, src, n, false);
+      queue_push_copy (agent->omp_async_queue, dst, src, n);
       return true;
     }
 
@@ -3916,15 +3911,7 @@ GOMP_OFFLOAD_openacc_async_host2dev (int device, void *dst, const void *src,
 {
   struct agent_info *agent = get_agent_info (device);
   assert (agent == aq->agent);
-  /* The source data does not necessarily remain live until the deferred
-     copy happens.  Taking a snapshot of the data here avoids reading
-     uninitialised data later, but means that (a) data is copied twice and
-     (b) modifications to the copied data between the "spawning" point of
-     the asynchronous kernel and when it is executed will not be seen.
-     But, that is probably correct.  */
-  void *src_copy = GOMP_PLUGIN_malloc (n);
-  memcpy (src_copy, src, n);
-  queue_push_copy (aq, dst, src_copy, n, true);
+  queue_push_copy (aq, dst, src, n);
   return true;
 }
 
@@ -3936,7 +3923,7 @@ GOMP_OFFLOAD_openacc_async_dev2host (int device, void *dst, const void *src,
 {
   struct agent_info *agent = get_agent_info (device);
   assert (agent == aq->agent);
-  queue_push_copy (aq, dst, src, n, false);
+  queue_push_copy (aq, dst, src, n);
   return true;
 }
 
index bb09d501dd69a128ffff97ed636a4f56de194035..5576e57f82280dd53c1b98fc52e77c11de8d45c5 100644 (file)
@@ -214,13 +214,24 @@ goacc_device_copy_async (struct gomp_device_descr *devicep,
                                            struct goacc_asyncqueue *),
                         const char *dst, void *dstaddr,
                         const char *src, const void *srcaddr,
+                        const void *srcaddr_orig,
                         size_t size, struct goacc_asyncqueue *aq)
 {
   if (!copy_func (devicep->target_id, dstaddr, srcaddr, size, aq))
     {
       gomp_mutex_unlock (&devicep->lock);
-      gomp_fatal ("Copying of %s object [%p..%p) to %s object [%p..%p) failed",
-                 src, srcaddr, srcaddr + size, dst, dstaddr, dstaddr + size);
+      if (srcaddr_orig && srcaddr_orig != srcaddr)
+       gomp_fatal ("Copying of %s object [%p..%p)"
+                   " via buffer %s object [%p..%p)"
+                   " to %s object [%p..%p) failed",
+                   src, srcaddr_orig, srcaddr_orig + size,
+                   src, srcaddr, srcaddr + size,
+                   dst, dstaddr, dstaddr + size);
+      else
+       gomp_fatal ("Copying of %s object [%p..%p)"
+                   " to %s object [%p..%p) failed",
+                   src, srcaddr, srcaddr + size,
+                   dst, dstaddr, dstaddr + size);
     }
 }
 
@@ -317,11 +328,16 @@ gomp_to_device_kind_p (int kind)
     }
 }
 
+/* Copy host memory to an offload device.  In asynchronous mode (if AQ is
+   non-NULL), when the source data is stack or may otherwise be deallocated
+   before the asynchronous copy takes place, EPHEMERAL must be passed as
+   TRUE.  */
+
 attribute_hidden void
 gomp_copy_host2dev (struct gomp_device_descr *devicep,
                    struct goacc_asyncqueue *aq,
                    void *d, const void *h, size_t sz,
-                   struct gomp_coalesce_buf *cbuf)
+                   bool ephemeral, struct gomp_coalesce_buf *cbuf)
 {
   if (cbuf)
     {
@@ -349,8 +365,23 @@ gomp_copy_host2dev (struct gomp_device_descr *devicep,
        }
     }
   if (__builtin_expect (aq != NULL, 0))
-    goacc_device_copy_async (devicep, devicep->openacc.async.host2dev_func,
-                            "dev", d, "host", h, sz, aq);
+    {
+      void *h_buf = (void *) h;
+      if (ephemeral)
+       {
+         /* We're queueing up an asynchronous copy from data that may
+            disappear before the transfer takes place (i.e. because it is a
+            stack local in a function that is no longer executing).  Make a
+            copy of the data into a temporary buffer in those cases.  */
+         h_buf = gomp_malloc (sz);
+         memcpy (h_buf, h, sz);
+       }
+      goacc_device_copy_async (devicep, devicep->openacc.async.host2dev_func,
+                              "dev", d, "host", h_buf, h, sz, aq);
+      if (ephemeral)
+       /* Free temporary buffer once the transfer has completed.  */
+       devicep->openacc.async.queue_callback_func (aq, free, h_buf);
+    }
   else
     gomp_device_copy (devicep, devicep->host2dev_func, "dev", d, "host", h, sz);
 }
@@ -362,7 +393,7 @@ gomp_copy_dev2host (struct gomp_device_descr *devicep,
 {
   if (__builtin_expect (aq != NULL, 0))
     goacc_device_copy_async (devicep, devicep->openacc.async.dev2host_func,
-                            "host", h, "dev", d, sz, aq);
+                            "host", h, "dev", d, NULL, sz, aq);
   else
     gomp_device_copy (devicep, devicep->dev2host_func, "host", h, "dev", d, sz);
 }
@@ -521,7 +552,7 @@ gomp_map_vars_existing (struct gomp_device_descr *devicep,
                        (void *) (oldn->tgt->tgt_start + oldn->tgt_offset
                                  + newn->host_start - oldn->host_start),
                        (void *) newn->host_start,
-                       newn->host_end - newn->host_start, cbuf);
+                       newn->host_end - newn->host_start, false, cbuf);
 
   gomp_increment_refcount (oldn, refcount_set);
 }
@@ -548,8 +579,8 @@ gomp_map_pointer (struct target_mem_desc *tgt, struct goacc_asyncqueue *aq,
       cur_node.tgt_offset = (uintptr_t) NULL;
       gomp_copy_host2dev (devicep, aq,
                          (void *) (tgt->tgt_start + target_offset),
-                         (void *) &cur_node.tgt_offset,
-                         sizeof (void *), cbuf);
+                         (void *) &cur_node.tgt_offset, sizeof (void *),
+                         true, cbuf);
       return;
     }
   /* Add bias to the pointer value.  */
@@ -569,7 +600,8 @@ gomp_map_pointer (struct target_mem_desc *tgt, struct goacc_asyncqueue *aq,
      to initialize the pointer with.  */
   cur_node.tgt_offset -= bias;
   gomp_copy_host2dev (devicep, aq, (void *) (tgt->tgt_start + target_offset),
-                     (void *) &cur_node.tgt_offset, sizeof (void *), cbuf);
+                     (void *) &cur_node.tgt_offset, sizeof (void *),
+                     true, cbuf);
 }
 
 static void
@@ -702,7 +734,7 @@ gomp_attach_pointer (struct gomp_device_descr *devicep,
                  (void *) (n->tgt->tgt_start + n->tgt_offset), (void *) data);
 
       gomp_copy_host2dev (devicep, aq, (void *) devptr, (void *) &data,
-                         sizeof (void *), cbufp);
+                         sizeof (void *), true, cbufp);
     }
   else
     gomp_debug (1, "%s: attach count for %p -> %u\n", __FUNCTION__,
@@ -755,7 +787,7 @@ gomp_detach_pointer (struct gomp_device_descr *devicep,
                  (void *) target);
 
       gomp_copy_host2dev (devicep, aq, (void *) devptr, (void *) &target,
-                         sizeof (void *), cbufp);
+                         sizeof (void *), true, cbufp);
     }
   else
     gomp_debug (1, "%s: attach count for %p -> %u\n", __FUNCTION__,
@@ -1218,7 +1250,7 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
                len = sizes[i];
                gomp_copy_host2dev (devicep, aq,
                                    (void *) (tgt->tgt_start + tgt_size),
-                                   (void *) hostaddrs[i], len, cbufp);
+                                   (void *) hostaddrs[i], len, false, cbufp);
                tgt_size += len;
                continue;
              case GOMP_MAP_FIRSTPRIVATE_INT:
@@ -1312,7 +1344,7 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
                                              + cur_node.host_start
                                              - n->host_start),
                                    (void *) &cur_node.tgt_offset,
-                                   sizeof (void *), cbufp);
+                                   sizeof (void *), true, cbufp);
                cur_node.tgt_offset = n->tgt->tgt_start + n->tgt_offset
                                      + cur_node.host_start - n->host_start;
                continue;
@@ -1450,7 +1482,8 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
                                        (void *) (tgt->tgt_start
                                                  + k->tgt_offset),
                                        (void *) k->host_start,
-                                       k->host_end - k->host_start, cbufp);
+                                       k->host_end - k->host_start,
+                                       false, cbufp);
                    break;
                  case GOMP_MAP_POINTER:
                    gomp_map_pointer (tgt, aq,
@@ -1462,7 +1495,8 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
                                        (void *) (tgt->tgt_start
                                                  + k->tgt_offset),
                                        (void *) k->host_start,
-                                       k->host_end - k->host_start, cbufp);
+                                       k->host_end - k->host_start,
+                                       false, cbufp);
                    tgt->list[i].has_null_ptr_assoc = false;
 
                    for (j = i + 1; j < mapnum; j++)
@@ -1525,7 +1559,7 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
                                        (void *) (tgt->tgt_start
                                                  + k->tgt_offset),
                                        (void *) k->host_start,
-                                       sizeof (void *), cbufp);
+                                       sizeof (void *), false, cbufp);
                    break;
                  default:
                    gomp_mutex_unlock (&devicep->lock);
@@ -1541,7 +1575,7 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
                    /* We intentionally do not use coalescing here, as it's not
                       data allocated by the current call to this function.  */
                    gomp_copy_host2dev (devicep, aq, (void *) n->tgt_offset,
-                                       &tgt_addr, sizeof (void *), NULL);
+                                       &tgt_addr, sizeof (void *), true, NULL);
                  }
                array++;
              }
@@ -1556,7 +1590,7 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
          gomp_copy_host2dev (devicep, aq,
                              (void *) (tgt->tgt_start + i * sizeof (void *)),
                              (void *) &cur_node.tgt_offset, sizeof (void *),
-                             cbufp);
+                             true, cbufp);
        }
     }
 
@@ -1568,7 +1602,8 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
                            (void *) (tgt->tgt_start + cbuf.chunks[c].start),
                            (char *) cbuf.buf + (cbuf.chunks[c].start
                                                 - cbuf.chunks[0].start),
-                           cbuf.chunks[c].end - cbuf.chunks[c].start, NULL);
+                           cbuf.chunks[c].end - cbuf.chunks[c].start,
+                           true, NULL);
       free (cbuf.buf);
       cbuf.buf = NULL;
       cbufp = NULL;
@@ -1892,7 +1927,7 @@ gomp_update (struct gomp_device_descr *devicep, size_t mapnum, void **hostaddrs,
 
            if (GOMP_MAP_COPY_TO_P (kind & typemask))
              gomp_copy_host2dev (devicep, NULL, devaddr, hostaddr, size,
-                                 NULL);
+                                 false, NULL);
            if (GOMP_MAP_COPY_FROM_P (kind & typemask))
              gomp_copy_dev2host (devicep, NULL, hostaddr, devaddr, size);
          }
index cd87aec56ff025503415ddb33c0aa2af6180a647..9f2bed8aca82c5ef9fe6ee069440247ac734016a 100644 (file)
@@ -3,8 +3,6 @@
    Due to one data mapping, this isn't using the libgomp 'cbuf' buffering.
 */
 
-/* { dg-xfail-run-if "TODO" { openacc_radeon_accel_selected } } */
-
 
 #include <stdlib.h>