From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 46375 invoked by alias); 5 Nov 2019 15:09:00 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 46366 invoked by uid 89); 5 Nov 2019 15:09:00 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-12.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.1 spammy=nouveau, Device, applicable, HX-Languages-Length:5696 X-HELO: esa2.mentor.iphmx.com Received: from esa2.mentor.iphmx.com (HELO esa2.mentor.iphmx.com) (68.232.141.98) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 05 Nov 2019 15:08:58 +0000 IronPort-SDR: bDXOI8n5+PCPRrsYmqED5ggCo7YAcbKEYupWhoM16BTne0JZe+kykNa3jARgYMthwBFnsUYDgX AYo1Sm92JurhXWOi/qB9uk6J5ixSEKa9wkYp6sXy2qtCSqppaWeOr35LyxOOgocxEkesFkyiqo z6MlgY+/CtuKCWHUAd4el0V4qTNIsl+Qg/eOoJ4juAJfFNoLUfSvo1BEQRiw6cLLt4cc1KTC+g qLYZ+65buva1pztvbN8DLg9zWkUV08MKPeq+18yJEMbQcDIw7onzx5qUEmk3nu8FmaeGUrv4w+ CI8= Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa2.mentor.iphmx.com with ESMTP; 05 Nov 2019 07:08:56 -0800 IronPort-SDR: vIjVaQ8EmrcpSVIm0Uyfybjhfwh3cvpRKGJ5qbObPocvuAjZkYWps113NbrtTh5ShrzJ/29yFw HedG8tjKbM3IdjalGKZVptdnuArVM4fS2GYo6ULwMEygSHI7n9cmttmZb9mgdFxuWT98hDLls1 qu8kS977aansKFLHpnyaXxEtts0n6vFbC0U3qplZXoscmlS9ZE5dzxfHqQleG6OJl6Vv7mY5+R cE3S2u0tM1PZuBVKUmwywMj7p0CxRFBNkIa3KWqTUDvtxN6Vqe4fnpBAMTkBJwdfJqZ5Qm2ZwP kPA= Subject: Re: Add OpenACC 2.6 `acc_get_property' support To: Thomas Schwinge , Jakub Jelinek CC: References: <87imp01jr3.fsf@euler.schwinge.homeip.net> From: "Harwath, Frederik" X-Pep-Version: 2.0 Message-ID: <42917472-9d50-a93b-c804-e6086792eb62@codesourcery.com> Date: Tue, 05 Nov 2019 15:09:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.1 MIME-Version: 1.0 In-Reply-To: <87imp01jr3.fsf@euler.schwinge.homeip.net> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Return-Path: frederik@codesourcery.com X-IsSubscribed: yes X-SW-Source: 2019-11/txt/msg00304.txt.bz2 Hi Thomas, > On 07.10.19 20:41, Thomas Schwinge wrote: > > On 2018-12-03T16:51:14+0000, "Maciej W. Rozycki" 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 =3D { .val =3D 0 }; > > + > > + pthread_mutex_lock (&ptx_dev_lock); > > + > > + if (!nvptx_init () || n >=3D 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 (oac= c_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 =3D 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 =3D 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/deviceQuer= y/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 stri= ng > > 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 > > +#include > > +#include > > +#include > > + > > +int main () > > +{ > > + const char *s; > > + size_t v; > > + int r; > > + > > + /* Verify that the vendor is a proper non-empty string. */ > > + s =3D acc_get_property_string (0, acc_device_default, acc_property_v= endor); > > + r =3D !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