Hi Julian! On 2020-06-16T15:39:45-0700, Julian Brown <jul...@codesourcery.com> wrote: > This patch fixes a set of XFAILs
The overall goal of couse is not to resolve XFAILs, but to implement the expected behavior. :-) > in some recently-added patches by > skipping a detach operation on "no-op" exit data operations for blocks > with zero dynamic refcount. So this is one aspect of <https://gcc.gnu.org/PR95203> "OpenACC 2.6 deep copy: attach/detach API routines: no-op behavior". > This takes advantage of the ordering of > detach clauses with respect to associated data-movement clauses: i.e., > they are grouped together adjacently. I'm not convinced that it's sufficient to just special-case these cases. Instead, per the OpenACC "Data Clause Actions" etc., shouldn't basically all 'gomp_fatal's that we have on 'attach'/'detach' code paths turn into no-ops ("no action is taken")? And I'd then again like to bring forward my idea from another review: | we may (rather easily?) add a flag variable (ICV; | initialized from an environment variable) to guard this checking | behavior? Here: to *keep* the 'gomp_fatal's. | I suppose we may now have a few libgomp testcases that | actually do [check things via expected 'gomp_fatal's], | which wouldn't work any longer [...]. Such testcases could then 'dg-set-target-env-var "GOMP_ATTACH_FATAL" "1"' (better name is desirable), and have one variant with and one variant without that enabled. Before you start re-working the patch, let's please first get agreement on what exactly we intend to achieve. Grüße Thomas > libgomp/ > * oacc-mem.c (find_group_last): Handle detach operations. > (goacc_exit_data_internal): Detect detachments that are part of copyout > operations, and suppress them if dynamic refcount is zero. > * testsuite/libgomp.oacc-fortran/mdc-refcount-1-1-1.f90: Remove XFAILs. > * testsuite/libgomp.oacc-fortran/mdc-refcount-1-1-2.F90: Fix typo. > * testsuite/libgomp.oacc-fortran/mdc-refcount-1-2-1.f90: Remove XFAILs. > * testsuite/libgomp.oacc-fortran/mdc-refcount-1-2-2.f90: Likewise. > * testsuite/libgomp.oacc-fortran/mdc-refcount-1-3-1.f90: Likewise. > * testsuite/libgomp.oacc-fortran/mdc-refcount-1-3-2.f90: Likewise. > * testsuite/libgomp.oacc-fortran/mdc-refcount-1-4-1.f90: Likewise. > * testsuite/libgomp.oacc-fortran/mdc-refcount-1-4-2.f90: Likewise. > --- > libgomp/oacc-mem.c | 54 ++++++++++++++++--- > .../mdc-refcount-1-1-1.f90 | 6 +-- > .../mdc-refcount-1-1-2.F90 | 2 +- > .../mdc-refcount-1-2-1.f90 | 6 +-- > .../mdc-refcount-1-2-2.f90 | 6 +-- > .../mdc-refcount-1-3-1.f90 | 6 +-- > .../mdc-refcount-1-3-2.f90 | 5 +- > .../mdc-refcount-1-4-1.f90 | 6 +-- > .../mdc-refcount-1-4-2.f90 | 5 +- > 9 files changed, 55 insertions(+), 41 deletions(-) > > diff --git a/libgomp/oacc-mem.c b/libgomp/oacc-mem.c > index 745cb132621..f852652c048 100644 > --- a/libgomp/oacc-mem.c > +++ b/libgomp/oacc-mem.c > @@ -987,7 +987,9 @@ find_group_last (int pos, size_t mapnum, size_t *sizes, > unsigned short *kinds) > { > case GOMP_MAP_TO_PSET: > if (pos + 1 < mapnum > - && (kinds[pos + 1] & 0xff) == GOMP_MAP_ATTACH) > + && ((kinds[pos + 1] & 0xff) == GOMP_MAP_ATTACH > + || (kinds[pos + 1] & 0xff) == GOMP_MAP_DETACH > + || (kinds[pos + 1] & 0xff) == GOMP_MAP_FORCE_DETACH)) > return pos + 1; > > while (pos + 1 < mapnum > @@ -1010,6 +1012,8 @@ find_group_last (int pos, size_t mapnum, size_t *sizes, > unsigned short *kinds) > break; > > case GOMP_MAP_ATTACH: > + case GOMP_MAP_DETACH: > + case GOMP_MAP_FORCE_DETACH: > return pos; > > default: > @@ -1025,7 +1029,9 @@ find_group_last (int pos, size_t mapnum, size_t *sizes, > unsigned short *kinds) > /* We can have a single GOMP_MAP_ATTACH mapping after a to/from > mapping. */ > if (pos + 1 < mapnum > - && (kinds[pos + 1] & 0xff) == GOMP_MAP_ATTACH) > + && ((kinds[pos + 1] & 0xff) == GOMP_MAP_ATTACH > + || (kinds[pos + 1] & 0xff) == GOMP_MAP_DETACH > + || (kinds[pos + 1] & 0xff) == GOMP_MAP_FORCE_DETACH)) > return pos + 1; > > /* We can have zero or more GOMP_MAP_POINTER mappings after a to/from > @@ -1168,15 +1174,43 @@ goacc_exit_data_internal (struct gomp_device_descr > *acc_dev, size_t mapnum, > { > gomp_mutex_lock (&acc_dev->lock); > > - /* Handle "detach" before copyback/deletion of mapped data. */ > - for (size_t i = 0; i < mapnum; ++i) > + /* Handle "detach" before copyback/deletion of mapped data. If this isn't > a > + standalone "detach" clause, take care to skip the "detach" operation if > + the dynamic refcount of the data to be detached is zero. */ > + for (size_t grp = 0; grp < mapnum; grp++) > { > - unsigned char kind = kinds[i] & 0xff; > + size_t i = grp, group_last = find_group_last (grp, mapnum, sizes, > kinds); > + unsigned char kind = kinds[grp] & 0xff; > bool finalize = false; > + > switch (kind) > { > + case GOMP_MAP_TO_PSET: > + case GOMP_MAP_TOFROM: > + case GOMP_MAP_FROM: > + case GOMP_MAP_FORCE_FROM: > + case GOMP_MAP_RELEASE: > + case GOMP_MAP_DELETE: > + { > + if (i + 1 >= mapnum) > + break; > + kind = kinds[i + 1] & 0xff; > + if (kind != GOMP_MAP_FORCE_DETACH && kind != GOMP_MAP_DETACH) > + break; > + splay_tree_key n = lookup_host (acc_dev, hostaddrs[i], sizes[i]); > + if (n == NULL) > + { > + gomp_mutex_unlock (&acc_dev->lock); > + gomp_fatal ("target data not mapped for detach operation"); > + } > + i++; > + if (n->dynamic_refcount == 0) > + break; > + } > + /* Fallthrough. */ > + > case GOMP_MAP_FORCE_DETACH: > - finalize = true; > + finalize = (kind == GOMP_MAP_FORCE_DETACH); > /* Fallthrough. */ > > case GOMP_MAP_DETACH: > @@ -1197,9 +1231,15 @@ goacc_exit_data_internal (struct gomp_device_descr > *acc_dev, size_t mapnum, > gomp_detach_pointer (acc_dev, aq, n, hostaddr, finalize, NULL); > } > break; > + case GOMP_MAP_STRUCT: > + case GOMP_MAP_POINTER: > + /* Ignore. */ > + break; > default: > - ; > + gomp_fatal (">>>> goacc_exit_data_internal UNHANDLED kind 0x%.2x", > + kind); > } > + grp = group_last; > } > > for (size_t i = 0; i < mapnum; ++i) > diff --git a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-1-1.f90 > b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-1-1.f90 > index 445cbabb8ca..7171affb9f0 100644 > --- a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-1-1.f90 > +++ b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-1-1.f90 > @@ -24,12 +24,8 @@ program main > print *, "CheCKpOInT1" > ! { dg-output ".*CheCKpOInT1(\n|\r\n|\r)" } > !$acc exit data delete(var%a) finalize > - !TODO goacc_exit_data_internal: Assertion `is_tgt_unmapped || > num_mappings > 1' failed. > - !TODO { dg-output ".*\[Aa\]ssert.*is_tgt_unmapped" { target { ! > openacc_host_selected } } } ! Scan for what we expect in the "XFAILed" case > (without actually XFAILing). > - !TODO { dg-shouldfail "XFAILed" { ! openacc_host_selected } } ! ... > instead of 'dg-xfail-run-if' so that 'dg-output' is evaluated at all. > - !TODO { dg-final { if { [dg-process-target { xfail { ! > openacc_host_selected } }] == "F" } { xfail "[testname-for-summary] really is > XFAILed" } } } ! ... so that we still get an XFAIL visible in the log. > print *, "CheCKpOInT2" > - ! { dg-output ".CheCKpOInT2(\n|\r\n|\r)" { target { openacc_host_selected > } } } > + ! { dg-output ".*CheCKpOInT2(\n|\r\n|\r)" } > if (acc_is_present(var%a)) stop 3 > if (.not. acc_is_present(var)) stop 4 > > diff --git a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-1-2.F90 > b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-1-2.F90 > index 7b206ac2042..2aa46189e9a 100644 > --- a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-1-2.F90 > +++ b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-1-2.F90 > @@ -6,4 +6,4 @@ > #include "mdc-refcount-1-1-1.f90" > > ! { dg-output ".*CheCKpOInT1(\n|\r\n|\r)" } > -! { dg-output ".CheCKpOInT2(\n|\r\n|\r)" } > +! { dg-output ".*CheCKpOInT2(\n|\r\n|\r)" } > diff --git a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-2-1.f90 > b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-2-1.f90 > index 8554534b2f2..9a10aa5a781 100644 > --- a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-2-1.f90 > +++ b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-2-1.f90 > @@ -26,12 +26,8 @@ program main > print *, "CheCKpOInT1" > ! { dg-output ".*CheCKpOInT1(\n|\r\n|\r)" } > !$acc exit data delete(var%a) finalize > - !TODO goacc_exit_data_internal: Assertion `is_tgt_unmapped || > num_mappings > 1' failed. > - !TODO { dg-output ".*\[Aa\]ssert.*is_tgt_unmapped" { target { ! > openacc_host_selected } } } ! Scan for what we expect in the "XFAILed" case > (without actually XFAILing). > - !TODO { dg-shouldfail "XFAILed" { ! openacc_host_selected } } ! ... > instead of 'dg-xfail-run-if' so that 'dg-output' is evaluated at all. > - !TODO { dg-final { if { [dg-process-target { xfail { ! > openacc_host_selected } }] == "F" } { xfail "[testname-for-summary] really is > XFAILed" } } } ! ... so that we still get an XFAIL visible in the log. > print *, "CheCKpOInT2" > - ! { dg-output ".CheCKpOInT2(\n|\r\n|\r)" { target { openacc_host_selected > } } } > + ! { dg-output ".*CheCKpOInT2(\n|\r\n|\r)" } > if (acc_is_present(var%a)) stop 3 > if (.not. acc_is_present(var)) stop 4 > > diff --git a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-2-2.f90 > b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-2-2.f90 > index 8e696cc70e8..f506adf8e91 100644 > --- a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-2-2.f90 > +++ b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-2-2.f90 > @@ -26,12 +26,8 @@ program main > print *, "CheCKpOInT1" > ! { dg-output ".*CheCKpOInT1(\n|\r\n|\r)" } > !$acc exit data delete(var%a) > - !TODO goacc_exit_data_internal: Assertion `is_tgt_unmapped || > num_mappings > 1' failed. > - !TODO { dg-output ".*\[Aa\]ssert.*is_tgt_unmapped" { target { ! > openacc_host_selected } } } ! Scan for what we expect in the "XFAILed" case > (without actually XFAILing). > - !TODO { dg-shouldfail "XFAILed" { ! openacc_host_selected } } ! ... > instead of 'dg-xfail-run-if' so that 'dg-output' is evaluated at all. > - !TODO { dg-final { if { [dg-process-target { xfail { ! > openacc_host_selected } }] == "F" } { xfail "[testname-for-summary] really is > XFAILed" } } } ! ... so that we still get an XFAIL visible in the log. > print *, "CheCKpOInT2" > - ! { dg-output ".CheCKpOInT2(\n|\r\n|\r)" { target { openacc_host_selected > } } } > + ! { dg-output ".*CheCKpOInT2(\n|\r\n|\r)" } > if (acc_is_present(var%a)) stop 3 > if (.not. acc_is_present(var)) stop 4 > > diff --git a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-3-1.f90 > b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-3-1.f90 > index 070a6f8e149..450d95d3686 100644 > --- a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-3-1.f90 > +++ b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-3-1.f90 > @@ -27,12 +27,8 @@ program main > print *, "CheCKpOInT1" > ! { dg-output ".*CheCKpOInT1(\n|\r\n|\r)" } > !$acc exit data delete(var%a) finalize > - !TODO goacc_exit_data_internal: Assertion `is_tgt_unmapped || > num_mappings > 1' failed. > - !TODO { dg-output ".*\[Aa\]ssert.*is_tgt_unmapped" { target { ! > openacc_host_selected } } } ! Scan for what we expect in the "XFAILed" case > (without actually XFAILing). > - !TODO { dg-shouldfail "XFAILed" { ! openacc_host_selected } } ! ... > instead of 'dg-xfail-run-if' so that 'dg-output' is evaluated at all. > - !TODO { dg-final { if { [dg-process-target { xfail { ! > openacc_host_selected } }] == "F" } { xfail "[testname-for-summary] really is > XFAILed" } } } ! ... so that we still get an XFAIL visible in the log. > print *, "CheCKpOInT2" > - ! { dg-output ".CheCKpOInT2(\n|\r\n|\r)" { target { openacc_host_selected > } } } > + ! { dg-output ".*CheCKpOInT2(\n|\r\n|\r)" } > if (acc_is_present(var%a)) stop 3 > if (.not. acc_is_present(var)) stop 4 > > diff --git a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-3-2.f90 > b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-3-2.f90 > index 3c4bbda7f66..35efad4138a 100644 > --- a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-3-2.f90 > +++ b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-3-2.f90 > @@ -27,11 +27,8 @@ program main > print *, "CheCKpOInT1" > ! { dg-output ".*CheCKpOInT1(\n|\r\n|\r)" } > !$acc exit data delete(var%a) > - !TODO { dg-output "(\n|\r\n|\r)libgomp: attach count > underflow(\n|\r\n|\r)$" { target { ! openacc_host_selected } } } ! Scan for > what we expect in the "XFAILed" case (without actually XFAILing). > - !TODO { dg-shouldfail "XFAILed" { ! openacc_host_selected } } ! ... > instead of 'dg-xfail-run-if' so that 'dg-output' is evaluated at all. > - !TODO { dg-final { if { [dg-process-target { xfail { ! > openacc_host_selected } }] == "F" } { xfail "[testname-for-summary] really is > XFAILed" } } } ! ... so that we still get an XFAIL visible in the log. > print *, "CheCKpOInT2" > - ! { dg-output ".CheCKpOInT2(\n|\r\n|\r)" { target { openacc_host_selected > } } } > + ! { dg-output ".*CheCKpOInT2(\n|\r\n|\r)" } > if (acc_is_present(var%a)) stop 3 > if (.not. acc_is_present(var)) stop 4 > > diff --git a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-4-1.f90 > b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-4-1.f90 > index b22e411567f..816562fc055 100644 > --- a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-4-1.f90 > +++ b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-4-1.f90 > @@ -26,12 +26,8 @@ program main > print *, "CheCKpOInT1" > ! { dg-output ".*CheCKpOInT1(\n|\r\n|\r)" } > !$acc exit data detach(var%a) finalize > - !TODO goacc_exit_data_internal: Assertion `is_tgt_unmapped || > num_mappings > 1' failed. > - !TODO { dg-output ".*\[Aa\]ssert.*is_tgt_unmapped" { target { ! > openacc_host_selected } } } ! Scan for what we expect in the "XFAILed" case > (without actually XFAILing). > - !TODO { dg-shouldfail "XFAILed" { ! openacc_host_selected } } ! ... > instead of 'dg-xfail-run-if' so that 'dg-output' is evaluated at all. > - !TODO { dg-final { if { [dg-process-target { xfail { ! > openacc_host_selected } }] == "F" } { xfail "[testname-for-summary] really is > XFAILed" } } } ! ... so that we still get an XFAIL visible in the log. > print *, "CheCKpOInT2" > - ! { dg-output ".CheCKpOInT2(\n|\r\n|\r)" { target { openacc_host_selected > } } } > + ! { dg-output ".*CheCKpOInT2(\n|\r\n|\r)" } > !$acc exit data delete(var%a) > if (acc_is_present(var%a)) stop 3 > if (.not. acc_is_present(var)) stop 4 > diff --git a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-4-2.f90 > b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-4-2.f90 > index 476cd5c1bee..b98bfd74924 100644 > --- a/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-4-2.f90 > +++ b/libgomp/testsuite/libgomp.oacc-fortran/mdc-refcount-1-4-2.f90 > @@ -27,11 +27,8 @@ program main > print *, "CheCKpOInT1" > ! { dg-output ".*CheCKpOInT1(\n|\r\n|\r)" } > !$acc exit data delete(var%a) > - !TODO { dg-output "(\n|\r\n|\r)libgomp: attach count > underflow(\n|\r\n|\r)$" { target { ! openacc_host_selected } } } ! Scan for > what we expect in the "XFAILed" case (without actually XFAILing). > - !TODO { dg-shouldfail "XFAILed" { ! openacc_host_selected } } ! ... > instead of 'dg-xfail-run-if' so that 'dg-output' is evaluated at all. > - !TODO { dg-final { if { [dg-process-target { xfail { ! > openacc_host_selected } }] == "F" } { xfail "[testname-for-summary] really is > XFAILed" } } } ! ... so that we still get an XFAIL visible in the log. > print *, "CheCKpOInT2" > - ! { dg-output ".CheCKpOInT2(\n|\r\n|\r)" { target { openacc_host_selected > } } } > + ! { dg-output ".*CheCKpOInT2(\n|\r\n|\r)" } > if (acc_is_present(var%a)) stop 3 > if (.not. acc_is_present(var)) stop 4 > > -- > 2.23.0 ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
