[07/13] OpenACC 2.6 deep copy: libgomp parts

Message ID 65540b92dff74db1f15af930f87f7096d03e7efe.1576648001.git.julian@codesourcery.com
State New
Headers show
Series
  • OpenACC 2.6 manual deep copy support
Related show

Commit Message

Julian Brown Dec. 18, 2019, 6:03 a.m.
This patch has been broken out of the "OpenACC 2.6 manual deep copy
support" patch, last posted here:

  https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02376.html

This part contains the libgomp runtime support for the GOMP_MAP_ATTACH and
GOMP_MAP_DETACH mapping kinds (etc.), as introduced by the front-end
patches following in this series.

Tested alongside other patches in this series with offloading to
NVPTX. OK?

Thanks,

Julian

ChangeLog

	include/
	* gomp-constants.h (GOMP_MAP_FLAG_SPECIAL_4, GOMP_MAP_DEEP_COPY):
	Define.
	(gomp_map_kind): Add GOMP_MAP_ATTACH, GOMP_MAP_DETACH,
	GOMP_MAP_FORCE_DETACH.

	libgomp/
	* libgomp.h (struct target_var_desc): Add do_detach flag.
	* oacc-init.c (acc_shutdown_1): Free aux block if present.
	* oacc-mem.c (find_group_last): Add SIZES parameter. Support
	struct components.  Tidy up and add some new checks.
	(goacc_enter_data_internal): Update call to find_group_last.
	(goacc_exit_data_internal): Support detach operations and
	GOMP_MAP_STRUCT.
	(GOACC_enter_exit_data): Handle initial GOMP_MAP_STRUCT or
	GOMP_MAP_FORCE_PRESENT in finalization detection code.  Handle
	attach/detach in enter/exit data detection code.
	* target.c (gomp_map_vars_existing): Initialise do_detach field of
	tgt_var_desc.
	(gomp_map_vars_internal): Support attach.
	(gomp_unmap_vars_internal): Support detach.
---
 include/gomp-constants.h |  10 ++++
 libgomp/libgomp.h        |   2 +
 libgomp/oacc-mem.c       | 121 +++++++++++++++++++++++++++++++++------
 libgomp/target.c         |  51 ++++++++++++++++-
 4 files changed, 166 insertions(+), 18 deletions(-)

-- 
2.23.0

Comments

Thomas Schwinge Dec. 21, 2019, 11:01 p.m. | #1
Hi!

On 2019-12-17T22:03:47-0800, Julian Brown <julian@codesourcery.com> wrote:
> This part contains the libgomp runtime support for the GOMP_MAP_ATTACH and

> GOMP_MAP_DETACH mapping kinds (etc.), as introduced by the front-end

> patches following in this series.


This (r279625) regressed the same OpenMP 'omp declare target link'
functionality/test case that I previously discussed in
<http://mid.mail-archive.com/87pniuuhkj.fsf@euler.schwinge.homeip.net>,
and/or
<http://mid.mail-archive.com/87k18vu1zr.fsf@euler.schwinge.homeip.net>:

    PASS: libgomp.c/target-link-1.c (test for excess errors)
    [-PASS:-]{+FAIL:+} libgomp.c/target-link-1.c execution test

