Re: [PATCH, OpenACC, 7/8] Multi-dimensional dynamic array support for OpenACC data clauses, libgomp support

[Jakub Jelinek <jakub@redhat.com>]

On Thu, Dec 06, 2018 at 10:19:43PM +0800, Chung-Lin Tang wrote:
> > Why do you call the non-contiguous arrays dynamic arrays?  Is that some OpenACC term?
> > I'd also prefix those with gomp_ and it is important to make it clear what
> > is the ABI type shared with the compiler and what are the internal types.
> > struct gomp_array_descr would look more natural to me.
> 
> Well it's not particularly an OpenACC term, just that non-contiguous arrays are
> often multi-dimensional arrays dynamically allocated and created through (arrays of) pointers.
> Are you strongly opposed to this naming? If so, I can adjust this part.

The way how those arrays are created (and it doesn't have to be dynamically
allocated) doesn't affect their representation.
There are various terms that describe various data structures, like Iliffe
vectors, jagged/ragged arrays, dope vectors.
I guess it depends on what kind of data structures does this new framework
support, if the Iliffe vectors (arrays of pointers), or just flat but
strided arrays, etc.

> > > +  for (i = 0; i < mapnum; i++)
> > > +    {
> > > +      int kind = get_kind (short_mapkind, kinds, i);
> > > +      if (GOMP_MAP_DYNAMIC_ARRAY_P (kind & typemask))
> > > +	{
> > > +	  da_data_row_num += gomp_dynamic_array_count_rows (hostaddrs[i]);
> > > +	  da_info_num += 1;
> > > +	}
> > > +    }
> > 
> > I'm not really happy by adding several extra loops which will not do
> > anything in the case there are no non-contiguous arrays being mapped (for
> > now always for OpenMP (OpenMP 5 has support for non-contigious target update
> > to/from though) and guess rarely for OpenACC).
> > Can't you use some flag bit in flags passed to GOMP_target* etc. and do the
> > above loop only if the compiler indicated there are any?
> 
> I originally strived to not have that loop, but because each row in the last dimension
> is mapped as its own target_var_desc, we need to count them at this stage to allocate
> the right number at start. Otherwise a realloc later seems even more ugly...
> 
> We currently don't have a suitable flag word argument in GOMP_target*, GOACC_parallel*, etc.
> I am not sure if such a feature warrants changing the interface.
> 
> If you are weary of OpenMP being affected, I can add a condition to restrict such processing
> to only (pragma_kind == GOMP_MAP_VARS_OPENACC). Is that okay? (at least before making any
> larger runtime interface adjustments)

That will still cost you doing that loop for OpenACC constructs that don't
have any of these non-contiguous arrays.  GOMP_target_ext has flags
argument, but GOACC_paralel_keyed doesn't.  It has ... and you could perhaps
encode some flag in there.  Or, could these array descriptors be passed
first in the list of vars, so instead of a loop to check for these you could
just check the first one?

> > > +  tgt = gomp_malloc (sizeof (*tgt)
> > > +		     + sizeof (tgt->list[0]) * (mapnum + da_data_row_num));
> > > +  tgt->list_count = mapnum + da_data_row_num;
> > >     tgt->refcount = pragma_kind == GOMP_MAP_VARS_ENTER_DATA ? 0 : 1;
> > >     tgt->device_descr = devicep;
> > >     struct gomp_coalesce_buf cbuf, *cbufp = NULL;
> > 
> > > @@ -687,6 +863,55 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t mapnum,
> > >   	}
> > >       }
> > > +  /* For dynamic arrays. Each data row is one target item, separated from
> > > +     the normal map clause items, hence we order them after mapnum.  */
> > > +  for (i = 0, da_index = 0, row_start = 0; i < mapnum; i++)
> > 
> > Even if nothing is in flags, you could just avoid this loop if the previous
> > loop(s) haven't found any noncontiguous arrays.
> 
> I'll add a bit more checking to avoid these cases.

	Jakub