Hi Jakub! On 2019-11-06T18:43:39+0000, Julian Brown <jul...@codesourcery.com> wrote: > In particular, [a new big patch] incorporates the idea [...] > relating to adding the new "attach_count" field to the memory-mapping > splay tree key type without growing that structure in the common case > (i.e. when structure components are not being mapped, or for OpenMP). > In short, a new auxiliary structure is added containing the previous > "link_key" and "attach_count" fields: so, you can either have both > pointers (though of course one of them may be NULL), or in the common > case no aux pointer at all, so no growth in the base struct size.
That was in response to: > On Fri, 18 Oct 2019 18:47:08 +0200 > Thomas Schwinge <tho...@codesourcery.com> wrote: >> While reviewing >> <http://mid.mail-archive.com/20191003163505.49997-2-julian@codesourcery.com> >> "OpenACC reference count overhaul", I've just now stumbled over one >> thing that originally was designed here: >> >> On 2018-12-10T19:41:37+0000, Julian Brown <jul...@codesourcery.com> >> wrote: >> > On Fri, 7 Dec 2018 14:50:19 +0100 >> > Jakub Jelinek <ja...@redhat.com> wrote: >> > >> >> On Fri, Nov 30, 2018 at 03:41:09AM -0800, Julian Brown wrote: >> >> > @@ -918,8 +920,13 @@ struct splay_tree_key_s { >> >> > uintptr_t tgt_offset; >> >> > /* Reference count. */ >> >> > uintptr_t refcount; >> >> > - /* Dynamic reference count. */ >> >> > - uintptr_t dynamic_refcount; >> >> > + /* Reference counts beyond those that represent genuine references >> >> > in the >> >> > + linked splay tree key/target memory structures, e.g. for multiple >> >> > OpenACC >> >> > + "present increment" operations (via "acc enter data") refering to >> >> > the same >> >> > + host-memory block. */ >> >> > + uintptr_t virtual_refcount; >> >> > + /* For a block with attached pointers, the attachment counters for >> >> > each. */ >> >> > + unsigned short *attach_count; >> >> > /* Pointer to the original mapping of "omp declare target link" >> >> > object. */ >> >> > splay_tree_key link_key; >> >> > }; >> >> >> >> This is something I'm worried about a lot, the nodes keep growing >> >> way too much. >> >> Is that just a would-be-nice-to-avoid, or is it an actual problem? >> >> If the latter, can we maybe move some data into on-the-side data >> structures, say an associative array keyed by [something suitable]? I >> would assume that compared to actual host to/from device data movement >> (or even lookup etc.), lookup of values from such an associative array >> should be relatively cheap? > > I'd be extremely wary of adding a completely separate off-the-side > structure to keep track of attachment counters: the reference-counting > behaviour is already complicated enough, and the risk of messing things > up with another indirectly-linked structure to keep track of is too > high (never mind the extra runtime overhead). (Well, ACK; it was just an idea to think in a different direction maybe.) > With the approach in this > patch, at least the extra info for link_key/attach_count is directly > accessible from the splay tree key struct via pointer indirection. > > This version entails slight additional overhead (another malloc'd > block and another pointer indirection) for the link_key field (and also > for the attach_count pointer). I've not benchmarked memory use or > performance though, so I'm not sure how much impact this has on real > code. I extracted the changes related to that from Julian's big patch, see attached "In 'libgomp/target.c', 'struct splay_tree_key_s', use 'struct splay_tree_aux' for infrequently-used or API-specific data ". Is this OK to commit? If approving this patch, please respond with "Reviewed-by: NAME <EMAIL>" so that your effort will be recorded in the commit log, see <https://gcc.gnu.org/wiki/Reviewed-by>. As part of the OpenACC manual deep copy changes, we'll then later apply: struct splay_tree_aux { /* Pointer to the original mapping of "omp declare target link" object. */ splay_tree_key link_key; + /* For a block with attached pointers, the attachment counters for each. + Only used for OpenACC. */ + uintptr_t *attach_count; }; Grüße Thomas
From 6f81ae8189c5a53d9ab414363bfefd249b78e7c1 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tho...@codesourcery.com>
Date: Tue, 17 Dec 2019 16:11:40 +0100
Subject: [PATCH] In 'libgomp/target.c', 'struct splay_tree_key_s', use 'struct
splay_tree_aux' for infrequently-used or API-specific data
---
libgomp/libgomp.h | 10 ++++++++--
libgomp/target.c | 23 ++++++++++++++++-------
2 files changed, 24 insertions(+), 9 deletions(-)
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index b2cd07dfa67..d65a1fa250b 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -989,6 +989,13 @@ struct target_mem_desc {
#define OFFSET_POINTER (~(uintptr_t) 1)
#define OFFSET_STRUCT (~(uintptr_t) 2)
+/* Auxiliary structure for infrequently-used or API-specific data. */
+
+struct splay_tree_aux {
+ /* Pointer to the original mapping of "omp declare target link" object. */
+ splay_tree_key link_key;
+};
+
struct splay_tree_key_s {
/* Address of the host object. */
uintptr_t host_start;
@@ -1002,8 +1009,7 @@ struct splay_tree_key_s {
uintptr_t refcount;
/* Dynamic reference count. */
uintptr_t dynamic_refcount;
- /* Pointer to the original mapping of "omp declare target link" object. */
- splay_tree_key link_key;
+ struct splay_tree_aux *aux;
};
/* The comparison function. */
diff --git a/libgomp/target.c b/libgomp/target.c
index 795b9351134..d00334ce9e6 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -972,13 +972,15 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
kind & typemask, cbufp);
else
{
- k->link_key = NULL;
+ 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->link_key = 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;
@@ -1096,7 +1098,7 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
kind);
}
- if (k->link_key)
+ if (k->aux && k->aux->link_key)
{
/* Set link pointer on target to the device address of the
mapped object. */
@@ -1211,8 +1213,14 @@ gomp_remove_var_internal (struct gomp_device_descr *devicep, splay_tree_key k,
{
bool is_tgt_unmapped = false;
splay_tree_remove (&devicep->mem_map, k);
- if (k->link_key)
- splay_tree_insert (&devicep->mem_map, (splay_tree_node) k->link_key);
+ if (k->aux)
+ {
+ if (k->aux->link_key)
+ splay_tree_insert (&devicep->mem_map,
+ (splay_tree_node) k->aux->link_key);
+ free (k->aux);
+ k->aux = NULL;
+ }
if (aq)
devicep->openacc.async.queue_callback_func (aq, gomp_unref_tgt_void,
(void *) k->tgt);
@@ -1439,7 +1447,7 @@ gomp_load_image_to_device (struct gomp_device_descr *devicep, unsigned version,
k->tgt_offset = target_table[i].start;
k->refcount = REFCOUNT_INFINITY;
k->dynamic_refcount = 0;
- k->link_key = NULL;
+ k->aux = NULL;
array->left = NULL;
array->right = NULL;
splay_tree_insert (&devicep->mem_map, array);
@@ -1472,7 +1480,7 @@ 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->dynamic_refcount = 0;
- k->link_key = NULL;
+ k->aux = NULL;
array->left = NULL;
array->right = NULL;
splay_tree_insert (&devicep->mem_map, array);
@@ -2742,6 +2750,7 @@ omp_target_associate_ptr (const void *host_ptr, const void *device_ptr,
k->tgt_offset = (uintptr_t) device_ptr + device_offset;
k->refcount = REFCOUNT_INFINITY;
k->dynamic_refcount = 0;
+ k->aux = NULL;
array->left = NULL;
array->right = NULL;
splay_tree_insert (&devicep->mem_map, array);
--
2.17.1
signature.asc
Description: PGP signature
