Re: [PATCH, OpenACC, 7/8] Multi-dimensional dynamic array support for OpenACC data clauses, libgomp support
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
Index: libgomp/target.c
===
--- libgomp/target.c(revision 267050)
+++ libgomp/target.c(working copy)
@@ -477,6 +477,151 @@ gomp_map_val (struct target_mem_desc *tgt, void **
return tgt->tgt_start + tgt->list[i].offset;
}
+/* Definitions for data structures describing dynamic, non-contiguous arrays
+ (Note: interfaces with compiler)
+
+ The compiler generates a descriptor for each such array, places the
+ descriptor on stack, and passes the address of the descriptor to the libgomp
+ runtime as a normal map argument. The runtime then processes the array
+ data structure setup, and replaces the argument with the new actual
+ array address for the child function.
+
+ Care must be taken such that the struct field and layout assumptions
+ of struct gomp_array_dim, gomp_array_descr_type inside the compiler
+ be consistant with the below declarations. */
+
+struct gomp_array_dim {
+ size_t base;
+ size_t length;
+ size_t elem_size;
+ size_t is_array;
+};
+
+struct gomp_array_descr_type {
+ void *ptr;
+ size_t ndims;
+ struct gomp_array_dim dims[];
+};
+
+/* Internal dynamic array info struct, used only here inside the runtime. */
+
+struct da_info
+{
+ struct gomp_array_descr_type *descr;
+ size_t map_index;
+ size_t ptrblock_size;
+ size_t data_row_num;
+ size_t data_row_size;
+};
+
+static size_t
+gomp_dynamic_array_count_rows (struct gomp_array_descr_type *descr)
+{
+ size_t nrows = 1;
+ for (size_t d = 0; d < descr->ndims - 1; d++)
+nrows *= descr->dims[d].length / sizeof (void *);
+
Re: [PATCH, OpenACC, 7/8] Multi-dimensional dynamic array support for OpenACC data clauses, libgomp support
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
Re: [PATCH, OpenACC, 7/8] Multi-dimensional dynamic array support for OpenACC data clauses, libgomp support
Hi Jakub, thanks for the swift review a few weeks ago, and apologies I haven't
been able
to respond sooner.
On 2018/10/16 9:13 PM, Jakub Jelinek wrote:>> +/* 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.
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.
I think the suggested 'gomp_array_descr' identifier looks descriptive, I'll
revise that in an update,
as well as add more comments to better describe its ABI significance with the
compiler.
+ 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)
+ 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.
Thanks,
Chung-Lin
Re: [PATCH, OpenACC, 7/8] Multi-dimensional dynamic array support for OpenACC data clauses, libgomp support
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
