Hi!

On 2020-05-20T20:11:00+0100, Julian Brown <julian@codesourcery.com> wrote:
> On Wed, 20 May 2020 16:52:02 +0200
> Thomas Schwinge <thomas@codesourcery.com> wrote:
>> On 2019-12-17T22:03:47-0800, Julian Brown <julian@codesourcery.com>
>> wrote:
>> > --- a/libgomp/oacc-mem.c
>> > +++ b/libgomp/oacc-mem.c
>>
>> >  static int
>> > -find_group_last (int pos, size_t mapnum, unsigned short *kinds)
>> > +find_group_last (int pos, size_t mapnum, size_t *sizes, unsigned short *kinds) {
>> >    unsigned char kind0 = kinds[pos] & 0xff;
>> > -  int first_pos = pos, last_pos = pos;
>> > +  int first_pos = pos;
>> >
>> > -  if (kind0 == GOMP_MAP_TO_PSET)
>> > +  switch (kind0)
>> >      {
>> > +    case GOMP_MAP_TO_PSET:
>> >        while (pos + 1 < mapnum && (kinds[pos + 1] & 0xff) == GOMP_MAP_POINTER)
>> > -  last_pos = ++pos;
>> > +  pos++;
>> >        /* We expect at least one GOMP_MAP_POINTER after a GOMP_MAP_TO_PSET.  */
>> > -      assert (last_pos > first_pos);
>> > -    }
>> > -  else
>> > -    {
>> > +      assert (pos > first_pos);
>> > +      break;
>> > +
>> > +    case GOMP_MAP_STRUCT:
>> > +      pos += sizes[pos];
>> > +      break;
>> > +
>> > +    case GOMP_MAP_POINTER:
>> > +    case GOMP_MAP_ALWAYS_POINTER:
>> > +      /* These mappings are only expected after some other mapping.  If we
>> > +   see one by itself, something has gone wrong.  */
>> > +      gomp_fatal ("unexpected mapping");
>> > +      break;
>> > +
>> > +    default:
>> >        /* GOMP_MAP_ALWAYS_POINTER can only appear directly after some other mapping.  */
>> > -      if (pos + 1 < mapnum
>> > -    && (kinds[pos + 1] & 0xff) == GOMP_MAP_ALWAYS_POINTER)
>> > -  return pos + 1;
>> > +      if (pos + 1 < mapnum)
>> > +  {
>> > +    unsigned char kind1 = kinds[pos + 1] & 0xff;
>> > +    if (kind1 == GOMP_MAP_ALWAYS_POINTER)
>> > +      return pos + 1;
>> > +  }
>> >
>> > -      /* We can have one or several GOMP_MAP_POINTER mappings after a to/from
>> > +      /* We can have zero or more GOMP_MAP_POINTER mappings after a to/from (etc.) mapping.  */
>> >        while (pos + 1 < mapnum && (kinds[pos + 1] & 0xff) == GOMP_MAP_POINTER)
>> > -  last_pos = ++pos;
>> > +  pos++;
>> >      }
>> >
>> > -  return last_pos;
>> > +  return pos;
>> >  }
>>
>> So this now causes grouped (!) mapping of all of 'GOMP_MAP_STRUCT',
>> that is, all its "members" at once.
>>
>> This, I suppose, mandated the removal of (some of) the
>> 'is_tgt_unmapped' checking (unfortunately committed not here, but as
>> part of r279621 "OpenACC reference count overhaul"), where we had
>> unmapping code (conceptually) similar to:
>>
>>     bool is_tgt_unmapped = gomp_remove_var (acc_dev, n);
>>     assert (is_tgt_unmapped);
>>
>> I'd introduced this a little bit earlier, finding this a simple yet
>> effective run-time, low-overhead consistency checking of (certain
>> aspects of) reference counting -- so just noting here that it's
>> somewhat bad that we can't have this anymore "just" because of
>> 'GOMP_MAP_STRUCT'.  (Maybe there is a way to get it back; that's for
>> later?)
>
> I'm actually looking at this now as part of revisiting the refcounting
> work. I'm seeing what I can come up with in terms of being able to keep
> the runtime test (and fixing the other part you mentioned).

Good idea to skip the checking if 'num_mappings > 1', thanks!  You've
included these changes as part of a different, bigger patch; I've split
them out, and pushed "[OpenACC] Repair/restore 'is_tgt_unmapped'
checking" to master branch in commit
06ec61726d192659cd446e59a91e78745037f0fd, and releases/gcc-10 branch in
commit 125621f569cfac9f4caa6afc1976d42b3d21359e, see attached.

Also note how this checking triggers for OpenACC 'finalize' clause usage
in 'libgomp.oacc-fortran/deep-copy-6.f90' (which I had relatedly XFAILed
before); so good to see that it's useful for something!  (..., and did
your reference count self-checking not catch this case?)


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