From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa3.mentor.iphmx.com (esa3.mentor.iphmx.com [68.232.137.180]) by sourceware.org (Postfix) with ESMTPS id BA40B3844079 for ; Fri, 3 Jul 2020 15:29:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org BA40B3844079 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=Thomas_Schwinge@mentor.com IronPort-SDR: a+6AcvG+BEoWuTBiikkW/fElWVMzebXIzpAKcHgEpYgtFTrF5kvJgi61DpitDlBjSK54ews29t 2hwzYBldddoIHbaRw/CAPpf4KUVVow1uUMgX99b+wHNYdb8C3rUtLL8g3LD5ey8Rk9Ajq1q87P S+s5AvBGfgopgtzl+X642zIni9i+AChBTaxSiSH7mWHhee9V9vT9VsRGrAI28HlAoeG6Syc3g1 36TmC3Nhz5rWhNkXOlKgNYjPD4puhYPArndx1GY7TDOeeb0nlrJo1tB/45JyDpFab+1YAjURc5 vVM= X-IronPort-AV: E=Sophos;i="5.75,308,1589270400"; d="scan'208,223";a="50563857" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa3.mentor.iphmx.com with ESMTP; 03 Jul 2020 07:29:18 -0800 IronPort-SDR: Yc3aWO2YhEXCjs3gmSqdI0tcokhusttEcIFV4z6nYDEyxQbD2Si79q12GmHmojB90QP5PSoHuQ tSrJPhBk/hTH+B26yosNWpF4p0hz4PCwKQFZmY73reZTKDm4qbYysZG9J9/qZBLosighPHrrLp meN9nnAGAD1lAD6JUlD39NTuZrE2s0l4lO+x8bh920aHMZWAJkEaJtK0bUFSZSqCqB1rC2YTca xFsLX8820eUA1paX6ZsA7MyXUZSIo88KD6vJUjvcBETCVU9ahKhS8r3RSMnnR4gTYyHlGwMGQ7 wz8= From: Thomas Schwinge To: Julian Brown , CC: Jakub Jelinek , Subject: Re: [PATCH 02/13] OpenACC reference count overhaul In-Reply-To: <87eeq31k6e.fsf@euler.schwinge.homeip.net> References: <491e3ca360313930f8f2f5686ffd386cf2fad04e.1576648001.git.julian@codesourcery.com> <87zha39asn.fsf@euler.schwinge.homeip.net> <87eeq31k6e.fsf@euler.schwinge.homeip.net> User-Agent: Notmuch/0.29.1+93~g67ed7df (https://notmuchmail.org) Emacs/26.3 (x86_64-pc-linux-gnu) Date: Fri, 3 Jul 2020 17:29:00 +0200 Message-ID: <87wo3ky5vn.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_LOTSOFHASH, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 03 Jul 2020 15:29:22 -0000 --=-=-= Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi! To move us one small step forward: On 2020-06-25T13:03:53+0200, I wrote: > Ping, in particular my question about different 'GOMP_MAP_FORCE_FROM' vs. > 'GOMP_MAP_FROM' handling. > > (I have not yet looked whether 'GOMP_MAP_ALWAYS_FROM' may be generate > nowadays, given your pending front end/middle end patches.) It isn't, at least not given the current test cases, and I'm not aware of data movement in OpenACC with (OpenMP) "always" semantics. > On 2020-05-19T17:58:16+0200, I wrote: >> On 2019-12-17T22:02:27-0800, Julian Brown wrot= e: >>> --- a/libgomp/oacc-mem.c >>> +++ b/libgomp/oacc-mem.c >> >> (Unhelpful diff trimmed.) >> >>> +/* Unmap variables for OpenACC "exit data", with optional finalization >>> + (affecting all mappings in this operation). */ >> >>> +static void >>> +goacc_exit_data_internal (struct gomp_device_descr *acc_dev, size_t ma= pnum, >>> + void **hostaddrs, size_t *sizes, >>> + unsigned short *kinds, bool finalize, goacc_aq aq= ) >>> +{ >>> + gomp_mutex_lock (&acc_dev->lock); >> >>> + for (size_t i =3D 0; i < mapnum; ++i) >>> { >> >>> + unsigned char kind =3D kinds[i] & 0xff; >>> + bool copyfrom =3D false; >> >>> + switch (kind) >> >>> + case GOMP_MAP_FROM: >>> + case GOMP_MAP_FORCE_FROM: >>> + case GOMP_MAP_ALWAYS_FROM: >>> + copyfrom =3D true; >>> + /* Fallthrough. */ >> >> What is the case that a 'GOMP_MAP_ALWAYS_FROM' would be generated for >> OpenACC code? Putting an 'assert' here, it never triggers, given the >> current set of libgomp test cases. If there is such a case, we should >> add a test case, otherwise, I suggest we do put an 'assert' here (whilst >> leaving in the supposedly correct code, if you'd like), to document that >> this not currently expected, and thus not tested? Instead of keeping dead code, I decided it's better to just "[OpenACC] Remove (unused) 'GOMP_MAP_ALWAYS_FROM' handling from 'libgomp/oacc-mem.c:goacc_exit_data_internal'"; pushed to master branch in commit 995aba5867b1c64b2b56a200ef16b135effe85f7, and releases/gcc-10 branch in commit ddce10e77f04410c4ce376e6efdf520a7311a11b, see attached. Should a 'GOMP_MAP_ALWAYS_FROM' now ever appear (I don't see how), it will be diagnosed via the 'gomp_fatal' with 'UNHANDLED kind'. >>> + >>> + case GOMP_MAP_TO_PSET: >>> + case GOMP_MAP_POINTER: >>> + case GOMP_MAP_DELETE: >>> + case GOMP_MAP_RELEASE: >>> + { >>> + struct splay_tree_key_s cur_node; >>> + cur_node.host_start =3D (uintptr_t) hostaddrs[i]; >>> + cur_node.host_end =3D cur_node.host_start >>> + + (kind =3D=3D GOMP_MAP_POINTER >>> + ? sizeof (void *) : sizes[i]); >>> + splay_tree_key n >>> + =3D splay_tree_lookup (&acc_dev->mem_map, &cur_node); >>> + >>> + if (n =3D=3D NULL) >>> + continue; >>> + >>> + if (finalize) >>> + { >>> + if (n->refcount !=3D REFCOUNT_INFINITY) >>> + n->refcount -=3D n->virtual_refcount; >>> + n->virtual_refcount =3D 0; >>> + } >>> + >>> + if (n->virtual_refcount > 0) >>> + { >>> + if (n->refcount !=3D REFCOUNT_INFINITY) >>> + n->refcount--; >>> + n->virtual_refcount--; >>> + } >>> + else if (n->refcount > 0 && n->refcount !=3D REFCOUNT_INFINITY) >>> + n->refcount--; >>> + >>> + if (copyfrom >>> + && (kind !=3D GOMP_MAP_FROM || n->refcount =3D=3D 0)) >>> + gomp_copy_dev2host (acc_dev, aq, (void *) cur_node.host_start= , >>> + (void *) (n->tgt->tgt_start + n->tgt_offs= et >>> + + cur_node.host_start >>> + - n->host_start), >>> + cur_node.host_end - cur_node.host_start); >> >> That 'kind !=3D GOMP_MAP_FROM' conditional looks wrong to me. This shou= ld >> instead be 'kind =3D=3D GOMP_MAP_ALWAYS_FROM'? Or, get removed, togethe= r >> with the 'GOMP_MAP_ALWAYS_FROM' handling above? But definitely >> 'GOMP_MAP_FORCE_FROM' and 'GOMP_MAP_FROM' need to be handled the same, a= s >> far as I can tell? I've now pushed "[OpenACC] Revert always-copyfrom behavior for 'GOMP_MAP_FORCE_FROM' in 'libgomp/oacc-mem.c:goacc_exit_data_internal'" to master branch in commit e7f3f7fe08bdd49367f682398e1d2f4e6b60ef84, and releases/gcc-10 branch in commit 50666d23b52794774eefbeff046d5c3235db8b99, see attached. >>> + >>> + if (n->refcount =3D=3D 0) >>> + gomp_remove_var_async (acc_dev, n, aq); >>> + } >>> + break; >>> + default: >>> + gomp_fatal (">>>> goacc_exit_data_internal UNHANDLED kind 0x%.2x"= , >>> + kind); >>> } >>> } >>> >>> gomp_mutex_unlock (&acc_dev->lock); >> >>> } Gr=C3=BC=C3=9Fe Thomas ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstra=C3=9Fe 201, 80634 M=C3=BCnch= en / Germany Registergericht M=C3=BCnchen HRB 106955, Gesch=C3=A4ftsf=C3=BChrer: Thomas = Heurung, Alexander Walter --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename="0001-OpenACC-Remove-unused-GOMP_MAP_ALWAYS_FROM-handling-.patch" >From 995aba5867b1c64b2b56a200ef16b135effe85f7 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge Date: Thu, 14 May 2020 19:17:32 +0200 Subject: [PATCH] [OpenACC] Remove (unused) 'GOMP_MAP_ALWAYS_FROM' handling from 'libgomp/oacc-mem.c:goacc_exit_data_internal' This had gotten added in commit 378da98fcc907d05002bcd3d6ff7951f0cf485e5 (r279621) "OpenACC reference count overhaul", but it doesn't have any use in OpenACC. libgomp/ * oacc-mem.c (goacc_exit_data_internal): Remove 'GOMP_MAP_ALWAYS_FROM' handling. --- libgomp/oacc-mem.c | 1 - 1 file changed, 1 deletion(-) diff --git a/libgomp/oacc-mem.c b/libgomp/oacc-mem.c index 936ae649dd93..1a0cd4caf287 100644 --- a/libgomp/oacc-mem.c +++ b/libgomp/oacc-mem.c @@ -1102,7 +1102,6 @@ goacc_exit_data_internal (struct gomp_device_descr *acc_dev, size_t mapnum, { case GOMP_MAP_FROM: case GOMP_MAP_FORCE_FROM: - case GOMP_MAP_ALWAYS_FROM: copyfrom = true; /* Fallthrough. */ -- 2.27.0 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename="0001-OpenACC-Remove-unused-GOMP_MAP_ALWAYS_FROM-handl.g10.patch" >From ddce10e77f04410c4ce376e6efdf520a7311a11b Mon Sep 17 00:00:00 2001 From: Thomas Schwinge Date: Thu, 14 May 2020 19:17:32 +0200 Subject: [PATCH] [OpenACC] Remove (unused) 'GOMP_MAP_ALWAYS_FROM' handling from 'libgomp/oacc-mem.c:goacc_exit_data_internal' This had gotten added in commit 378da98fcc907d05002bcd3d6ff7951f0cf485e5 (r279621) "OpenACC reference count overhaul", but it doesn't have any use in OpenACC. libgomp/ * oacc-mem.c (goacc_exit_data_internal): Remove 'GOMP_MAP_ALWAYS_FROM' handling. (cherry picked from commit 995aba5867b1c64b2b56a200ef16b135effe85f7) --- libgomp/oacc-mem.c | 1 - 1 file changed, 1 deletion(-) diff --git a/libgomp/oacc-mem.c b/libgomp/oacc-mem.c index 936ae649dd93..1a0cd4caf287 100644 --- a/libgomp/oacc-mem.c +++ b/libgomp/oacc-mem.c @@ -1102,7 +1102,6 @@ goacc_exit_data_internal (struct gomp_device_descr *acc_dev, size_t mapnum, { case GOMP_MAP_FROM: case GOMP_MAP_FORCE_FROM: - case GOMP_MAP_ALWAYS_FROM: copyfrom = true; /* Fallthrough. */ -- 2.27.0 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename="0001-OpenACC-Revert-always-copyfrom-behavior-for-GOMP_MAP.patch" >From e7f3f7fe08bdd49367f682398e1d2f4e6b60ef84 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge Date: Thu, 14 May 2020 20:48:10 +0200 Subject: [PATCH] [OpenACC] Revert always-copyfrom behavior for 'GOMP_MAP_FORCE_FROM' in 'libgomp/oacc-mem.c:goacc_exit_data_internal' As done for 'GOMP_MAP_FROM', also for 'GOMP_MAP_FORCE_FROM' we should only 'gomp_copy_dev2host' if 'n->refcount == 0'. This had gotten altered in commit 378da98fcc907d05002bcd3d6ff7951f0cf485e5 (r279621) "OpenACC reference count overhaul". libgomp/ * oacc-mem.c (goacc_exit_data_internal): Revert always-copyfrom behavior for 'GOMP_MAP_FORCE_FROM'. * testsuite/libgomp.oacc-c-c++-common/pr92843-1.c: Adjust XFAIL. --- libgomp/oacc-mem.c | 17 +++++++++-------- .../libgomp.oacc-c-c++-common/pr92843-1.c | 10 +++++++++- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/libgomp/oacc-mem.c b/libgomp/oacc-mem.c index 1a0cd4caf287..4fb78ee96348 100644 --- a/libgomp/oacc-mem.c +++ b/libgomp/oacc-mem.c @@ -1144,16 +1144,17 @@ goacc_exit_data_internal (struct gomp_device_descr *acc_dev, size_t mapnum, else if (n->refcount > 0 && n->refcount != REFCOUNT_INFINITY) n->refcount--; - if (copyfrom - && (kind != GOMP_MAP_FROM || n->refcount == 0)) - gomp_copy_dev2host (acc_dev, aq, (void *) cur_node.host_start, - (void *) (n->tgt->tgt_start + n->tgt_offset - + cur_node.host_start - - n->host_start), - cur_node.host_end - cur_node.host_start); - if (n->refcount == 0) { + if (copyfrom) + { + void *d = (void *) (n->tgt->tgt_start + n->tgt_offset + + cur_node.host_start - n->host_start); + gomp_copy_dev2host (acc_dev, aq, + (void *) cur_node.host_start, d, + cur_node.host_end - cur_node.host_start); + } + if (aq) /* TODO We can't do the 'is_tgt_unmapped' checking -- see the 'gomp_unref_tgt' comment in diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/pr92843-1.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/pr92843-1.c index f16c46a37bfb..78fe1402ad46 100644 --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/pr92843-1.c +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/pr92843-1.c @@ -1,10 +1,10 @@ /* Verify that 'acc_copyout' etc. is a no-op if there's still a structured reference count. */ -/* { dg-xfail-run-if "TODO PR92843" { *-*-* } } */ /* { dg-skip-if "" { *-*-* } { "*" } { "-DACC_MEM_SHARED=0" } } */ #include +#include #include #include @@ -135,7 +135,15 @@ test_acc_data () assert (acc_is_present (h, sizeof h)); assign_array (h, N, c1); + fprintf (stderr, "CheCKpOInT1\n"); + // { dg-output ".*CheCKpOInT1(\n|\r\n|\r)" } acc_copyout_finalize (h, sizeof h); + //TODO goacc_exit_datum: 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. + fprintf (stderr, "CheCKpOInT2\n"); + // { dg-output ".CheCKpOInT2(\n|\r\n|\r)" { target { openacc_host_selected } } } assert (acc_is_present (h, sizeof h)); verify_array (h, N, c1); -- 2.27.0 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename="0001-OpenACC-Revert-always-copyfrom-behavior-for-GOMP.g10.patch" >From 50666d23b52794774eefbeff046d5c3235db8b99 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge Date: Thu, 14 May 2020 20:48:10 +0200 Subject: [PATCH] [OpenACC] Revert always-copyfrom behavior for 'GOMP_MAP_FORCE_FROM' in 'libgomp/oacc-mem.c:goacc_exit_data_internal' As done for 'GOMP_MAP_FROM', also for 'GOMP_MAP_FORCE_FROM' we should only 'gomp_copy_dev2host' if 'n->refcount == 0'. This had gotten altered in commit 378da98fcc907d05002bcd3d6ff7951f0cf485e5 (r279621) "OpenACC reference count overhaul". libgomp/ * oacc-mem.c (goacc_exit_data_internal): Revert always-copyfrom behavior for 'GOMP_MAP_FORCE_FROM'. * testsuite/libgomp.oacc-c-c++-common/pr92843-1.c: Adjust XFAIL. (cherry picked from commit e7f3f7fe08bdd49367f682398e1d2f4e6b60ef84) --- libgomp/oacc-mem.c | 17 +++++++++-------- .../libgomp.oacc-c-c++-common/pr92843-1.c | 10 +++++++++- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/libgomp/oacc-mem.c b/libgomp/oacc-mem.c index 1a0cd4caf287..4fb78ee96348 100644 --- a/libgomp/oacc-mem.c +++ b/libgomp/oacc-mem.c @@ -1144,16 +1144,17 @@ goacc_exit_data_internal (struct gomp_device_descr *acc_dev, size_t mapnum, else if (n->refcount > 0 && n->refcount != REFCOUNT_INFINITY) n->refcount--; - if (copyfrom - && (kind != GOMP_MAP_FROM || n->refcount == 0)) - gomp_copy_dev2host (acc_dev, aq, (void *) cur_node.host_start, - (void *) (n->tgt->tgt_start + n->tgt_offset - + cur_node.host_start - - n->host_start), - cur_node.host_end - cur_node.host_start); - if (n->refcount == 0) { + if (copyfrom) + { + void *d = (void *) (n->tgt->tgt_start + n->tgt_offset + + cur_node.host_start - n->host_start); + gomp_copy_dev2host (acc_dev, aq, + (void *) cur_node.host_start, d, + cur_node.host_end - cur_node.host_start); + } + if (aq) /* TODO We can't do the 'is_tgt_unmapped' checking -- see the 'gomp_unref_tgt' comment in diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/pr92843-1.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/pr92843-1.c index f16c46a37bfb..78fe1402ad46 100644 --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/pr92843-1.c +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/pr92843-1.c @@ -1,10 +1,10 @@ /* Verify that 'acc_copyout' etc. is a no-op if there's still a structured reference count. */ -/* { dg-xfail-run-if "TODO PR92843" { *-*-* } } */ /* { dg-skip-if "" { *-*-* } { "*" } { "-DACC_MEM_SHARED=0" } } */ #include +#include #include #include @@ -135,7 +135,15 @@ test_acc_data () assert (acc_is_present (h, sizeof h)); assign_array (h, N, c1); + fprintf (stderr, "CheCKpOInT1\n"); + // { dg-output ".*CheCKpOInT1(\n|\r\n|\r)" } acc_copyout_finalize (h, sizeof h); + //TODO goacc_exit_datum: 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. + fprintf (stderr, "CheCKpOInT2\n"); + // { dg-output ".CheCKpOInT2(\n|\r\n|\r)" { target { openacc_host_selected } } } assert (acc_is_present (h, sizeof h)); verify_array (h, N, c1); -- 2.27.0 --=-=-=--