Add OpenACC 2.6 `acc_get_property' support
Thomas Schwinge
thomas@codesourcery.com
Mon Oct 7 18:41:00 GMT 2019
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-OpenACC-2.6-acc_get_property-support.patch
Type: text/x-diff
Size: 30338 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20191007/b35f20dc/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-OpenACC-2.6-acc_get_property-support-restore-Int.patch
Type: text/x-diff
Size: 2392 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20191007/b35f20dc/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 658 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20191007/b35f20dc/attachment.sig>
More information about the Gcc-patches
mailing list