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

[Jakub Jelinek <jakub at redhat dot com>]

On Tue, Oct 16, 2018 at 08:57:00PM +0800, Chung-Lin Tang wrote:
> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -490,6 +490,140 @@ gomp_map_val (struct target_mem_desc *tgt, void **hostaddrs, size_t i)
>    return tgt->tgt_start + tgt->list[i].offset;
>  }
>  
> +/* Dynamic array related data structures, interfaces with the compiler.  */
> +
> +struct da_dim {
> +  size_t base;
> +  size_t length;
> +  size_t elem_size;
> +  size_t is_array;
> +};
> +
> +struct da_descr_type {
> +  void *ptr;
> +  size_t ndims;
> +  struct da_dim dims[];
> +};

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.

> +  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?

> +  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.

> @@ -976,6 +1210,108 @@ gomp_map_vars (struct gomp_device_descr *devicep, size_t mapnum,
>  		array++;
>  	      }
>  	  }
> +
> +      /* Processing of dynamic array rows.  */
> +      for (i = 0, da_index = 0, row_start = 0; i < mapnum; i++)
> +	{
> +	  int kind = get_kind (short_mapkind, kinds, i);
> +	  if (!GOMP_MAP_DYNAMIC_ARRAY_P (kind & typemask))
> +	    continue;

Again.

	Jakub