[openacc] Fix acc_shutdown issue

Message ID 71cc8f92-6ed7-22a5-ef08-79a3906ffd61@codesourcery.com
State New
Headers show
Series
  • [openacc] Fix acc_shutdown issue
Related show

Commit Message

Cesar Philippidis Sept. 20, 2018, 5:05 p.m.
Attached is an old gomp4 patch that allegedly fixes an shutdown runtime
issue involving OpenACC accelerators. Unfortunately, the original patch
didn't include a test case, nor did it generate any regressions in the
libgomp testsuite when I reverted it in og8.

With that said, I like how this patch eliminates the redundant use of
gomp_mutex_lock to unmap variables (because gomp_unmap_vars already
acquires a lock). However, the trade-off is that it does increase
tgt->list_count to num_funcs + num_vars.

Does anyone have any strong opinion on this patch and is it OK for
trunk? I bootstrapped and regtested it for x86_64 Linux with nvptx
offloading and I didn't encounter any regressions.

Thanks,
Cesar

Comments

Julian Brown Nov. 30, 2018, 12:05 p.m. | #1
On Thu, 20 Sep 2018 10:05:30 -0700
Cesar Philippidis <cesar@codesourcery.com> wrote:

> Attached is an old gomp4 patch that allegedly fixes an shutdown

> runtime issue involving OpenACC accelerators. Unfortunately, the

> original patch didn't include a test case, nor did it generate any

> regressions in the libgomp testsuite when I reverted it in og8.

> 

> With that said, I like how this patch eliminates the redundant use of

> gomp_mutex_lock to unmap variables (because gomp_unmap_vars already

> acquires a lock). However, the trade-off is that it does increase

> tgt->list_count to num_funcs + num_vars.

> 

> Does anyone have any strong opinion on this patch and is it OK for

> trunk? I bootstrapped and regtested it for x86_64 Linux with nvptx

> offloading and I didn't encounter any regressions.


I'd like to withdraw this patch (on behalf of Cesar, who has left
Mentor). It's been superseded by:

  https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02556.html

Thanks,

Julian

Patch

[OpenACC] Fix acc_shutdown issue

2018-XX-YY  James Norris <jnorris@codesourcery.com>
	    Cesar Philippidis  <cesar@codesourcery.com>

	libgomp/
	* oacc-init.c (acc_shutdown_1): Replace use of gomp_free_memmap with
	gomp_unmap_vars.
	* target.c (gomp_load_image_to_device): Fix initialization.
	(gomp_free_memmap): Remove.

(cherry picked from gomp-4_0-branch r226045)
---
 libgomp/libgomp.h   |  1 -
 libgomp/oacc-init.c |  9 ++++++---
 libgomp/target.c    | 27 +++++++++------------------
 3 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index 3a8cc2bd7d6..5c11e97616d 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -1003,7 +1003,6 @@  extern struct target_mem_desc *gomp_map_vars (struct gomp_device_descr *,
 					      enum gomp_map_vars_kind);
 extern void gomp_unmap_vars (struct target_mem_desc *, bool);
 extern void gomp_init_device (struct gomp_device_descr *);
-extern void gomp_free_memmap (struct splay_tree_s *);
 extern void gomp_unload_device (struct gomp_device_descr *);
 extern bool gomp_remove_var (struct gomp_device_descr *, splay_tree_key);
 
diff --git a/libgomp/oacc-init.c b/libgomp/oacc-init.c
index 8842e7218cb..957bb9f31f9 100644
--- a/libgomp/oacc-init.c
+++ b/libgomp/oacc-init.c
@@ -303,9 +303,12 @@  acc_shutdown_1 (acc_device_t d)
 
       if (walk->dev)
 	{
-	  gomp_mutex_lock (&walk->dev->lock);
-	  gomp_free_memmap (&walk->dev->mem_map);
-	  gomp_mutex_unlock (&walk->dev->lock);
+	  while (walk->dev->mem_map.root)
+	    {
+	      struct target_mem_desc *tgt = walk->dev->mem_map.root->key.tgt;
+
+	      gomp_unmap_vars (tgt, false);
+	    }
 
 	  walk->dev = NULL;
 	  walk->base_dev = NULL;
diff --git a/libgomp/target.c b/libgomp/target.c
index dda041cdbef..9ddc8d6c038 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -1184,14 +1184,17 @@  gomp_load_image_to_device (struct gomp_device_descr *devicep, unsigned version,
     }
 
   /* Insert host-target address mapping into splay tree.  */
-  struct target_mem_desc *tgt = gomp_malloc (sizeof (*tgt));
+  struct target_mem_desc *tgt =
+	  gomp_malloc (sizeof (*tgt)
+		       + sizeof (tgt->list[0])
+		       * (num_funcs + num_vars) * sizeof (*tgt->array));
   tgt->array = gomp_malloc ((num_funcs + num_vars) * sizeof (*tgt->array));
   tgt->refcount = REFCOUNT_INFINITY;
   tgt->tgt_start = 0;
   tgt->tgt_end = 0;
   tgt->to_free = NULL;
   tgt->prev = NULL;
-  tgt->list_count = 0;
+  tgt->list_count = num_funcs + num_vars;
   tgt->device_descr = devicep;
   splay_tree_node array = tgt->array;
 
@@ -1204,6 +1207,8 @@  gomp_load_image_to_device (struct gomp_device_descr *devicep, unsigned version,
       k->tgt_offset = target_table[i].start;
       k->refcount = REFCOUNT_INFINITY;
       k->link_key = NULL;
+      tgt->list[i].key = k;
+      tgt->refcount++;
       array->left = NULL;
       array->right = NULL;
       splay_tree_insert (&devicep->mem_map, array);
@@ -1236,6 +1241,8 @@  gomp_load_image_to_device (struct gomp_device_descr *devicep, unsigned version,
       k->tgt_offset = target_var->start;
       k->refcount = target_size & link_bit ? REFCOUNT_LINK : REFCOUNT_INFINITY;
       k->link_key = NULL;
+      tgt->list[i].key = k;
+      tgt->refcount++;
       array->left = NULL;
       array->right = NULL;
       splay_tree_insert (&devicep->mem_map, array);
@@ -1454,22 +1461,6 @@  gomp_unload_device (struct gomp_device_descr *devicep)
     }
 }
 
-/* Free address mapping tables.  MM must be locked on entry, and remains locked
-   on return.  */
-
-attribute_hidden void
-gomp_free_memmap (struct splay_tree_s *mem_map)
-{
-  while (mem_map->root)
-    {
-      struct target_mem_desc *tgt = mem_map->root->key.tgt;
-
-      splay_tree_remove (mem_map, &mem_map->root->key);
-      free (tgt->array);
-      free (tgt);
-    }
-}
-
 /* Host fallback for GOMP_target{,_ext} routines.  */
 
 static void
-- 
2.17.1