| Message ID | b8284a64-b220-5f78-8476-8180ad9194f7@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | libgomp – fix handling of 'target enter data' | expand |
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
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
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
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
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
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
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