diff mbox series

Add OpenACC 2.6 `acc_get_property' support

Message ID 87imp01jr3.fsf@euler.schwinge.homeip.net
State New
Headers show
Series Add OpenACC 2.6 `acc_get_property' support | expand

Commit Message

Thomas Schwinge Oct. 7, 2019, 6:41 p.m. UTC 
Hi Jakub!

On 2018-12-03T16:51:14+0000, "Maciej W. Rozycki" <macro@codesourcery.com> wrote:
> Add generic support for the OpenACC 2.6 `acc_get_property' and 
> `acc_get_property_string' routines [...]

..., which allow for user code to query the implementation for stuff like:

> OpenACC vendor: GNU
> OpenACC name: GOMP
> OpenACC driver: 1.0
>
> with the host driver and output like:
>
> OpenACC vendor: Nvidia
> OpenACC total memory: 12651462656
> OpenACC free memory: 12202737664
> OpenACC name: TITAN V
> OpenACC driver: 9.1
>
> with the NVPTX driver.

(With 's%OpenACC%Device%', as I understand this; see my comment below.)

Before Frederik starts working on integrating this into GCC trunk, do you
(Jakub) agree with the libgomp plugin interface changes as implemented by
Maciej?  For example, top-level 'GOMP_OFFLOAD_get_property' function in
'struct gomp_device_descr' instead of stuffing this into its
'acc_dispatch_t openacc'.  (I never understood why the OpenACC functions
need to be segregated like they are.)

For reference I'm attaching the latest version of the patch, that is the
commit from openacc-gcc-9-branch, plus a small fix-up.


And, some first remarks (or, "thinking aloud", not a conclusive review)
while I have a look at this myself:

> --- a/include/gomp-constants.h
> +++ b/include/gomp-constants.h
> @@ -215,10 +215,24 @@ enum gomp_map_kind
>  #define GOMP_DEVICE_NVIDIA_PTX		5
>  #define GOMP_DEVICE_INTEL_MIC		6
>  #define GOMP_DEVICE_HSA			7
> +#define GOMP_DEVICE_CURRENT		8

This is used for 'acc_device_current', relevant only for
'acc_get_property', to return "the value of the property for the current
device".  This should probably use a more special (negative?) value
instead of eight, so that when additional real device types are added
later on, we can just add them with increasing numbers, and keep the
scanning code simple.

(Use of 'acc_device_current' as an argument to other functions taking an
'acc_device_t' is undefined, and should be rejected with 'gomp_fatal'?)

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 have not checked how 'acc_device_current' is actually implemented in
the following.)

> +/* Device property codes.  Keep in sync with
> +   libgomp/{openacc.h,openacc.f90,openacc_lib.h}:acc_device_property_t
> +   as well as libgomp/libgomp-plugin.h.  */

Same thing, libgomp-internal, not sure whether to list these here?

> +/* Start from 1 to catch uninitialized use.  */

Hmm, not sure about that either.  Don't think we're generally doing that?

(But I see PGI have 'acc_property_none = 0', oh well.)

> +#define GOMP_DEVICE_PROPERTY_MEMORY		1
> +#define GOMP_DEVICE_PROPERTY_FREE_MEMORY	2
> +#define GOMP_DEVICE_PROPERTY_NAME		0x10001
> +#define GOMP_DEVICE_PROPERTY_VENDOR		0x10002
> +#define GOMP_DEVICE_PROPERTY_DRIVER		0x10003
> +
> +/* Internal property mask to tell numeric and string values apart.  */
> +#define GOMP_DEVICE_PROPERTY_STRING_MASK	0x10000


> --- a/libgomp/plugin/plugin-nvptx.c
> +++ b/libgomp/plugin/plugin-nvptx.c

> +union gomp_device_property_value
> +GOMP_OFFLOAD_get_property (int n, int prop)
> +{
> +  union gomp_device_property_value propval = { .val = 0 };
> +
> +  pthread_mutex_lock (&ptx_dev_lock);
> +
> +  if (!nvptx_init () || n >= nvptx_get_num_devices ())
> +    {
> +      pthread_mutex_unlock (&ptx_dev_lock);
> +      return propval;
> +    }

Isn't it implicit that 'get_num_devices' has been called while loading
the plugin, so we don't have to do any initialization that here?  (But I
may be misremembering that.)

