Hi Kwok,
sorry for the belated review.
[Some minor but trivial changes are required due to bitrotting -
both in gimplify.cc and in libgomp/target.c]
Kwok Cheung Yeung wrote:
Subject: [PATCH 04/11] openmp, fortran: Add support for map iterators in
OpenMP target construct (Fortran)
This adds support for iterators in map clauses within OpenMP
'target' constructs in Fortran.
Some special handling for struct field maps has been added to libgomp in
order to handle arrays of derived types.
...
@@ -10589,7 +10634,8 @@ build_omp_struct_comp_nodes (enum tree_code code, tree
grp_start, tree grp_end,
static tree
extract_base_bit_offset (tree base, poly_int64 *bitposp,
poly_offset_int *poffsetp,
- bool *variable_offset)
+ bool *variable_offset,
+ tree iterator)
{
tree offset;
poly_int64 bitsize, bitpos;
@@ -10599,6 +10645,19 @@ extract_base_bit_offset (tree base, poly_int64
*bitposp,
STRIP_NOPS (base);
+ if (iterator)
+ {
+ /* Replace any iterator variables with constant zero. This will give us
+ the nominal offset and bit position of the first element, which is
+ all we should need to lay out the mappings. The actual locations
+ of the iterated mappings are elsewhere.
+ E.g. "array[i].field" gives "16" (say), not "i * 32 + 16". */
+ tree it;
+ for (it = iterator; it; it = TREE_CHAIN (it))
+ base = simplify_replace_tree (base, TREE_VEC_ELT (it, 0),
+ TREE_VEC_ELT (it, 1));
+ }
I don't quite understand this. First, it is not replaced by 0 -
if so, there had been an issue for:
integer :: array(-5:-3)
... iterator(i=-5:-3)... : array(i)
as 'array(0)' is invalid. But it is replaced by the "lower bound" -
but not of the whole array, but for the iterator range (i.e. using
the beginning/start value of the iterator variable).
When I try:
type (array_ptr) :: x(-5:DIM1)
!$omp target map(iterator(i=2:DIM1), to: x(i)%arr(:)) map(from: sum)
I get coffset = 568, while using i=-5:DIM1, I get 8.
* * *
However, I think code wise this part is okay, i.e. it is only a
comment issue. But I have admittedly not fully digested
omp_accumulate_sibling_list. – Just using the begin value seems
to be fine though.
Hence, can you update the comment & crosscheck that the actual
code gen is okay?
* * *
@@ -10613,6 +10672,8 @@ extract_base_bit_offset (tree base, poly_int64 *bitposp,
{
poffset = 0;
*variable_offset = (offset != NULL_TREE);
+ if (iterator && *variable_offset)
+ *variable_offset = contains_vars (offset);
I don't understand why that's needed. If there is a constant, this else branch
is not entered. Thus, this code only gets touched for non-constant offsets,
which - except for NULL_TREE - should imply variable_offset, what the old
code does.
The new code now overrides a 'true' by checking whether the offset calculation
only contains a DECL_VARIABLE.
I don't understand what case this is supposed to cover. From the call side,
it just implies GOMP_MAP_STRUCT_UNORD - and that has a lot to do with nonconst
values but not much with DECL_VARIABLE.
For the code, it seems as if the idea is that this should be false in some
cases.
Actually, already for something simple, it is not really ordered:
map(iterator(i=0:10)... : a(10-i)%...)
map(iterator(i=10:0:-1)... : a(i)%...)
is ordered by in reverse order - where the begin value is constant and
'offset' hence is a number. I think that's fine, just for completeness.
Additionally, what makes DECL_VARIABLE special? What about a PARAM_DECL
instead? It shouldn't make a difference.
And what about a FUNC_DECL as in:
!$omp target map(iterator(i=0:DIM1/4-1, j=0:3), to: x(f (i, j))%arr(:)) &
with f = i * 4 + j + 1 in case you wondered.
Here, i=0 and j=0 for the offset calculation → x( f(0,0) )%...
i.e. no variable decl, but still not constant.
Thus, in that case, poffset = 0 - which is honored if
(and only if) variable_offset == false - but that happens
here.
* * *
BTW: I was about to suggest that you shouldn't have only iterators that
start with the base address as in 'array[0]' / 'array(lbound(array))',
but it seems as if using 'f()' covers that case sufficiently, even if
that still spans the whole array. - It should be well enough hidden
such that also the other case should work
* * *
--- a/libgomp/target.c
+++ b/libgomp/target.c
...
@@ -1065,33 +1103,35 @@ gomp_merge_iterator_maps (size_t *mapnum, void
***hostaddrs, size_t **sizes,
for (size_t i = 0; i < *mapnum; i++)
{
- if ((*sizes)[i] == SIZE_MAX)
+ int map_type = get_kind (true, *skinds, i) & 0xff;
I think we should use 'const int typemask = 0xff;' as we do elsewhere
(albeit not fully consistent).
* * *
Otherwise, LGTM.
Thanks,
Tobias
PS: I noticed that with OpenMP_VV (formerly solve_vv) that the C
testcase fails (ICE) – and, with this patch, the Fortran ones pass
I am pretty certain that the C one is fixed by a later patch in the
series such that no action is required for now. If not, we can still
come back to this.
