* [Patch] libgomp: Fix reverse-offload for GOMP_MAP_TO_PSET
@ 2023-02-06 11:52 Tobias Burnus
2023-02-07 22:51 ` Thomas Schwinge
0 siblings, 1 reply; 4+ messages in thread
From: Tobias Burnus @ 2023-02-06 11:52 UTC (permalink / raw)
To: gcc-patches, Jakub Jelinek
[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]
Seems as if I missed a GOMP_MAP_TO_PSET issue before. As nvptx is
XFAILed before, I only found it when testing on AMDGCN.
For an array-descriptor 'ai' variable, omplower has:
map(tofrom:MEM <integer(kind=4)[0:]> [(integer(kind=4)[0:] *)D.4346] [len: D.4345])
map(to:ai [pointer set, len: 64])
map(alloc:ai.data [pointer assign, bias: 0])
The latter reaches GCC with the same address as 'ai' – i.e. the
one of the array descriptor. This then needs to be dereferenced
to get the address of the actual pointer.
The patch assumes (and asserts) that 'ai.data' is part of the 'ai'
such that I can use the host address of 'ai' to access the data.
If that's not guaranteed, we have to find another way (e.g. another
lookup). But so far it seems to hold and I have not seen a bias
other than 0.
With that patch, libgomp.fortran/reverse-offload-5.f90 now works
with AMDGCN.
OK? Any comments to the attached patch?
Tobias
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
[-- Attachment #2: rev-fix-2.diff --]
[-- Type: text/x-patch, Size: 2384 bytes --]
libgomp: Fix reverse-offload for GOMP_MAP_TO_PSET
libgomp/
* target.c (gomp_target_rev): Dereference ptr
to get device address.
* libgomp.fortran/reverse-offload-5.f90: Add test
for unallocated allocatable.
libgomp/target.c | 7 ++++++-
libgomp/testsuite/libgomp.fortran/reverse-offload-5.f90 | 6 ++++--
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/libgomp/target.c b/libgomp/target.c
index c1682caea13..5cdd845291a 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -3579,8 +3579,13 @@ gomp_target_rev (uint64_t fn_ptr, uint64_t mapnum, uint64_t devaddrs_ptr,
}
int k;
n2 = NULL;
- cdata[i].present = true;
+ /* Dereference devaddrs[j] to get the device addr. */
+ assert (devaddrs[j]-sizes[j] == cdata[i].devaddr);
+ devaddrs[j] = *(uint64_t *) (devaddrs[i] + sizes[j]);
+ cdata[j].present = true;
cdata[j].devaddr = devaddrs[j];
+ if (devaddrs[j] == 0)
+ continue;
k = gomp_map_cdata_lookup (cdata, devaddrs, kinds, sizes, j,
devaddrs[j],
devaddrs[j] + sizeof (void*),
diff --git a/libgomp/testsuite/libgomp.fortran/reverse-offload-5.f90 b/libgomp/testsuite/libgomp.fortran/reverse-offload-5.f90
index ef7eb7bdd52..16810eb47de 100644
--- a/libgomp/testsuite/libgomp.fortran/reverse-offload-5.f90
+++ b/libgomp/testsuite/libgomp.fortran/reverse-offload-5.f90
@@ -24,7 +24,7 @@ s2 = 55
!$omp target map(to: A, A2, s1, s2)
block
- integer, allocatable :: ai(:), ai2(:), si1, si2
+ integer, allocatable :: ai(:), ai2(:), ai3(:), si1, si2, si3
a = a * 2
a2 = a2 * 3
@@ -38,7 +38,7 @@ block
!$omp target device (ancestor:1) &
!$omp& map(to: A, s1, ai, si1) map(always, to: a2, s2) &
- !$omp& map(tofrom: ai2, si2)
+ !$omp& map(tofrom: ai2, si2, ai3, si3)
if (shared_mem) then
if (any (a /= 2 * [1,2,3,4])) stop 1
if (s1 /= 4 * 532) stop 2
@@ -52,6 +52,7 @@ block
if (any (ai2 /= [8,4,7,1])) stop 8
if (si1 /= 64) stop 9
if (si2 /= 765) stop 10
+ if (allocated (ai3) .or. allocated(si3)) stop 26
a = a*3
a2 = a2*7
@@ -80,6 +81,7 @@ block
endif
if (any (ai2 /= 21 * [8,4,7,1])) stop 24
if (si2 /= 31 * 765) stop 25
+ if (allocated (ai3) .or. allocated(si3)) stop 27
deallocate (ai, ai2, si1, si2)
end block
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch] libgomp: Fix reverse-offload for GOMP_MAP_TO_PSET
2023-02-06 11:52 [Patch] libgomp: Fix reverse-offload for GOMP_MAP_TO_PSET Tobias Burnus
@ 2023-02-07 22:51 ` Thomas Schwinge
2023-02-09 9:23 ` Tobias Burnus
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Schwinge @ 2023-02-07 22:51 UTC (permalink / raw)
To: Tobias Burnus; +Cc: gcc-patches, Jakub Jelinek
Hi Tobias!
On 2023-02-06T12:52:11+0100, Tobias Burnus <tobias@codesourcery.com> wrote:
> Seems as if I missed a GOMP_MAP_TO_PSET issue before. As nvptx is
> XFAILed before, I only found it when testing on AMDGCN.
>
> For an array-descriptor 'ai' variable, omplower has:
> map(tofrom:MEM <integer(kind=4)[0:]> [(integer(kind=4)[0:] *)D.4346] [len: D.4345])
> map(to:ai [pointer set, len: 64])
> map(alloc:ai.data [pointer assign, bias: 0])
>
> The latter reaches GCC with the same address as 'ai' – i.e. the
> one of the array descriptor. This then needs to be dereferenced
> to get the address of the actual pointer.
> The patch assumes (and asserts) that 'ai.data' is part of the 'ai'
> such that I can use the host address of 'ai' to access the data.
> If that's not guaranteed, we have to find another way (e.g. another
> lookup). But so far it seems to hold and I have not seen a bias
> other than 0.
>
> With that patch, libgomp.fortran/reverse-offload-5.f90 now works
> with AMDGCN.
Confirming the latter, thanks. (..., but I've not actually reviewed the
code changes.)
> OK? Any comments to the attached patch?
Yes:
> libgomp: Fix reverse-offload for GOMP_MAP_TO_PSET
>
> libgomp/
> * target.c (gomp_target_rev): Dereference ptr
> to get device address.
> * libgomp.fortran/reverse-offload-5.f90: Add test
> for unallocated allocatable.
>
> libgomp/target.c | 7 ++++++-
> libgomp/testsuite/libgomp.fortran/reverse-offload-5.f90 | 6 ++++--
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/libgomp/target.c b/libgomp/target.c
> index c1682caea13..5cdd845291a 100644
> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -3579,8 +3579,13 @@ gomp_target_rev (uint64_t fn_ptr, uint64_t mapnum, uint64_t devaddrs_ptr,
> }
> int k;
> n2 = NULL;
> - cdata[i].present = true;
> + /* Dereference devaddrs[j] to get the device addr. */
> + assert (devaddrs[j]-sizes[j] == cdata[i].devaddr);
> + devaddrs[j] = *(uint64_t *) (devaddrs[i] + sizes[j]);
For x86_64-pc-linux-gnu '-m32' multilib:
[...]
[...]/source-gcc/libgomp/target.c: In function ‘gomp_target_rev’:
[...]/source-gcc/libgomp/target.c:3615:36: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
3615 | devaddrs[j] = *(uint64_t *) (devaddrs[i] + sizes[j]);
| ^
cc1: all warnings being treated as errors
make[9]: *** [target.lo] Error 1
make[9]: Leaving directory `[...]/build-gcc/x86_64-pc-linux-gnu/32/libgomp'
[...]
I suppose you'd do similar to what you already have a few lines above;
that is, cast through 'uintptr_t':
devaddrs[j] = *(uint64_t *) (uintptr_t) (devaddrs[i] + sizes[j]);
Grüße
Thomas
> + cdata[j].present = true;
> cdata[j].devaddr = devaddrs[j];
> + if (devaddrs[j] == 0)
> + continue;
> k = gomp_map_cdata_lookup (cdata, devaddrs, kinds, sizes, j,
> devaddrs[j],
> devaddrs[j] + sizeof (void*),
> diff --git a/libgomp/testsuite/libgomp.fortran/reverse-offload-5.f90 b/libgomp/testsuite/libgomp.fortran/reverse-offload-5.f90
> index ef7eb7bdd52..16810eb47de 100644
> --- a/libgomp/testsuite/libgomp.fortran/reverse-offload-5.f90
> +++ b/libgomp/testsuite/libgomp.fortran/reverse-offload-5.f90
> @@ -24,7 +24,7 @@ s2 = 55
>
> !$omp target map(to: A, A2, s1, s2)
> block
> - integer, allocatable :: ai(:), ai2(:), si1, si2
> + integer, allocatable :: ai(:), ai2(:), ai3(:), si1, si2, si3
>
> a = a * 2
> a2 = a2 * 3
> @@ -38,7 +38,7 @@ block
>
> !$omp target device (ancestor:1) &
> !$omp& map(to: A, s1, ai, si1) map(always, to: a2, s2) &
> - !$omp& map(tofrom: ai2, si2)
> + !$omp& map(tofrom: ai2, si2, ai3, si3)
> if (shared_mem) then
> if (any (a /= 2 * [1,2,3,4])) stop 1
> if (s1 /= 4 * 532) stop 2
> @@ -52,6 +52,7 @@ block
> if (any (ai2 /= [8,4,7,1])) stop 8
> if (si1 /= 64) stop 9
> if (si2 /= 765) stop 10
> + if (allocated (ai3) .or. allocated(si3)) stop 26
>
> a = a*3
> a2 = a2*7
> @@ -80,6 +81,7 @@ block
> endif
> if (any (ai2 /= 21 * [8,4,7,1])) stop 24
> if (si2 /= 31 * 765) stop 25
> + if (allocated (ai3) .or. allocated(si3)) stop 27
>
> deallocate (ai, ai2, si1, si2)
> end block
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch] libgomp: Fix reverse-offload for GOMP_MAP_TO_PSET
2023-02-07 22:51 ` Thomas Schwinge
@ 2023-02-09 9:23 ` Tobias Burnus
2023-02-15 10:17 ` Jakub Jelinek
0 siblings, 1 reply; 4+ messages in thread
From: Tobias Burnus @ 2023-02-09 9:23 UTC (permalink / raw)
To: Thomas Schwinge; +Cc: gcc-patches, Jakub Jelinek
[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]
(Updated to fix -m32 build, otherwise unchanged.)
Any other comments?
On 07.02.23 23:51, Thomas Schwinge wrote:
> On 2023-02-06T12:52:11+0100, Tobias Burnus <tobias@codesourcery.com> wrote:
>> Seems as if I missed a GOMP_MAP_TO_PSET issue before. As nvptx is
>> XFAILed before, I only found it when testing on AMDGCN.
>>
>> For an array-descriptor 'ai' variable, omplower has:
>> map(tofrom:MEM <integer(kind=4)[0:]> [(integer(kind=4)[0:] *)D.4346] [len: D.4345])
>> map(to:ai [pointer set, len: 64])
>> map(alloc:ai.data [pointer assign, bias: 0])
>>
>> The latter reaches GCC with the same address as 'ai' – i.e. the
>> one of the array descriptor. This then needs to be dereferenced
>> to get the address of the actual pointer.
>> The patch assumes (and asserts) that 'ai.data' is part of the 'ai'
>> such that I can use the host address of 'ai' to access the data.
>> If that's not guaranteed, we have to find another way (e.g. another
>> lookup). But so far it seems to hold and I have not seen a bias
>> other than 0.
>>
>> With that patch, libgomp.fortran/reverse-offload-5.f90 now works
>> with AMDGCN.
...
>> OK? Any comments to the attached patch?
...
> For x86_64-pc-linux-gnu '-m32' multilib:
...
> I suppose you'd do similar to what you already have a few lines above;
> that is, cast through 'uintptr_t':
>
> devaddrs[j] = *(uint64_t *) (uintptr_t) (devaddrs[i] + sizes[j]);
Tobias
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
[-- Attachment #2: rev-fix-2-v2.diff --]
[-- Type: text/x-patch, Size: 2410 bytes --]
libgomp: Fix reverse-offload for GOMP_MAP_TO_PSET
libgomp/
* target.c (gomp_target_rev): Dereference ptr
to get device address.
* libgomp.fortran/reverse-offload-5.f90: Add test
for unallocated allocatable.
libgomp/target.c | 8 +++++++-
libgomp/testsuite/libgomp.fortran/reverse-offload-5.f90 | 6 ++++--
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/libgomp/target.c b/libgomp/target.c
index c1682caea13..88f29908a08 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -3579,8 +3579,14 @@ gomp_target_rev (uint64_t fn_ptr, uint64_t mapnum, uint64_t devaddrs_ptr,
}
int k;
n2 = NULL;
- cdata[i].present = true;
+ /* Dereference devaddrs[j] to get the device addr. */
+ assert (devaddrs[j]-sizes[j] == cdata[i].devaddr);
+ devaddrs[j] = *(uint64_t *) (uintptr_t) (devaddrs[i]
+ + sizes[j]);
+ cdata[j].present = true;
cdata[j].devaddr = devaddrs[j];
+ if (devaddrs[j] == 0)
+ continue;
k = gomp_map_cdata_lookup (cdata, devaddrs, kinds, sizes, j,
devaddrs[j],
devaddrs[j] + sizeof (void*),
diff --git a/libgomp/testsuite/libgomp.fortran/reverse-offload-5.f90 b/libgomp/testsuite/libgomp.fortran/reverse-offload-5.f90
index ef7eb7bdd52..16810eb47de 100644
--- a/libgomp/testsuite/libgomp.fortran/reverse-offload-5.f90
+++ b/libgomp/testsuite/libgomp.fortran/reverse-offload-5.f90
@@ -24,7 +24,7 @@ s2 = 55
!$omp target map(to: A, A2, s1, s2)
block
- integer, allocatable :: ai(:), ai2(:), si1, si2
+ integer, allocatable :: ai(:), ai2(:), ai3(:), si1, si2, si3
a = a * 2
a2 = a2 * 3
@@ -38,7 +38,7 @@ block
!$omp target device (ancestor:1) &
!$omp& map(to: A, s1, ai, si1) map(always, to: a2, s2) &
- !$omp& map(tofrom: ai2, si2)
+ !$omp& map(tofrom: ai2, si2, ai3, si3)
if (shared_mem) then
if (any (a /= 2 * [1,2,3,4])) stop 1
if (s1 /= 4 * 532) stop 2
@@ -52,6 +52,7 @@ block
if (any (ai2 /= [8,4,7,1])) stop 8
if (si1 /= 64) stop 9
if (si2 /= 765) stop 10
+ if (allocated (ai3) .or. allocated(si3)) stop 26
a = a*3
a2 = a2*7
@@ -80,6 +81,7 @@ block
endif
if (any (ai2 /= 21 * [8,4,7,1])) stop 24
if (si2 /= 31 * 765) stop 25
+ if (allocated (ai3) .or. allocated(si3)) stop 27
deallocate (ai, ai2, si1, si2)
end block
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch] libgomp: Fix reverse-offload for GOMP_MAP_TO_PSET
2023-02-09 9:23 ` Tobias Burnus
@ 2023-02-15 10:17 ` Jakub Jelinek
0 siblings, 0 replies; 4+ messages in thread
From: Jakub Jelinek @ 2023-02-15 10:17 UTC (permalink / raw)
To: Tobias Burnus; +Cc: Thomas Schwinge, gcc-patches
On Thu, Feb 09, 2023 at 10:23:53AM +0100, Tobias Burnus wrote:
> libgomp: Fix reverse-offload for GOMP_MAP_TO_PSET
>
> libgomp/
> * target.c (gomp_target_rev): Dereference ptr
> to get device address.
> * libgomp.fortran/reverse-offload-5.f90: Add test
> for unallocated allocatable.
>
> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -3579,8 +3579,14 @@ gomp_target_rev (uint64_t fn_ptr, uint64_t mapnum, uint64_t devaddrs_ptr,
> }
> int k;
> n2 = NULL;
> - cdata[i].present = true;
> + /* Dereference devaddrs[j] to get the device addr. */
> + assert (devaddrs[j]-sizes[j] == cdata[i].devaddr);
Formatting, there should be spaces around - on both sides.
> + devaddrs[j] = *(uint64_t *) (uintptr_t) (devaddrs[i]
> + + sizes[j]);
> + cdata[j].present = true;
> cdata[j].devaddr = devaddrs[j];
> + if (devaddrs[j] == 0)
> + continue;
> k = gomp_map_cdata_lookup (cdata, devaddrs, kinds, sizes, j,
> devaddrs[j],
> devaddrs[j] + sizeof (void*),
Otherwise LGTM.
Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-02-15 10:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-06 11:52 [Patch] libgomp: Fix reverse-offload for GOMP_MAP_TO_PSET Tobias Burnus
2023-02-07 22:51 ` Thomas Schwinge
2023-02-09 9:23 ` Tobias Burnus
2023-02-15 10:17 ` Jakub Jelinek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).