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 <jul...@codesourcery.com> wrote:
>>> --- 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 mapnum,
>>> + void **hostaddrs, size_t *sizes,
>>> + unsigned short *kinds, bool finalize, goacc_aq aq)
>>> +{
>>> + gomp_mutex_lock (&acc_dev->lock);
>>
>>> + for (size_t i = 0; i < mapnum; ++i)
>>> {
>>
>>> + unsigned char kind = kinds[i] & 0xff;
>>> + bool copyfrom = false;
>>
>>> + switch (kind)
>>
>>> + case GOMP_MAP_FROM:
>>> + case GOMP_MAP_FORCE_FROM:
>>> + case GOMP_MAP_ALWAYS_FROM:
>>> + copyfrom = 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 = (uintptr_t) hostaddrs[i];
>>> + cur_node.host_end = cur_node.host_start
>>> + + (kind == GOMP_MAP_POINTER
>>> + ? sizeof (void *) : sizes[i]);
>>> + splay_tree_key n
>>> + = splay_tree_lookup (&acc_dev->mem_map, &cur_node);
>>> +
>>> + if (n == NULL)
>>> + continue;
>>> +
>>> + if (finalize)
>>> + {
>>> + if (n->refcount != REFCOUNT_INFINITY)
>>> + n->refcount -= n->virtual_refcount;
>>> + n->virtual_refcount = 0;
>>> + }
>>> +
>>> + if (n->virtual_refcount > 0)
>>> + {
>>> + if (n->refcount != REFCOUNT_INFINITY)
>>> + n->refcount--;
>>> + n->virtual_refcount--;
>>> + }
>>> + 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);
>>
>> That 'kind != GOMP_MAP_FROM' conditional looks wrong to me. This should
>> instead be 'kind == GOMP_MAP_ALWAYS_FROM'? Or, get removed, together
>> with the 'GOMP_MAP_ALWAYS_FROM' handling above? But definitely
>> 'GOMP_MAP_FORCE_FROM' and 'GOMP_MAP_FROM' need to be handled the same, as
>> 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 == 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üß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 995aba5867b1c64b2b56a200ef16b135effe85f7 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tho...@codesourcery.com>
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
>From ddce10e77f04410c4ce376e6efdf520a7311a11b Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tho...@codesourcery.com>
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
>From e7f3f7fe08bdd49367f682398e1d2f4e6b60ef84 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tho...@codesourcery.com>
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 <assert.h>
+#include <stdio.h>
#include <stdlib.h>
#include <openacc.h>
@@ -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
>From 50666d23b52794774eefbeff046d5c3235db8b99 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tho...@codesourcery.com>
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 <assert.h>
+#include <stdio.h>
#include <stdlib.h>
#include <openacc.h>
@@ -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
