Re: Make OpenACC 'acc_get_property' with 'acc_device_current' work (was: [PATCH] Add OpenACC 2.6 `acc_get_property' support)

[Harwath, Frederik]

Hi Thomas,

On 30.01.20 16:54, Thomas Schwinge wrote:
> 
> [...] the 'acc_device_current' interface should work already now.
> 
> [...] Please review
> the attached (Tobias the Fortran test cases, please), and test with AMD
> GCN offloading.  If approving this patch, please respond with

I have tested the patch with AMD GCN offloading and I have observed no 
regressions.
The new tests pass as expected and print the correct output.
Great that you have extended the Fortran tests!

> diff --git a/libgomp/oacc-init.c b/libgomp/oacc-init.c
> index ef12b4c16d01..c28c0f689ba2 100644
> --- a/libgomp/oacc-init.c
> +++ b/libgomp/oacc-init.c
> @@ -796,7 +796,9 @@ get_property_any (int ord, acc_device_t d, 
> acc_device_property_t prop)
> size_t
> acc_get_property (int ord, acc_device_t d, acc_device_property_t prop)
> {
> -  if (!known_device_type_p (d))
> +  if (d == acc_device_current)
> +; /* Allowed only for 'acc_get_property', 'acc_get_property_string'.  */
> +  else if (!known_device_type_p (d))
> unknown_device_type_error(d);

I don't like the empty if branch very much. Introducing a variable
(for instance, "bool allowed_device_type = acc_device_current
|| known_device_type(d);") would also provide a place for your comment.
You could also extract a function to avoid duplicating the explanation
in acc_get_property_string.

The patch looks good to me.

Reviewed-by: Frederik Harwath  

Best regards,
Frederik

Make OpenACC 'acc_get_property' with 'acc_device_current' work (was: [PATCH] Add OpenACC 2.6 `acc_get_property' support)

[Thomas Schwinge]

Hi!

On 2020-01-10T23:52:11+0100, I wrote:
> On 2019-12-21T23:02:38+0100, I wrote:
>> On 2019-12-20T17:46:57+0100, "Harwath, Frederik"  
>> wrote:
 > --- a/include/gomp-constants.h
 > +++ b/include/gomp-constants.h
>
 > +#define GOMP_DEVICE_CURRENT -3
>
 Should this actually get value '-1' instead of '-3'?  Or, is the OpenACC
 'acc_device_t' code already paying special attention to negative values
 '-1', '-2'?  (I don't think so.)
>
 | Also, 'acc_device_current' is a libgomp-internal thing (doesn't interface
 | with the compiler proper), so strictly speaking 'GOMP_DEVICE_CURRENT'
 | isn't needed in 'include/gomp-constants.h'.  But probably still a good
 | idea to list it there, in this canonical place, to keep the several lists
 | of device types coherent.
>
 I still wonder about that...  ;-)
>
>> I still think that 'GOMP_DEVICE_CURENT' should get value '-1' (and
>> probably be rename 'GOACC_DEVICE_CURRENT' to make more obvious that it's
>> not related to the 'GOMP_DEVICE_*' ones), but we shall have a look at
>> that later (before GCC 10 release); that's libgomp/OpenACC-internal,
>> doesn't affect anything else.
>
> That's still pending.  Recently,
>  "Missing definition
> for acc_device_current" got filed; let's (also/first) watch/wait what
> comes out of that.

(That's still pending, but) notwithstanding the specific value we'll use
eventually, the 'acc_device_current' interface should work already now.

..., but I noticed that we don't have any test cases for that (so by that
definition, it must be broken).  The curious guy that I am sometimes ;-)
I gave that a try, and... "of course"... it doesn't work.  Please review
the attached (Tobias the Fortran test cases, please), and test with AMD
GCN offloading.  If approving this patch, please respond with
"Reviewed-by: NAME " so that your effort will be recorded in the
commit log, see .


Grüße
 Thomas


From 5ce3725cf160f086e99c01e73c26a0bf5654f5b6 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Wed, 29 Jan 2020 22:11:15 +0100
Subject: [PATCH] Make OpenACC 'acc_get_property' with 'acc_device_current'
 work

	libgomp/
	* oacc-init.c (acc_get_property, acc_get_property_string): Allow
	'acc_device_current'.
	* openacc.f90 (module openacc): Export 'acc_device_current'.
	* testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c
	(expect_device_memory): Rename to...
	(expect_device_memory_properties): ... this.  Make 'static'.
	(expect_device_string_properties): Rename to...
	(expect_device_non_memory_properties): ... this.  Adjust all
	users.
	* testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.h: New
	file.
	* testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c: Use it.
	* testsuite/libgomp.oacc-c-c++-common/acc_get_property-gcn.c:
	Likewise.
	* testsuite/libgomp.oacc-c-c++-common/acc_get_property-host.c:
	Likewise.
	* testsuite/libgomp.oacc-c-c++-common/acc_get_property-nvptx.c:
	Likewise.
	* testsuite/libgomp.oacc-c-c++-common/acc_get_property-gcn.c: Add
	some more testing.
	* testsuite/libgomp.oacc-c-c++-common/acc_get_property-host.c:
	Likewise.
	* testsuite/libgomp.oacc-c-c++-common/acc_get_property-nvptx.c:
	Likewise.
	* testsuite/libgomp.oacc-c-c++-common/acc_get_property.c:
	Likewise.
	* testsuite/libgomp.oacc-fortran/acc_get_property.f90: Likewise.
	* testsuite/libgomp.oacc-fortran/acc_get_property-aux.f90: New
	file.
	* testsuite/libgomp.oacc-fortran/acc_get_property-host.F90: New
	file.
---
 libgomp/oacc-init.c   |   8 +-
 libgomp/openacc.f90   |   1 +
 .../acc_get_property-aux.c|  76 ++---
 .../acc_get_property-aux.h|  14 +++
 .../acc_get_property-gcn.c|  26 +++--
 .../acc_get_property-host.c   |  26 +++--
 .../acc_get_property-nvptx.c  |  27 +++--
 .../acc_get_property.c|  16 ++-
 .../acc_get_property-aux.f90  | 102 ++
 .../acc_get_property-host.F90 |  31 ++
 .../libgomp.oacc-fortran/acc_get_property.f90 |  15 ++-
 11 files changed, 274 insertions(+), 68 deletions(-)
 create mode 100644 libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.h
 create mode 100644 libgomp/testsuite/libgomp.oacc-fortran/acc_get_property-aux.f90
 create mode 100644 libgomp/testsuite/libgomp.oacc-fortran/acc_get_property-host.F90

diff --git a/libgomp/oacc-init.c b/libgomp/oacc-init.c
index ef12b4c16d01..c28c0f689ba2 100644
--- a/libgomp/oacc-init.c
+++ b/libgomp/oacc-init.c
@@ -796,7 +796,9 @@ get_property_any (int ord, acc_device_t d, acc_device_property_t prop)
 size_t
 acc_get_property (int ord, acc_device_t d, acc_device_property_t prop)
 {
-  if (!known_device_type_p (d))
+  if (d == acc_device_current)
+; /* Allowed only for 'acc_get_property',