From patchwork Tue Dec 17 16:43:24 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Thomas Schwinge X-Patchwork-Id: 1211563 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-516130-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="b8qnr1TQ"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 47ckVV1DyHz9sRc for ; Wed, 18 Dec 2019 03:43:44 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:in-reply-to:references:date:message-id :mime-version:content-type; q=dns; s=default; b=x1NmsQhftI12+8ED ntjYzMD8rEIWjLbjPydCePjM4PDFjny/YocTrd4d9Kd8yAlSBq9Viyt7D3ZIatB/ 4+tH4vMqEcS1i2pAkIf/3N2Pm9iA0sMMjiq+4S4AwgdfD1x4Q/N7DQ6W5S7/tqdv cO4je3bJ2V3GRLWHx7FaHCrCYB4= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:in-reply-to:references:date:message-id :mime-version:content-type; s=default; bh=hsJBTQmKrEXLGXwcXlxjHs UWfqU=; b=b8qnr1TQeA1c9IY27CefgtAIw9+LBjPvaQeQ9Q3M0LuaPK0ApcG006 4S3INHqaabsAiL6ERJi5TDLPZXNjfRyTfnr8shSfNPko9/nVZvTWcpLRr7iazstU FCda0sojaWXDOxv+z/anA/lnmeSfvUEWXT8VaOaCriskqNlGVwmcc= 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-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@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 17:43:24 +0100 Message-ID: <87k16uykb7.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 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: >> >> On 2018-12-10T19:41:37+0000, Julian Brown >> wrote: >> > On Fri, 7 Dec 2018 14:50:19 +0100 >> > Jakub Jelinek 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 " 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" 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 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