[2/7,OpenACC] Adjust dynamic reference count semantics

This patch adjusts the semantics of dynamic reference counts, as described
in the parent email. There are also two new test cases derived from
Thomas's test in the email:

https://gcc.gnu.org/pipermail/gcc-patches/2020-May/546166.html

that work now.

OK?

Julian

ChangeLog

	libgomp/
	* libgomp.h (struct splay_tree_key_s): Change virtual_refcount to
	dynamic_refcount.
	(struct gomp_device_descr): Remove GOMP_MAP_VARS_OPENACC_ENTER_DATA.
	* oacc-mem.c (acc_map_data): Substitute virtual_refcount for
	dynamic_refcount.
	(acc_unmap_data): Replace open-coded refcount handling with call to
	gomp_remove_var.
	(goacc_enter_datum): Adjust for dynamic_refcount semantics.  Use tgt
	returned from gomp_map_vars_async.  Update assertions.
	(goacc_exit_datum): Re-add some error checking.  Adjust for
	dynamic_refcount semantics.  Fix is_tgt_unmapped test for struct
	mappings.
	(goacc_enter_data_internal): Implement "present" case of dynamic
	memory-map handling here.  Update "non-present" case for
	dynamic_refcount semantics.
	(goacc_exit_data_internal): Update for dynamic_refcount semantics.
	Re-introduce error checking for tgt unmapping when appropriate.
	* target.c (gomp_map_vars_internal): Remove
	GOMP_MAP_VARS_OPENACC_ENTER_DATA handling.  Update for dynamic_refcount
	handling.
	(gomp_unmap_vars_internal): Remove virtual_refcount handling.
	(gomp_load_image_to_device): Substitute dynamic_refcount for
	virtual_refcount.

	libgomp/
	* testsuite/libgomp.oacc-c-c++-common/refcounting-1.c: New test.
	* testsuite/libgomp.oacc-c-c++-common/refcounting-2.c: New test.
---
 libgomp/libgomp.h                             |   8 +-
 libgomp/oacc-mem.c                            | 241 ++++++++++++------
 libgomp/target.c                              |  38 +--
 .../libgomp.oacc-c-c++-common/refcounting-1.c |  31 +++
 .../libgomp.oacc-c-c++-common/refcounting-2.c |  31 +++
 5 files changed, 243 insertions(+), 106 deletions(-)
 create mode 100644 libgomp/testsuite/libgomp.oacc-c-c++-common/refcounting-1.c
 create mode 100644 libgomp/testsuite/libgomp.oacc-c-c++-common/refcounting-2.c

Hi Julian!

On 2020-05-22T15:16:05-0700, Julian Brown <julian@codesourcery.com> wrote:
> This patch adjusts the semantics of dynamic reference counts, as described
> in the parent email.

Thanks!

A few questions, but no need to send an updated patch.

> --- a/libgomp/oacc-mem.c
> +++ b/libgomp/oacc-mem.c