(With nvptx offloading configured, as mentioned before, this test case
doesn't even build -- <https://gcc.gnu.org/PR81689> -- so, yes, this is
clearly insufficient test coverage of the 'omp declare target link'
functionality, but still we shouldn't regress it.)

What's causing the regression is:

> --- a/libgomp/target.c

> +++ b/libgomp/target.c

> @@ -1247,10 +1281,12 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,


|  		k->aux = NULL;
|  		if (n && n->refcount == REFCOUNT_LINK)
|  		  {
|  		    /* Replace target address of the pointer with target address
|  		       of mapped object in the splay tree.  */
|  		    splay_tree_remove (mem_map, n);
|  		    k->aux
|  		      = gomp_malloc_cleared (sizeof (struct splay_tree_aux));
|  		    k->aux->link_key = n;
|  		  }
|  		size_t align = (size_t) 1 << (kind >> rshift);
|  		tgt->list[i].key = k;
|  		k->tgt = tgt;
|  		if (field_tgt_clear != FIELD_TGT_EMPTY)
|  		  {
|  		    k->tgt_offset = k->host_start - field_tgt_base
|  				    + field_tgt_offset;
|  		    if (i == field_tgt_clear)
|  		      field_tgt_clear = FIELD_TGT_EMPTY;
|  		  }
|  		else
|  		  {
|  		    tgt_size = (tgt_size + align - 1) & ~(align - 1);
|  		    k->tgt_offset = tgt_size;
|  		    tgt_size += k->host_end - k->host_start;
|  		  }
>  		tgt->list[i].copy_from = GOMP_MAP_COPY_FROM_P (kind & typemask);

>  		tgt->list[i].always_copy_from

>  		  = GOMP_MAP_ALWAYS_FROM_P (kind & typemask);

> +		tgt->list[i].do_detach = false;

>  		tgt->list[i].offset = 0;

>  		tgt->list[i].length = k->host_end - k->host_start;

>  		k->refcount = 1;

>  		k->virtual_refcount = 0;

> +		k->aux = NULL;

>  		tgt->refcount++;

>  		array->left = NULL;

>  		array->right = NULL;


... that latter 'k->aux = NULL' assignment, which invalidates what the
'REFCOUNT_LINK' handling earlier set up.

I had intentionally left out this assignment in my "In
'libgomp/target.c', 'struct splay_tree_key_s', use 'struct
splay_tree_aux' for infrequently-used or API-specific data" patch,
<http://mid.mail-archive.com/87k16uykb7.fsf@euler.schwinge.homeip.net>,
and you also don't have that assignment in your r279620 "Use aux struct
in libgomp for infrequently-used/API-specific data" commit,
<http://mid.mail-archive.com/80e0dba326a4414fd2dbe8401dbd8d8f08445129.1576648001.git.julian@codesourcery.com>,
so curious why it now appears here -- hopefully just an oversight.

See attached "[OMP] Restore 'omp declare target link' handling";
committed to trunk in r279701.


Grüße
 Thomas
From dc669e8f45a5b1e61e746b6d2c6a23480fdd904f Mon Sep 17 00:00:00 2001
From: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>

Date: Sat, 21 Dec 2019 22:58:43 +0000
Subject: [PATCH] [OMP] Restore 'omp declare target link' handling

    PASS: libgomp.c/target-link-1.c (test for excess errors)
    [-PASS:-]{+FAIL:+} libgomp.c/target-link-1.c execution test

We need to revert one line of code change from r279625.

	libgomp/
	* target.c (gomp_map_vars_internal): Restore 'omp declare target
	link' handling.

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@279701 138bc75d-0d04-0410-961f-82ee72b054a4
---
 libgomp/ChangeLog | 5 +++++
 libgomp/target.c  | 1 -
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/libgomp/ChangeLog b/libgomp/ChangeLog
index 81b9d6788a1..7bc7d41da42 100644
--- a/libgomp/ChangeLog
+++ b/libgomp/ChangeLog
@@ -1,3 +1,8 @@
+2019-12-21  Thomas Schwinge  <thomas@codesourcery.com>
+
+	* target.c (gomp_map_vars_internal): Restore 'omp declare target
+	link' handling.
+
 2019-12-19  Julian Brown  <julian@codesourcery.com>
 
 	* testsuite/libgomp.oacc-fortran/class-ptr-param.f95: New test.
diff --git a/libgomp/target.c b/libgomp/target.c
index 50a9c2b1df3..bf30716cd85 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -1129,7 +1129,6 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
 		tgt->list[i].length = k->host_end - k->host_start;
 		k->refcount = 1;
 		k->virtual_refcount = 0;
-		k->aux = NULL;
 		tgt->refcount++;
 		array->left = NULL;
 		array->right = NULL;
-- 
2.17.1
Julian Brown Jan. 3, 2020, 12:24 p.m. | #2
Hi,

On Sun, 22 Dec 2019 00:01:10 +0100
Thomas Schwinge <thomas@codesourcery.com> wrote:

> I had intentionally left out this assignment in my "In

> 'libgomp/target.c', 'struct splay_tree_key_s', use 'struct

> splay_tree_aux' for infrequently-used or API-specific data" patch,

> <http://mid.mail-archive.com/87k16uykb7.fsf@euler.schwinge.homeip.net>,

> and you also don't have that assignment in your r279620 "Use aux

> struct in libgomp for infrequently-used/API-specific data" commit,

> <http://mid.mail-archive.com/80e0dba326a4414fd2dbe8401dbd8d8f08445129.1576648001.git.julian@codesourcery.com>,

> so curious why it now appears here -- hopefully just an oversight.


This was just an oversight (or a mismerge, perhaps). Thanks for fixing!

Cheers,

Julian

Patch

diff --git a/include/gomp-constants.h b/include/gomp-constants.h
index 9e356cdfeec..e8bd52e81bd 100644
--- a/include/gomp-constants.h
+++ b/include/gomp-constants.h
@@ -40,8 +40,11 @@ 
 #define GOMP_MAP_FLAG_SPECIAL_0		(1 << 2)
 #define GOMP_MAP_FLAG_SPECIAL_1		(1 << 3)
 #define GOMP_MAP_FLAG_SPECIAL_2		(1 << 4)
+#define GOMP_MAP_FLAG_SPECIAL_4		(1 << 6)
 #define GOMP_MAP_FLAG_SPECIAL		(GOMP_MAP_FLAG_SPECIAL_1 \
 					 | GOMP_MAP_FLAG_SPECIAL_0)