> +  switch (prop)
> +    {
> +    case GOMP_DEVICE_PROPERTY_MEMORY:
> +      {
> +	size_t total_mem;
> +	CUdevice dev;
> +
> +	CUDA_CALL_ERET (propval, cuDeviceGet, &dev, n);

Isn't that already known as 'ptx_devices[n]'?  (Likewise elsewhere.)

> +	CUDA_CALL_ERET (propval, cuDeviceTotalMem, &total_mem, dev);
> +	propval.val = total_mem;
> +      }
> +      break;

> +    case GOMP_DEVICE_PROPERTY_NAME:
> +      {
> +	static char name[256];
> +	CUdevice dev;
> +
> +	CUDA_CALL_ERET (propval, cuDeviceGet, &dev, n);
> +	CUDA_CALL_ERET (propval, cuDeviceGetName, name, sizeof (name), dev);
> +	propval.ptr = name;
> +      }
> +      break;

Uh, that's not thread-safe, is it?

From a quick look at OpenACC 2.7, that doesn't specify what exactly to
return here (that is, which "class" of memory; who's the "owner" of the
memory object).  (So, returning a 'malloc'ed object would be a memory
leak, for example.)  Best would probably be to return some 'const char *'
living in read-only program memory.  (But that likely is not available,
otherwise I suppose Maciej would have done that.)

Otherwise, perhaps make this 'name' a property of 'struct ptx_device' in
the nvptx plugin here, and keep it live while the device is open
('nvptx_open_device'), together with other per-device data?

> +    case GOMP_DEVICE_PROPERTY_DRIVER:
> +      {
> +	static char ver[11];
> +	int v;
> +
> +	CUDA_CALL_ERET (propval, cuDriverGetVersion, &v);
> +	snprintf (ver, sizeof (ver) - 1, "%u.%u", v / 1000, v % 1000 / 10);
> +	propval.ptr = ver;
> +      }
> +      break;

Similar here.

Is that 'snprintf' formatting the generic way to display a CUDA driver
version number?  

As, in theory, such Nvidia GPU offloading support could also be
implemented via the Nouveau/Mesa GalliumCompute driver, should the string
returned here actually include "CUDA Driver"?

> +    default:
> +      break;

Should this 'GOMP_PLUGIN_error' or even 'GOMP_PLUGIN_fatal'?  (Similar
then elsewhere.)


> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property.c
> @@ -0,0 +1,37 @@
> +/* Test the `acc_get_property' and '`acc_get_property_string' library
> +   functions. */
> +/* { dg-do run } */
> +
> +#include <openacc.h>
> +#include <stddef.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +int main ()
> +{
> +  const char *s;
> +  size_t v;
> +  int r;
> +
> +  /* Verify that the vendor is a proper non-empty string.  */
> +  s = acc_get_property_string (0, acc_device_default, acc_property_vendor);
> +  r = !s || !strlen (s);
> +  if (s)
> +    printf ("OpenACC vendor: %s\n", s);

Should we check the actual string returned, as defined by OpenACC/our
implementation, as applicable?  Use '#if defined ACC_DEVICE_TYPE_[...]'.
(See 'libgomp/testsuite/libgomp.oacc-c-c++-common/avoid-offloading-2.c',
for example.)

Isn't this the "Device vendor" instead of the "OpenACC vendor"?  Similar
for all other 'printf's?

> +
> +  /* For the rest just check that they do not crash.  */
> +  v = acc_get_property (0, acc_device_default, acc_property_memory);
> +  if (v)
> +    printf ("OpenACC total memory: %zd\n", v);
> +  v = acc_get_property (0, acc_device_default, acc_property_free_memory);
> +  if (v)
> +    printf ("OpenACC free memory: %zd\n", v);
> +  s = acc_get_property_string (0, acc_device_default, acc_property_name);
> +  if (s)
> +    printf ("OpenACC name: %s\n", s);
> +  s = acc_get_property_string (0, acc_device_default, acc_property_driver);
> +  if (s)
> +    printf ("OpenACC driver: %s\n", s);
> +
> +  return r;
> +}

Likewise, as we define the implementation, we can declare that something
reasonable must have been returned, so don't need 'if (s)' checks, but
should instead verify 's' to the extent possible.

> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.oacc-fortran/acc-get-property.f

Similar.  (See
'libgomp/testsuite/libgomp.oacc-fortran/avoid-offloading-2.f', for
example.)

These tests only use 'acc_device_default', should they also check other
valid as well as invalid values?


Grüße
 Thomas

Comments

Frederik Harwath Nov. 5, 2019, 3:08 p.m. UTC | #1 
Hi Thomas,

> On 07.10.19 20:41, Thomas Schwinge wrote:
> > On 2018-12-03T16:51:14+0000, "Maciej W. Rozycki" <macro@codesourcery.com> wrote:
> > Add generic support for the OpenACC 2.6 `acc_get_property' and
> > `acc_get_property_string' routines [...]
>
> ..., which allow for user code to query the implementation for stuff like:
>
> > OpenACC vendor: GNU
> > OpenACC name: GOMP
> > OpenACC driver: 1.0
>
> [...]
>
> > --- a/include/gomp-constants.h
> > +++ b/include/gomp-constants.h
> > @@ -215,10 +215,24 @@ enum gomp_map_kind
> >  #define GOMP_DEVICE_NVIDIA_PTX		5
> >  #define GOMP_DEVICE_INTEL_MIC		6
> >  #define GOMP_DEVICE_HSA			7
> > +#define GOMP_DEVICE_CURRENT		8
>
> This is used for 'acc_device_current', relevant only for
> 'acc_get_property', to return "the value of the property for the current
> device".  This should probably use a more special (negative?) value
> instead of eight, so that when additional real device types are added
> later on, we can just add them with increasing numbers, and keep the
> scanning code simple.

Yes, I use the first unused negative value.

> (Use of 'acc_device_current' as an argument to other functions taking an
> 'acc_device_t' is undefined, and should be rejected with 'gomp_fatal'?)

So far, there seems to be essentially no validity checking for acc_device_t
and other enums in the relevant parts of the code. I have added such checks
to public functions which take acc_device_t arguments.

> > --- a/libgomp/plugin/plugin-nvptx.c
> > +++ b/libgomp/plugin/plugin-nvptx.c
>
> > +union gomp_device_property_value
> > +GOMP_OFFLOAD_get_property (int n, int prop)
> > +{
> > +  union gomp_device_property_value propval = { .val = 0 };
> > +
> > +  pthread_mutex_lock (&ptx_dev_lock);
> > +
> > +  if (!nvptx_init () || n >= nvptx_get_num_devices ())
> > +    {
> > +      pthread_mutex_unlock (&ptx_dev_lock);
> > +      return propval;
> > +    }
>
> Isn't it implicit that 'get_num_devices' has been called while loading
> the plugin, so we don't have to do any initialization that here?  (But I
> may be misremembering that.)

Yes, a call path for nvptx_get_num_devices during initialization, in case we
are using the nvptx plugin:
acc_init (oacc_init.c) -> gomp_init_targets_once (target.c) ->
gomp_target_init (target.c) -> GOMP_OFFLOAD_get_num_devices (plugin-nvptx.c) ->
nvptx_get_num_devices

For nvptx_init, a call path is:
acc_init (oacc_init.c) -> acc_init_1 (oacc_init.c) -> gomp_init_device (oacc_init.c) ->
GOMP_OFFLOAD_init_device (plugin-nvptx.c) -> nvptx_init

Hence, yes, we should not call nvptx_init from here.


> > +  switch (prop)
> > +    {
> > +    case GOMP_DEVICE_PROPERTY_MEMORY:
> > +      {
> > +	size_t total_mem;
> > +	CUdevice dev;
> > +
> > +	CUDA_CALL_ERET (propval, cuDeviceGet, &dev, n);
>
> Isn't that already known as 'ptx_devices[n]'?  (Likewise elsewhere.)

Yes, that gets set during GOMP_OFFLOAD_init_device.

> +	CUDA_CALL_ERET (propval, cuDeviceTotalMem, &total_mem, dev);
> +	propval.val = total_mem;
> +      }
> +      break;

> +    case GOMP_DEVICE_PROPERTY_NAME:
> +      {
> +	static char name[256];
> +	CUdevice dev;
> +
> +	CUDA_CALL_ERET (propval, cuDeviceGet, &dev, n);
> +	CUDA_CALL_ERET (propval, cuDeviceGetName, name, sizeof (name), dev);
> +	propval.ptr = name;
> +      }
> +      break;

> Uh, that's not thread-safe, is it?

Not at all.

> Otherwise, perhaps make this 'name' a property of 'struct ptx_device' in
> the nvptx plugin here, and keep it live while the device is open
> ('nvptx_open_device'), together with other per-device data?

That's what I do now.

> Is that 'snprintf' formatting the generic way to display a CUDA driver
> version number?

At least, the same formatting is applied by NVidia's deviceQuery example
from cuda-samples
(i.e. https://github.com/NVIDIA/cuda-samples/blob/master/Samples/deviceQuery/deviceQuery.cpp#L106).
For me, the output yields "CUDA Driver Version / Runtime Version 9.1 / 9.1" with
the nvidia-cuda-toolkit 9.1.


> > As, in theory, such Nvidia GPU offloading support could also be
> > implemented via the Nouveau/Mesa GalliumCompute driver, should the string
> > returned here actually include "CUDA Driver"?

This seems like a good way to disambiguate between different drivers, but I am not sure if there
are any compatibility issues that we have to consider (PGI?). The standard does not impose
any restrictions on the format of the string.


> > +    default:
> > +      break;
>
> Should this 'GOMP_PLUGIN_error' or even 'GOMP_PLUGIN_fatal'?  (Similar
> then elsewhere.)

Yes, I chose GOMP_PLUGIN_error.

> > --- /dev/null
> > +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property.c
> > @@ -0,0 +1,37 @@
> > +/* Test the `acc_get_property' and '`acc_get_property_string' library
> > +   functions. */
> > +/* { dg-do run } */
> > +
> > +#include <openacc.h>
> > +#include <stddef.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +
> > +int main ()
> > +{
> > +  const char *s;
> > +  size_t v;
> > +  int r;
> > +
> > +  /* Verify that the vendor is a proper non-empty string.  */
> > +  s = acc_get_property_string (0, acc_device_default, acc_property_vendor);
> > +  r = !s || !strlen (s);
> > +  if (s)
> > +    printf ("OpenACC vendor: %s\n", s);
>
> Should we check the actual string returned, as defined by OpenACC/our
> implementation, as applicable?  Use '#if defined ACC_DEVICE_TYPE_[...]'.
> (See 'libgomp/testsuite/libgomp.oacc-c-c++-common/avoid-offloading-2.c',
> for example.)

Yes.

> Isn't this the "Device vendor" instead of the "OpenACC vendor"?  Similar
> for all other 'printf's?

Yes.


> These tests only use 'acc_device_default', should they also check other
> valid as well as invalid values?

That would be better.

Frederik
diff mbox series

Patch

From 1fa609ba73e9990ae7a65b083047f0ee219167b3 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Tue, 8 Jan 2019 15:21:35 +0100
Subject: [PATCH] Add OpenACC 2.6 `acc_get_property' support: restore Intel MIC
 offloading

The "OpenACC 2.6 `acc_get_property' support" changes regressed the relevant
libgomp OpenMP execution test cases to no longer consider Intel MIC offloading
because of:

    libgomp: while loading libgomp-plugin-intelmic.so.1: [...]/libgomp-plugin-intelmic.so.1: undefined symbol: GOMP_OFFLOAD_get_property

	liboffloadmic/
	* plugin/libgomp-plugin-intelmic.cpp (GOMP_OFFLOAD_get_property):
	New function.
---
 liboffloadmic/ChangeLog.openacc               |  5 +++++
 .../plugin/libgomp-plugin-intelmic.cpp        | 21 +++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/liboffloadmic/ChangeLog.openacc b/liboffloadmic/ChangeLog.openacc
index 521d4490f0b..75f8ee518e4 100644
--- a/liboffloadmic/ChangeLog.openacc
+++ b/liboffloadmic/ChangeLog.openacc
@@ -1,3 +1,8 @@ 
+2019-01-08  Thomas Schwinge  <thomas@codesourcery.com>
+
+	* plugin/libgomp-plugin-intelmic.cpp (GOMP_OFFLOAD_get_property):
+	New function.
+
 2019-02-26  Chung-Lin Tang  <cltang@codesourcery.com>
 
 	* plugin/libgomp-plugin-intelmic.cpp (GOMP_OFFLOAD_version):
diff --git a/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp b/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp
index d1678d0514e..ed78c8d9dd4 100644
--- a/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp
+++ b/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp
@@ -174,6 +174,27 @@  GOMP_OFFLOAD_get_num_devices (void)
   return num_devices;
 }
 
+extern "C" union gomp_device_property_value
+GOMP_OFFLOAD_get_property (int n, int prop)
+{
+  union gomp_device_property_value nullval = { .val = 0 };
+
+  if (n >= num_devices)
+    {
+      GOMP_PLUGIN_error
+       ("Request for a property of a non-existing Intel MIC device %i", n);
+      return nullval;
+    }
+
+  switch (prop)
+    {
+    case GOMP_DEVICE_PROPERTY_VENDOR:
+      return (union gomp_device_property_value) { .ptr = /* TODO: "error: invalid conversion from 'const void*' to 'void*' [-fpermissive]" */ (char *) "Intel" };
+    default:
+      return nullval;
+    }
+}
+
 static bool
 offload (const char *file, uint64_t line, int device, const char *name,
 	 int num_vars, VarDesc *vars, const void **async_data)
-- 
2.17.1