Re: [PATCH 07/13] OpenACC 2.6 deep copy: libgomp parts

[Thomas Schwinge <thomas@codesourcery.com>]

Hi Julian!

On 2019-12-17T22:03:47-0800, Julian Brown <julian@codesourcery.com> wrote:
> This part contains the libgomp runtime support for the GOMP_MAP_ATTACH and
> GOMP_MAP_DETACH mapping kinds (etc.), as introduced by the front-end
> patches following in this series.

> --- a/libgomp/target.c
> +++ b/libgomp/target.c

> @@ -1534,6 +1571,18 @@ gomp_unmap_vars_internal (struct target_mem_desc *tgt, bool do_copyfrom,

This is the code path at the end of a structured OpenACC 'data'
construct.

> +  /* We must perform detachments before any copies back to the host.  */
> +  for (i = 0; i < tgt->list_count; i++)
> +    {
> +      splay_tree_key k = tgt->list[i].key;
> +
> +      if (k != NULL && tgt->list[i].do_detach)
> +     gomp_detach_pointer (devicep, aq, k, tgt->list[i].key->host_start
> +                                          + tgt->list[i].offset,
> +                          k->refcount == 1, NULL);
> +    }

Can you please explain (as a source code comment) the logic for here
using 'k->refcount == 1' for the 'bool finalize' parameter of
'gomp_detach_pointer'; this somehow feels "strange"?

Nonwithstanding the question whether that's a valid thing to do or not,
but doesn't the current code hide the "attach count underflow" error if
you reach the above code with 'attach_count == 0' (user already
explicitly 'detach'ed), but then given 'k->refcount == 1' (thus
'finalize' semantics), 'gomp_detach_pointer' will then re-initialize
'attach_count = 1', and then do another 'gomp_copy_host2dev', etc.
instead of emitting an error.

(I have not attempted to produce a libgomp test case.)

Shouldn't this just always be 'finalize = false' given that there is no
'finalize' semantics for 'detach' on a structured OpenACC 'data'
constructs -- at least that's what I remember right now?


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