Re: [PATCH] OpenACC "present" subarrays: runtime API return value and unmapping fixes

[Thomas Schwinge]

Hi!

On 2019-11-14T17:02:02+0100, I wrote:
> [...] I couldn't really find wording in the
> OpenACC specification that explicitly permits such things.  But given
> that, for example, in OpenACC 2.7, 3.2.20. "acc_copyin", 'acc_copyin' is
> described to be "equivalent to the 'enter data' directive with a 'copyin'
> clause", and the latter supposedly (?) does allow such "subset subarray
> mappings", and in 2.7.6. "copyin clause" it is said that "An 'enter data'
> directive with a 'copyin' clause is functionally equivalent to a call to
> the 'acc_copyin' API routine", that's probably motivation enough to fix
> the latter to conform what the former supposedly already is allowing
> (though not implementing by means of 'enter data copyin' just calling
> 'acc_copyin' etc.
>
> I see that 2.7.6. "copyin clause" also states that "The restrictions
> regarding subarrays in the present clause apply to this clause", which
> per 2.7.4. "present clause" is that "If only a subarray of an array is
> present in the current device memory, the 'present' clause must specify
> the same subarray, or a subarray that is a proper subset of the subarray
> in the data lifetime".  From that we probably are to deduce that it's
> fine the other way round (as you've argued): if a subarray of an array
> (or, the whole array) is present in the current device memory, the
> 'present' clause may specify the same subarray, or a subarray that is a
> proper subset of the subarray in the data lifetime (my words).  Unless
> you object to that, we shall (later) try to get the clarified/amended in
> the OpenACC specification.

I filed  "Subset
subarray restrictions".


> Later (not now), we should then also add corresponding testing for actual
> 'data' etc. constructs being nested in that way.

> On 2019-11-09T01:04:21+, Julian Brown  wrote:
>> a couple of existing "shouldfail" tests no longer fail, and have been
>> adjusted accordingly.
>
> These should then actually be removed, or re-written, because in their
> current form they no longer make much sense, as far as I can tell:
>
> For example, 'libgomp.oacc-c-c++-common/lib-22.c':
>
> acc_copyin (h, N);
>
> ... followed by:
>
> acc_copyout (h + 1, N - 1);
>
> ... is now meant to no longer abort with a "surrounds2" message, but
> instead we now expect success, and '!acc_is_present'.
>
> I'll take care of that later on -- I have some more tests to add anyway.

See attached '[PR92511] More testing for OpenACC "present" subarrays',
committed to trunk in r279122.


Grüße
 Thomas


From 2d5187149761bb9566b2c221c9c7ae7a18c92822 Mon Sep 17 00:00:00 2001
From: tschwinge 
Date: Mon, 9 Dec 2019 11:40:36 +
Subject: [PATCH] [PR92511] More testing for OpenACC "present" subarrays

In particular, "subset subarrays".

	libgomp/
	PR libgomp/92511
	* testsuite/libgomp.oacc-c-c++-common/copyin-devptr-1.c: Remove
	this file...
	* testsuite/libgomp.oacc-c-c++-common/copyin-devptr-2.c: ..., and
	this file...
	* testsuite/libgomp.oacc-c-c++-common/lib-22.c: ..., and this
	file...
	* testsuite/libgomp.oacc-c-c++-common/lib-30.c: ..., and this
	file...
	* testsuite/libgomp.oacc-c-c++-common/subset-subarray-mappings-1-r-p.c:
	... with their content moved into, and extended in this new file.
	* testsuite/libgomp.oacc-c-c++-common/subset-subarray-mappings-1-d-a.c:
	New file.
	* testsuite/libgomp.oacc-c-c++-common/subset-subarray-mappings-1-d-p.c:
	Likewise.
	* testsuite/libgomp.oacc-c-c++-common/subset-subarray-mappings-1-r-a.c:
	Likewise.
	* testsuite/libgomp.oacc-c-c++-common/subset-subarray-mappings-2.c:
	Likewise.

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@279122 138bc75d-0d04-0410-961f-82ee72b054a4
---
 libgomp/ChangeLog |  20 +
 .../copyin-devptr-1.c |  28 -
 .../copyin-devptr-2.c |  35 --
 .../libgomp.oacc-c-c++-common/lib-22.c|  33 --
 .../libgomp.oacc-c-c++-common/lib-30.c|  30 -
 .../subset-subarray-mappings-1-d-a.c  |   7 +
 .../subset-subarray-mappings-1-d-p.c  |   7 +
 .../subset-subarray-mappings-1-r-a.c  |   7 +
 .../subset-subarray-mappings-1-r-p.c  | 514 ++
 .../subset-subarray-mappings-2.c  | 115 
 10 files changed, 670 insertions(+), 126 deletions(-)
 delete mode 100644 libgomp/testsuite/libgomp.oacc-c-c++-common/copyin-devptr-1.c
 delete mode 100644 libgomp/testsuite/libgomp.oacc-c-c++-common/copyin-devptr-2.c
 delete mode 100644 libgomp/testsuite/libgomp.oacc-c-c++-common/lib-22.c
 delete mode 100644 libgomp/testsuite/libgomp.oacc-c-c++-common/lib-30.c
 create mode 100644 libgomp/testsuite/libgomp.oacc-c-c++-common/subset-subarray-mappings-1-d-a.c
 create mode 100644 libgomp/testsuite/libgomp.oacc-c-c++-common/subset-subarray-mappings-1-d-p.c
 create mode 100644 libgomp/testsuite/libgomp.oacc-c-c++-common/subset-subarray-mappings-1-r-a.c
 create mode 100644 

Re: [PATCH] OpenACC "present" subarrays: runtime API return value and unmapping fixes

[Thomas Schwinge]

Hi Julian!

On 2019-11-09T01:04:21+, Julian Brown  wrote:
> This patch fixes an issue I noticed when investigating an answer
> for Thomas's question about device pointer return values in:
>
> https://gcc.gnu.org/ml/gcc-patches/2019-10/msg02260.html
>
> It looks to me like the return value for the present case is wrong in
> the existing code: in case of a acc_pcopyin or similar call that refers
> to a subarray of a larger block already mapped on the target, the
> device pointer return value will be the start of the larger block, not
> of the subarray being copied.

Note that I've filed  "[OpenACC] Support
subset subarray mappings", so please reference that one in the
ChangeLog/commit log.

Principal ACK for that problem, and it's solution
('libgomp/oacc-mem.c:present_create_copy' 'if (n)' change).

Then, I was confused, because I couldn't really find wording in the
OpenACC specification that explicitly permits such things.  But given
that, for example, in OpenACC 2.7, 3.2.20. "acc_copyin", 'acc_copyin' is
described to be "equivalent to the 'enter data' directive with a 'copyin'
clause", and the latter supposedly (?) does allow such "subset subarray
mappings", and in 2.7.6. "copyin clause" it is said that "An 'enter data'
directive with a 'copyin' clause is functionally equivalent to a call to
the 'acc_copyin' API routine", that's probably motivation enough to fix
the latter to conform what the former supposedly already is allowing
(though not implementing by means of 'enter data copyin' just calling
'acc_copyin' etc.

I see that 2.7.6. "copyin clause" also states that "The restrictions
regarding subarrays in the present clause apply to this clause", which
per 2.7.4. "present clause" is that "If only a subarray of an array is
present in the current device memory, the 'present' clause must specify
the same subarray, or a subarray that is a proper subset of the subarray
in the data lifetime".  From that we probably are to deduce that it's
fine the other way round (as you've argued): if a subarray of an array
(or, the whole array) is present in the current device memory, the
'present' clause may specify the same subarray, or a subarray that is a
proper subset of the subarray in the data lifetime (my words).  Unless
you object to that, we shall (later) try to get the clarified/amended in
the OpenACC specification.

Indeed I am confirming that such subset subarray mappings do work fine
with PGI 19.4 and 19.10 -- but only when using OpenACC directives, not
necessarily when using OpenACC runtime library calls, huh.  (That's not
our problem to solve, of course, and under the assumption that my test
case has actually been valid.)

Later (not now), we should then also add corresponding testing for actual
'data' etc. constructs being nested in that way.

> The attached patch corrects this issue, and also relaxes a restriction
> on acc_delete, acc_copyout (etc.) to allow them to unmap/copyout
> subarrays of a larger block already present on the target. There's no
> particular reason to disallow that, as far as I can tell.

(That's where PGI fails at runtime, but I have not analyzed how exactly
this fails -- let's first clarify that with OpenACC Technical Committee,
later on.)

> This is
> necessary to allow the new tests included with this patch to pass, and
> a couple of existing "shouldfail" tests no longer fail, and have been
> adjusted accordingly.

These should then actually be removed, or re-written, because in their
current form they no longer make much sense, as far as I can tell:

For example, 'libgomp.oacc-c-c++-common/lib-22.c':

acc_copyin (h, N);

... followed by:

acc_copyout (h + 1, N - 1);

... is now meant to no longer abort with a "surrounds2" message, but
instead we now expect success, and '!acc_is_present'.

I'll take care of that later on -- I have some more tests to add anyway.

> It's still an error to try to copy data beyond
> the bounds of a mapped block, and other existing tests cover those
> cases.

ACK.

> The calculation for the return value for the non-present case of
> present_create_copy has also been adjusted in anticipation of a new
> version of the above-linked patch.

But please back out this one, for it's not related to this bug fix, and
we shall take care of that in a later patch.  (No need for you to re-post
that one just for this.)

> Tested with offloading to nvptx. OK for trunk?

I'm see C++ compilation failures the new libgomp test cases; OK with
these resolved.  To record the review effort, please include
"Reviewed-by: Thomas Schwinge " in the commit
log, see .


Grüße
 Thomas


signature.asc
Description: PGP signature

[PATCH] OpenACC "present" subarrays: runtime API return value and unmapping fixes

[Julian Brown]

Hi,

This patch fixes an issue I noticed when investigating an answer
for Thomas's question about device pointer return values in:

https://gcc.gnu.org/ml/gcc-patches/2019-10/msg02260.html

It looks to me like the return value for the present case is wrong in
the existing code: in case of a acc_pcopyin or similar call that refers
to a subarray of a larger block already mapped on the target, the
device pointer return value will be the start of the larger block, not
of the subarray being copied.

The attached patch corrects this issue, and also relaxes a restriction
on acc_delete, acc_copyout (etc.) to allow them to unmap/copyout
subarrays of a larger block already present on the target. There's no
particular reason to disallow that, as far as I can tell. This is
necessary to allow the new tests included with this patch to pass, and
a couple of existing "shouldfail" tests no longer fail, and have been
adjusted accordingly. It's still an error to try to copy data beyond
the bounds of a mapped block, and other existing tests cover those
cases.

The calculation for the return value for the non-present case of
present_create_copy has also been adjusted in anticipation of a new
version of the above-linked patch.

Tested with offloading to nvptx. OK for trunk?

Julian

ChangeLog

libgomp/
* oacc-mem.c (present_create_copy): Fix device pointer return value in
case of "present" subarray.  Use tgt->tgt_start instead of tgt->to_free
in non-present/create case.
(delete_copyout): Change error condition to detect only copies outside
of mapped block.  Adjust error message accordingly.
* testsuite/libgomp.oacc-c-c++-common/copyin-devptr-1.c: New test.
* testsuite/libgomp.oacc-c-c++-common/copyin-devptr-2.c: New test.
* testsuite/libgomp.oacc-c-c++-common/lib-20.c: Adjust expected error
message.
* testsuite/libgomp.oacc-c-c++-common/lib-23.c: Likewise.
* testsuite/libgomp.oacc-c-c++-common/lib-22.c: Allow test to pass now.
* testsuite/libgomp.oacc-c-c++-common/lib-30.c: Likewise.
commit 00607b06c8e506b0f0744a230856e1e8776633c3
Author: Julian Brown 
Date:   Thu Nov 7 14:24:49 2019 -0800

OpenACC "present" subarrays: runtime API return value and unmapping fixes

libgomp/
* oacc-mem.c (present_create_copy): Fix device pointer return value in
case of "present" subarray.  Use tgt->tgt_start instead of tgt->to_free
in non-present/create case.
(delete_copyout): Change error condition to fail only on copies outside
of mapped block.  Adjust error message accordingly.
* testsuite/libgomp.oacc-c-c++-common/copyin-devptr-1.c: New test.
* testsuite/libgomp.oacc-c-c++-common/copyin-devptr-2.c: New test.
* testsuite/libgomp.oacc-c-c++-common/lib-20.c: Adjust expected error
message.
* testsuite/libgomp.oacc-c-c++-common/lib-23.c: Likewise.
* testsuite/libgomp.oacc-c-c++-common/lib-22.c: Allow test to pass now.
* testsuite/libgomp.oacc-c-c++-common/lib-30.c: Likewise.

diff --git a/libgomp/oacc-mem.c b/libgomp/oacc-mem.c
index 2f271009fb8..0a41f11210c 100644
--- a/libgomp/oacc-mem.c
+++ b/libgomp/oacc-mem.c
@@ -535,7 +535,7 @@ present_create_copy (unsigned f, void *h, size_t s, int async)
   if (n)
 {
   /* Present. */
-  d = (void *) (n->tgt->tgt_start + n->tgt_offset);
+  d = (void *) (n->tgt->tgt_start + n->tgt_offset + h - n->host_start);
 
   if (!(f & FLAG_PRESENT))
 {
@@ -584,7 +584,7 @@ present_create_copy (unsigned f, void *h, size_t s, int async)
 
   gomp_mutex_lock (_dev->lock);
 
-  d = tgt->to_free;
+  d = (void *) tgt->tgt_start;
   tgt->prev = acc_dev->openacc.data_environ;
   acc_dev->openacc.data_environ = tgt;
 
@@ -669,7 +669,6 @@ acc_pcopyin (void *h, size_t s)
 static void
 delete_copyout (unsigned f, void *h, size_t s, int async, const char *libfnname)
 {
-  size_t host_size;
   splay_tree_key n;
   void *d;
   struct goacc_thread *thr = goacc_thread ();
@@ -703,13 +702,12 @@ delete_copyout (unsigned f, void *h, size_t s, int async, const char *libfnname)
   d = (void *) (n->tgt->tgt_start + n->tgt_offset
 		+ (uintptr_t) h - n->host_start);
 
-  host_size = n->host_end - n->host_start;
-
-  if (n->host_start != (uintptr_t) h || host_size != s)
+  if ((uintptr_t) h < n->host_start || (uintptr_t) h + s > n->host_end)
 {
+  size_t host_size = n->host_end - n->host_start;
   gomp_mutex_unlock (_dev->lock);
-  gomp_fatal ("[%p,%d] surrounds2 [%p,+%d]",
-		  (void *) n->host_start, (int) host_size, (void *) h, (int) s);
+  gomp_fatal ("[%p,+%d] outside mapped block [%p,+%d]",
+		  (void *) h, (int) s, (void *) n->host_start, (int) host_size);
 }
 
   if (n->refcount == REFCOUNT_INFINITY)
diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/copyin-devptr-1.c