Hi Kwok,
Kwok Cheung Yeung wrote:
I find the following warning odd:
#pragma omp target map(iterator(i3=0:10, j3=0:20, k3=0:30), to:
x[i3+j3], y[j3+k3], z[k3+i3])
;
/* { dg-warning "iterator variable .i3. not used in clause
expression" "" { target *-*-* } .-2 } */
/* { dg-warning "iterator variable .j3. not used in clause
expression" "" { target *-*-* } .-3 } */
/* { dg-warning "iterator variable .k3. not used in clause
expression" "" { target *-*-* } .-4 } */
as all variables appears in expressions!
(I didn't realize the warning is about j3 not for not being used at all
– but that there is a list item for which it is not used.)
This is due to there being multiple maps in a single clause at the
source level. The FE allocates a separate clause for each map, but for
some reason the C FE always sets the clause location to after the
'map' for all clauses. I've updated the clause location to point to
the map expression (which is what the C++ and Fortran FEs do) which
hopefully should be clearer.
I was wondering whether it having the "proper" OMP_CLAUSE_LOCATION (c)
makes more sense in some cases such that it makes sense to be different
from EXPR_LOCATION (OMP_CLAUSE_DECL (c)).
However, I think of all uses of c_parser_omp_variable_list, having the
location of the actual expression is either equivalent or better – the
other diagnostic cases should already be handled during parsing.
And for other clauses, OMP_CLAUSE_LOCATION is properly set.
→ I think this change is fine, in particular, as it already matches what
C++ and Fortran do.
(Side note: EXPR_LOCATION (OMP_CLAUSE_DECL (c)) has additional two
issues: it has no location for DECL_P and at least for g++, I saw
locations pointing to the closing ')' of the 'map(' clause, which isn't
a good location, either.)
target-map-iterators-2.c:14:87: warning: iterator variable ‘j3’ not
used in clause expression [-Wopenmp]
14 | #pragma omp target map(iterator(i3=0:10, j3=0:20, k3=0:30),
to: x[i3+j3], y[j3+k3], z[k3+i3])
|
^
This location is much better.
I was wondering the wording can be improved, but looking at the output
when running the program, it is sufficient.
The wordings I was wondering about were "…not used in list item" or "in
this clause expression" or …, but I am not sure that's really better.
* * *
Currently the algorithm for adding code to the iterator loop after it
has been built relies on the presence of the loop, so removing the
loop for 0-1 iterations would complicate matters. Hopefully the
dead-code elimination can remove any unrequired loops, but if not it
could be added as an optimisation later.
OK. I think we still will have an overhead by passing an unused argument
to libgomp, but it should be a rare case - and the libgomp overhead is
not that large.
* * *
step == 0 currently results in an ICE, but that is in generic iterator
code and should probably be dealt with separately to this patch.
Can you create a PR and CC me?
* * *
I've added tests in gomp/target-map-iterators-2.c. I've also added a
warning if the number of iterations can be statically determined to be
zero.
Thanks! Regarding:
+ if (TREE_CONSTANT (loop.count) && integer_zerop (loop.count))
+ warning_at (OMP_CLAUSE_LOCATION (c), OPT_Wopenmp,
+ "iteration count is zero");
I want to note that integer_zerop already checks for TREE_CODE (…) ==
{INTEGER,COMPLEX,VECTOR}_CST such that it is safe to call it without
TREE_CONSTANT.
(Doing it as you did implies tiny performance improvement in the common
case (not const) – but causes a minute code-size increase of gimplify.o
[and tiny performance hit when it is constant].)
* * *
LGTM.
Thanks!
PS: The 3/11 C++ patch also LGTM, see older review email for it.
