Hi Julian!
On 2019-12-17T22:02:27-0800, Julian Brown <jul...@codesourcery.com> wrote:
> --- a/libgomp/oacc-mem.c
> +++ b/libgomp/oacc-mem.c
(Unhelpful diff trimmed.)
> +/* Unmap variables for OpenACC "exit data", with optional finalization
> + (affecting all mappings in this operation). */
> +static void
> +goacc_exit_data_internal (struct gomp_device_descr *acc_dev, size_t mapnum,
> + void **hostaddrs, size_t *sizes,
> + unsigned short *kinds, bool finalize, goacc_aq aq)
> +{
> + gomp_mutex_lock (&acc_dev->lock);
> + for (size_t i = 0; i < mapnum; ++i)
> {
> + unsigned char kind = kinds[i] & 0xff;
> + bool copyfrom = false;
> + switch (kind)
> + case GOMP_MAP_FROM:
> + case GOMP_MAP_FORCE_FROM:
> + case GOMP_MAP_ALWAYS_FROM:
> + copyfrom = true;
> + /* Fallthrough. */
What is the case that a 'GOMP_MAP_ALWAYS_FROM' would be generated for
OpenACC code? Putting an 'assert' here, it never triggers, given the
current set of libgomp test cases. If there is such a case, we should
add a test case, otherwise, I suggest we do put an 'assert' here (whilst
leaving in the supposedly correct code, if you'd like), to document that
this not currently expected, and thus not tested?
> +
> + case GOMP_MAP_TO_PSET:
> + case GOMP_MAP_POINTER:
> + case GOMP_MAP_DELETE:
> + case GOMP_MAP_RELEASE:
> + {
> + struct splay_tree_key_s cur_node;
> + cur_node.host_start = (uintptr_t) hostaddrs[i];
> + cur_node.host_end = cur_node.host_start
> + + (kind == GOMP_MAP_POINTER
> + ? sizeof (void *) : sizes[i]);
> + splay_tree_key n
> + = splay_tree_lookup (&acc_dev->mem_map, &cur_node);
> +
> + if (n == NULL)
> + continue;
> +
> + if (finalize)
> + {
> + if (n->refcount != REFCOUNT_INFINITY)
> + n->refcount -= n->virtual_refcount;
> + n->virtual_refcount = 0;
> + }
> +
> + if (n->virtual_refcount > 0)
> + {
> + if (n->refcount != REFCOUNT_INFINITY)
> + n->refcount--;
> + n->virtual_refcount--;
> + }
> + else if (n->refcount > 0 && n->refcount != REFCOUNT_INFINITY)
> + n->refcount--;
> +
> + if (copyfrom
> + && (kind != GOMP_MAP_FROM || n->refcount == 0))
> + gomp_copy_dev2host (acc_dev, aq, (void *) cur_node.host_start,
> + (void *) (n->tgt->tgt_start + n->tgt_offset
> + + cur_node.host_start
> + - n->host_start),
> + cur_node.host_end - cur_node.host_start);
That 'kind != GOMP_MAP_FROM' conditional looks wrong to me. This should
instead be 'kind == GOMP_MAP_ALWAYS_FROM'? Or, get removed, together
with the 'GOMP_MAP_ALWAYS_FROM' handling above? But definitely
'GOMP_MAP_FORCE_FROM' and 'GOMP_MAP_FROM' need to be handled the same, as
far as I can tell?
> +
> + if (n->refcount == 0)
> + gomp_remove_var_async (acc_dev, n, aq);
> + }
> + break;
> + default:
> + gomp_fatal (">>>> goacc_exit_data_internal UNHANDLED kind 0x%.2x",
> + kind);
> }
> }
>
> gomp_mutex_unlock (&acc_dev->lock);
> }
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
