On Tue, 24 Oct 2017, Jakub Jelinek wrote:
> loop transfering the addresses or firstprivate_int values to the device
> - where we issued mapnum host2dev transfers each just pointer-sized
> when we could have just prepared all the pointers in an array and host2dev
> copy them all together.
Can you please give an example OpenMP code? I thought such variables are
just fields of one omp_data_? struct that is copied all at once, but I guess
I'm misunderstanding.
> Thoughts on this?
I need some time to understand the patch well, at the moment I have just
a couple superficial comments, below.
> --- libgomp/target.c.jj 2017-04-20 14:59:08.296263304 +0200
> +++ libgomp/target.c 2017-10-23 19:08:14.348336118 +0200
> @@ -177,10 +177,77 @@ gomp_device_copy (struct gomp_device_des
> }
> }
>
> +struct gomp_map_cache
> +{
> + void *buf;
> + struct target_mem_desc *tgt;
> + size_t *chunks;
> + long chunk_cnt;
> + long use_cnt;
> +};
Would really appreciate comments for meaning of fields here. Also, is the
struct properly named? From the patch description I understood it to be a
copy coalescing buffer, not a cache.
> @@ -449,19 +531,34 @@ gomp_map_vars (struct gomp_device_descr
> size_t align = (size_t) 1 << (kind >> rshift);
> if (tgt_align < align)
> tgt_align = align;
> - tgt_size -= (uintptr_t) hostaddrs[first]
> - - (uintptr_t) hostaddrs[i];
> + tgt_size -= (uintptr_t) hostaddrs[first] - cur_node.host_start;
> tgt_size = (tgt_size + align - 1) & ~(align - 1);
> - tgt_size += cur_node.host_end - (uintptr_t) hostaddrs[i];
> + tgt_size += cur_node.host_end - cur_node.host_start;
> not_found_cnt += last - i;
> for (i = first; i <= last; i++)
> - tgt->list[i].key = NULL;
> + {
> + tgt->list[i].key = NULL;
> + switch (get_kind (short_mapkind, kinds, i) & typemask)
> + {
> + case GOMP_MAP_ALLOC:
> + case GOMP_MAP_FROM:
> + case GOMP_MAP_FORCE_ALLOC:
> + case GOMP_MAP_ALWAYS_FROM:
> + break;
> + default:
> + /* All the others copy data if newly allocated. */
> + gomp_cache_add (&cache, tgt_size - cur_node.host_end
> + + (uintptr_t) hostaddrs[i],
> + sizes[i]);
A similar switch needed to be duplicated below. Would it be appropriate to
pass the map kind to gomp_cache_add, or have a thin wrapper around it to have
checks for appropriate kinds in one place?
Thanks.
Alexander
