Re: [Patch] libgomp – fix handling of 'target enter data'

2020-04-10 Thread Jakub Jelinek via Gcc-patches 
On Fri, Apr 10, 2020 at 04:31:37PM +0200, Thomas Schwinge wrote:
> Aha, thanks -- that resolves doubts I had (but Julian and I couldn't
> allocate time to track down): see 'GOMP_target_enter_exit_data' mentioned
> in 
> ff., for example.
> 
> 
> Isn't that a pretty severe Fortran OpenMP offloading issue, and should
> get a PR allocated, and fixed on release branches, too?

On branches where target enter data is actually accepted we could backport
it, sure.

Jakub

Re: [Patch] libgomp – fix handling of 'target enter data'

2020-04-10 Thread Thomas Schwinge 
Hi!

On 2020-03-31T19:41:40+0200, Tobias Burnus  wrote:
> libgomp – fix handling of 'target enter data'
>
>   * target.c (GOMP_target_enter_exit_data): Handle PSET/MAP_POINTER.
>   * testsuite/libgomp.fortran/target-enter-data-1.f90: New.
>
>  libgomp/target.c   | 13 +++-
>  .../libgomp.fortran/target-enter-data-1.f90| 36 
> ++
>  2 files changed, 48 insertions(+), 1 deletion(-)

> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -2480,7 +2480,9 @@ GOMP_target_enter_exit_data (int device, size_t mapnum, 
> void **hostaddrs,
>   }
>  }
>
> -  size_t i;
> +  /* The variables are mapped separately such that they can be released
> + independently.  */
> +  size_t i, j;
>if ((flags & GOMP_TARGET_FLAG_EXIT_DATA) == 0)
>  for (i = 0; i < mapnum; i++)
>if ((kinds[i] & 0xff) == GOMP_MAP_STRUCT)
> @@ -2489,6 +2491,15 @@ GOMP_target_enter_exit_data (int device, size_t 
> mapnum, void **hostaddrs,
>&kinds[i], true, GOMP_MAP_VARS_ENTER_DATA);
> i += sizes[i];
>   }
> +  else if ((kinds[i] & 0xff) == GOMP_MAP_TO_PSET)
> + {
> +   for (j = i + 1; j < mapnum; j++)
> + if (!GOMP_MAP_POINTER_P (get_kind (true, kinds, j) & 0xff))
> +   break;
> +   gomp_map_vars (devicep, j-i, &hostaddrs[i], NULL, &sizes[i],
> +  &kinds[i], true, GOMP_MAP_VARS_ENTER_DATA);
> +   i += j - i - 1;
> + }
>else
>   gomp_map_vars (devicep, 1, &hostaddrs[i], NULL, &sizes[i], &kinds[i],
>  true, GOMP_MAP_VARS_ENTER_DATA);

Aha, thanks -- that resolves doubts I had (but Julian and I couldn't
allocate time to track down): see 'GOMP_target_enter_exit_data' mentioned
in 
ff., for example.


Isn't that a pretty severe Fortran OpenMP offloading issue, and should
get a PR allocated, and fixed on release branches, too?


> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.fortran/target-enter-data-1.f90
> @@ -0,0 +1,36 @@
> +program main
> +[...]

I pushed to master branch commit 6b816a5f0ed078cb2d380e10e68a95fb7e3d6778
"Add 'dg-do run' to 'libgomp.fortran/target-enter-data-1.f90'", see
attached.


Grüße
 Thomas


-
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander 
Walter
>From 6b816a5f0ed078cb2d380e10e68a95fb7e3d6778 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Wed, 1 Apr 2020 23:26:56 +0200
Subject: [PATCH] Add 'dg-do run' to 'libgomp.fortran/target-enter-data-1.f90'
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fix-up for commit 689418b97e5eb6a221871a2439bca3e6283ac579 "libgomp – fix
handling of 'target enter data'".

	libgomp/
	* testsuite/libgomp.fortran/target-enter-data-1.f90: Add 'dg-do
	run'.
---
 libgomp/ChangeLog | 5 +
 libgomp/testsuite/libgomp.fortran/target-enter-data-1.f90 | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/libgomp/ChangeLog b/libgomp/ChangeLog
index 0e4958f0c67..beff3d65b44 100644
--- a/libgomp/ChangeLog
+++ b/libgomp/ChangeLog
@@ -1,3 +1,8 @@
+2020-04-10  Thomas Schwinge  
+
+	* testsuite/libgomp.fortran/target-enter-data-1.f90: Add 'dg-do
+	run'.
+
 2020-04-08  Tobias Burnus  
 
 	PR middle-end/94120
diff --git a/libgomp/testsuite/libgomp.fortran/target-enter-data-1.f90 b/libgomp/testsuite/libgomp.fortran/target-enter-data-1.f90
index 91dedebf0a0..39faffd44c2 100644
--- a/libgomp/testsuite/libgomp.fortran/target-enter-data-1.f90
+++ b/libgomp/testsuite/libgomp.fortran/target-enter-data-1.f90
@@ -1,3 +1,5 @@
+! { dg-do run }
+
 program main
   implicit none
   integer, allocatable, dimension(:) :: AA, BB, CC, DD
-- 
2.17.1

Re: [Patch] libgomp – fix handling of 'target enter data'

2020-03-31 Thread Jakub Jelinek via Gcc-patches 
On Tue, Mar 31, 2020 at 07:41:40PM +0200, Tobias Burnus wrote:
> On 3/31/20 5:35 PM, Jakub Jelinek via Gcc-patches wrote:
> 
> > Doing the mappings separately is intentional, while for target data or
> > target region mappings it is very likely they will be all released together
> > as well, so it doesn't matter if they all go from the single same mapping,
> > for target enter data it is often not the case.
> > If everything is mapped together, then it can't be really freed until all
> > the mappings get refcount of 0.
> > So, instead of the change you've posted, there should be in
> > GOMP_target_enter_exit_data code to also handle GOMP_MAP_TO_PSET and put the
> > right number of mappings after it in one block and leave the rest separate.
> 
> Done in that way – including adding a rational comment :-)
> 
> OK for mainline?

LGTM, thanks.

> libgomp – fix handling of 'target enter data'
> 
>   * target.c (GOMP_target_enter_exit_data): Handle PSET/MAP_POINTER.
>   * testsuite/libgomp.fortran/target-enter-data-1.f90: New.

Jakub

Re: [Patch] libgomp – fix handling of 'target enter data'

2020-03-31 Thread Tobias Burnus 

On 3/31/20 5:35 PM, Jakub Jelinek via Gcc-patches wrote:


Doing the mappings separately is intentional, while for target data or
target region mappings it is very likely they will be all released together
as well, so it doesn't matter if they all go from the single same mapping,
for target enter data it is often not the case.
If everything is mapped together, then it can't be really freed until all
the mappings get refcount of 0.
So, instead of the change you've posted, there should be in
GOMP_target_enter_exit_data code to also handle GOMP_MAP_TO_PSET and put the
right number of mappings after it in one block and leave the rest separate.


Done in that way – including adding a rational comment :-)

OK for mainline?

Tobias

-
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander 
Walter
libgomp – fix handling of 'target enter data'

	* target.c (GOMP_target_enter_exit_data): Handle PSET/MAP_POINTER.
	* testsuite/libgomp.fortran/target-enter-data-1.f90: New.

 libgomp/target.c   | 13 +++-
 .../libgomp.fortran/target-enter-data-1.f90| 36 ++
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/libgomp/target.c b/libgomp/target.c
index c99dd5196fa..36425477dcb 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -2480,7 +2480,9 @@ GOMP_target_enter_exit_data (int device, size_t mapnum, void **hostaddrs,
 	}
 }
 
-  size_t i;
+  /* The variables are mapped separately such that they can be released
+ independently.  */
+  size_t i, j;
   if ((flags & GOMP_TARGET_FLAG_EXIT_DATA) == 0)
 for (i = 0; i < mapnum; i++)
   if ((kinds[i] & 0xff) == GOMP_MAP_STRUCT)
@@ -2489,6 +2491,15 @@ GOMP_target_enter_exit_data (int device, size_t mapnum, void **hostaddrs,
 			 &kinds[i], true, GOMP_MAP_VARS_ENTER_DATA);
 	  i += sizes[i];
 	}
+  else if ((kinds[i] & 0xff) == GOMP_MAP_TO_PSET)
+	{
+	  for (j = i + 1; j < mapnum; j++)
+	if (!GOMP_MAP_POINTER_P (get_kind (true, kinds, j) & 0xff))
+	  break;
+	  gomp_map_vars (devicep, j-i, &hostaddrs[i], NULL, &sizes[i],
+			 &kinds[i], true, GOMP_MAP_VARS_ENTER_DATA);
+	  i += j - i - 1;
+	}
   else
 	gomp_map_vars (devicep, 1, &hostaddrs[i], NULL, &sizes[i], &kinds[i],
 		   true, GOMP_MAP_VARS_ENTER_DATA);
diff --git a/libgomp/testsuite/libgomp.fortran/target-enter-data-1.f90 b/libgomp/testsuite/libgomp.fortran/target-enter-data-1.f90
new file mode 100644
index 000..91dedebf0a0
--- /dev/null
+++ b/libgomp/testsuite/libgomp.fortran/target-enter-data-1.f90
@@ -0,0 +1,36 @@
+program main
+  implicit none
+  integer, allocatable, dimension(:) :: AA, BB, CC, DD
+  integer :: i, N = 20
+
+  allocate(BB(N))
+  AA = [(i, i=1,N)]
+
+  !$omp target enter data map(alloc: BB)
+  !$omp target enter data map(to: AA)
+
+  !$omp target
+BB = 3 * AA
+  !$omp end target
+
+  !$omp target exit data map(delete: AA)
+  !$omp target exit data map(from: BB)
+
+  if (any (BB /= [(3*i, i=1,N)])) stop 1
+  if (any (AA /= [(i, i=1,N)])) stop 2
+
+
+  CC = 31 * BB
+  DD = [(-i, i=1,N)]
+
+  !$omp target enter data map(to: CC) map(alloc: DD)
+
+  !$omp target
+DD = 5 * CC
+  !$omp end target
+
+  !$omp target exit data map(delete: CC) map(from: DD)
+
+  if (any (CC /= [(31*3*i, i=1,N)])) stop 3
+  if (any (DD /= [(31*3*5*i, i=1,N)])) stop 4
+end

Re: [Patch] libgomp – fix handling of 'target enter data'

2020-03-31 Thread Jakub Jelinek via Gcc-patches 
On Tue, Mar 31, 2020 at 05:14:17PM +0200, Tobias Burnus wrote:
> gomp_map_vars_internal contains:
> 
>   if ((kind & typemask) == GOMP_MAP_TO_PSET)
> {
>   size_t j;
>   for (j = i + 1; j < mapnum; j++)
> if (!GOMP_MAP_POINTER_P (get_kind (short_mapkind, kinds, j)
> 
> where one accesses not only the i-th hostaddr but items following it.
> 
> On the other hand, in GOMP_target_enter_exit_data one currently has:
> 
> for (i = 0; i < mapnum; i++)
>   if ((kinds[i] & 0xff) == GOMP_MAP_STRUCT) …
>   else
> gomp_map_vars (devicep, 1, &hostaddrs[i], NULL, &sizes[i], &kinds[i],
>true, GOMP_MAP_VARS_ENTER_DATA);
> passing the argument one by one.
> 
> I first thought to pass all non MAP_STRUCT items as block before the
> MAP_STRUCT, then a MAP_STRUCT and then try to group the next items.

Doing the mappings separately is intentional, while for target data or
target region mappings it is very likely they will be all released together
as well, so it doesn't matter if they all go from the single same mapping,
for target enter data it is often not the case.
If everything is mapped together, then it can't be really freed until all
the mappings get refcount of 0.
So, instead of the change you've posted, there should be in
GOMP_target_enter_exit_data code to also handle GOMP_MAP_TO_PSET and put the
right number of mappings after it in one block and leave the rest separate.

Jakub