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

[Chung-Lin Tang <chunglin_tang at mentor dot com>]

On 2018/12/6 10:43 PM, Jakub Jelinek wrote:
> 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?

Hi Jakub,
I have revised the patch to rename the main struct da_* types into struct gomp_array_* and
added more detailed descriptions in the comments (though admittedly the "dynamic array" term
is not purged completely).

I have opted for the place-at-start-of-chain route, this should avoid all the tests and
additional iterating when such arrays are not used. There's also another omp-low.c update in
another patch.

Besides the revised whole patch, I have also attached a v1-v2 diff showing the changes in between.
Tested with offloading to ensure no regressions.

Thanks,
Chung-Lin

Attachment: openacc-da-07.libgomp-target.v2.patch
Description: Text document

Attachment: 07-target.c.v1-v2.diff
Description: Text document