Hi Julian! On 2020-06-18T19:21:57+0100, Julian Brown wrote: > On Tue, 9 Jun 2020 12:41:21 +0200 > Thomas Schwinge wrote: >> On 2020-06-05T21:31:08+0100, Julian Brown >> wrote: >> > On Fri, 5 Jun 2020 13:17:09 +0200 >> > Thomas Schwinge wrote: >> >> On 2019-12-17T21:03:47-0800, Julian Brown >> >> wrote: >> >> > This part contains the libgomp runtime support for the >> >> > GOMP_MAP_ATTACH and GOMP_MAP_DETACH mapping kinds >> >> >> >> > --- a/libgomp/target.c >> >> > +++ b/libgomp/target.c >> >> >> >> > @@ -1203,6 +1211,32 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep, >> >> >> >> > + 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; >> >> > + } >> >> >> >> For the OpenACC runtime API 'acc_attach' etc. routines they don't, >> >> so what's the conceptual reason that for the corresponding OpenACC >> >> directive variants, 'GOMP_MAP_ATTACH' etc. here participate in >> >> reference counting ('n->refcount++' above)? I understand OpenACC >> >> 'attach'/'detach' clauses to be simple "executable clauses", which >> >> just update some values somewhere (say, like >> >> 'GOMP_MAP_ALWAYS_POINTER'), but they don't alter any mapping state, >> >> thus wouldn't appear to need reference counting? >> > >> > IIUC, n->refcount is not directly the "structural reference count" >> > as seen at source level, but rather counts the number of >> > target_var_descs in the lists appended to each target_mem_desc -- >> > and GOMP_MAP_ATTACH have variable entries in those lists. >> >> That may be OK if that's purely an implementation detail that isn't >> visible to the user, however: >> >> > That's not the case for the API >> > routines. >> >> As I had mentioned, the problem is: in contrast to 'acc_attach', an >> OpenACC 'enter data' directive with 'attach' clause currently uses >> this same reference-counted code path, and thus such an 'attach' >> without corresponding 'detach' inhibits unmapping; [...] > > The attached patch stops attach/detach operations from affecting > reference counts (either structured or dynamic). This isn't as invasive > as I'd imagined: we can extend the use of the "do_detach" flag in > target_mem_descs' variable lists to mark mappings that correspond to > attach operations, then use that flag to avoid refcount > increment/decrements. Thanks, ACK. > (The flag should possibly be renamed now.) How about: - /* True if variable should be detached at end of region. */ - bool do_detach; + /* True if this is for OpenACC 'attach'. */ + bool is_attach; (Changing that similarly is obvious/pre-approved.) > Tested with offloading to NVPTX. OK? I've adjusted the patch for current GCC sources, and did some further changes/cleanup; see below, and attached "[OpenACC] Deep copy attach/detach should not affect reference counts". If you're happy with that, that's OK for master and releases/gcc-10 (once un-frozen) branches. > --- a/libgomp/oacc-mem.c > +++ b/libgomp/oacc-mem.c > @@ -1131,7 +1134,9 @@ goacc_enter_data_internal (struct gomp_device_descr *acc_dev, size_t mapnum, > if (tgt->list[j].key == n) > { > for (size_t k = 0; k < groupnum; k++) > - if (j + k < tgt->list_count && tgt->list[j + k].key) > + if (j + k < tgt->list_count > + && tgt->list[j + k].key > + && !tgt->list[j + k].do_detach) > { > tgt->list[j + k].key->refcount++; > tgt->list[j + k].key->dynamic_refcount++; > @@ -1156,7 +1161,7 @@ goacc_enter_data_internal (struct gomp_device_descr *acc_dev, size_t mapnum, > for (size_t j = 0; j < tgt->list_count; j++) > { > n = tgt->list[j].key; > - if (n) > + if (n && !tgt->list[j].do_detach) > n->dynamic_refcount++; > } > } If I understand correctly, relatedly, we can also "strengthen" the 'is_tgt_unmapped' checking (nowadays centralized in 'goacc_exit_datum_1') by excluding any 'do_detach' ones from '++num_mappings'. Done. > --- a/libgomp/target.c > +++ b/libgomp/target.c > @@ -382,7 +382,7 @@ gomp_map_vars_existing (struct gomp_device_descr *devicep, > (void *) newn->host_start, > newn->host_end - newn->host_start, cbuf); > > - if (oldn->refcount != REFCOUNT_INFINITY) > + if (oldn->refcount != REFCOUNT_INFINITY && kind != GOMP_MAP_ATTACH) > oldn->refcount++; > } That's always-true. Removed. > --- /dev/null > +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/attach-detach-rc-1.c > @@ -0,0 +1,50 @@ > +/* { dg-skip-if "" { *-*-* } { "-DACC_MEM_SHARED=1" } } */ > + > +#include > +#include > + > +#define N 1024 > + > +struct mystr { > + int pad; > + int *data; > +}; The 'pad' is no longer needed with PR95270 "OpenACC 'enter data attach' looks up target memory object displaced by pointer size" fixed. > +[...] > --- /dev/null > +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/attach-detach-rc-2.c > @@ -0,0 +1,4 @@ > +/* { dg-skip-if "" { *-*-* } { "-DACC_MEM_SHARED=1" } } */ > +/* { dg-additional-options "-DATTACH_VIA_DIRECTIVE" } */ > + > +#include "attach-detach-rc-1.c" I've merged/extended 'libgomp.oacc-c-c++-common/attach-detach-rc-1.c', 'libgomp.oacc-c-c++-common/attach-detach-rc-2.c' into 'libgomp.oacc-c-c++-common/mdc-refcount-1.c', and further added 'libgomp.oacc-c-c++-common/mdc-refcount-2.c', and 'libgomp.oacc-c-c++-common/mdc-refcount-3.c'. Grüße Thomas ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter