libgomp – fix handling of 'target enter data'

Message ID b8284a64-b220-5f78-8476-8180ad9194f7@codesourcery.com
State New
Headers show
Series
  • libgomp – fix handling of 'target enter data'
Related show

Commit Message

Tobias Burnus March 31, 2020, 3:14 p.m.
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.

However, I could not find a difference between MAP_STRUCT and not,
hence, I now decided to simply pass the data on "as is".

OK for mainline? (What about older versions?)

Cheers,

Tobias

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter

Comments

Iain Buclaw via Gcc-patches March 31, 2020, 3:35 p.m. | #1
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
Tobias Burnus March 31, 2020, 5:41 p.m. | #2
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 00000000000..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
Iain Buclaw via Gcc-patches March 31, 2020, 6:03 p.m. | #3
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
Thomas Schwinge April 10, 2020, 2:31 p.m. | #4
Hi!

On 2020-03-31T19:41:40+0200, Tobias Burnus <tobias@codesourcery.com> 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 <http://mid.mail-archive.com/87pngsz1qy.fsf@euler.schwinge.homeip.net>
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 <thomas@codesourcery.com>

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  <thomas@codesourcery.com>
+
+	* testsuite/libgomp.fortran/target-enter-data-1.f90: Add 'dg-do
+	run'.
+
 2020-04-08  Tobias Burnus  <tobias@codesourcery.com>
 
 	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
Iain Buclaw via Gcc-patches April 10, 2020, 2:44 p.m. | #5
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 <http://mid.mail-archive.com/87pngsz1qy.fsf@euler.schwinge.homeip.net>

> 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

Patch

libgomp – fix handling of 'target enter data'

	* target.c (GOMP_target_enter_exit_data): Call gomp_map_vars with
	multiples args at once.
	* testsuite/libgomp.fortran/target-enter-data-1.f90: New.

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

diff --git a/libgomp/target.c b/libgomp/target.c
index c99dd5196fa..3de85434f5d 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -2480,18 +2480,9 @@  GOMP_target_enter_exit_data (int device, size_t mapnum, void **hostaddrs,
 	}
     }
 
-  size_t i;
   if ((flags & GOMP_TARGET_FLAG_EXIT_DATA) == 0)
-    for (i = 0; i < mapnum; i++)
-      if ((kinds[i] & 0xff) == GOMP_MAP_STRUCT)
-	{
-	  gomp_map_vars (devicep, sizes[i] + 1, &hostaddrs[i], NULL, &sizes[i],
-			 &kinds[i], true, GOMP_MAP_VARS_ENTER_DATA);
-	  i += sizes[i];
-	}
-      else
-	gomp_map_vars (devicep, 1, &hostaddrs[i], NULL, &sizes[i], &kinds[i],
-		       true, GOMP_MAP_VARS_ENTER_DATA);
+    gomp_map_vars (devicep, mapnum, hostaddrs, NULL, sizes, kinds,
+		   true, GOMP_MAP_VARS_ENTER_DATA);
   else
     gomp_exit_data (devicep, mapnum, hostaddrs, sizes, kinds);
 }
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 00000000000..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