From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 104775 invoked by alias); 17 Dec 2019 16:43:37 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 104762 invoked by uid 89); 17 Dec 2019 16:43:36 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-19.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_PASS autolearn=ham version=3.3.1 spammy=growing, genuine, relating, rshift X-HELO: esa3.mentor.iphmx.com Received: from esa3.mentor.iphmx.com (HELO esa3.mentor.iphmx.com) (68.232.137.180) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 17 Dec 2019 16:43:34 +0000 IronPort-SDR: lDsKgbV4hGrRfxl92ZP0F/lDaRGFiR54x1LnsVNU8dNXoM3qdSa5U9rft9KdFy5+7xlg2/EHIj uGmEDiXAH7tIRvUIcKYOvkGq5q8Jqt821xE16lshHjOJJ0xgpxEQmw9lFgKtmaQy66LgHy1LmM PMgC29R727USm1qbH7uqRMZ/qr3tir7y4jgvSmHm+X3O9859h3W9YasAhkmZhzNQtjfrhiiXSl UtTmpeOojNWsYAijOdm2/NoE8ei4U3i0FjNJJZZCBhbvGBGhQ6UV84jdkPZqOoTfVDPtKO5oLS 9ug= Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa3.mentor.iphmx.com with ESMTP; 17 Dec 2019 08:43:32 -0800 IronPort-SDR: 2MAdwmDV1kCIxXpgAwmZ+9jB1xNISv86BiQknWpzWGg1l05UxZN6GXNGY+Rm4B8n4RuIO76FRA nvSJeQKxdgPmli8wbz0pm6fKXNTSNhdgfcd3+328+T32cpHvKu6pzAW4TxhiB3VhyG7tA6jfRR vY0hhlpK1tasVXPWoLYwpRIoUxGGSiq1CBL93iR+y3u3WvUvEBnfnz9LsAdi4PbrfwKnRQ/Hbu 9KesTsIWL2hJrUfdCe7XDVZzI1uJ3/rx5wSpIysJdYB+5x5NgS3c7S+z9/s81boUQN+2o4ac7w suI= From: Thomas Schwinge To: Jakub Jelinek CC: Julian Brown , , Subject: In 'libgomp/target.c', 'struct splay_tree_key_s', use 'struct splay_tree_aux' for infrequently-used or API-specific data (was: [PATCH] OpenACC 2.6 manual deep copy support (attach/detach)) In-Reply-To: <20191106184339.3f5e6430@squid.athome> References: <1543578069-386-1-git-send-email-julian@codesourcery.com> <20181207135019.GI12380@tucnak> <20181210194137.27720f3e@squid.athome> <87pniuuhkj.fsf@euler.schwinge.homeip.net> <20191106184339.3f5e6430@squid.athome> User-Agent: Notmuch/0.29.1+93~g67ed7df (https://notmuchmail.org) Emacs/26.1 (x86_64-pc-linux-gnu) Date: Tue, 17 Dec 2019 16:53:00 -0000 Message-ID: <87k16uykb7.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="==-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Return-Path: tschwing@mentor.com X-SW-Source: 2019-12/txt/msg01209.txt.bz2 --==-=-= Content-Type: multipart/mixed; boundary="=-=-=" --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Content-length: 4283 Hi Jakub! On 2019-11-06T18:43:39+0000, Julian Brown 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 wrote: >> While reviewing >> >> "OpenACC reference count overhaul", I've just now stumbled over one >> thing that originally was designed here: >>=20 >> On 2018-12-10T19:41:37+0000, Julian Brown >> wrote: >> > On Fri, 7 Dec 2018 14:50:19 +0100 >> > Jakub Jelinek wrote: >> >=20=20 >> >> On Fri, Nov 30, 2018 at 03:41:09AM -0800, Julian Brown wrote:=20=20 >> >> > @@ -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 referenc= es in the >> >> > + linked splay tree key/target memory structures, e.g. for mult= iple OpenACC >> >> > + "present increment" operations (via "acc enter data") referin= g to the same >> >> > + host-memory block. */ >> >> > + uintptr_t virtual_refcount; >> >> > + /* For a block with attached pointers, the attachment counters f= or each. */ >> >> > + unsigned short *attach_count; >> >> > /* Pointer to the original mapping of "omp declare target link" = object. */ >> >> > splay_tree_key link_key; >> >> > };=20=20=20=20 >> >>=20 >> >> This is something I'm worried about a lot, the nodes keep growing >> >> way too much.=20=20 >>=20 >> Is that just a would-be-nice-to-avoid, or is it an actual problem? >>=20 >> 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 " so that your effort will be recorded in the commit log, see . 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" obje= ct. */ splay_tree_key link_key; + /* For a block with attached pointers, the attachment counters for e= ach. + Only used for OpenACC. */ + uintptr_t *attach_count; }; Gr=C3=BC=C3=9Fe Thomas --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0001-In-libgomp-target.c-struct-splay_tree_key_s-us.trunk.patch Content-Transfer-Encoding: quoted-printable Content-length: 4137 =46rom 6f81ae8189c5a53d9ab414363bfefd249b78e7c1 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge Date: Tue, 17 Dec 2019 16:11:40 +0100 Subject: [PATCH] In 'libgomp/target.c', 'struct splay_tree_key_s', use 'str= uct 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) =20 +/* 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; }; =20 /* 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 *dev= icep, kind & typemask, cbufp); else { - k->link_key =3D NULL; + k->aux =3D NULL; if (n && n->refcount =3D=3D 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 =3D n; + k->aux + =3D gomp_malloc_cleared (sizeof (struct splay_tree_aux)); + k->aux->link_key =3D n; } size_t align =3D (size_t) 1 << (kind >> rshift); tgt->list[i].key =3D k; @@ -1096,7 +1098,7 @@ gomp_map_vars_internal (struct gomp_device_descr *dev= icep, kind); } =20 - 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 =3D 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 =3D 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 =3D target_table[i].start; k->refcount =3D REFCOUNT_INFINITY; k->dynamic_refcount =3D 0; - k->link_key =3D NULL; + k->aux =3D NULL; array->left =3D NULL; array->right =3D 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 =3D target_var->start; k->refcount =3D target_size & link_bit ? REFCOUNT_LINK : REFCOUNT_IN= FINITY; k->dynamic_refcount =3D 0; - k->link_key =3D NULL; + k->aux =3D NULL; array->left =3D NULL; array->right =3D 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 =3D (uintptr_t) device_ptr + device_offset; k->refcount =3D REFCOUNT_INFINITY; k->dynamic_refcount =3D 0; + k->aux =3D NULL; array->left =3D NULL; array->right =3D NULL; splay_tree_insert (&devicep->mem_map, array); --=20 2.17.1 --=-=-=-- --==-=-= Content-Type: application/pgp-signature; name="signature.asc" Content-length: 658 -----BEGIN PGP SIGNATURE----- iQGzBAEBCgAdFiEEU9WEfWKGQazCmycCAKI7+41Q4XkFAl35BawACgkQAKI7+41Q 4XlOGwv9GM6boSLzacGtn2yYDbxOCEieAwoAJxEwpmwSF2rCk6o/iYK4+A80e7Uc mmXOxXlKunEgEKwotW5v1iic8GGUXCCcUA57Ef2QTog5s5hjq6cw1tFYShOWbB8G 54kKXc+r9Py5/uPetQUl72n0A8xWmBgHf6Gte0R7pq54c1/jdY/n/ninK/86ufbQ ul2z2SquLKoznc9PpNsvVYBUQHLX0jrhjGlhQYM6sqNYW7AKEoA0Xt3scd1C4EgB zHxXqV3gxxF2CqnU3AFgE3TpykRPVgYhb6oxoMHiBHUMcPqlLICDzFKCnZbr+V+u rBZhFX37Pxf6dIUZImLtrWOkJxCo81EjZ7vmsyOjGVtFLS9PP0cda27ppXz/IqfN oOmYl1SXta7rZxkx1VHuTN2wPYSdVwIkl5zhgD5IOJdEIQ3XKan0wjEkxxEQj1Dd 74Rh5LaefXmq+buh8G+m6A0nIRUWvEY6yH4fjminLmvTmPJVRwqvFzKeuabCS+zm omfX2ocw =SRD/ -----END PGP SIGNATURE----- --==-=-=--