+#define GOMP_MAP_DEEP_COPY		(GOMP_MAP_FLAG_SPECIAL_4 \
+					 | GOMP_MAP_FLAG_SPECIAL_2)
 /* Flag to force a specific behavior (or else, trigger a run-time error).  */
 #define GOMP_MAP_FLAG_FORCE		(1 << 7)
 
@@ -127,6 +130,13 @@  enum gomp_map_kind
     /* Decrement usage count and deallocate if zero.  */
     GOMP_MAP_RELEASE =			(GOMP_MAP_FLAG_SPECIAL_2
 					 | GOMP_MAP_DELETE),
+    /* In OpenACC, attach a pointer to a mapped struct field.  */
+    GOMP_MAP_ATTACH =			(GOMP_MAP_DEEP_COPY | 0),
+    /* In OpenACC, detach a pointer to a mapped struct field.  */
+    GOMP_MAP_DETACH =			(GOMP_MAP_DEEP_COPY | 1),
+    /* In OpenACC, detach a pointer to a mapped struct field.  */
+    GOMP_MAP_FORCE_DETACH =		(GOMP_MAP_DEEP_COPY
+					 | GOMP_MAP_FLAG_FORCE | 1),
 
     /* Internal to GCC, not used in libgomp.  */
     /* Do not map, but pointer assign a pointer instead.  */
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index 2017991b59c..6141cc117bc 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -948,6 +948,8 @@  struct target_var_desc {
   bool copy_from;
   /* True if data always should be copied from device to host at the end.  */
   bool always_copy_from;
+  /* True if variable should be detached at end of region.  */
+  bool do_detach;
   /* Relative offset against key host_start.  */
   uintptr_t offset;
   /* Actual length.  */
diff --git a/libgomp/oacc-mem.c b/libgomp/oacc-mem.c
index 08507791399..ce9f2759dfa 100644
--- a/libgomp/oacc-mem.c
+++ b/libgomp/oacc-mem.c
@@ -956,33 +956,48 @@  acc_detach_finalize_async (void **hostaddr, int async)
    mappings.  */
 
 static int
-find_group_last (int pos, size_t mapnum, unsigned short *kinds)
+find_group_last (int pos, size_t mapnum, size_t *sizes, unsigned short *kinds)
 {
   unsigned char kind0 = kinds[pos] & 0xff;
-  int first_pos = pos, last_pos = pos;
+  int first_pos = pos;
 
-  if (kind0 == GOMP_MAP_TO_PSET)
+  switch (kind0)
     {
+    case GOMP_MAP_TO_PSET:
       while (pos + 1 < mapnum && (kinds[pos + 1] & 0xff) == GOMP_MAP_POINTER)
-	last_pos = ++pos;
+	pos++;
       /* We expect at least one GOMP_MAP_POINTER after a GOMP_MAP_TO_PSET.  */
-      assert (last_pos > first_pos);
-    }
-  else
-    {
+      assert (pos > first_pos);
+      break;
+
+    case GOMP_MAP_STRUCT:
+      pos += sizes[pos];
+      break;
+
+    case GOMP_MAP_POINTER:
+    case GOMP_MAP_ALWAYS_POINTER:
+      /* These mappings are only expected after some other mapping.  If we
+	 see one by itself, something has gone wrong.  */
+      gomp_fatal ("unexpected mapping");
+      break;
+
+    default:
       /* GOMP_MAP_ALWAYS_POINTER can only appear directly after some other
 	 mapping.  */
-      if (pos + 1 < mapnum
-	  && (kinds[pos + 1] & 0xff) == GOMP_MAP_ALWAYS_POINTER)
-	return pos + 1;
+      if (pos + 1 < mapnum)
+	{
+	  unsigned char kind1 = kinds[pos + 1] & 0xff;
+	  if (kind1 == GOMP_MAP_ALWAYS_POINTER)
+	    return pos + 1;
+	}
 
-      /* We can have one or several GOMP_MAP_POINTER mappings after a to/from
+      /* We can have zero or more GOMP_MAP_POINTER mappings after a to/from
 	 (etc.) mapping.  */
       while (pos + 1 < mapnum && (kinds[pos + 1] & 0xff) == GOMP_MAP_POINTER)
-	last_pos = ++pos;
+	pos++;
     }
 
-  return last_pos;
+  return pos;
 }
 
 /* Map variables for OpenACC "enter data".  We can't just call
@@ -996,7 +1011,7 @@  goacc_enter_data_internal (struct gomp_device_descr *acc_dev, size_t mapnum,
 {
   for (size_t i = 0; i < mapnum; i++)
     {
-      int group_last = find_group_last (i, mapnum, kinds);
+      int group_last = find_group_last (i, mapnum, sizes, kinds);
 
       gomp_map_vars_async (acc_dev, aq,
 			   (group_last - i) + 1,
@@ -1018,6 +1033,33 @@  goacc_exit_data_internal (struct gomp_device_descr *acc_dev, size_t mapnum,
 {
   gomp_mutex_lock (&acc_dev->lock);
 
+  /* Handle "detach" before copyback/deletion of mapped data.  */
+  for (size_t i = 0; i < mapnum; ++i)
+    {
+      unsigned char kind = kinds[i] & 0xff;
+      switch (kind)
+	{
+	case GOMP_MAP_DETACH:
+	case GOMP_MAP_FORCE_DETACH:
+	  {
+	    struct splay_tree_key_s cur_node;
+	    uintptr_t hostaddr = (uintptr_t) hostaddrs[i];
+	    cur_node.host_start = hostaddr;
+	    cur_node.host_end = cur_node.host_start + sizeof (void *);
+	    splay_tree_key n
+	      = splay_tree_lookup (&acc_dev->mem_map, &cur_node);
+
+	    if (n == NULL)
+	      gomp_fatal ("struct not mapped for detach operation");
+
+	    gomp_detach_pointer (acc_dev, aq, n, hostaddr, finalize, NULL);
+	  }
+	  break;
+	default:
+	  ;
+	}
+    }
+
   for (size_t i = 0; i < mapnum; ++i)
     {
       unsigned char kind = kinds[i] & 0xff;
@@ -1035,6 +1077,8 @@  goacc_exit_data_internal (struct gomp_device_descr *acc_dev, size_t mapnum,
 	case GOMP_MAP_POINTER:
 	case GOMP_MAP_DELETE:
 	case GOMP_MAP_RELEASE:
+	case GOMP_MAP_DETACH:
+	case GOMP_MAP_FORCE_DETACH:
 	  {
 	    struct splay_tree_key_s cur_node;
 	    cur_node.host_start = (uintptr_t) hostaddrs[i];
@@ -1075,6 +1119,39 @@  goacc_exit_data_internal (struct gomp_device_descr *acc_dev, size_t mapnum,
 	      gomp_remove_var_async (acc_dev, n, aq);
 	  }
 	  break;
+
+	case GOMP_MAP_STRUCT:
+	  {
+	    int elems = sizes[i];
+	    for (int j = 1; j <= elems; j++)
+	      {
+		struct splay_tree_key_s k;
+		k.host_start = (uintptr_t) hostaddrs[i + j];
+		k.host_end = k.host_start + sizes[i + j];
+		splay_tree_key str;
+		str = splay_tree_lookup (&acc_dev->mem_map, &k);
+		if (str)
+		  {
+		    if (finalize)
+		      {
+			str->refcount -= str->virtual_refcount;
+			str->virtual_refcount = 0;
+		      }
+		    if (str->virtual_refcount > 0)
+		      {
+			str->refcount--;
+			str->virtual_refcount--;
+		      }
+		    else if (str->refcount > 0)
+		      str->refcount--;
+		    if (str->refcount == 0)
+		      gomp_remove_var_async (acc_dev, str, aq);
+		  }
+	      }
+	    i += elems;
+	  }
+	  break;
+
 	default:
 	  gomp_fatal (">>>> goacc_exit_data_internal UNHANDLED kind 0x%.2x",
 			  kind);
@@ -1107,8 +1184,13 @@  GOACC_enter_exit_data (int flags_m, size_t mapnum, void **hostaddrs,
   if (mapnum > 0)
     {
       unsigned char kind = kinds[0] & 0xff;
+
+      if (kind == GOMP_MAP_STRUCT || kind == GOMP_MAP_FORCE_PRESENT)
+	kind = kinds[1] & 0xff;
+
       if (kind == GOMP_MAP_DELETE
-	  || kind == GOMP_MAP_FORCE_FROM)
+	  || kind == GOMP_MAP_FORCE_FROM
+	  || kind == GOMP_MAP_FORCE_DETACH)
 	finalize = true;
     }
 
@@ -1117,11 +1199,14 @@  GOACC_enter_exit_data (int flags_m, size_t mapnum, void **hostaddrs,
     {
       unsigned char kind = kinds[i] & 0xff;
 
-      if (kind == GOMP_MAP_POINTER || kind == GOMP_MAP_TO_PSET)
+      if (kind == GOMP_MAP_POINTER
+	  || kind == GOMP_MAP_TO_PSET
+	  || kind == GOMP_MAP_STRUCT)
 	continue;
 
       if (kind == GOMP_MAP_FORCE_ALLOC
 	  || kind == GOMP_MAP_FORCE_PRESENT
+	  || kind == GOMP_MAP_ATTACH
 	  || kind == GOMP_MAP_FORCE_TO
 	  || kind == GOMP_MAP_TO
 	  || kind == GOMP_MAP_ALLOC)
@@ -1132,6 +1217,8 @@  GOACC_enter_exit_data (int flags_m, size_t mapnum, void **hostaddrs,
 
       if (kind == GOMP_MAP_RELEASE
 	  || kind == GOMP_MAP_DELETE
+	  || kind == GOMP_MAP_DETACH
+	  || kind == GOMP_MAP_FORCE_DETACH
 	  || kind == GOMP_MAP_FROM
 	  || kind == GOMP_MAP_FORCE_FROM)
 	break;
diff --git a/libgomp/target.c b/libgomp/target.c
index 1f429900113..6fa94dec6ce 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -540,6 +540,7 @@  gomp_map_vars_existing (struct gomp_device_descr *devicep,
   tgt_var->key = oldn;
   tgt_var->copy_from = GOMP_MAP_COPY_FROM_P (kind);
   tgt_var->always_copy_from = GOMP_MAP_ALWAYS_FROM_P (kind);
+  tgt_var->do_detach = kind == GOMP_MAP_ATTACH;
   tgt_var->offset = newn->host_start - oldn->host_start;
   tgt_var->length = newn->host_end - newn->host_start;
 
@@ -978,8 +979,15 @@  gomp_map_vars_internal (struct gomp_device_descr *devicep,
 	  has_firstprivate = true;
 	  continue;
 	}
+      else if ((kind & typemask) == GOMP_MAP_ATTACH)
+	{
+	  tgt->list[i].key = NULL;
+	  has_firstprivate = true;
+	  continue;
+	}
       cur_node.host_start = (uintptr_t) hostaddrs[i];
-      if (!GOMP_MAP_POINTER_P (kind & typemask))
+      if (!GOMP_MAP_POINTER_P (kind & typemask)
+	  && (kind & typemask) != GOMP_MAP_ATTACH)
 	cur_node.host_end = cur_node.host_start + sizes[i];
       else
 	cur_node.host_end = cur_node.host_start + sizeof (void *);
@@ -1203,6 +1211,32 @@  gomp_map_vars_internal (struct gomp_device_descr *devicep,
 		cur_node.tgt_offset = n->tgt->tgt_start + n->tgt_offset
 				      + cur_node.host_start - n->host_start;
 		continue;
+	      case GOMP_MAP_ATTACH:
+		{
+		  cur_node.host_start = (uintptr_t) hostaddrs[i];
+		  cur_node.host_end = cur_node.host_start + sizeof (void *);
+		  splay_tree_key n = splay_tree_lookup (mem_map, &cur_node);
+		  if (n != NULL)
+		    {
+		      tgt->list[i].key = n;
+		      tgt->list[i].offset = cur_node.host_start - n->host_start;
+		      tgt->list[i].length = n->host_end - n->host_start;
+		      tgt->list[i].copy_from = false;
+		      tgt->list[i].always_copy_from = false;
+		      tgt->list[i].do_detach
+			= (pragma_kind != GOMP_MAP_VARS_OPENACC_ENTER_DATA);
+		      n->refcount++;
+		    }
+		  else
+		    {
+		      gomp_mutex_unlock (&devicep->lock);
+		      gomp_fatal ("outer struct not mapped for attach");
+		    }
+		  gomp_attach_pointer (devicep, aq, mem_map, n,
+				       (uintptr_t) hostaddrs[i], sizes[i],
+				       cbufp);
+		  continue;
+		}
 	      default:
 		break;
 	      }
@@ -1247,10 +1281,12 @@  gomp_map_vars_internal (struct gomp_device_descr *devicep,
 		tgt->list[i].copy_from = GOMP_MAP_COPY_FROM_P (kind & typemask);
 		tgt->list[i].always_copy_from
 		  = GOMP_MAP_ALWAYS_FROM_P (kind & typemask);
+		tgt->list[i].do_detach = false;
 		tgt->list[i].offset = 0;
 		tgt->list[i].length = k->host_end - k->host_start;
 		k->refcount = 1;
 		k->virtual_refcount = 0;
+		k->aux = NULL;
 		tgt->refcount++;
 		array->left = NULL;
 		array->right = NULL;
@@ -1301,6 +1337,7 @@  gomp_map_vars_internal (struct gomp_device_descr *devicep,
 			  tgt->list[j].key = k;
 			  tgt->list[j].copy_from = false;
 			  tgt->list[j].always_copy_from = false;
+			  tgt->list[j].do_detach = false;
 			  if (k->refcount != REFCOUNT_INFINITY)
 			    k->refcount++;
 			  gomp_map_pointer (tgt, aq,
@@ -1534,6 +1571,18 @@  gomp_unmap_vars_internal (struct target_mem_desc *tgt, bool do_copyfrom,
     }
 
   size_t i;
+
+  /* We must perform detachments before any copies back to the host.  */
+  for (i = 0; i < tgt->list_count; i++)
+    {
+      splay_tree_key k = tgt->list[i].key;
+
+      if (k != NULL && tgt->list[i].do_detach)
+	gomp_detach_pointer (devicep, aq, k, tgt->list[i].key->host_start
+					     + tgt->list[i].offset,
+			     k->refcount == 1, NULL);
+    }
+
   for (i = 0; i < tgt->list_count; i++)
     {
       splay_tree_key k = tgt->list[i].key;