On Tue, Jun 30, 2015 at 03:19:30PM +0300, Ilya Verbin wrote:
> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -580,10 +581,16 @@ gomp_unmap_vars (struct target_mem_desc *tgt, bool
> do_copyfrom)
> bool do_unmap = false;
> if (k->refcount > 1)
> k->refcount--;
> - else if (k->async_refcount > 0)
> - k->async_refcount--;
> - else
> - do_unmap = true;
> + else if (k->refcount == 1)
> + {
> + if (k->async_refcount > 0)
> + k->async_refcount--;
> + else
> + {
> + k->refcount--;
> + do_unmap = true;
> + }
> + }
What is the rationale of this hunk change?
BTW, we'll likely need to treat also refcount == INT_MAX as special (never
decrease it), because I believe declare target vars are supposed to have
refcount of infinity rather than just 2GB-1.
> @@ -1160,13 +1167,61 @@ GOMP_target_enter_exit_data (int device, size_t
> mapnum, void **hostaddrs,
> }
>
> if (is_enter_data)
> - {
> - /* TODO */
> - }
> + gomp_map_vars (devicep, mapnum, hostaddrs, NULL, sizes, kinds, true,
> false);
This will leak the return value. Either we need to arrange not to allocate
it for enter data, or we need to assign it to some variable and free
immediately (we don't want to perform the release operations for it).
> else
> - {
> - /* TODO */
> - }
> + for (i = 0; i < mapnum; i++)
> + {
> + struct splay_tree_key_s cur_node;
> + unsigned char kind = kinds[i] & typemask;
> + switch (kind)
> + {
> + case GOMP_MAP_FROM:
> + case GOMP_MAP_ALWAYS_FROM:
> + case GOMP_MAP_DELETE:
> + case GOMP_MAP_RELEASE:
> + cur_node.host_start = (uintptr_t) hostaddrs[i];
> + cur_node.host_end = cur_node.host_start + sizes[i];
> + gomp_mutex_lock (&devicep->lock);
I don't really like locking the mutex for each map clause in exit data
separately. Perhaps just add a gomp_exit_data function similar to
gomp_map_vars that will run this loop and be surrounded by the locking,
or do it inline, but with the lock/unlock around the whole loop.
exit data construct must have at least one map clause, so it doesn't make
sense not to lock immediately.
> + splay_tree_key k = splay_tree_lookup (&devicep->mem_map, &cur_node);
> + if (!k)
> + {
> + gomp_mutex_unlock (&devicep->lock);
> + continue;
> + }
> +
> + if (k->refcount > 0)
> + k->refcount--;
> + if (kind == GOMP_MAP_DELETE)
> + k->refcount = 0;
See above, I believe delete should not delete refcount == INT_MAX
mappings.
Jakub
