Sandra wrote in "[PATCH v6 00/11] openmp: Add support for iterators in OpenMP
mapping clauses":
Part 8, "openmp: Fix struct handling for OpenMP iterators" was reviewed here:
https://gcc.gnu.org/pipermail/gcc-patches/2025-September/696169.html
- "I think this patch badly needs a testcase.... Otherwise, LGTM."
- As suggested, I've added the new testcase (iterator-with-structs.c)
to part 9 instead.
Sandra Loosemore wrote:
+++ b/libgomp/testsuite/libgomp.c-c++-common/iterator-with-structs.c
...
+ #pragma omp target enter data map(iterator(i=0:5), to: s[i].t)
+ for (int i = 0; i < 5; i++)
+ if (s[i].t.n != i)
+ __builtin_abort ();
+ #pragma omp target exit data map(iterator(i=0:5), release: s[i].t)
I think I saw such a testcase before - I think in the iterator series...
This test does not make sense (besides the ICE test and not running
into libgomp runtime checks). It does:
* Copy data to the device
* Run some code *on* the *host*
* Copy data back from the device
I think you want to add 'omp target' or 'omp target parallel for'
before the for loop to actually execute the testcase on the device.
My bet is that it will just work, but better check it.
* * *
Using 'grep' I found one testcase that has (still) the same issue,
but it is compile only:
gcc/testsuite/gfortran.dg/gomp/target-map-iterators-6.f90
!$omp target data map(iterator(it = 1:DIM), tofrom: x(it)%ptr(:))
do i = 1, DIM
x(i)%ptr(1) = i
end do
!$omp end target data
This is has the same bogus pattern, but as it is a dump test, it does not
matter as much. (Still a comment or adding 'omp target' would be nicer
as well.)
Semantic wise it is worse: If there is no self map, the data gets
overridden by copying it from the device at the end of the 'target data'
region, which is really bogus. - Also OpenMP demands that code is written
such that self map or not, the same result is produced - otherwise it is
unspecified behavior. [But as it is only compiled ...]
* * *
Except for the minor testcase issue, the two patches (8+9 of 11) still
LGTM. (Admittedly, I only browsed through the patches, but still ...)
Thanks,
Tobias
