Thanks for the quick review! See the updated patch attached and inline
comments below.
On 28/11/2025 16:28, Tobias Burnus wrote:
Paul-Antoine Arras wrote:
Regarding (1), the OpenMP spec has the following restriction: "If
multiple list
items are explicitly mapped on the same construct and have the same
containing
array or have base pointers that share original storage, and if any of
the list
items do not have corresponding list items that are present in the
device data
environment prior to a task encountering the construct, then the list
items must
refer to *the same array elements* of either the containing array or the
implicit array of the base pointers."
Because tiles is allocatable, we cannot prove at compile time that array
elements are the same, so the check is deferred to libgomp. But there the
condition enforcing that all addresses are the same is too strict, so
this patch
relaxes it to only check that addresses are sorted in increasing order.
Yes, better false negative than false positives when doing error checks.
For allocatables (for which deep mapping applies), they are contiguous
and the code walks them increasing index/memory position.
The OpenMP spec allows (2) as long as it is implicit, without
extending the
original mapping. So this patch sets the GOMP_MAP_IMPLICIT flag
appropriately
on deep maps at compile time to let libgomp know that it is fine.
PR fortran/120505
gcc/ChangeLog:
* omp-low.cc (lower_omp_target): Set GOMP_MAP_IMPLICIT flag.
libgomp/ChangeLog:
* target.c (gomp_map_vars_internal): Allow struct mapping from
different
containing array elements as long as adresses are in increasing
order.
* testsuite/libgomp.c-c++-common/map-arrayofstruct-2.c: Adjust
dg-output.
* testsuite/libgomp.c-c++-common/map-arrayofstruct-3.c: Likewise.
* testsuite/libgomp.fortran/map-subarray-5.f90: Likewise.
* testsuite/libgomp.fortran/map-subarray-10.f90: New test.
* testsuite/libgomp.fortran/map-subarray-9.f90: New test.
(The two testcases differ by one using tiles(1)%...and tiles(1)%...
the other tiles(1)%..., tiles(2)%...)
* * *
--- a/gcc/omp-low.cc
+++ b/gcc/omp-low.cc
@@ -13240,7 +13240,20 @@ lower_omp_target (gimple_stmt_iterator
*gsi_p, omp_context *ctx)
- case OMP_CLAUSE_MAP: tkind2 = OMP_CLAUSE_MAP_KIND (c); break;
+ case OMP_CLAUSE_MAP:
For bystanders: This for the deep mappying code only → if (deep_map_cnt)
and lang_hooks.decls.omp_deep_mapping.
* * *
+ gomp_fatal ("Mapped array elements must be in "
+ "increasing address order (got %p > %p)",
+ hostaddrs[j], hostaddrs[j + 1]);
Shouldn't this be "must be the same or in increasing …" to match the
check and also to match the specification?
Reworded message as suggested.
+++ b/libgomp/testsuite/libgomp.fortran/map-subarray-10.f90
@@ -0,0 +1,33 @@
+! { dg-do run }
+
"! PR fortran/120505" – just as crossref?
Added crossref to both test files.
+!$omp target
+ if (any (var%tiles(1)%den1 /= reshape([1,2,3,4],[2,2]))) stop 1
+ if (any (var%tiles(2)%den2 /= reshape([11,22,33,44],[2,2]))) stop 1
+ var%tiles(1)%den1 = var%tiles(1)%den1 + 5
+ var%tiles(2)%den2 = var%tiles(2)%den2 + 7
+!$omp end target
Check whether the result is fine, e.g.
if (any (var%tiles(1)%den1 /= 5 + reshape([1,2,3,4],[2,2]))) stop 1
if (any (var%tiles(2)%den2 /= 7 + reshape([11,22,33,44],[2,2]))) stop 1
* * *
+++ b/libgomp/testsuite/libgomp.fortran/map-subarray-9.f90
...
+!$omp target
+ if (any (var%tiles(1)%den1 /= reshape([1,2,3,4],[2,2]))) stop 1
+ if (any (var%tiles(1)%den2 /= reshape([11,22,33,44],[2,2]))) stop 1
+ var%tiles(1)%den1 = var%tiles(1)%den1 + 5
+ var%tiles(1)%den2 = var%tiles(1)%den2 + 7
+!$omp end target
check whether it has been updated:
if (any (var%tiles(1)%den1 /= 5 + reshape([1,2,3,4],[2,2]))) stop 1
if (any (var%tiles(1)%den2 /= 7 + reshape([11,22,33,44],[2,2]))) stop 1
Note: needs 'target update' or 'target exit data' before the check.
(both cases).
Done for both test files.
* * *
Otherwise, LGTM.
Thanks for the patch!
Tobias
Unless otherwise instructed, I'll push the revised patch in a few minutes.
Thanks,
--
PA
From f8f1bc8a5059d695e69769271ef7f15842561a18 Mon Sep 17 00:00:00 2001
From: Paul-Antoine Arras <[email protected]>
Date: Fri, 28 Nov 2025 15:40:44 +0100
Subject: [PATCH] OpenMP/Fortran: Allow explicit map followed by implicit deep
mapping [PR120505]
Consider the following source code, assuming tiles is allocatable:
```
!$omp target enter data map(var%tiles(1)%den1, var%tiles(1)%den2) ! (1)
[...]
!$omp target ! implicitly maps var, which triggers deep mapping of tiles (2)
```
Each omp directive causes a run-time error in libgomp:
(1) libgomp: Mapped array elements must be the same (0x14d729c0 vs 0x14d72a18)
(2) libgomp: Trying to map into device [0x3704ca50..0x3704cb00) object when
[0x3704ca50..0x3704caa8) is already mapped
Regarding (1), the OpenMP spec has the following restriction: "If multiple list
items are explicitly mapped on the same construct and have the same containing
array or have base pointers that share original storage, and if any of the list
items do not have corresponding list items that are present in the device data
environment prior to a task encountering the construct, then the list items must
refer to *the same array elements* of either the containing array or the
implicit array of the base pointers."
Because tiles is allocatable, we cannot prove at compile time that array
elements are the same, so the check is deferred to libgomp. But there the
condition enforcing that all addresses are the same is too strict, so this patch
relaxes it to only check that addresses are sorted in increasing order.
The OpenMP spec allows (2) as long as it is implicit, without extending the
original mapping. So this patch sets the GOMP_MAP_IMPLICIT flag appropriately
on deep maps at compile time to let libgomp know that it is fine.
This patch ensures that such user code is accepted by:
(1) Setting the GOMP_MAP_IMPLICIT flag appropriately on deep maps;
(2) Relaxing the restriction on struct mapping from different containing arrays,
so that the element index need not be the same, instead addresses must be sorted
in increasing order.
This fixes the two errors currently seen when running SPEC HPC clvleaf
benchmark. However, further mapping issues prevent the benchmark from running to
completion.
PR fortran/120505
gcc/ChangeLog:
* omp-low.cc (lower_omp_target): Set GOMP_MAP_IMPLICIT flag.
libgomp/ChangeLog:
* target.c (gomp_map_vars_internal): Allow struct mapping from different
containing array elements as long as adresses are in increasing order.
* testsuite/libgomp.c-c++-common/map-arrayofstruct-2.c: Adjust
dg-output.
* testsuite/libgomp.c-c++-common/map-arrayofstruct-3.c: Likewise.
* testsuite/libgomp.fortran/map-subarray-5.f90: Likewise.
* testsuite/libgomp.fortran/map-subarray-10.f90: New test.
* testsuite/libgomp.fortran/map-subarray-9.f90: New test.
---
gcc/omp-low.cc | 15 ++++++-
libgomp/target.c | 10 ++---
.../map-arrayofstruct-2.c | 2 +-
.../map-arrayofstruct-3.c | 2 +-
.../libgomp.fortran/map-subarray-10.f90 | 40 +++++++++++++++++++
.../libgomp.fortran/map-subarray-5.f90 | 2 +-
.../libgomp.fortran/map-subarray-9.f90 | 40 +++++++++++++++++++
7 files changed, 102 insertions(+), 9 deletions(-)
create mode 100644 libgomp/testsuite/libgomp.fortran/map-subarray-10.f90
create mode 100644 libgomp/testsuite/libgomp.fortran/map-subarray-9.f90
diff --git a/gcc/omp-low.cc b/gcc/omp-low.cc
index d36756e33a5..6fd685cdecd 100644
--- a/gcc/omp-low.cc
+++ b/gcc/omp-low.cc
@@ -13240,7 +13240,20 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx)
unsigned HOST_WIDE_INT tkind2;
switch (OMP_CLAUSE_CODE (c))
{
- case OMP_CLAUSE_MAP: tkind2 = OMP_CLAUSE_MAP_KIND (c); break;
+ case OMP_CLAUSE_MAP:
+ tkind2 = OMP_CLAUSE_MAP_KIND (c);
+ if (OMP_CLAUSE_MAP_RUNTIME_IMPLICIT_P (c)
+ && (((tkind2 & GOMP_MAP_FLAG_SPECIAL_BITS)
+ & ~GOMP_MAP_IMPLICIT)
+ == 0))
+ {
+ /* If this is an implicit map, and the GOMP_MAP_IMPLICIT
+ bits are not interfered by other special bit
+ encodings, then turn the GOMP_IMPLICIT_BIT flag on
+ for the runtime to see. */
+ tkind2 |= GOMP_MAP_IMPLICIT;
+ }
+ break;
case OMP_CLAUSE_FIRSTPRIVATE: tkind2 = GOMP_MAP_TO; break;
case OMP_CLAUSE_TO: tkind2 = GOMP_MAP_TO; break;
case OMP_CLAUSE_FROM: tkind2 = GOMP_MAP_FROM; break;
diff --git a/libgomp/target.c b/libgomp/target.c
index 49d4218b1f7..af7c702d439 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -1665,14 +1665,14 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
case GOMP_MAP_STRUCT_UNORD:
if (sizes[i] > 1)
{
- void *first = hostaddrs[i + 1];
for (size_t j = i + 1; j < i + sizes[i]; j++)
- if (hostaddrs[j + 1] != first)
+ if (hostaddrs[j + 1] < hostaddrs[j])
{
gomp_mutex_unlock (&devicep->lock);
- gomp_fatal ("Mapped array elements must be the "
- "same (%p vs %p)", first,
- hostaddrs[j + 1]);
+ gomp_fatal (
+ "Mapped array elements must be the same or in "
+ "increasing address order (got %p > %p)",
+ hostaddrs[j], hostaddrs[j + 1]);
}
}
/* Fallthrough. */
diff --git a/libgomp/testsuite/libgomp.c-c++-common/map-arrayofstruct-2.c b/libgomp/testsuite/libgomp.c-c++-common/map-arrayofstruct-2.c
index ff7ce0eb162..55bd60772a0 100644
--- a/libgomp/testsuite/libgomp.c-c++-common/map-arrayofstruct-2.c
+++ b/libgomp/testsuite/libgomp.c-c++-common/map-arrayofstruct-2.c
@@ -54,5 +54,5 @@ int main (void)
}
/* { dg-output "(\n|\r|\r\n)" { target offload_device_nonshared_as } } */
-/* { dg-output "libgomp: Mapped array elements must be the same .*(\n|\r|\r\n)+" { target offload_device_nonshared_as } } */
+/* { dg-output "libgomp: Mapped array elements must be the same or in increasing address order .*(\n|\r|\r\n)+" { target offload_device_nonshared_as } } */
/* { dg-shouldfail "" { offload_device_nonshared_as } } */
diff --git a/libgomp/testsuite/libgomp.c-c++-common/map-arrayofstruct-3.c b/libgomp/testsuite/libgomp.c-c++-common/map-arrayofstruct-3.c
index 770ac2ae1aa..0352682d042 100644
--- a/libgomp/testsuite/libgomp.c-c++-common/map-arrayofstruct-3.c
+++ b/libgomp/testsuite/libgomp.c-c++-common/map-arrayofstruct-3.c
@@ -64,5 +64,5 @@ int main (void)
}
/* { dg-output "(\n|\r|\r\n)" { target offload_device_nonshared_as } } */
-/* { dg-output "libgomp: Mapped array elements must be the same .*(\n|\r|\r\n)+" { target offload_device_nonshared_as } } */
+/* { dg-output "libgomp: Mapped array elements must be the same or in increasing address order .*(\n|\r|\r\n)+" { target offload_device_nonshared_as } } */
/* { dg-shouldfail "" { offload_device_nonshared_as } } */
diff --git a/libgomp/testsuite/libgomp.fortran/map-subarray-10.f90 b/libgomp/testsuite/libgomp.fortran/map-subarray-10.f90
new file mode 100644
index 00000000000..9afb8458849
--- /dev/null
+++ b/libgomp/testsuite/libgomp.fortran/map-subarray-10.f90
@@ -0,0 +1,40 @@
+! { dg-do run }
+
+! PR fortran/120505
+
+! This test case checks that explicit mapping of allocatable DT components from
+! different containing array elements followed by implicit deep mapping works.
+
+module m
+type t
+ integer, allocatable :: den1(:,:), den2(:,:)
+end type t
+
+type t2
+ type(t), allocatable :: tiles(:)
+end type t2
+
+type(t2) :: var
+end
+
+use m
+
+allocate(var%tiles(2))
+var%tiles(1)%den1 = reshape([1,2,3,4],[2,2])
+var%tiles(2)%den2 = reshape([11,22,33,44],[2,2])
+
+!$omp target enter data map(var%tiles(1)%den1, var%tiles(2)%den2)
+
+!$omp target
+ if (any (var%tiles(1)%den1 /= reshape([1,2,3,4],[2,2]))) stop 1
+ if (any (var%tiles(2)%den2 /= reshape([11,22,33,44],[2,2]))) stop 1
+ var%tiles(1)%den1 = var%tiles(1)%den1 + 5
+ var%tiles(2)%den2 = var%tiles(2)%den2 + 7
+!$omp end target
+
+!$omp target exit data map(var%tiles(1)%den1, var%tiles(2)%den2)
+
+if (any (var%tiles(1)%den1 /= 5 + reshape([1,2,3,4],[2,2]))) stop 1
+if (any (var%tiles(2)%den2 /= 7 + reshape([11,22,33,44],[2,2]))) stop 1
+
+end
diff --git a/libgomp/testsuite/libgomp.fortran/map-subarray-5.f90 b/libgomp/testsuite/libgomp.fortran/map-subarray-5.f90
index 59ad01ab76b..7bf3102018e 100644
--- a/libgomp/testsuite/libgomp.fortran/map-subarray-5.f90
+++ b/libgomp/testsuite/libgomp.fortran/map-subarray-5.f90
@@ -50,5 +50,5 @@ end do
end
! { dg-output "(\n|\r|\r\n)" { target offload_device_nonshared_as } }
-! { dg-output "libgomp: Mapped array elements must be the same .*(\n|\r|\r\n)+" { target offload_device_nonshared_as } }
+! { dg-output "libgomp: Mapped array elements must be the same or in increasing address order .*(\n|\r|\r\n)+" { target offload_device_nonshared_as } }
! { dg-shouldfail "" { offload_device_nonshared_as } }
diff --git a/libgomp/testsuite/libgomp.fortran/map-subarray-9.f90 b/libgomp/testsuite/libgomp.fortran/map-subarray-9.f90
new file mode 100644
index 00000000000..f3101559903
--- /dev/null
+++ b/libgomp/testsuite/libgomp.fortran/map-subarray-9.f90
@@ -0,0 +1,40 @@
+! { dg-do run }
+
+! PR fortran/120505
+
+! This test case checks that explicit mapping of allocatable DT components
+! followed by implicit deep mapping works.
+
+module m
+type t
+ integer, allocatable :: den1(:,:), den2(:,:)
+end type t
+
+type t2
+ type(t), allocatable :: tiles(:)
+end type t2
+
+type(t2) :: var
+end
+
+use m
+
+allocate(var%tiles(1))
+var%tiles(1)%den1 = reshape([1,2,3,4],[2,2])
+var%tiles(1)%den2 = reshape([11,22,33,44],[2,2])
+
+!$omp target enter data map(var%tiles(1)%den1, var%tiles(1)%den2)
+
+!$omp target
+ if (any (var%tiles(1)%den1 /= reshape([1,2,3,4],[2,2]))) stop 1
+ if (any (var%tiles(1)%den2 /= reshape([11,22,33,44],[2,2]))) stop 1
+ var%tiles(1)%den1 = var%tiles(1)%den1 + 5
+ var%tiles(1)%den2 = var%tiles(1)%den2 + 7
+!$omp end target
+
+!$omp target exit data map(var%tiles(1)%den1, var%tiles(1)%den2)
+
+if (any (var%tiles(1)%den1 /= 5 + reshape([1,2,3,4],[2,2]))) stop 1
+if (any (var%tiles(1)%den2 /= 7 + reshape([11,22,33,44],[2,2]))) stop 1
+
+end
--
2.51.0