> @@ -1018,13 +1036,102 @@ goacc_enter_data_internal (struct gomp_device_descr *acc_dev, size_t mapnum,
>  {
>    for (size_t i = 0; i < mapnum; i++)
>      {
> -      int group_last = find_group_last (i, mapnum, sizes, kinds);
> +      splay_tree_key n;
> +      size_t group_last = find_group_last (i, mapnum, sizes, kinds);
> +      bool struct_p = false;
> +      size_t size, groupnum = (group_last - i) + 1;
>
> -      gomp_map_vars_async (acc_dev, aq,
> -                        (group_last - i) + 1,
> -                        &hostaddrs[i], NULL,
> -                        &sizes[i], &kinds[i], true,
> -                        GOMP_MAP_VARS_OPENACC_ENTER_DATA);
> +      switch (kinds[i] & 0xff)
> +     {
> +     case GOMP_MAP_STRUCT:
> +       {
> +         int last = i + sizes[i];

The 'last' calculated here must always equal the 'group_last' calculated
above.  ;-) (... so we might just use 'group_last' instead of 'last' in
the following.)

> +         size = (uintptr_t) hostaddrs[last] + sizes[last]
> +                - (uintptr_t) hostaddrs[i];
> +         struct_p = true;
> +       }
> +       break;
> +
> +     case GOMP_MAP_ATTACH:
> +       size = sizeof (void *);
> +       break;
> +
> +     default:
> +       size = sizes[i];
> +     }
> +
> +      n = lookup_host (acc_dev, hostaddrs[i], size);
> +

> +      if (n && struct_p)
> +     {
> +       if (n->refcount != REFCOUNT_INFINITY)
> +         n->refcount += groupnum - 1;
> +       n->dynamic_refcount += groupnum - 1;
> +       gomp_mutex_unlock (&acc_dev->lock);
> +     }

Is the 'GOMP_MAP_STRUCT' handling here specifically necessary, or is that
just an optimization of the 'n && groupnum > 1' case below?

> +      else if (n && groupnum == 1)
> +     {
> +       void *h = hostaddrs[i];
> +       size_t s = sizes[i];
> +
> +       /* A standalone attach clause.  */
> +       if ((kinds[i] & 0xff) == GOMP_MAP_ATTACH)
> +         gomp_attach_pointer (acc_dev, aq, &acc_dev->mem_map, n,
> +                              (uintptr_t) h, s, NULL);
> +       else if (h + s > (void *) n->host_end)
> +         {
> +           gomp_mutex_unlock (&acc_dev->lock);
> +           gomp_fatal ("[%p,+%d] not mapped", (void *)h, (int)s);
> +         }
> +
> +       assert (n->refcount != REFCOUNT_LINK);
> +       if (n->refcount != REFCOUNT_INFINITY)
> +         n->refcount++;
> +       n->dynamic_refcount++;
> +
> +       gomp_mutex_unlock (&acc_dev->lock);
> +     }

> +      else if (n && groupnum > 1)
> +     {
> +       assert (n->refcount != REFCOUNT_INFINITY
> +               && n->refcount != REFCOUNT_LINK);
> +
> +       bool processed = false;
> +
> +       struct target_mem_desc *tgt = n->tgt;
> +       for (size_t j = 0; j < tgt->list_count; j++)
> +         if (tgt->list[j].key == n)
> +           {
> +             for (size_t k = 0; k < groupnum; k++)
> +               if (j + k < tgt->list_count && tgt->list[j + k].key)
> +                 {
> +                   tgt->list[j + k].key->refcount++;
> +                   tgt->list[j + k].key->dynamic_refcount++;
> +                 }
> +             processed = true;
> +           }
> +
> +       gomp_mutex_unlock (&acc_dev->lock);
> +       if (!processed)
> +         gomp_fatal ("dynamic refcount incrementing failed for "
> +                     "pointer/pset");
> +     }

Please add some text to explain the nested 'j', 'k' loops and their 'if'
conditionals, and the 'groupnum' usage in the 'k' loop boundary.  Should
the 'k' loop maybe run 'for (size_t k = j; k < tgt->list_count; ++k)'
(..., or is 'groupnum' relevant?), and in the loop body then use 'k'
instead of 'j + k'?  (Maybe I've now confused myself, staring at this for
a while...)

> +      else if (hostaddrs[i])
> +     {
> +       gomp_mutex_unlock (&acc_dev->lock);
> +
> +       struct target_mem_desc *tgt
> +         = gomp_map_vars_async (acc_dev, aq, groupnum, &hostaddrs[i], NULL,
> +                                &sizes[i], &kinds[i], true,
> +                                GOMP_MAP_VARS_ENTER_DATA);
> +       assert (tgt);
> +       for (size_t j = 0; j < tgt->list_count; j++)
> +         {
> +           n = tgt->list[j].key;
> +           if (n)
> +             n->dynamic_refcount++;
> +         }
> +     }

... else nothing.  This latter "nothing" case (not present, and no
'hostaddrs[i]') is exercised by
'libgomp.oacc-fortran/optional-data-enter-exit.f90' (only).  Is that
alright?

>
>        i = group_last;
>      }


> @@ -1137,45 +1241,40 @@ goacc_exit_data_internal (struct gomp_device_descr *acc_dev, size_t mapnum,

(Diff slightly edited.)

>           if (n->refcount == 0)
> -           gomp_remove_var_async (acc_dev, n, aq);

> +           {
> +             if (aq)
> +               {
> +                 /* TODO The way the following code is currently
> +                    implemented, we need the 'is_tgt_unmapped' return
> +                    value from 'gomp_remove_var', so can't use
> +                    'gomp_remove_var_async' here -- see the
> +                    'gomp_unref_tgt' comment in
> +                    <http://mid.mail-archive.com/878snl36eu.fsf@euler.schwinge.homeip.net>;
> +                    PR92881 -- so have to synchronize here.  */
> +                 if (!acc_dev->openacc.async.synchronize_func (aq))
> +                   {
> +                     gomp_mutex_unlock (&acc_dev->lock);
> +                     gomp_fatal ("synchronize failed");
> +                   }
> +               }

As far as I understand, it's no longer true that "The way the following
code is [...] implemented, we need the 'is_tgt_unmapped' return value
from 'gomp_remove_var'".  In particular, we now can/should "use
'gomp_remove_var_async' here", and no longer "have to synchronize here"?

Indeed I'm happy to see that the logic below no longer depends on
'is_tgt_unmapped' for its loop exit condition.  Instead of the above,
this now can use the standard pattern:

    if (aq)
      /* TODO We can't do the 'is_tgt_unmapped' checking -- see the
         'gomp_unref_tgt' comment in
         <http://mid.mail-archive.com/878snl36eu.fsf@euler.schwinge.homeip.net>;
         PR92881.  */
      gomp_remove_var_async (acc_dev, n, aq);
    else
      { [as follows] }

> +             int num_mappings = 0;
> +             /* If the target_mem_desc represents a single data mapping, we
> +                can check that it is freed when this splay tree key's
> +                refcount reaches zero.  Otherwise (e.g. for a struct
> +                mapping with multiple members), fall back to skipping the
> +                test.  */
> +             for (int j = 0; j < n->tgt->list_count; j++)
> +               if (n->tgt->list[j].key)
> +                 num_mappings++;
> +             bool is_tgt_unmapped = gomp_remove_var (acc_dev, n);
> +             assert (num_mappings > 1 || is_tgt_unmapped);
> +           }
>         }
>         break;

For reference, the old logic (mandating what was described in the comment
above) was:

    bool is_tgt_unmapped = false;
    for (size_t i = 0; i < t->list_count; i++)
     {
       is_tgt_unmapped = gomp_remove_var (acc_dev, t->list[i].key);
       if (is_tgt_unmapped)
         break;
     }
    assert (is_tgt_unmapped);


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

Hi Julian!

On 2020-06-03T14:36:14+0200, I wrote:
> On 2020-05-22T15:16:05-0700, Julian Brown <julian@codesourcery.com> wrote:
>> This patch adjusts the semantics of dynamic reference counts, as described
>> in the parent email.
>
> Thanks!
>
> A few questions, but no need to send an updated patch.
>
>> --- a/libgomp/oacc-mem.c
>> +++ b/libgomp/oacc-mem.c
>
>> @@ -1018,13 +1036,102 @@ goacc_enter_data_internal (struct gomp_device_descr *acc_dev, size_t mapnum,
>>  {
>>    for (size_t i = 0; i < mapnum; i++)
>>      {
>> -      int group_last = find_group_last (i, mapnum, sizes, kinds);
>> +      splay_tree_key n;
>> +      size_t group_last = find_group_last (i, mapnum, sizes, kinds);
>> +      bool struct_p = false;
>> +      size_t size, groupnum = (group_last - i) + 1;
>>
>> -      gomp_map_vars_async (acc_dev, aq,
>> -                       (group_last - i) + 1,
>> -                       &hostaddrs[i], NULL,
>> -                       &sizes[i], &kinds[i], true,
>> -                       GOMP_MAP_VARS_OPENACC_ENTER_DATA);
>> +      switch (kinds[i] & 0xff)
>> +    {
>> +    case GOMP_MAP_STRUCT:
>> +      {
>> +        int last = i + sizes[i];
>
> The 'last' calculated here must always equal the 'group_last' calculated
> above.  ;-) (... so we might just use 'group_last' instead of 'last' in
> the following.)
>
>> +        size = (uintptr_t) hostaddrs[last] + sizes[last]
>> +               - (uintptr_t) hostaddrs[i];
>> +        struct_p = true;
>> +      }
>> +      break;
>> +
>> +    case GOMP_MAP_ATTACH:
>> +      size = sizeof (void *);
>> +      break;
>> +
>> +    default:
>> +      size = sizes[i];
>> +    }
>> +
>> +      n = lookup_host (acc_dev, hostaddrs[i], size);
>> +
>
>> +      if (n && struct_p)
>> +    {
>> +      if (n->refcount != REFCOUNT_INFINITY)
>> +        n->refcount += groupnum - 1;
>> +      n->dynamic_refcount += groupnum - 1;
>> +      gomp_mutex_unlock (&acc_dev->lock);
>> +    }
>
> Is the 'GOMP_MAP_STRUCT' handling here specifically necessary, or is that
> just an optimization of the 'n && groupnum > 1' case below?

Eh, OK, I think I see where this is going; the 'n && groupnum > 1' case
below might not necessarily take care of the 'groupnum - 1' refcounts
that we're filing here?

>> +      else if (n && groupnum == 1)
>> +    {
>> +      void *h = hostaddrs[i];
>> +      size_t s = sizes[i];
>> +
>> +      /* A standalone attach clause.  */
>> +      if ((kinds[i] & 0xff) == GOMP_MAP_ATTACH)
>> +        gomp_attach_pointer (acc_dev, aq, &acc_dev->mem_map, n,
>> +                             (uintptr_t) h, s, NULL);
>> +      else if (h + s > (void *) n->host_end)
>> +        {
>> +          gomp_mutex_unlock (&acc_dev->lock);
>> +          gomp_fatal ("[%p,+%d] not mapped", (void *)h, (int)s);
>> +        }
>> +
>> +      assert (n->refcount != REFCOUNT_LINK);
>> +      if (n->refcount != REFCOUNT_INFINITY)
>> +        n->refcount++;
>> +      n->dynamic_refcount++;
>> +
>> +      gomp_mutex_unlock (&acc_dev->lock);
>> +    }
>
>> +      else if (n && groupnum > 1)
>> +    {
>> +      assert (n->refcount != REFCOUNT_INFINITY
>> +              && n->refcount != REFCOUNT_LINK);
>> +
>> +      bool processed = false;
>> +
>> +      struct target_mem_desc *tgt = n->tgt;
>> +      for (size_t j = 0; j < tgt->list_count; j++)
>> +        if (tgt->list[j].key == n)
>> +          {
>> +            for (size_t k = 0; k < groupnum; k++)
>> +              if (j + k < tgt->list_count && tgt->list[j + k].key)
>> +                {
>> +                  tgt->list[j + k].key->refcount++;
>> +                  tgt->list[j + k].key->dynamic_refcount++;
>> +                }
>> +            processed = true;
>> +          }
>> +
>> +      gomp_mutex_unlock (&acc_dev->lock);
>> +      if (!processed)
>> +        gomp_fatal ("dynamic refcount incrementing failed for "
>> +                    "pointer/pset");
>> +    }
>
> Please add some text to explain the nested 'j', 'k' loops and their 'if'
> conditionals, and the 'groupnum' usage in the 'k' loop boundary.  Should
> the 'k' loop maybe run 'for (size_t k = j; k < tgt->list_count; ++k)'
> (..., or is 'groupnum' relevant?), and in the loop body then use 'k'
> instead of 'j + k'?  (Maybe I've now confused myself, staring at this for
> a while...)

Audacious as I am sometimes, I did put a '__builtin_abort' right after
'tgt->list[j].key == n' -- and it doesn't trigger one single time for the
current libgomp test cases, meaning this is all dead code?  I'm confused.

>> +      else if (hostaddrs[i])
>> +    {
>> +      gomp_mutex_unlock (&acc_dev->lock);
>> +
>> +      struct target_mem_desc *tgt
>> +        = gomp_map_vars_async (acc_dev, aq, groupnum, &hostaddrs[i], NULL,
>> +                               &sizes[i], &kinds[i], true,
>> +                               GOMP_MAP_VARS_ENTER_DATA);
>> +      assert (tgt);
>> +      for (size_t j = 0; j < tgt->list_count; j++)
>> +        {
>> +          n = tgt->list[j].key;
>> +          if (n)
>> +            n->dynamic_refcount++;
>> +        }
>> +    }
>
> ... else nothing.  This latter "nothing" case (not present, and no
> 'hostaddrs[i]') is exercised by
> 'libgomp.oacc-fortran/optional-data-enter-exit.f90' (only).  Is that
> alright?
>
>>
>>        i = group_last;
>>      }
>
>
>> @@ -1137,45 +1241,40 @@ goacc_exit_data_internal (struct gomp_device_descr *acc_dev, size_t mapnum,
>
> (Diff slightly edited.)
>
>>          if (n->refcount == 0)
>> -          gomp_remove_var_async (acc_dev, n, aq);
>
>> +          {
>> +            if (aq)
>> +              {
>> +                /* TODO The way the following code is currently
>> +                   implemented, we need the 'is_tgt_unmapped' return
>> +                   value from 'gomp_remove_var', so can't use
>> +                   'gomp_remove_var_async' here -- see the
>> +                   'gomp_unref_tgt' comment in
>> +                   <http://mid.mail-archive.com/878snl36eu.fsf@euler.schwinge.homeip.net>;
>> +                   PR92881 -- so have to synchronize here.  */
>> +                if (!acc_dev->openacc.async.synchronize_func (aq))
>> +                  {
>> +                    gomp_mutex_unlock (&acc_dev->lock);
>> +                    gomp_fatal ("synchronize failed");
>> +                  }
>> +              }
>
> As far as I understand, it's no longer true that "The way the following
> code is [...] implemented, we need the 'is_tgt_unmapped' return value
> from 'gomp_remove_var'".  In particular, we now can/should "use
> 'gomp_remove_var_async' here", and no longer "have to synchronize here"?
>
> Indeed I'm happy to see that the logic below no longer depends on
> 'is_tgt_unmapped' for its loop exit condition.  Instead of the above,
> this now can use the standard pattern:
>
>     if (aq)
>       /* TODO We can't do the 'is_tgt_unmapped' checking -- see the
>          'gomp_unref_tgt' comment in
>          <http://mid.mail-archive.com/878snl36eu.fsf@euler.schwinge.homeip.net>;
>          PR92881.  */
>       gomp_remove_var_async (acc_dev, n, aq);
>     else
>       { [as follows] }
>
>> +            int num_mappings = 0;
>> +            /* If the target_mem_desc represents a single data mapping, we
>> +               can check that it is freed when this splay tree key's
>> +               refcount reaches zero.  Otherwise (e.g. for a struct
>> +               mapping with multiple members), fall back to skipping the
>> +               test.  */
>> +            for (int j = 0; j < n->tgt->list_count; j++)
>> +              if (n->tgt->list[j].key)
>> +                num_mappings++;
>> +            bool is_tgt_unmapped = gomp_remove_var (acc_dev, n);
>> +            assert (num_mappings > 1 || is_tgt_unmapped);
>> +          }
>>        }
>>        break;
>
> For reference, the old logic (mandating what was described in the comment
> above) was:
>
>     bool is_tgt_unmapped = false;
>     for (size_t i = 0; i < t->list_count; i++)
>      {
>        is_tgt_unmapped = gomp_remove_var (acc_dev, t->list[i].key);
>        if (is_tgt_unmapped)
>          break;
>      }
>     assert (is_tgt_unmapped);


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

On Wed, 3 Jun 2020 17:19:47 +0200
Thomas Schwinge <thomas@codesourcery.com> wrote:

> Hi Julian!
> 
> On 2020-06-03T14:36:14+0200, I wrote:
> > On 2020-05-22T15:16:05-0700, Julian Brown <julian@codesourcery.com>
> > wrote:  
> >> This patch adjusts the semantics of dynamic reference counts, as
> >> described in the parent email.  
> >
> > Thanks!
> >
> > A few questions, but no need to send an updated patch.
> >  
> >> --- a/libgomp/oacc-mem.c
> >> +++ b/libgomp/oacc-mem.c  
> >  
> >> @@ -1018,13 +1036,102 @@ goacc_enter_data_internal (struct
> >> gomp_device_descr *acc_dev, size_t mapnum, {
> >>    for (size_t i = 0; i < mapnum; i++)
> >>      {
> >> -      int group_last = find_group_last (i, mapnum, sizes, kinds);
> >> +      splay_tree_key n;
> >> +      size_t group_last = find_group_last (i, mapnum, sizes,
> >> kinds);
> >> +      bool struct_p = false;
> >> +      size_t size, groupnum = (group_last - i) + 1;
> >>  
> >> -      gomp_map_vars_async (acc_dev, aq,
> >> -			   (group_last - i) + 1,
> >> -			   &hostaddrs[i], NULL,
> >> -			   &sizes[i], &kinds[i], true,
> >> -			   GOMP_MAP_VARS_OPENACC_ENTER_DATA);
> >> +      switch (kinds[i] & 0xff)
> >> +	{
> >> +	case GOMP_MAP_STRUCT:
> >> +	  {
> >> +	    int last = i + sizes[i];  
> >
> > The 'last' calculated here must always equal the 'group_last'
> > calculated above.  ;-) (... so we might just use 'group_last'
> > instead of 'last' in the following.)
> >  
> >> +	    size = (uintptr_t) hostaddrs[last] + sizes[last]
> >> +		   - (uintptr_t) hostaddrs[i];
> >> +	    struct_p = true;
> >> +	  }
> >> +	  break;
> >> +
> >> +	case GOMP_MAP_ATTACH:
> >> +	  size = sizeof (void *);
> >> +	  break;
> >> +
> >> +	default:
> >> +	  size = sizes[i];
> >> +	}
> >> +
> >> +      n = lookup_host (acc_dev, hostaddrs[i], size);
> >> +  
> >  
> >> +      if (n && struct_p)
> >> +	{
> >> +	  if (n->refcount != REFCOUNT_INFINITY)
> >> +	    n->refcount += groupnum - 1;
> >> +	  n->dynamic_refcount += groupnum - 1;
> >> +	  gomp_mutex_unlock (&acc_dev->lock);
> >> +	}  
> >
> > Is the 'GOMP_MAP_STRUCT' handling here specifically necessary, or
> > is that just an optimization of the 'n && groupnum > 1' case below?
> >  
> 
> Eh, OK, I think I see where this is going; the 'n && groupnum > 1'
> case below might not necessarily take care of the 'groupnum - 1'
> refcounts that we're filing here?

Right. GOMP_MAP_STRUCT is a little special in this case.

> >> +      else if (n && groupnum == 1)
> >> +	{
> >> +	  void *h = hostaddrs[i];
> >> +	  size_t s = sizes[i];
> >> +
> >> +	  /* A standalone attach clause.  */
> >> +	  if ((kinds[i] & 0xff) == GOMP_MAP_ATTACH)
> >> +	    gomp_attach_pointer (acc_dev, aq, &acc_dev->mem_map,
> >> n,
> >> +				 (uintptr_t) h, s, NULL);
> >> +	  else if (h + s > (void *) n->host_end)
> >> +	    {
> >> +	      gomp_mutex_unlock (&acc_dev->lock);
> >> +	      gomp_fatal ("[%p,+%d] not mapped", (void *)h,
> >> (int)s);
> >> +	    }
> >> +
> >> +	  assert (n->refcount != REFCOUNT_LINK);
> >> +	  if (n->refcount != REFCOUNT_INFINITY)
> >> +	    n->refcount++;
> >> +	  n->dynamic_refcount++;
> >> +
> >> +	  gomp_mutex_unlock (&acc_dev->lock);
> >> +	}  
> >  
> >> +      else if (n && groupnum > 1)
> >> +	{
> >> +	  assert (n->refcount != REFCOUNT_INFINITY
> >> +		  && n->refcount != REFCOUNT_LINK);
> >> +
> >> +	  bool processed = false;
> >> +
> >> +	  struct target_mem_desc *tgt = n->tgt;
> >> +	  for (size_t j = 0; j < tgt->list_count; j++)
> >> +	    if (tgt->list[j].key == n)
> >> +	      {
> >> +		for (size_t k = 0; k < groupnum; k++)
> >> +		  if (j + k < tgt->list_count && tgt->list[j +
> >> k].key)
> >> +		    {
> >> +		      tgt->list[j + k].key->refcount++;
> >> +		      tgt->list[j + k].key->dynamic_refcount++;
> >> +		    }
> >> +		processed = true;
> >> +	      }
> >> +
> >> +	  gomp_mutex_unlock (&acc_dev->lock);
> >> +	  if (!processed)
> >> +	    gomp_fatal ("dynamic refcount incrementing failed for
> >> "
> >> +			"pointer/pset");
> >> +	}  
> >
> > Please add some text to explain the nested 'j', 'k' loops and their
> > 'if' conditionals, and the 'groupnum' usage in the 'k' loop
> > boundary.  Should the 'k' loop maybe run 'for (size_t k = j; k <
> > tgt->list_count; ++k)' (..., or is 'groupnum' relevant?), and in
> > the loop body then use 'k' instead of 'j + k'?  (Maybe I've now
> > confused myself, staring at this for a while...)  
> 
> Audacious as I am sometimes, I did put a '__builtin_abort' right after
> 'tgt->list[j].key == n' -- and it doesn't trigger one single time for
> the current libgomp test cases, meaning this is all dead code?  I'm
> confused.

Huh, I didn't expect that! Indeed that stanza appears to be dead code
(at least with mapping clauses generated from current GCC). The reason
is a late bug-fix to the manual deep copy code that strips
GOMP_MAP_TO_PSET and GOMP_MAP_POINTER from OpenACC enter/exit mappings
altogether. (In
https://gcc.gnu.org/legacy-ml/gcc-patches/2019-12/msg01253.html).

That means "grouped" mappings are actually only now used
for GOMP_MAP_STRUCT, so actually even more of the find_group_last
function is probably dead now too, modulo backward compatibility issues.

Rewinding a bit, here is an explanation of the problem that the removal
of those clauses fixes, in case we want to revisit that.

With the attached patch (reverting the fix), the attached test case
fails (e.g. compiled at -O0). The problem is that with a dynamic data
lifetime, it's possible for an array descriptor on the stack to go out
of scope before the array data it is associated with does. This might
well be violating either Fortran rules or OpenACC semantics -- if that's
the case, then we had no problem here. (I did see a similar problem "in
the wild", but hadn't come up with a standalone test case until now.)

The attached test case starts out with a explicit-shape array local. It
passes this to a subroutine "enterdata_wrapper". This subroutine
fabricates an assumed-shape array pointer to its argument (creating an
array descriptor), and passes it to another subroutine "enterdata".

The "enterdata" subroutine then performs an OpenACC "enter data"
operation with the array -- whose data comes from the original
explicit-shape array in the main program, but whose descriptor comes
from the stack frame of the caller (i.e. "enterdata_wrapper"). This
descriptor then goes out of scope before returning to the main program.

The test case tries to fiddle with the stack layout by adding arbitrary
other arrays, and does the same dance again with nested subroutines to
perform an "exit data" operation.  But now the address of the (new)
descriptor is different, and the unmapping operation fails.

In short -- OpenACC "enter data" operations can (could) create hidden
dangling references to array descriptors, in some circumstances.

So, the fix was to strip out GOMP_MAP_TO_PSET (and GOMP_MAP_POINTER,
which I don't think has any meaning on these directives) from OpenACC
"enter data" and "exit data" directives altogether. If an array has a
descriptor when we get to a compute kernel, that descriptor is copied
to the target anyway, *even for present clauses*, so passing the
array descriptor to "enter data" descriptor doesn't appear to be
necessary, even in cases where it stays in scope before unmapping from
the target.

So, questions:

1. Does the attached program violate Fortran semantics in some way?

2. Or OpenACC semantics?

3. Are there unintended side-effects of removing GOMP_MAP_TO_PSET and
   GOMP_MAP_POINTER from OpenACC enter/exit data directives?

4. Should the clauses be stripped from the equivalent OpenMP directives
   too?

(FAOD, I'm not asking for review on the attached patch at this time.)

HTH,

Julian