Hi Thomas,
> On 07.10.19 20:41, Thomas Schwinge wrote:
> > On 2018-12-03T16:51:14+0000, "Maciej W. Rozycki" <ma...@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
