On Fri, 5 Jun 2020 12:39:46 +0200
Thomas Schwinge <tho...@codesourcery.com> wrote:
> Hi Julian!
>
> On 2019-12-17T22:03:47-0800, Julian Brown <jul...@codesourcery.com>
> wrote:
> > This part contains the libgomp runtime support for the
> > GOMP_MAP_ATTACH and GOMP_MAP_DETACH mapping kinds
>
> > --- a/libgomp/oacc-mem.c
> > +++ b/libgomp/oacc-mem.c
>
> > @@ -1018,6 +1033,33 @@ goacc_exit_data_internal (struct
> > gomp_device_descr *acc_dev, size_t mapnum, {
> > gomp_mutex_lock (&acc_dev->lock);
> >
> > + /* Handle "detach" before copyback/deletion of mapped data. */
> > + for (size_t i = 0; i < mapnum; ++i)
> > + {
> > + unsigned char kind = kinds[i] & 0xff;
> > + switch (kind)
> > + {
> > + case GOMP_MAP_DETACH:
> > + case GOMP_MAP_FORCE_DETACH:
> > + {
> > + struct splay_tree_key_s cur_node;
> > + uintptr_t hostaddr = (uintptr_t) hostaddrs[i];
> > + cur_node.host_start = hostaddr;
> > + cur_node.host_end = cur_node.host_start + sizeof (void
> > *);
> > + splay_tree_key n
> > + = splay_tree_lookup (&acc_dev->mem_map, &cur_node);
> > +
> > + if (n == NULL)
> > + gomp_fatal ("struct not mapped for detach
> > operation"); +
> > + gomp_detach_pointer (acc_dev, aq, n, hostaddr,
> > finalize, NULL);
> > + }
> > + break;
> > + default:
> > + ;
> > + }
> > + }
> > +
> > for (size_t i = 0; i < mapnum; ++i)
> > {
> > unsigned char kind = kinds[i] & 0xff;
>
> What's the reason that we're not instead emitting any 'detach'
> mappings in the expected order (that is, first), which would avoid
> this double-traversal here? Given that 'mapnum' typically won't
> exceed the dozens, the code we now got doesn't have a big run-time
> cost, of course, but it's still a bit ugly, conceptually, for no
> apparent reason, unless I'm confused?
This is a weakness in the implementation -- the existing code in
gimplify.c that reorders clauses for GOMP_MAP_STRUCT, etc. is
sufficiently fiddly that I didn't want to mess with it. (I suppose
there's no reason the reordering needs to be done entirely in one pass,
though.)
Unfortunately that means (I now realise) that we're locked in
to supporting unordered detach clauses in libgomp going forwards anyway,
even if we fix the ordering in gimplify.c now.
Julian
