| Message ID | 20191114153531.7493-1-frederik@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add OpenACC 2.6 `acc_get_property' support | expand |
Hi Frederik! On 2019-11-14T16:35:31+0100, Frederik Harwath <frederik@codesourcery.com> wrote: > this patch implements OpenACC 2.6 "acc_get_property" and related functions. I'm attaching a version rebased on top of current GCC trunk, as well as an incremental patch; please review these changes, and merge into yours. > There is no AMD GCN support yet. This will be added later on. ACK, just to note that there now is a 'libgomp/plugin/plugin-gcn.c' that at least needs to get a stub implementation (can mostly copy from 'libgomp/plugin/plugin-hsa.c'?) as otherwise the build will fail. Tobias has generally reviewed the Fortran bits, correct? As I mentioned before ("thinking aloud"): | 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.) Jakub didn't answer, but I now myself decided that we should group this with the other OpenACC libgomp-plugin functions, as this interface is defined in terms of OpenACC-specific stuff such as 'acc_device_t'. Frederik, please work on that, also try to move function definitions etc. into appropriate places in case they aren't; ask if you need help. > Add generic support for the OpenACC 2.6 `acc_get_property' and > `acc_get_property_string' routines, as well as full handlers for the > host and the NVPTX offload targets and minimal handlers for the HSA > and Intel MIC offload targets. > > Included are C/C++ and Fortran tests that, in particular, print > the property values for acc_property_vendor, acc_property_memory, > acc_property_free_memory, acc_property_name, and acc_property_driver. > The output looks as follows: > > Vendor: GNU > Name: GOMP > Total memory: 0 > Free memory: 0 > Driver: 1.0 > > with the host driver (where the memory related properties are not > supported for the host device and yield 0, conforming to the standard) > and output like: > > OpenACC vendor: Nvidia > OpenACC total memory: 12651462656 > OpenACC free memory: 12202737664 > OpenACC name: TITAN V > OpenACC driver: CUDA Driver 9.1 > > with the NVPTX driver. That needs to be updated. > .../acc-get-property-2.c | 68 +++++++++ > .../acc-get-property-3.c | 19 +++ > .../acc-get-property-aux.c | 60 ++++++++ > .../acc-get-property.c | 75 ++++++++++ > .../libgomp.oacc-fortran/acc-get-property.f90 | 80 ++++++++++ Please name all these 'acc_get_property*', which is the name of the interface tested. > --- a/include/gomp-constants.h > +++ b/include/gomp-constants.h > @@ -178,6 +178,20 @@ enum gomp_map_kind > > #define GOMP_DEVICE_ICV -1 > #define GOMP_DEVICE_HOST_FALLBACK -2 > +#define GOMP_DEVICE_CURRENT -3 As I mentioned before ("thinking aloud"): | 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 [now has a] special (negative?) value | [...], 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. Not should if this should be grouped with 'GOMP_DEVICE_ICV', 'GOMP_DEVICE_HOST_FALLBACK', for it is not related to there. 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.) | (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'?) That should now be implemented; r278937 "Validate acc_device_t uses". | 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 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 | Same thing, libgomp-internal, not sure whether to list these here? > + as well as libgomp/libgomp-plugin.h. */ (Not sure why 'libgomp/libgomp-plugin.h' is relevant 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 (Maybe should use an 'enum'?) Maybe this stuff should move from 'include/gomp-constants.h' to 'libgomp/oacc-int.h'. I'll think about that again, when I'm awake again tomorrow. ;-) > --- a/libgomp/libgomp-plugin.h > +++ b/libgomp/libgomp-plugin.h > @@ -54,6 +54,13 @@ enum offload_target_type > OFFLOAD_TARGET_TYPE_GCN = 8 > }; > > +/* Container type for passing device properties. */ > +union gomp_device_property_value > +{ > + void *ptr; > + uintmax_t val; > +}; Why wouldn't that be 'size_t', 'const char *', as the actual data types used? (Maybe I'm missing something.) > --- a/libgomp/libgomp.map > +++ b/libgomp/libgomp.map > @@ -502,6 +502,14 @@ GOACC_2.0.1 { > GOACC_parallel_keyed; > } GOACC_2.0; > > +OACC_2.6 { > + global: > + acc_get_property; > + acc_get_property_h_; > + acc_get_property_string; > + acc_get_property_string_h_; > +} OACC_2.5; > + > GOMP_PLUGIN_1.0 { > global: > GOMP_PLUGIN_malloc; That's not correct: 'OACC_2.6' should come after 'OACC_2.5.1', and also inherit from that one. > --- a/libgomp/oacc-init.c > +++ b/libgomp/oacc-init.c > +static union gomp_device_property_value > +get_property_any (int ord, acc_device_t d, acc_device_property_t prop) > +{ > + if (!acc_known_device_type (d)) > + unknown_device_type_error(d); Checking isn't needed here for this is an internal interface? > + > + union gomp_device_property_value propval; > + struct gomp_device_descr *dev; > + struct goacc_thread *thr; Generally, in new code, we try to place these next to their first use. > + > + if (d == acc_device_none) > + return (union gomp_device_property_value) { .val = 0 }; > + > + goacc_lazy_initialize (); > + thr = goacc_thread (); > + > + if (d == acc_device_current && (!thr || !thr->dev)) > + return (union gomp_device_property_value) { .val = 0 }; Should we use a 'nullval' here, as used elsewhere? Also, this checking seems a bit convoluted; shouldn't this be integrated into the following? It's certainly not necessary to special-case 'acc_device_none' before 'goacc_lazy_initialize' etc.? > + > + if (d == acc_device_current) > + { > + dev = thr->dev; > + } > + else > + { > + int num_devices; > + > + gomp_mutex_lock (&acc_device_lock); > + > + dev = resolve_device (d, false); Why call this without 'fail_is_error' flag here? > + > + num_devices = dev->get_num_devices_func (); > + > + if (num_devices <= 0 || ord >= num_devices) > + acc_dev_num_out_of_range (d, ord, num_devices); > + > + dev += ord; > + > + gomp_mutex_lock (&dev->lock); > + if (dev->state == GOMP_DEVICE_UNINITIALIZED) > + gomp_init_device (dev); > + gomp_mutex_unlock (&dev->lock); > + > + gomp_mutex_unlock (&acc_device_lock); > + } > + > + assert (dev); > + > + propval = dev->get_property_func (dev->target_id, prop); > + > + return propval; > +} I'll have to look at this again, tomorrow. > --- a/libgomp/openacc.f90 > +++ b/libgomp/openacc.f90 > @@ -28,7 +28,7 @@ > ! <http://www.gnu.org/licenses/>. > > module openacc_kinds > - use iso_fortran_env, only: int32 > + use iso_fortran_env, only: int32, int64 > implicit none > > private :: int32 > @@ -47,6 +47,21 @@ module openacc_kinds > integer (acc_device_kind), parameter :: acc_device_not_host = 4 > integer (acc_device_kind), parameter :: acc_device_nvidia = 5 > integer (acc_device_kind), parameter :: acc_device_gcn = 8 > + integer (acc_device_kind), parameter :: acc_device_current = -3 > + > + public :: acc_device_property > + > + integer, parameter :: acc_device_property = int64 Why 'int64'? I changed this to 'int32', but please tell if there's a reason for 'int64'. > + > + public :: acc_property_memory, acc_property_free_memory > + public :: acc_property_name, acc_property_vendor, acc_property_driver > + > + ! Keep in sync with include/gomp-constants.h. > + integer (acc_device_property), parameter :: acc_property_memory = 1 > + integer (acc_device_property), parameter :: acc_property_free_memory = 2 > + integer (acc_device_property), parameter :: acc_property_name = int(Z'10001') > + integer (acc_device_property), parameter :: acc_property_vendor = int(Z'10002') > + integer (acc_device_property), parameter :: acc_property_driver = int(Z'10003') > > public :: acc_handle_kind > Per Tobias' recent "libgomp/openacc.f90 – clean-up public/private attributes" changes, some of this needs to move/change (done, I think). > @@ -87,6 +102,24 @@ module openacc_internal > integer (acc_device_kind) d > end subroutine > > + function acc_get_property_h (n, d, p) > + import > + implicit none (type, external) > + integer (acc_device_property) :: acc_get_property_h > + integer, value :: n > + integer (acc_device_kind), value :: d > + integer (acc_device_property), value :: p > + end function > + > + subroutine acc_get_property_string_h (n, d, p, s) > + import > + implicit none (type, external) > + integer, value :: n > + integer (acc_device_kind), value :: d > + integer (acc_device_property), value :: p > + character (*) :: s > + end subroutine > + > function acc_get_device_num_h (d) > import > integer acc_get_device_num_h No reason to place that between 'acc_set_device_num_h' and 'acc_get_device_num_h'? I moved it after the latter, like done elsewhere. Is it a conscious decision that we're not supporting the new 'acc_get_property' interface via 'openacc_lib.h', which is (or, used to be) an alternative to the Fortran 'openacc' module? As of OpenACC 2.5, 'openacc_lib.h' has been deprecated ("no longer supported"), but so far, we continued to support it, and it's (maybe?) strange when that one now works for everything but the 'acc_get_property' interface? Or, is that a statement that users really should move to the Fortran 'openacc' module? > --- a/libgomp/openacc.h > +++ b/libgomp/openacc.h > @@ -59,9 +59,20 @@ typedef enum acc_device_t { > _ACC_device_hwm, > /* Ensure enumeration is layout compatible with int. */ > _ACC_highest = __INT_MAX__, > - _ACC_neg = -1 > + _ACC_neg = -1, > + acc_device_current = -3 > } acc_device_t; (Update per final 'GOMP_DEVICE_CURRENT' value. If settling on '-1', 'acc_device_current' may *replace* the placeholder '_ACC_neg'.) > +typedef enum acc_device_property_t { > + /* Keep in sync with include/gomp-constants.h. */ > + /* Start from 1 to catch uninitialized use. */ > + acc_property_memory = 1, > + acc_property_free_memory = 2, > + acc_property_name = 0x10001, > + acc_property_vendor = 0x10002, > + acc_property_driver = 0x10003 > +} acc_device_property_t; Do we also need the magic here so that "Ensure enumeration is layout compatible with int"? But I see that is not done for the 'typedef enum acc_async_t' either. I don't remember the history behind that. > --- a/libgomp/plugin/plugin-hsa.c > +++ b/libgomp/plugin/plugin-hsa.c > @@ -699,6 +699,32 @@ GOMP_OFFLOAD_get_num_devices (void) > return hsa_context.agent_count; > } > > +/* Part of the libgomp plugin interface. Return the value of property > + PROP of agent number N. */ > + > +union gomp_device_property_value > +GOMP_OFFLOAD_get_property (int n, int prop) > +{ > + union gomp_device_property_value nullval = { .val = 0 }; > + > + if (!init_hsa_context ()) > + return nullval; I'm not familiar with that code, but similar to other plugins, 'init_hsa_context' already is called via 'GOMP_OFFLOAD_get_num_devices' (and 'GOMP_OFFLOAD_init_device', hmm...), so probably don't need to call it here? > + if (n >= hsa_context.agent_count) > + { > + GOMP_PLUGIN_error > + ("Request for a property of a non-existing HSA device %i", n); > + return nullval; > + } (Probably this would eventually call 'get_agent_info' to resolve 'n', which then already does the checking done here. No need to look into that right now.) > + > + switch (prop) > + { > + case GOMP_DEVICE_PROPERTY_VENDOR: > + return (union gomp_device_property_value) { .ptr = "AMD" }; > + default: > + return nullval; > + } > +} Not sure if "AMD" is actually correct here -- isn't HSA a vendor-independent standard? > --- a/libgomp/plugin/plugin-nvptx.c > +++ b/libgomp/plugin/plugin-nvptx.c > @@ -304,6 +305,7 @@ struct ptx_device > }; > > static struct ptx_device **ptx_devices; > +static char cuda_driver_version[30]; > > static inline struct nvptx_thread * > nvptx_thread (void) > @@ -330,6 +332,12 @@ nvptx_init (void) > CUDA_CALL (cuDeviceGetCount, &ndevs); > ptx_devices = GOMP_PLUGIN_malloc_cleared (sizeof (struct ptx_device *) > * ndevs); > + > + int v; > + CUDA_CALL_ERET (NULL, cuDriverGetVersion, &v); > + snprintf (cuda_driver_version, sizeof (cuda_driver_version) - 1, > + "CUDA Driver %u.%u", v / 1000, v % 1000 / 10); > + > return true; > } I moved these further up; this is relevant at the top-level, global in this plugin. Why the '- 1' in 'sizeof (cuda_driver_version) - 1'? (I removed that.) > @@ -293,6 +293,7 @@ struct ptx_device > int max_threads_per_block; > int max_threads_per_multiprocessor; > int default_dims[GOMP_DIM_MAX]; > + char* name; > > struct ptx_image_data *images; /* Images loaded on device. */ > pthread_mutex_t image_lock; /* Lock for above list. */ The number of 'struct ptx_device' instances will always be low, so let's just embed 'name' into this (done), so that we can avoid the dynamic allocation: > @@ -491,6 +499,10 @@ nvptx_open_device (int n) > for (int i = 0; i != GOMP_DIM_MAX; i++) > ptx_dev->default_dims[i] = 0; > > + const int max_name_len = 256; > + ptx_dev->name = GOMP_PLUGIN_malloc(max_name_len); > + CUDA_CALL_ERET (NULL, cuDeviceGetName, ptx_dev->name, max_name_len, dev); > + > ptx_dev->images = NULL; > pthread_mutex_init (&ptx_dev->image_lock, NULL); > > @@ -520,6 +532,7 @@ nvptx_close_device (struct ptx_device *ptx_dev) > if (!ptx_dev->ctx_shared) > CUDA_CALL (cuCtxDestroy, ptx_dev->ctx); > > + free (ptx_dev->name); > free (ptx_dev); > return true; > } As 'acc_get_property' is not an interface commonly used, as an alternative to "embed", we could also again make this a pointer, but "allocated, initialized upon first use", but I'm not convinced it's worth the effort. > +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); Everything (?) else seems to be accessing 'ptx_devices' without locking? (I don't quickly understand the locking protocol used there... Will look again tomorrow.) > + > + if (n >= nvptx_get_num_devices () || n < 0 || ptx_devices[n] == NULL) > + { > + pthread_mutex_unlock (&ptx_dev_lock); > + return propval; > + } > + > + struct ptx_device *ptx_dev = ptx_devices[n]; > + switch (prop) > + { > + case GOMP_DEVICE_PROPERTY_MEMORY: > + { > + size_t total_mem; > + > + CUDA_CALL_ERET (propval, cuDeviceTotalMem, &total_mem, ptx_dev->dev); > + propval.val = total_mem; > + } > + break; > + case GOMP_DEVICE_PROPERTY_FREE_MEMORY: > + { > + size_t total_mem; > + size_t free_mem; > + CUdevice ctxdev; > + > + CUDA_CALL_ERET (propval, cuCtxGetDevice, &ctxdev); > + if (ptx_dev->dev == ctxdev) > + CUDA_CALL_ERET (propval, cuMemGetInfo, &free_mem, &total_mem); > + else if (ptx_dev->ctx) > + { > + CUcontext old_ctx; > + > + CUDA_CALL_ERET (propval, cuCtxPushCurrent, ptx_dev->ctx); > + CUDA_CALL_ERET (propval, cuMemGetInfo, &free_mem, &total_mem); > + CUDA_CALL_ASSERT (cuCtxPopCurrent, &old_ctx); > + } > + else > + { > + CUcontext new_ctx; > + > + CUDA_CALL_ERET (propval, cuCtxCreate, &new_ctx, CU_CTX_SCHED_AUTO, > + ptx_dev->dev); > + CUDA_CALL_ERET (propval, cuMemGetInfo, &free_mem, &total_mem); > + CUDA_CALL_ASSERT (cuCtxDestroy, new_ctx); > + } > + propval.val = free_mem; > + } > + break; (Have not yet reviewed that CUDA magic. Do you understand that?) > + case GOMP_DEVICE_PROPERTY_NAME: > + propval.ptr = ptx_dev->name; > + break; > + case GOMP_DEVICE_PROPERTY_VENDOR: > + propval.ptr = "Nvidia"; > + break; > + case GOMP_DEVICE_PROPERTY_DRIVER: > + propval.ptr = cuda_driver_version; > + break; > + default: > + GOMP_PLUGIN_error("Unknown OpenACC device-property"); > + } I remember I asked: "Should this 'GOMP_PLUGIN_error' or even 'GOMP_PLUGIN_fatal'? (Similar then elsewhere.)" You only changed that here, and I now see that OpenACC 2.6, 3.2.6. "acc_get_property" actually states that "If the value of 'property' is not one of the known values for that query routine, or that property has no value for the specified device, 'acc_get_property' will return 0 and 'acc_get_property_string' will return NULL (in C or C++) or an blank string (in Fortran)". So this means this should actually not call 'GOMP_PLUGIN_error' but instead return zero etc.? Please also make sure that we have testsuite coverage for all the different cases possible. (Especially different Fortran interfaces.) I see 'libgomp/oacc-host.c:host_get_property', 'libgomp/plugin/plugin-hsa.c:GOMP_OFFLOAD_get_property', 'liboffloadmic/plugin/libgomp-plugin-intelmic.cpp:GOMP_OFFLOAD_get_property' do have a 'default: return nullval'; that's probably what we need to do here, too? > + > + pthread_mutex_unlock (&ptx_dev_lock); > + return propval; > +} > --- /dev/null > +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property-2.c FAIL: libgomp.oacc-c++/../libgomp.oacc-c-c++-common/acc-get-property-2.c -DACC_DEVICE_TYPE_nvidia=1 -DACC_MEM_SHARED=0 -foffload=nvptx-none -O0 (test for excess errors) UNRESOLVED: libgomp.oacc-c++/../libgomp.oacc-c-c++-common/acc-get-property-2.c -DACC_DEVICE_TYPE_nvidia=1 -DACC_MEM_SHARED=0 -foffload=nvptx-none -O0 compilation failed to produce executable [...]/libgomp.oacc-c-c++-common/acc-get-property-2.c:61:28: error: invalid conversion from 'void*' to 'char*' [-fpermissive] 61 | char *driver = malloc(sizeof(char) * 40); | ~~~~~~^~~~~~~~~~~~~~~~~~~ | | | void* > + char *driver = malloc(sizeof(char) * 40); > + snprintf (driver, 40, "CUDA Driver %u.%u", driver_version / 1000, > + driver_version % 1000 / 10); Let's just use 'char driver[30]', and 'sizeof driver'? > --- a/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp > +++ b/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp > @@ -174,6 +174,28 @@ 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: > + /* TODO: "error: invalid conversion from 'const void*' to 'void*' [-fpermissive]" */ > + return (union gomp_device_property_value) { .ptr = (char *) "Intel" }; Type cast maybe unnecessary per my 'libgomp/libgomp-plugin.h' comment above? > + default: > + return nullval; > + } > +} Grüße Thomas
Hi, On Tue, Dec 17 2019, Thomas Schwinge wrote: > On 2019-11-14T16:35:31+0100, Frederik Harwath <frederik@codesourcery.com> wrote: >> this patch implements OpenACC 2.6 "acc_get_property" and related functions. > > [...] > >> --- a/libgomp/plugin/plugin-hsa.c >> +++ b/libgomp/plugin/plugin-hsa.c >> @@ -699,6 +699,32 @@ GOMP_OFFLOAD_get_num_devices (void) >> return hsa_context.agent_count; >> } >> >> +/* Part of the libgomp plugin interface. Return the value of property >> + PROP of agent number N. */ >> + >> +union gomp_device_property_value >> +GOMP_OFFLOAD_get_property (int n, int prop) >> +{ >> + union gomp_device_property_value nullval = { .val = 0 }; >> + >> + if (!init_hsa_context ()) >> + return nullval; > > I'm not familiar with that code, but similar to other plugins, > 'init_hsa_context' already is called via 'GOMP_OFFLOAD_get_num_devices' > (and 'GOMP_OFFLOAD_init_device', hmm...), so probably don't need to call > it here? I assume you always want to get the number of devices before querying their properties but OTOH there is no harm in calling the initialization function. >> + >> + switch (prop) >> + { >> + case GOMP_DEVICE_PROPERTY_VENDOR: >> + return (union gomp_device_property_value) { .ptr = "AMD" }; >> + default: >> + return nullval; >> + } >> +} > > Not sure if "AMD" is actually correct here -- isn't HSA a > vendor-independent standard? > Yes, it is supposed to be. I think HSA is the correct "vendor" too, essentially an abbreviation for HSA Foundation. Thanks, Martin
On 16/12/2019 23:00, Thomas Schwinge wrote: >> There is no AMD GCN support yet. This will be added later on. > > ACK, just to note that there now is a 'libgomp/plugin/plugin-gcn.c' that > at least needs to get a stub implementation (can mostly copy from > 'libgomp/plugin/plugin-hsa.c'?) as otherwise the build will fail. The code exists on the OG9 branch. It was omitted from the trunk submission because the other half of the properties support wasn't there yet. Andrew
Hi Thomas, thanks for the review! I have attached a revised patch. > > There is no AMD GCN support yet. This will be added later on. > > ACK, just to note that there now is a 'libgomp/plugin/plugin-gcn.c' that > at least needs to get a stub implementation (can mostly copy from > 'libgomp/plugin/plugin-hsa.c'?) as otherwise the build will fail. Yes, I have added a stub. A full implementation will follow soon. The implementation in the OG9 branch that Andrew mentioned will need a bit of polishing. > Tobias has generally reviewed the Fortran bits, correct? Yes, he has done that internally. > | 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.) > > Jakub didn't answer, but I now myself decided that we should group this > with the other OpenACC libgomp-plugin functions, as this interface is > defined in terms of OpenACC-specific stuff such as 'acc_device_t'. > Frederik, please work on that, also try to move function definitions etc. > into appropriate places in case they aren't; ask if you need help. > That needs to be updated. Is it ok to do this in a separate follow-up patch? > > .../acc-get-property-2.c | 68 +++++++++ > > .../acc-get-property-3.c | 19 +++ > > .../acc-get-property-aux.c | 60 ++++++++ > > .../acc-get-property.c | 75 ++++++++++ > > .../libgomp.oacc-fortran/acc-get-property.f90 | 80 ++++++++++ > > Please name all these 'acc_get_property*', which is the name of the > interface tested. Ok. > > --- a/include/gomp-constants.h > > +++ b/include/gomp-constants.h > > @@ -178,6 +178,20 @@ enum gomp_map_kind > >=20=20 > > #define GOMP_DEVICE_ICV -1 > > #define GOMP_DEVICE_HOST_FALLBACK -2 > > +#define GOMP_DEVICE_CURRENT -3 > [...] > > Not should if this should be grouped with 'GOMP_DEVICE_ICV', > 'GOMP_DEVICE_HOST_FALLBACK', for it is not related to there. > > [...] > > 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. > still wonder about that... ;-) I have removed GOMP_DEVICE_CURRENT from gomp-constants.h. Changing the value of GOMP_DEVICE_ICV violates the following static asserts in oacc-parallel.c: /* In the ABI, the GOACC_FLAGs are encoded as an inverted bitmask, so that we continue to support the following two legacy values. */ _Static_assert (GOACC_FLAGS_UNMARSHAL (GOMP_DEVICE_ICV) == 0, "legacy GOMP_DEVICE_ICV broken"); _Static_assert (GOACC_FLAGS_UNMARSHAL (GOMP_DEVICE_HOST_FALLBACK) == GOACC_FLAG_HOST_FALLBACK, "legacy GOMP_DEVICE_HOST_FALLBACK broken"); > > +/* Device property codes. Keep in sync with > > + libgomp/{openacc.h,openacc.f90,openacc_lib.h}:acc_device_property_t > > | Same thing, libgomp-internal, not sure whether to list these here? > > > + as well as libgomp/libgomp-plugin.h. */ > > (Not sure why 'libgomp/libgomp-plugin.h' is relevant here?) It does not seem to be relevant. Right now, openacc_lib.h is also not relevant. I have removed both file names from the comment. > > +#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 > > (Maybe should use an 'enum'?) I have changed this to an enum. However, this does not improve the code much, since we cannot use the enum for the function arguments in the plugins because gomp-constants.h is not included from there. > Maybe this stuff should move from 'include/gomp-constants.h' to > 'libgomp/oacc-int.h'. I'll think about that again, when I'm awake again > tomorrow. ;-) Have you made up your mind yet? :-) > > --- a/libgomp/libgomp-plugin.h > > +++ b/libgomp/libgomp-plugin.h > > @@ -54,6 +54,13 @@ enum offload_target_type > > OFFLOAD_TARGET_TYPE_GCN =3D 8 > > }; > >=20=20 > > +/* Container type for passing device properties. */ > > +union gomp_device_property_value > > +{ > > + void *ptr; > > + uintmax_t val; > > +}; > > Why wouldn't that be 'size_t', 'const char *', as the actual data types > used? (Maybe I'm missing something.) I do not see a reason for this either. Changed. > > --- a/libgomp/libgomp.map > > +++ b/libgomp/libgomp.map > > @@ -502,6 +502,14 @@ GOACC_2.0.1 { > > GOACC_parallel_keyed; > > } GOACC_2.0; > >=20=20 > > +OACC_2.6 { > > + global: > > + acc_get_property; > > + acc_get_property_h_; > > + acc_get_property_string; > > + acc_get_property_string_h_; > > +} OACC_2.5; > > + > > GOMP_PLUGIN_1.0 { > > global: > > GOMP_PLUGIN_malloc; > > That's not correct: 'OACC_2.6' should come after 'OACC_2.5.1', and > also inherit from that one. Fixed. > > --- a/libgomp/oacc-init.c > > +++ b/libgomp/oacc-init.c > > > +static union gomp_device_property_value > > +get_property_any (int ord, acc_device_t d, acc_device_property_t prop) > > +{ > > + if (!acc_known_device_type (d)) > > + unknown_device_type_error(d); > > Checking isn't needed here for this is an internal interface? Right. > > + > > + union gomp_device_property_value propval; > > + struct gomp_device_descr *dev; > > + struct goacc_thread *thr; > > Generally, in new code, we try to place these next to their first use. Very reasonable. Adapted. > > + > > + if (d =3D=3D acc_device_none) > > + return (union gomp_device_property_value) { .val =3D 0 }; > > + > > + goacc_lazy_initialize (); > > + thr =3D goacc_thread (); > > + > > + if (d =3D=3D acc_device_current && (!thr || !thr->dev)) > > + return (union gomp_device_property_value) { .val =3D 0 }; > > Should we use a 'nullval' here, as used elsewhere? We could, but we can also remove those checks completely. > Also, this checking seems a bit convoluted; shouldn't this be integrated > into the following? It's certainly not necessary to special-case > 'acc_device_none' before 'goacc_lazy_initialize' etc.? Yes, I suppose that the original implementer wanted to handle those boundary cases as efficiently as possible. But it is not strictly necessary to do this and I agree that the code becomes more readable without those checks. > > + > > + if (d =3D=3D acc_device_current) > > + { > > + dev =3D thr->dev; > > + } > > + else > > + { > > + int num_devices; > > + > > + gomp_mutex_lock (&acc_device_lock); > > + > > + dev =3D resolve_device (d, false); > > Why call this without 'fail_is_error' flag here? Good question. I have set it to true to ensure that we get the device type checking that resolve_device performs. > > --- a/libgomp/openacc.f90 > > +++ b/libgomp/openacc.f90 > > @@ -28,7 +28,7 @@ > > ! <http://www.gnu.org/licenses/>. > >=20=20 > > module openacc_kinds > > - use iso_fortran_env, only: int32 > > + use iso_fortran_env, only: int32, int64 > > implicit none > >=20=20 > > private :: int32 > > @@ -47,6 +47,21 @@ module openacc_kinds > > integer (acc_device_kind), parameter :: acc_device_not_host =3D 4 > > integer (acc_device_kind), parameter :: acc_device_nvidia =3D 5 > > integer (acc_device_kind), parameter :: acc_device_gcn =3D 8 > > + integer (acc_device_kind), parameter :: acc_device_current =3D -3 > > + > > + public :: acc_device_property > > + > > + integer, parameter :: acc_device_property =3D int64 > > Why 'int64'? I changed this to 'int32', but please tell if there's a > reason for 'int64'. int32 is too narrow as - conforming to the OpenACC spec - acc_device_property is also used for the return type of acc_get_property (a bit strang, isn't it?). int64 also did not seem quite right. I have changed the type of acc_device_property to c_size_t to match the type that is used internally and as the return type of the corresponding C function. > Is it a conscious decision that we're not supporting the new > 'acc_get_property' interface via 'openacc_lib.h', which is (or, used to > be) an alternative to the Fortran 'openacc' module? It was not my decision to leave it out. I have to admit that I did not notice the omission. > As of OpenACC 2.5, 'openacc_lib.h' has been deprecated ("no longer > supported"), but so far, we continued to support it, and it's (maybe?) > strange when that one now works for everything but the 'acc_get_property' > interface? Or, is that a statement that users really should move to the > Fortran 'openacc' module? Should they? You are probably best qualified to answer this :-). > > +typedef enum acc_device_property_t { > > + /* Keep in sync with include/gomp-constants.h. */ > > + /* Start from 1 to catch uninitialized use. */ > > + acc_property_memory =3D 1, > > + acc_property_free_memory =3D 2, > > + acc_property_name =3D 0x10001, > > + acc_property_vendor =3D 0x10002, > > + acc_property_driver =3D 0x10003 > > +} acc_device_property_t; > > Do we also need the magic here so that "Ensure enumeration is layout > compatible with int"? But I see that is not done for the 'typedef enum > acc_async_t' either. I don't remember the history behind that. I understand the "Ensure enumeration is layout compatible with int" comment for acc_device_t, but I fail to see how those values achieve this. If you do not see a good reason to keep those magic values, I would change them to 3, 4, 5 and if we need the "layout compatibility", I would rather do this as it is done for acc_device_t. > > --- a/libgomp/plugin/plugin-hsa.c > > +++ b/libgomp/plugin/plugin-hsa.c > > @@ -699,6 +699,32 @@ GOMP_OFFLOAD_get_num_devices (void) > > return hsa_context.agent_count; > > } > >=20=20 > > +/* Part of the libgomp plugin interface. Return the value of property > > + PROP of agent number N. */ > > + > > +union gomp_device_property_value > > +GOMP_OFFLOAD_get_property (int n, int prop) > > +{ > > + union gomp_device_property_value nullval =3D { .val =3D 0 }; > > + > > + if (!init_hsa_context ()) > > + return nullval; > > I'm not familiar with that code, but similar to other plugins, > 'init_hsa_context' already is called via 'GOMP_OFFLOAD_get_num_devices' > (and 'GOMP_OFFLOAD_init_device', hmm...), so probably don't need to call > it here? Since Martin Jambor wrote that the call does no harm, I would just keep it. > > + > > + switch (prop) > > + { > > + case GOMP_DEVICE_PROPERTY_VENDOR: > > + return (union gomp_device_property_value) { .ptr =3D "AMD" }; > > + default: > > + return nullval; > > + } > > +} > > Not sure if "AMD" is actually correct here -- isn't HSA a > vendor-independent standard? I have changed "AMD" to "HSA". > > +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); > > Everything (?) else seems to be accessing 'ptx_devices' without locking? > (I don't quickly understand the locking protocol used there... Will look > again tomorrow.) GOMP_OFFLOAD_init_device, GOMP_OFFLOAD_fini_device take the lock before accessing ptx_devices. I kept it. > > + CUDA_CALL_ERET (propval, cuCtxGetDevice, &ctxdev); > > + if (ptx_dev->dev =3D=3D ctxdev) > > + CUDA_CALL_ERET (propval, cuMemGetInfo, &free_mem, &total_mem); > > + else if (ptx_dev->ctx) > > + { > > + CUcontext old_ctx; > > + > > + CUDA_CALL_ERET (propval, cuCtxPushCurrent, ptx_dev->ctx); > > + CUDA_CALL_ERET (propval, cuMemGetInfo, &free_mem, &total_mem); > > + CUDA_CALL_ASSERT (cuCtxPopCurrent, &old_ctx); > > + } > > + else > > + { > > + CUcontext new_ctx; > > + > > + CUDA_CALL_ERET (propval, cuCtxCreate, &new_ctx, CU_CTX_SCHED_AUTO, > > + ptx_dev->dev); > > + CUDA_CALL_ERET (propval, cuMemGetInfo, &free_mem, &total_mem); > > + CUDA_CALL_ASSERT (cuCtxDestroy, new_ctx); > > + } > > > (Have not yet reviewed that CUDA magic. Do you understand that?) Yes, sort of. If the current thread's CUDA context is the a context for the device, perform the query (cuMemGetInfo) right away. Otherwise, we have to set the context first to ensure that we query the right device. Note that this is not necessary for the functions that are used to retrieve the values of the other properties because they use CUDA functions that take a device argument. If the ptx_dev already contains a context for the device, set it temporarily for the duration of the query. Otherwise, do the same with a newly created context for the device and dispose of the new context afterwards. The last case might arise only if the device has not been initialized, as GOMP_OFFLOAD_init_device calls nvptx_open_device which creates the context ptx_dev->ctx for ptx_dev->dev. > > + case GOMP_DEVICE_PROPERTY_DRIVER: > > + propval.ptr =3D cuda_driver_version; > > + break; > > + default: > > + GOMP_PLUGIN_error("Unknown OpenACC device-property"); > > + } > > [...] > > I see 'libgomp/oacc-host.c:host_get_property', > 'libgomp/plugin/plugin-hsa.c:GOMP_OFFLOAD_get_property', > 'liboffloadmic/plugin/libgomp-plugin-intelmic.cpp:GOMP_OFFLOAD_get_property' > do have a 'default: return nullval'; that's probably what we need to do > here, too? Yes, I have changed this and added test cases that check the return value for invalid properties. > > --- a/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp > [...] > > + case GOMP_DEVICE_PROPERTY_VENDOR: > > + /* TODO: "error: invalid conversion from 'const void*' to 'void*' = > [-fpermissive]" */ > > + return (union gomp_device_property_value) { .ptr =3D (char *) "In= > tel" }; > > Type cast maybe unnecessary per my 'libgomp/libgomp-plugin.h' comment above? Correct. Is it ok to commit the patch to trunk? Best regards, Frederik
Hi Frederik! On 2019-12-20T17:46:57+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote: >> | 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.) >> >> Jakub didn't answer, but I now myself decided that we should group this >> with the other OpenACC libgomp-plugin functions, as this interface is >> defined in terms of OpenACC-specific stuff such as 'acc_device_t'. >> Frederik, please work on that, also try to move function definitions etc. >> into appropriate places in case they aren't; ask if you need help. >> That needs to be updated. > > Is it ok to do this in a separate follow-up patch? Yes. This doesn't affect anything but the libgomp-plugin interface. >> > --- a/include/gomp-constants.h >> > +++ b/include/gomp-constants.h >> > @@ -178,6 +178,20 @@ enum gomp_map_kind >> >=20=20 >> > #define GOMP_DEVICE_ICV -1 >> > #define GOMP_DEVICE_HOST_FALLBACK -2 >> > +#define GOMP_DEVICE_CURRENT -3 >> [...] >> >> Not should if this should be grouped with 'GOMP_DEVICE_ICV', >> 'GOMP_DEVICE_HOST_FALLBACK', for it is not related to there. >> >> [...] >> >> 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. >> still wonder about that... ;-) > Changing the value of GOMP_DEVICE_ICV violates the following static asserts in oacc-parallel.c: [...] Hmm, I didn't intend to suggest changing the 'GOMP_DEVICE_ICV' value, which indeed can't/shouldn't be done. 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. >> Maybe this stuff should move from 'include/gomp-constants.h' to >> 'libgomp/oacc-int.h'. I'll think about that again, when I'm awake again >> tomorrow. ;-) > > Have you made up your mind yet? :-) Still sleepy. ;-) > Is it ok to commit the patch to trunk? OK, thanks. And then some follow-up/clean-up next year, also including some of the open questions that I've snipped off here. Grüße Thomas
Hi Thomas, >> Is it ok to commit the patch to trunk? > > OK, thanks. And then some follow-up/clean-up next year, also including > some of the open questions that I've snipped off here. Right, thanks for the review! I have committed the patch as r279710 with a minor change: I have disabled the new acc_get_property.{c,f90} tests for the amdgcn offload target for now. Best regards, Frederik
Hi! On 2019-12-21T23:02:38+0100, I wrote: > On 2019-12-20T17:46:57+0100, "Harwath, Frederik" <frederik@codesourcery.com> 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, <https://github.com/OpenACC/openacc-spec/issues/256> "Missing definition for acc_device_current" got filed; let's (also/first) watch/wait what comes out of that. Also pending is the 'libgomp/plugin/plugin-gcn.c' update; Frederik is working on that. A few other (minor) open issue we shall discuss at some later point in time. >>> | 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.) >>> >>> Jakub didn't answer, but I now myself decided that we should group this >>> with the other OpenACC libgomp-plugin functions, as this interface is >>> defined in terms of OpenACC-specific stuff such as 'acc_device_t'. Done. This also means that we don't anymore need (stub) implementations in the libgomp plugins that don't implement OpenACC support. >>> Maybe [stuff] should move from 'include/gomp-constants.h' to >>> 'libgomp/oacc-int.h'. I'll think about that again, when I'm awake again >>> tomorrow. ;-) >> >> Have you made up your mind yet? :-) > > Still sleepy. ;-) Done. In the end, 'libgomp/libgomp-plugin.h' is the place to be, as that defines, well, the libgomp plugin interface. ;-) See attached "OpenACC 'acc_get_property' cleanup"; committed to trunk in r280150. Grüße Thomas
Hi Frederik! On 2019-12-20T17:46:57+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote: > --- /dev/null > +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-2.c I suggest to rename this one to 'acc_get_property-nvptx.c'. > @@ -0,0 +1,68 @@ > +/* Test the `acc_get_property' and '`acc_get_property_string' library > + functions on Nvidia devices by comparing property values with > + those obtained through the CUDA API. */ > +/* { dg-additional-sources acc_get_property-aux.c } */ > +/* { dg-additional-options "-lcuda -lcudart" } */ > +/* { dg-do run { target openacc_nvidia_accel_selected } } */ > + > +#include <openacc.h> > +#include <cuda.h> > +#include <cuda_runtime_api.h> > +#include <string.h> > +#include <stdio.h> > + > +void expect_device_properties > +(acc_device_t dev_type, int dev_num, > + int expected_total_mem, int expected_free_mem, > + const char* expected_vendor, const char* expected_name, > + const char* expected_driver); > + > +int main () > +{ > + int dev_count; > + cudaGetDeviceCount (&dev_count); > + > + for (int dev_num = 0; dev_num < dev_count; ++dev_num) > + { > + if (cudaSetDevice (dev_num) != cudaSuccess) > + { > + fprintf (stderr, "cudaSetDevice failed.\n"); > + abort (); > + } > + > + printf("Checking device %d\n", dev_num); > + > + const char *vendor = "Nvidia"; > + size_t free_mem; > + size_t total_mem; > + if (cudaMemGetInfo(&free_mem, &total_mem) != cudaSuccess) > + { > + fprintf (stderr, "cudaMemGetInfo failed.\n"); > + abort (); > + } > + > + struct cudaDeviceProp p; > + if (cudaGetDeviceProperties(&p, dev_num) != cudaSuccess) > + { > + fprintf (stderr, "cudaGetDeviceProperties failed.\n"); > + abort (); > + } > + > + int driver_version; > + if (cudaDriverGetVersion(&driver_version) != cudaSuccess) > + { > + fprintf (stderr, "cudaDriverGetVersion failed.\n"); > + abort (); > + } > + /* The version string should contain the version of the CUDA Toolkit > + in the same MAJOR.MINOR format that is used by Nvidia. > + The format string below is the same that is used by the deviceQuery > + program, which belongs to Nvidia's CUDA samples, to print the version. */ > + char driver[30]; > + snprintf (driver, sizeof driver, "CUDA Driver %u.%u", > + driver_version / 1000, driver_version % 1000 / 10); > + > + expect_device_properties(acc_device_nvidia, dev_num, This assumes that the 'cuda*' interfaces and OpenACC/libgomp interfaces handle/order device numbers in the same way -- which it seems they do, but just noting this in case this becomes an issue at some point. > + total_mem, free_mem, vendor, p.name, driver); > + } > +} So I just witnessed a FAIL here, because: Expected acc_property_free_memory to equal -929226752, but was -928956416. Aside from improper data types being used for storing/printing the memory information, we have to expect 'acc_property_free_memory' to change between two invocations. ;-) > --- /dev/null > +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-3.c I suggest to rename this one to 'acc_get_property-host.c'. > @@ -0,0 +1,19 @@ > +/* Test the `acc_get_property' and '`acc_get_property_string' library > + functions for the host device. */ > +/* { dg-additional-sources acc_get_property-aux.c } */ > +/* { dg-do run } */ > + > +#include <openacc.h> > +#include <stdio.h> > + > +void expect_device_properties > +(acc_device_t dev_type, int dev_num, > + int expected_total_mem, int expected_free_mem, > + const char* expected_vendor, const char* expected_name, > + const char* expected_driver); > + > +int main() > +{ > + printf ("Checking acc_device_host device properties\n"); > + expect_device_properties (acc_device_host, 0, 0, 0, "GNU", "GOMP", "1.0"); > +} > --- /dev/null > +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c > @@ -0,0 +1,80 @@ > +/* Auxiliary functions for acc_get_property tests */ > +/* { dg-do compile { target skip-all-targets } } */ > + > +#include <openacc.h> > +#include <stdlib.h> > +#include <stdio.h> > +#include <string.h> > + > +void expect_device_properties > +(acc_device_t dev_type, int dev_num, > + int expected_total_mem, int expected_free_mem, > + const char* expected_vendor, const char* expected_name, > + const char* expected_driver) > +{ > + const char *vendor = acc_get_property_string (dev_num, dev_type, > + acc_property_vendor); > + if (strcmp (vendor, expected_vendor)) > + { > + fprintf (stderr, "Expected acc_property_vendor to equal \"%s\", " > + "but was \"%s\".\n", expected_vendor, vendor); > + abort (); > + } > + > + int total_mem = acc_get_property (dev_num, dev_type, > + acc_property_memory); > + if (total_mem != expected_total_mem) > + { > + fprintf (stderr, "Expected acc_property_memory to equal %d, " > + "but was %d.\n", expected_total_mem, total_mem); > + abort (); > + > + } > + > + int free_mem = acc_get_property (dev_num, dev_type, > + acc_property_free_memory); > + if (free_mem != expected_free_mem) > + { > + fprintf (stderr, "Expected acc_property_free_memory to equal %d, " > + "but was %d.\n", expected_free_mem, free_mem); > + abort (); > + } Better to just verify that 'free_mem >= 0' (by means of 'size_t' data type, I suppose), and 'free_mem <= total_mem'? (..., and for avoidance of doubt: I think there's no point in special-casing this one for 'acc_device_host' where we know that 'free_mem' is always zero -- this may change in the future.) > + > + const char *name = acc_get_property_string (dev_num, dev_type, > + acc_property_name); > + if (strcmp (name, expected_name)) > + { > + fprintf(stderr, "Expected acc_property_name to equal \"%s\", " > + "but was \"%s\".\n", expected_name, name); > + abort (); > + } > + > + const char *driver = acc_get_property_string (dev_num, dev_type, > + acc_property_driver); > + if (strcmp (expected_driver, driver)) > + { > + fprintf (stderr, "Expected acc_property_driver to equal %s, " > + "but was %s.\n", expected_driver, driver); > + abort (); > + } > + > + int unknown_property = 16058; > + int v = acc_get_property (dev_num, dev_type, (acc_device_property_t)unknown_property); > + if (v != 0) > + { > + fprintf (stderr, "Expected value of unknown numeric property to equal 0, " > + "but was %d.\n", v); > + abort (); > + } > + > + int unknown_property2 = -16058; > + const char *s = acc_get_property_string (dev_num, dev_type, (acc_device_property_t)unknown_property2); > + if (s != NULL) > + { > + fprintf (stderr, "Expected value of unknown string property to be NULL, " > + "but was %d.\n", s); > + abort (); > + } > + > + > +} Grüße Thomas
Hi Thomas, I have attached a patch containing the changes that you suggested. On 16.01.20 17:00, Thomas Schwinge wrote: > On 2019-12-20T17:46:57+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote: >> --- /dev/null >> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-2.c > > I suggest to rename this one to 'acc_get_property-nvptx.c'> [...] >> --- /dev/null >> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-3.c > I suggest to rename this one to 'acc_get_property-host.c'. I renamed both. > This assumes that the 'cuda*' interfaces and OpenACC/libgomp interfaces > handle/order device numbers in the same way -- which it seems they do, > but just noting this in case this becomes an issue at some point. Correct, I have added a corresponding comment to acc_get_property-nvptx.c. > Aside from improper data types being used for storing/printing the memory > information, we have to expect 'acc_property_free_memory' to change > between two invocations. ;-) Right! I have removed the assertion and changed it into ... > > Better to just verify that 'free_mem >= 0' (by means of 'size_t' data > type, I suppose), and 'free_mem <= total_mem'? ... this. > > (..., and for avoidance of doubt: I think there's no point in > special-casing this one for 'acc_device_host' where we know that > 'free_mem' is always zero -- this may change in the future.) Sure! But with the new "free_mem <= total_mem" assertion and since we assert total_mem == 0 and since free_mem >= 0, we effectively also assert that in the test right now ;-). Ok to push the commit to master? Best regards, Frederik
Hi Frederik! On 2020-01-20T15:01:01+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote: > I have attached a patch containing the changes that you suggested. > On 16.01.20 17:00, Thomas Schwinge wrote: >> On 2019-12-20T17:46:57+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote: > Ok to push the commit to master? Thanks, OK. Reviewed-by: Thomas Schwinge <thomas@codesourcery.com> As a low-priority follow-up, please look into: source-gcc/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c: In function 'expect_device_properties': source-gcc/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c:74:24: warning: format '%d' expects argument of type 'int', but argument 3 has type 'const char *' [-Wformat=] 74 | fprintf (stderr, "Expected value of unknown string property to be NULL, " | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 75 | "but was %d.\n", s); | ~ | | | const char * source-gcc/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c:75:19: note: format string is defined here 75 | "but was %d.\n", s); | ~^ | | | int | %s ..., and (random example): > int unknown_property = 16058; > - int v = acc_get_property (dev_num, dev_type, (acc_device_property_t)unknown_property); > + size_t v = acc_get_property (dev_num, dev_type, (acc_device_property_t)unknown_property); > if (v != 0) > { > fprintf (stderr, "Expected value of unknown numeric property to equal 0, " > - "but was %d.\n", v); > + "but was %zd.\n", v); > abort (); > } ..., shouldn't that be '%zu' given that 'size_t' is 'unsigned'? libgomp.oacc-c-c++-common/acc_get_property-aux.c: fprintf (stderr, "Expected acc_property_memory to equal %zd, " libgomp.oacc-c-c++-common/acc_get_property-aux.c: "but was %zd.\n", expected_memory, total_mem); libgomp.oacc-c-c++-common/acc_get_property-aux.c: ", but free memory was %zd and total memory was %zd.\n", libgomp.oacc-c-c++-common/acc_get_property-aux.c: "but was %zd.\n", v); libgomp.oacc-c-c++-common/acc_get_property.c: printf (" Total memory: %zd\n", v); libgomp.oacc-c-c++-common/acc_get_property.c: printf (" Free memory: %zd\n", v); Grüße Thomas
Hi Thomas, On 23.01.20 15:32, Thomas Schwinge wrote: > On 2020-01-20T15:01:01+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote: >> On 16.01.20 17:00, Thomas Schwinge wrote: >>> On 2019-12-20T17:46:57+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote: >> Ok to push the commit to master? > > Thanks, OK. Reviewed-by: Thomas Schwinge <thomas@codesourcery.com> Thank you. Committed as 4bd03ed69bd789278a0286017b692f49052ffe5c, including the changes to the size_t formatting. Best regards, Frederik > > > As a low-priority follow-up, please look into: > > source-gcc/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c: In function 'expect_device_properties': > source-gcc/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c:74:24: warning: format '%d' expects argument of type 'int', but argument 3 has type 'const char *' [-Wformat=] > 74 | fprintf (stderr, "Expected value of unknown string property to be NULL, " > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 75 | "but was %d.\n", s); > | ~ > | | > | const char * > source-gcc/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c:75:19: note: format string is defined here > 75 | "but was %d.\n", s); > | ~^ > | | > | int > | %s > > ..., and (random example): > >> int unknown_property = 16058; >> - int v = acc_get_property (dev_num, dev_type, (acc_device_property_t)unknown_property); >> + size_t v = acc_get_property (dev_num, dev_type, (acc_device_property_t)unknown_property); >> if (v != 0) >> { >> fprintf (stderr, "Expected value of unknown numeric property to equal 0, " >> - "but was %d.\n", v); >> + "but was %zd.\n", v); >> abort (); >> } > > ..., shouldn't that be '%zu' given that 'size_t' is 'unsigned'? > > libgomp.oacc-c-c++-common/acc_get_property-aux.c: fprintf (stderr, "Expected acc_property_memory to equal %zd, " > libgomp.oacc-c-c++-common/acc_get_property-aux.c: "but was %zd.\n", expected_memory, total_mem); > libgomp.oacc-c-c++-common/acc_get_property-aux.c: ", but free memory was %zd and total memory was %zd.\n", > libgomp.oacc-c-c++-common/acc_get_property-aux.c: "but was %zd.\n", v); > libgomp.oacc-c-c++-common/acc_get_property.c: printf (" Total memory: %zd\n", v); > libgomp.oacc-c-c++-common/acc_get_property.c: printf (" Free memory: %zd\n", v); > > > Grüße > Thomas >
Hi! On 2019-12-20T17:46:57+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote: >> > --- a/libgomp/libgomp-plugin.h >> > +++ b/libgomp/libgomp-plugin.h >> > @@ -54,6 +54,13 @@ enum offload_target_type >> > OFFLOAD_TARGET_TYPE_GCN =3D 8 >> > }; >> >=20=20 >> > +/* Container type for passing device properties. */ >> > +union gomp_device_property_value >> > +{ >> > + void *ptr; >> > + uintmax_t val; >> > +}; >> >> Why wouldn't that be 'size_t', 'const char *', as the actual data types >> used? (Maybe I'm missing something.) > > I do not see a reason for this either. Changed. For reference: C/C++ has 'size_t' ('acc_get_property'), or 'const char*' ('acc_get_property_string') return types. >> > --- a/libgomp/openacc.f90 >> > +++ b/libgomp/openacc.f90 >> > @@ -28,7 +28,7 @@ >> > ! <http://www.gnu.org/licenses/>. >> >=20=20 >> > module openacc_kinds >> > - use iso_fortran_env, only: int32 >> > + use iso_fortran_env, only: int32, int64 >> > implicit none >> >=20=20 >> > private :: int32 >> > @@ -47,6 +47,21 @@ module openacc_kinds >> > integer (acc_device_kind), parameter :: acc_device_not_host =3D 4 >> > integer (acc_device_kind), parameter :: acc_device_nvidia =3D 5 >> > integer (acc_device_kind), parameter :: acc_device_gcn =3D 8 >> > + integer (acc_device_kind), parameter :: acc_device_current =3D -3 >> > + >> > + public :: acc_device_property >> > + >> > + integer, parameter :: acc_device_property =3D int64 >> >> Why 'int64'? I changed this to 'int32', but please tell if there's a >> reason for 'int64'. > > int32 is too narrow as - conforming to the OpenACC spec - acc_device_property > is also used for the return type of acc_get_property (a bit strang, isn't it?). > int64 also did not seem quite right. I have changed the type of acc_device_property > to c_size_t to match the type that is used internally and as the return type of the > corresponding C function. I filed <https://github.com/OpenACC/openacc-spec/issues/260> "Fortran 'acc_get_property' return type": | During review/implementation of `acc_get_property` in GCC, @frederik-h | found that for Fortran `function acc_get_property`, the return type is | specified as `integer(acc_device_property) :: acc_get_property`, | whereas in C/C++, it is `size_t`. For avoidance of doubt: it's correct | to map the C/C++ `acc_device_property_t property` formal parameter to | Fortran `integer(acc_device_property), value :: property`, but it's not | clear why `integer(acc_device_property)` is also used as the function's | return type -- the return type/values don't actually (conceptually) | relate to the `integer(acc_device_property)` data type. | | Should we use `c_size_t` for Fortran `acc_get_property` return type to | explicitly match C/C++, or use plain `integer` (as used in all other | interfaces taking `size_t` in C/C++ -- currently only as input formal | parameters though)? Grüße Thomas > --- a/libgomp/libgomp.texi > +++ b/libgomp/libgomp.texi > +@node acc_get_property > +@section @code{acc_get_property} -- Get device property. > +@cindex acc_get_property > +@cindex acc_get_property_string > +@table @asis > +@item @emph{Description} > +These routines return the value of the specified @var{property} for the > +device being queried according to @var{devicenum} and @var{devicetype}. > +Integer-valued and string-valued properties are returned by > +@code{acc_get_property} and @code{acc_get_property_string} respectively. > +The Fortran @code{acc_get_property_string} subroutine returns the string > +retrieved in its fourth argument while the remaining entry points are > +functions, which pass the return value as their result. > + > +@item @emph{C/C++}: > +@multitable @columnfractions .20 .80 > +@item @emph{Prototype}: @tab @code{size_t acc_get_property(int devicenum, acc_device_t devicetype, acc_device_property_t property);} > +@item @emph{Prototype}: @tab @code{const char *acc_get_property_string(int devicenum, acc_device_t devicetype, acc_device_property_t property);} > +@end multitable > + > +@item @emph{Fortran}: > +@multitable @columnfractions .20 .80 > +@item @emph{Interface}: @tab @code{function acc_get_property(devicenum, devicetype, property)} > +@item @emph{Interface}: @tab @code{subroutine acc_get_property_string(devicenum, devicetype, property, string)} > +@item @tab @code{integer devicenum} > +@item @tab @code{integer(kind=acc_device_kind) devicetype} > +@item @tab @code{integer(kind=acc_device_property) property} > +@item @tab @code{integer(kind=acc_device_property) acc_get_property} > +@item @tab @code{character(*) string} > +@end multitable > + > +@item @emph{Reference}: > +@uref{https://www.openacc.org, OpenACC specification v2.6}, section > +3.2.6. > +@end table > --- a/libgomp/openacc.f90 > +++ b/libgomp/openacc.f90 > @@ -31,16 +31,18 @@ > > module openacc_kinds > use iso_fortran_env, only: int32 > + use iso_c_binding, only: c_size_t > implicit none > > public > - private :: int32 > + private :: int32, c_size_t > > @@ -89,6 +100,24 @@ module openacc_internal > integer (acc_device_kind) d > end function > > + function acc_get_property_h (n, d, p) > + import > + implicit none (type, external) > + integer (acc_device_property) :: acc_get_property_h > + integer, value :: n > + integer (acc_device_kind), value :: d > + integer (acc_device_property), value :: p > + end function > + > + subroutine acc_get_property_string_h (n, d, p, s) > + import > + implicit none (type, external) > + integer, value :: n > + integer (acc_device_kind), value :: d > + integer (acc_device_property), value :: p > + character (*) :: s > + end subroutine > + > function acc_async_test_h (a) > logical acc_async_test_h > integer a > @@ -508,6 +537,26 @@ module openacc_internal > integer (c_int), value :: d > end function > > + function acc_get_property_l (n, d, p) & > + bind (C, name = "acc_get_property") > + use iso_c_binding, only: c_int, c_size_t > + implicit none (type, external) > + integer (c_size_t) :: acc_get_property_l > + integer (c_int), value :: n > + integer (c_int), value :: d > + integer (c_int), value :: p > + end function > + > + function acc_get_property_string_l (n, d, p) & > + bind (C, name = "acc_get_property_string") > + use iso_c_binding, only: c_int, c_ptr > + implicit none (type, external) > + type (c_ptr) :: acc_get_property_string_l > + integer (c_int), value :: n > + integer (c_int), value :: d > + integer (c_int), value :: p > + end function > @@ -758,6 +814,14 @@ module openacc > procedure :: acc_get_device_num_h > end interface > > + interface acc_get_property > + procedure :: acc_get_property_h > + end interface > + > + interface acc_get_property_string > + procedure :: acc_get_property_string_h > + end interface > + > interface acc_async_test > procedure :: acc_async_test_h > end interface > @@ -976,6 +1040,63 @@ function acc_get_device_num_h (d) > acc_get_device_num_h = acc_get_device_num_l (d) > end function > > +function acc_get_property_h (n, d, p) > + use iso_c_binding, only: c_int, c_size_t > + use openacc_internal, only: acc_get_property_l > + use openacc_kinds > + implicit none (type, external) > + integer (acc_device_property) :: acc_get_property_h > + integer, value :: n > + integer (acc_device_kind), value :: d > + integer (acc_device_property), value :: p > + > + integer (c_int) :: pint > + > + pint = int (p, c_int) > + acc_get_property_h = acc_get_property_l (n, d, pint) > +end function > + > +subroutine acc_get_property_string_h (n, d, p, s) > + use iso_c_binding, only: c_char, c_int, c_ptr, c_f_pointer, c_associated > + use openacc_internal, only: acc_get_property_string_l > + use openacc_kinds > + implicit none (type, external) > + integer, value :: n > + integer (acc_device_kind), value :: d > + integer (acc_device_property), value :: p > + character (*) :: s > + > + integer (c_int) :: pint > + type (c_ptr) :: cptr > + integer :: clen > + character (kind=c_char, len=1), pointer, contiguous :: sptr (:) > + integer :: slen > + integer :: i > + > + interface > + function strlen (s) bind (C, name = "strlen") > + use iso_c_binding, only: c_ptr, c_size_t > + type (c_ptr), intent(in), value :: s > + integer (c_size_t) :: strlen > + end function strlen > + end interface > + > + pint = int (p, c_int) > + cptr = acc_get_property_string_l (n, d, pint) > + s = "" > + if (.not. c_associated (cptr)) then > + return > + end if > + > + clen = int (strlen (cptr)) > + call c_f_pointer (cptr, sptr, [clen]) > + > + slen = min (clen, len (s)) > + do i = 1, slen > + s (i:i) = sptr (i) > + end do > +end subroutine
Hi! On 2019-12-17T00:00:04+0100, I wrote: > On 2019-11-14T16:35:31+0100, Frederik Harwath <frederik@codesourcery.com> wrote: >> this patch implements OpenACC 2.6 "acc_get_property" and related functions. > As I mentioned before ("thinking aloud"): > > | [...] 'acc_device_current' [is] relevant only for > | 'acc_get_property', to return "the value of the property for the current > | device". This [now has a] special (negative?) value > | [...], 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. > 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.) Now pushed this change to master branch in commit a5d0bc12e1bfa956941cd9c49d5b978256bd11ec "[OpenACC] Set 'acc_device_current = -1'", see attached. Grüße Thomas ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
diff --git a/include/gomp-constants.h b/include/gomp-constants.h index 9e356cdfeec9..150b12d0d6fd 100644 --- a/include/gomp-constants.h +++ b/include/gomp-constants.h @@ -178,6 +178,20 @@ enum gomp_map_kind #define GOMP_DEVICE_ICV -1 #define GOMP_DEVICE_HOST_FALLBACK -2 +#define GOMP_DEVICE_CURRENT -3 + +/* 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. */ +/* Start from 1 to catch uninitialized use. */ +#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 /* GOMP_task/GOMP_taskloop* flags argument. */ #define GOMP_TASK_FLAG_UNTIED (1 << 0) diff --git a/libgomp/libgomp-plugin.h b/libgomp/libgomp-plugin.h index 037558c43f56..5f968855f652 100644 --- a/libgomp/libgomp-plugin.h +++ b/libgomp/libgomp-plugin.h @@ -54,6 +54,13 @@ enum offload_target_type OFFLOAD_TARGET_TYPE_GCN = 8 }; +/* Container type for passing device properties. */ +union gomp_device_property_value +{ + void *ptr; + uintmax_t val; +}; + /* Opaque type to represent plugin-dependent implementation of an OpenACC asynchronous queue. */ struct goacc_asyncqueue; @@ -94,6 +101,7 @@ extern const char *GOMP_OFFLOAD_get_name (void); extern unsigned int GOMP_OFFLOAD_get_caps (void); extern int GOMP_OFFLOAD_get_type (void); extern int GOMP_OFFLOAD_get_num_devices (void); +extern union gomp_device_property_value GOMP_OFFLOAD_get_property (int, int); extern bool GOMP_OFFLOAD_init_device (int); extern bool GOMP_OFFLOAD_fini_device (int); extern unsigned GOMP_OFFLOAD_version (void); diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h index bab733d2b2da..c3f89a26532c 100644 --- a/libgomp/libgomp.h +++ b/libgomp/libgomp.h @@ -1106,6 +1106,7 @@ struct gomp_device_descr __typeof (GOMP_OFFLOAD_get_caps) *get_caps_func; __typeof (GOMP_OFFLOAD_get_type) *get_type_func; __typeof (GOMP_OFFLOAD_get_num_devices) *get_num_devices_func; + __typeof (GOMP_OFFLOAD_get_property) *get_property_func; __typeof (GOMP_OFFLOAD_init_device) *init_device_func; __typeof (GOMP_OFFLOAD_fini_device) *fini_device_func; __typeof (GOMP_OFFLOAD_version) *version_func; diff --git a/libgomp/libgomp.map b/libgomp/libgomp.map index c79430f8d8d1..fba4effbb3eb 100644 --- a/libgomp/libgomp.map +++ b/libgomp/libgomp.map @@ -502,6 +502,14 @@ GOACC_2.0.1 { GOACC_parallel_keyed; } GOACC_2.0; +OACC_2.6 { + global: + acc_get_property; + acc_get_property_h_; + acc_get_property_string; + acc_get_property_string_h_; +} OACC_2.5; + GOMP_PLUGIN_1.0 { global: GOMP_PLUGIN_malloc; diff --git a/libgomp/libgomp.texi b/libgomp/libgomp.texi index 6db895f62726..91afca4589d9 100644 --- a/libgomp/libgomp.texi +++ b/libgomp/libgomp.texi @@ -1849,6 +1849,7 @@ acceleration device. * acc_get_device_type:: Get type of device accelerator to be used. * acc_set_device_num:: Set device number to use. * acc_get_device_num:: Get device number to be used. +* acc_get_property:: Get device property. * acc_async_test:: Tests for completion of a specific asynchronous operation. * acc_async_test_all:: Tests for completion of all asychronous @@ -2038,6 +2039,44 @@ region. +@node acc_get_property +@section @code{acc_get_property} -- Get device property. +@cindex acc_get_property +@cindex acc_get_property_string +@table @asis +@item @emph{Description} +These routines return the value of the specified @var{property} for the +device being queried according to @var{devicenum} and @var{devicetype}. +Integer-valued and string-valued properties are returned by +@code{acc_get_property} and @code{acc_get_property_string} respectively. +The Fortran @code{acc_get_property_string} subroutine returns the string +retrieved in its fourth argument while the remaining entry points are +functions, which pass the return value as their result. + +@item @emph{C/C++}: +@multitable @columnfractions .20 .80 +@item @emph{Prototype}: @tab @code{size_t acc_get_property(int devicenum, acc_device_t devicetype, acc_device_property_t property);} +@item @emph{Prototype}: @tab @code{const char *acc_get_property_string(int devicenum, acc_device_t devicetype, acc_device_property_t property);} +@end multitable + +@item @emph{Fortran}: +@multitable @columnfractions .20 .80 +@item @emph{Interface}: @tab @code{function acc_get_property(devicenum, devicetype, property)} +@item @emph{Interface}: @tab @code{subroutine acc_get_property_string(devicenum, devicetype, property, string)} +@item @tab @code{integer devicenum} +@item @tab @code{integer(kind=acc_device_kind) devicetype} +@item @tab @code{integer(kind=acc_device_property) property} +@item @tab @code{integer(kind=acc_device_property) acc_get_property} +@item @tab @code{character(*) string} +@end multitable + +@item @emph{Reference}: +@uref{https://www.openacc.org, OpenACC specification v2.6}, section +3.2.6. +@end table + + + @node acc_async_test @section @code{acc_async_test} -- Test for completion of a specific asynchronous operation. @table @asis diff --git a/libgomp/oacc-host.c b/libgomp/oacc-host.c index cbcac9bf7b33..70005c3656db 100644 --- a/libgomp/oacc-host.c +++ b/libgomp/oacc-host.c @@ -59,6 +59,27 @@ host_get_num_devices (void) return 1; } +static union gomp_device_property_value +host_get_property (int n, int prop) +{ + union gomp_device_property_value nullval = { .val = 0 }; + + if (n >= host_get_num_devices ()) + return nullval; + + switch (prop) + { + case GOMP_DEVICE_PROPERTY_NAME: + return (union gomp_device_property_value) { .ptr = "GOMP" }; + case GOMP_DEVICE_PROPERTY_VENDOR: + return (union gomp_device_property_value) { .ptr = "GNU" }; + case GOMP_DEVICE_PROPERTY_DRIVER: + return (union gomp_device_property_value) { .ptr = VERSION }; + default: + return nullval; + } +} + static bool host_init_device (int n __attribute__ ((unused))) { @@ -248,6 +269,7 @@ static struct gomp_device_descr host_dispatch = .get_caps_func = host_get_caps, .get_type_func = host_get_type, .get_num_devices_func = host_get_num_devices, + .get_property_func = host_get_property, .init_device_func = host_init_device, .fini_device_func = host_fini_device, .version_func = host_version, diff --git a/libgomp/oacc-init.c b/libgomp/oacc-init.c index e1568c535b32..5d8523cf3259 100644 --- a/libgomp/oacc-init.c +++ b/libgomp/oacc-init.c @@ -93,6 +93,21 @@ get_openacc_name (const char *name) return name; } + +static int +acc_known_device_type (acc_device_t __arg) +{ + return __arg >= 0 && __arg < _ACC_device_hwm; +} + + +static void +unknown_device_type_error (unsigned invalid_type) +{ + gomp_fatal ("unknown device type %u", invalid_type); +} + + static const char * name_of_acc_device_t (enum acc_device_t type) { @@ -103,8 +118,9 @@ name_of_acc_device_t (enum acc_device_t type) case acc_device_host: return "host"; case acc_device_not_host: return "not_host"; case acc_device_nvidia: return "nvidia"; - default: gomp_fatal ("unknown device type %u", (unsigned) type); + default: unknown_device_type_error(type); } + return 0; /** Never reached **/ } /* ACC_DEVICE_LOCK must be held before calling this function. If FAIL_IS_ERROR @@ -123,7 +139,7 @@ resolve_device (acc_device_t d, bool fail_is_error) if (goacc_device_type) { /* Lookup the named device. */ - while (++d != _ACC_device_hwm) + while (acc_known_device_type (++d)) if (dispatchers[d] && !strcasecmp (goacc_device_type, get_openacc_name (dispatchers[d]->name)) @@ -147,7 +163,7 @@ resolve_device (acc_device_t d, bool fail_is_error) case acc_device_not_host: /* Find the first available device after acc_device_not_host. */ - while (++d != _ACC_device_hwm) + while (acc_known_device_type (++d)) if (dispatchers[d] && dispatchers[d]->get_num_devices_func () > 0) goto found; if (d_arg == acc_device_default) @@ -168,7 +184,7 @@ resolve_device (acc_device_t d, bool fail_is_error) break; default: - if (d > _ACC_device_hwm) + if (!acc_known_device_type (d)) { if (fail_is_error) goto unsupported_device; @@ -328,7 +344,6 @@ acc_shutdown_1 (acc_device_t d) gomp_unload_device (acc_dev); gomp_mutex_unlock (&acc_dev->lock); } - gomp_mutex_lock (&goacc_thread_lock); /* Free target-specific TLS data and close all devices. */ @@ -494,7 +509,6 @@ goacc_attach_host_thread_to_device (int ord) thr->api_info = NULL; /* Initially, all callbacks for all events are enabled. */ thr->prof_callbacks_enabled = true; - thr->target_tls = acc_dev->openacc.create_thread_data_func (ord); } @@ -505,12 +519,15 @@ goacc_attach_host_thread_to_device (int ord) void acc_init (acc_device_t d) { + if (!acc_known_device_type (d)) + unknown_device_type_error(d); + gomp_init_targets_once (); gomp_mutex_lock (&acc_device_lock); cached_base_dev = acc_init_1 (d, acc_construct_runtime_api, 0); gomp_mutex_unlock (&acc_device_lock); - + goacc_attach_host_thread_to_device (-1); } @@ -519,6 +536,9 @@ ialias (acc_init) void acc_shutdown (acc_device_t d) { + if (!acc_known_device_type (d)) + unknown_device_type_error(d); + gomp_init_targets_once (); gomp_mutex_lock (&acc_device_lock); @@ -533,6 +553,9 @@ ialias (acc_shutdown) int acc_get_num_devices (acc_device_t d) { + if (!acc_known_device_type (d)) + unknown_device_type_error(d); + int n = 0; struct gomp_device_descr *acc_dev; @@ -564,6 +587,9 @@ ialias (acc_get_num_devices) void acc_set_device_type (acc_device_t d) { + if (!acc_known_device_type (d)) + unknown_device_type_error(d); + struct gomp_device_descr *base_dev, *acc_dev; struct goacc_thread *thr = goacc_thread (); @@ -637,7 +663,8 @@ acc_get_device_type (void) } assert (res != acc_device_default - && res != acc_device_not_host); + && res != acc_device_not_host + && res != acc_device_current); return res; } @@ -647,12 +674,12 @@ ialias (acc_get_device_type) int acc_get_device_num (acc_device_t d) { + if (!acc_known_device_type (d)) + unknown_device_type_error(d); + const struct gomp_device_descr *dev; struct goacc_thread *thr = goacc_thread (); - if (d >= _ACC_device_hwm) - gomp_fatal ("unknown device type %u", (unsigned) d); - acc_prof_info prof_info; acc_api_info api_info; bool profiling_p = GOACC_PROFILING_SETUP_P (thr, &prof_info, &api_info); @@ -682,6 +709,9 @@ ialias (acc_get_device_num) void acc_set_device_num (int ord, acc_device_t d) { + if (!acc_known_device_type (d)) + unknown_device_type_error(d); + struct gomp_device_descr *base_dev, *acc_dev; int num_devices; @@ -723,6 +753,87 @@ acc_set_device_num (int ord, acc_device_t d) ialias (acc_set_device_num) +static union gomp_device_property_value +get_property_any (int ord, acc_device_t d, acc_device_property_t prop) +{ + if (!acc_known_device_type (d)) + unknown_device_type_error(d); + + union gomp_device_property_value propval; + struct gomp_device_descr *dev; + struct goacc_thread *thr; + + if (d == acc_device_none) + return (union gomp_device_property_value) { .val = 0 }; + + goacc_lazy_initialize (); + thr = goacc_thread (); + + if (d == acc_device_current && (!thr || !thr->dev)) + return (union gomp_device_property_value) { .val = 0 }; + + if (d == acc_device_current) + { + dev = thr->dev; + } + else + { + int num_devices; + + gomp_mutex_lock (&acc_device_lock); + + dev = resolve_device (d, false); + + num_devices = dev->get_num_devices_func (); + + if (num_devices <= 0 || ord >= num_devices) + acc_dev_num_out_of_range (d, ord, num_devices); + + dev += ord; + + gomp_mutex_lock (&dev->lock); + if (dev->state == GOMP_DEVICE_UNINITIALIZED) + gomp_init_device (dev); + gomp_mutex_unlock (&dev->lock); + + gomp_mutex_unlock (&acc_device_lock); + } + + assert (dev); + + propval = dev->get_property_func (dev->target_id, prop); + + return propval; +} + +size_t +acc_get_property (int ord, acc_device_t d, acc_device_property_t prop) +{ + if (!acc_known_device_type (d)) + unknown_device_type_error(d); + + if (prop & GOMP_DEVICE_PROPERTY_STRING_MASK) + return 0; + else + return get_property_any (ord, d, prop).val; +} + +ialias (acc_get_property) + +const char * +acc_get_property_string (int ord, acc_device_t d, acc_device_property_t prop) +{ + if (!acc_known_device_type (d)) + unknown_device_type_error(d); + + if (prop & GOMP_DEVICE_PROPERTY_STRING_MASK) + return get_property_any (ord, d, prop).ptr; + else + return NULL; +} + +ialias (acc_get_property_string) + /* For -O and higher, the compiler always attempts to expand acc_on_device, but if the user disables the builtin, or calls it via a pointer, we'll need this version. @@ -733,6 +844,9 @@ ialias (acc_set_device_num) int __attribute__ ((__optimize__ ("O2"))) acc_on_device (acc_device_t dev) { + if (!acc_known_device_type (dev)) + unknown_device_type_error(dev); + return __builtin_acc_on_device (dev); } @@ -763,6 +877,9 @@ goacc_runtime_initialize (void) attribute_hidden void goacc_save_and_set_bind (acc_device_t d) { + if (!acc_known_device_type (d)) + unknown_device_type_error (d); + struct goacc_thread *thr = goacc_thread (); assert (!thr->saved_bound_dev); diff --git a/libgomp/openacc.f90 b/libgomp/openacc.f90 index 831a157e703d..f574c7e31a8c 100644 --- a/libgomp/openacc.f90 +++ b/libgomp/openacc.f90 @@ -28,7 +28,7 @@ ! <http://www.gnu.org/licenses/>. module openacc_kinds - use iso_fortran_env, only: int32 + use iso_fortran_env, only: int32, int64 implicit none private :: int32 @@ -47,6 +47,21 @@ module openacc_kinds integer (acc_device_kind), parameter :: acc_device_not_host = 4 integer (acc_device_kind), parameter :: acc_device_nvidia = 5 integer (acc_device_kind), parameter :: acc_device_gcn = 8 + integer (acc_device_kind), parameter :: acc_device_current = -3 + + public :: acc_device_property + + integer, parameter :: acc_device_property = int64 + + public :: acc_property_memory, acc_property_free_memory + public :: acc_property_name, acc_property_vendor, acc_property_driver + + ! Keep in sync with include/gomp-constants.h. + integer (acc_device_property), parameter :: acc_property_memory = 1 + integer (acc_device_property), parameter :: acc_property_free_memory = 2 + integer (acc_device_property), parameter :: acc_property_name = int(Z'10001') + integer (acc_device_property), parameter :: acc_property_vendor = int(Z'10002') + integer (acc_device_property), parameter :: acc_property_driver = int(Z'10003') public :: acc_handle_kind @@ -87,6 +102,24 @@ module openacc_internal integer (acc_device_kind) d end subroutine + function acc_get_property_h (n, d, p) + import + implicit none (type, external) + integer (acc_device_property) :: acc_get_property_h + integer, value :: n + integer (acc_device_kind), value :: d + integer (acc_device_property), value :: p + end function + + subroutine acc_get_property_string_h (n, d, p, s) + import + implicit none (type, external) + integer, value :: n + integer (acc_device_kind), value :: d + integer (acc_device_property), value :: p + character (*) :: s + end subroutine + function acc_get_device_num_h (d) import integer acc_get_device_num_h @@ -512,6 +545,26 @@ module openacc_internal integer (c_int), value :: d end function + function acc_get_property_l (n, d, p) & + bind (C, name = "acc_get_property") + use iso_c_binding, only: c_int, c_size_t + implicit none (type, external) + integer (c_size_t) :: acc_get_property_l + integer (c_int), value :: n + integer (c_int), value :: d + integer (c_int), value :: p + end function + + function acc_get_property_string_l (n, d, p) & + bind (C, name = "acc_get_property_string") + use iso_c_binding, only: c_int, c_ptr + implicit none (type, external) + type (c_ptr) :: acc_get_property_string_l + integer (c_int), value :: n + integer (c_int), value :: d + integer (c_int), value :: p + end function + function acc_async_test_l (a) & bind (C, name = "acc_async_test") use iso_c_binding, only: c_int @@ -753,6 +806,14 @@ module openacc procedure :: acc_get_device_num_h end interface + interface acc_get_property + procedure :: acc_get_property_h + end interface + + interface acc_get_property_string + procedure :: acc_get_property_string_h + end interface + interface acc_async_test procedure :: acc_async_test_h end interface @@ -971,6 +1032,59 @@ function acc_get_device_num_h (d) acc_get_device_num_h = acc_get_device_num_l (d) end function +function acc_get_property_h (n, d, p) + use iso_c_binding, only: c_int + use openacc_internal, only: acc_get_property_l + use openacc_kinds + implicit none (type, external) + integer (acc_device_property) :: acc_get_property_h + integer, value :: n + integer (acc_device_kind), value :: d + integer (acc_device_property), value :: p + + integer (c_int) :: pint + + pint = int (p, c_int) + acc_get_property_h = acc_get_property_l (n, d, pint) +end function + +subroutine acc_get_property_string_h (n, d, p, s) + use iso_c_binding, only: c_char, c_int, c_ptr, c_f_pointer + use openacc_internal, only: acc_get_property_string_l + use openacc_kinds + implicit none (type, external) + integer, value :: n + integer (acc_device_kind), value :: d + integer (acc_device_property), value :: p + character (*) :: s + + integer (c_int) :: pint + type (c_ptr) :: cptr + integer :: clen + character (kind=c_char, len=1), pointer, contiguous :: sptr (:) + integer :: slen + integer :: i + + interface + function strlen (s) bind (C, name = "strlen") + use iso_c_binding, only: c_ptr, c_size_t + type (c_ptr), intent(in), value :: s + integer (c_size_t) :: strlen + end function strlen + end interface + + pint = int (p, c_int) + cptr = acc_get_property_string_l (n, d, pint) + clen = int (strlen (cptr)) + call c_f_pointer (cptr, sptr, [clen]) + + s = "" + slen = min (clen, len (s)) + do i = 1, slen + s (i:i) = sptr (i) + end do +end subroutine + function acc_async_test_h (a) use openacc_internal, only: acc_async_test_l logical acc_async_test_h diff --git a/libgomp/openacc.h b/libgomp/openacc.h index 42c861caabf7..49340b7fb6d4 100644 --- a/libgomp/openacc.h +++ b/libgomp/openacc.h @@ -59,9 +59,20 @@ typedef enum acc_device_t { _ACC_device_hwm, /* Ensure enumeration is layout compatible with int. */ _ACC_highest = __INT_MAX__, - _ACC_neg = -1 + _ACC_neg = -1, + acc_device_current = -3 } acc_device_t; +typedef enum acc_device_property_t { + /* Keep in sync with include/gomp-constants.h. */ + /* Start from 1 to catch uninitialized use. */ + acc_property_memory = 1, + acc_property_free_memory = 2, + acc_property_name = 0x10001, + acc_property_vendor = 0x10002, + acc_property_driver = 0x10003 +} acc_device_property_t; + typedef enum acc_async_t { /* Keep in sync with include/gomp-constants.h. */ acc_async_noval = -1, @@ -73,6 +84,10 @@ void acc_set_device_type (acc_device_t) __GOACC_NOTHROW; acc_device_t acc_get_device_type (void) __GOACC_NOTHROW; void acc_set_device_num (int, acc_device_t) __GOACC_NOTHROW; int acc_get_device_num (acc_device_t) __GOACC_NOTHROW; +size_t acc_get_property + (int, acc_device_t, acc_device_property_t) __GOACC_NOTHROW; +const char *acc_get_property_string + (int, acc_device_t, acc_device_property_t) __GOACC_NOTHROW; int acc_async_test (int) __GOACC_NOTHROW; int acc_async_test_all (void) __GOACC_NOTHROW; void acc_wait (int) __GOACC_NOTHROW; diff --git a/libgomp/plugin/cuda-lib.def b/libgomp/plugin/cuda-lib.def index a16badcfa9de..cd91b39b1d27 100644 --- a/libgomp/plugin/cuda-lib.def +++ b/libgomp/plugin/cuda-lib.def @@ -8,6 +8,9 @@ CUDA_ONE_CALL (cuCtxSynchronize) CUDA_ONE_CALL (cuDeviceGet) CUDA_ONE_CALL (cuDeviceGetAttribute) CUDA_ONE_CALL (cuDeviceGetCount) +CUDA_ONE_CALL (cuDeviceGetName) +CUDA_ONE_CALL (cuDeviceTotalMem) +CUDA_ONE_CALL (cuDriverGetVersion) CUDA_ONE_CALL (cuEventCreate) CUDA_ONE_CALL (cuEventDestroy) CUDA_ONE_CALL (cuEventElapsedTime) @@ -35,6 +38,7 @@ CUDA_ONE_CALL (cuMemcpyHtoDAsync) CUDA_ONE_CALL (cuMemFree) CUDA_ONE_CALL (cuMemFreeHost) CUDA_ONE_CALL (cuMemGetAddressRange) +CUDA_ONE_CALL (cuMemGetInfo) CUDA_ONE_CALL (cuMemHostGetDevicePointer) CUDA_ONE_CALL (cuModuleGetFunction) CUDA_ONE_CALL (cuModuleGetGlobal) diff --git a/libgomp/plugin/plugin-hsa.c b/libgomp/plugin/plugin-hsa.c index 409e138aaca3..491ea5769b90 100644 --- a/libgomp/plugin/plugin-hsa.c +++ b/libgomp/plugin/plugin-hsa.c @@ -699,6 +699,32 @@ GOMP_OFFLOAD_get_num_devices (void) return hsa_context.agent_count; } +/* Part of the libgomp plugin interface. Return the value of property + PROP of agent number N. */ + +union gomp_device_property_value +GOMP_OFFLOAD_get_property (int n, int prop) +{ + union gomp_device_property_value nullval = { .val = 0 }; + + if (!init_hsa_context ()) + return nullval; + if (n >= hsa_context.agent_count) + { + GOMP_PLUGIN_error + ("Request for a property of a non-existing HSA device %i", n); + return nullval; + } + + switch (prop) + { + case GOMP_DEVICE_PROPERTY_VENDOR: + return (union gomp_device_property_value) { .ptr = "AMD" }; + default: + return nullval; + } +} + /* Part of the libgomp plugin interface. Initialize agent number N so that it can be used for computation. Return TRUE on success. */ diff --git a/libgomp/plugin/plugin-nvptx.c b/libgomp/plugin/plugin-nvptx.c index 911d0f66a6e4..0e9f9ff69ba8 100644 --- a/libgomp/plugin/plugin-nvptx.c +++ b/libgomp/plugin/plugin-nvptx.c @@ -284,7 +284,7 @@ struct ptx_device bool map; bool concur; bool mkern; - int mode; + int mode; int clock_khz; int num_sms; int regs_per_block; @@ -293,6 +293,7 @@ struct ptx_device int max_threads_per_block; int max_threads_per_multiprocessor; int default_dims[GOMP_DIM_MAX]; + char* name; struct ptx_image_data *images; /* Images loaded on device. */ pthread_mutex_t image_lock; /* Lock for above list. */ @@ -304,6 +305,7 @@ struct ptx_device }; static struct ptx_device **ptx_devices; +static char cuda_driver_version[30]; static inline struct nvptx_thread * nvptx_thread (void) @@ -330,6 +332,12 @@ nvptx_init (void) CUDA_CALL (cuDeviceGetCount, &ndevs); ptx_devices = GOMP_PLUGIN_malloc_cleared (sizeof (struct ptx_device *) * ndevs); + + int v; + CUDA_CALL_ERET (NULL, cuDriverGetVersion, &v); + snprintf (cuda_driver_version, sizeof (cuda_driver_version) - 1, + "CUDA Driver %u.%u", v / 1000, v % 1000 / 10); + return true; } @@ -491,6 +499,10 @@ nvptx_open_device (int n) for (int i = 0; i != GOMP_DIM_MAX; i++) ptx_dev->default_dims[i] = 0; + const int max_name_len = 256; + ptx_dev->name = GOMP_PLUGIN_malloc(max_name_len); + CUDA_CALL_ERET (NULL, cuDeviceGetName, ptx_dev->name, max_name_len, dev); + ptx_dev->images = NULL; pthread_mutex_init (&ptx_dev->image_lock, NULL); @@ -520,6 +532,7 @@ nvptx_close_device (struct ptx_device *ptx_dev) if (!ptx_dev->ctx_shared) CUDA_CALL (cuCtxDestroy, ptx_dev->ctx); + free (ptx_dev->name); free (ptx_dev); return true; } @@ -1104,6 +1117,76 @@ GOMP_OFFLOAD_get_num_devices (void) return nvptx_get_num_devices (); } +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 (n >= nvptx_get_num_devices () || n < 0 || ptx_devices[n] == NULL) + { + pthread_mutex_unlock (&ptx_dev_lock); + return propval; + } + + struct ptx_device *ptx_dev = ptx_devices[n]; + switch (prop) + { + case GOMP_DEVICE_PROPERTY_MEMORY: + { + size_t total_mem; + + CUDA_CALL_ERET (propval, cuDeviceTotalMem, &total_mem, ptx_dev->dev); + propval.val = total_mem; + } + break; + case GOMP_DEVICE_PROPERTY_FREE_MEMORY: + { + size_t total_mem; + size_t free_mem; + CUdevice ctxdev; + + CUDA_CALL_ERET (propval, cuCtxGetDevice, &ctxdev); + if (ptx_dev->dev == ctxdev) + CUDA_CALL_ERET (propval, cuMemGetInfo, &free_mem, &total_mem); + else if (ptx_dev->ctx) + { + CUcontext old_ctx; + + CUDA_CALL_ERET (propval, cuCtxPushCurrent, ptx_dev->ctx); + CUDA_CALL_ERET (propval, cuMemGetInfo, &free_mem, &total_mem); + CUDA_CALL_ASSERT (cuCtxPopCurrent, &old_ctx); + } + else + { + CUcontext new_ctx; + + CUDA_CALL_ERET (propval, cuCtxCreate, &new_ctx, CU_CTX_SCHED_AUTO, + ptx_dev->dev); + CUDA_CALL_ERET (propval, cuMemGetInfo, &free_mem, &total_mem); + CUDA_CALL_ASSERT (cuCtxDestroy, new_ctx); + } + propval.val = free_mem; + } + break; + case GOMP_DEVICE_PROPERTY_NAME: + propval.ptr = ptx_dev->name; + break; + case GOMP_DEVICE_PROPERTY_VENDOR: + propval.ptr = "Nvidia"; + break; + case GOMP_DEVICE_PROPERTY_DRIVER: + propval.ptr = cuda_driver_version; + break; + default: + GOMP_PLUGIN_error("Unknown OpenACC device-property"); + } + + pthread_mutex_unlock (&ptx_dev_lock); + return propval; +} + bool GOMP_OFFLOAD_init_device (int n) { diff --git a/libgomp/target.c b/libgomp/target.c index 84d6daa76ca8..ffd689a9afe7 100644 --- a/libgomp/target.c +++ b/libgomp/target.c @@ -2752,6 +2752,7 @@ gomp_load_plugin_for_device (struct gomp_device_descr *device, DLSYM (get_caps); DLSYM (get_type); DLSYM (get_num_devices); + DLSYM (get_property); DLSYM (init_device); DLSYM (fini_device); DLSYM (load_image); diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property-2.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property-2.c new file mode 100644 index 000000000000..b97eef7777ee --- /dev/null +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property-2.c @@ -0,0 +1,68 @@ +/* Test the `acc_get_property' and '`acc_get_property_string' library + functions on Nvidia devices by comparing property values with + those obtained through the CUDA API. */ +/* { dg-additional-sources acc-get-property-aux.c } */ +/* { dg-additional-options "-lcuda -lcudart" } */ +/* { dg-do run { target openacc_nvidia_accel_selected } } */ + +#include <openacc.h> +#include <cuda.h> +#include <cuda_runtime_api.h> +#include <string.h> +#include <stdio.h> + +void expect_device_properties +(acc_device_t dev_type, int dev_num, + int expected_total_mem, int expected_free_mem, + const char* expected_vendor, const char* expected_name, + const char* expected_driver); + +int main () +{ + int dev_count; + cudaGetDeviceCount (&dev_count); + + for (int dev_num = 0; dev_num < dev_count; ++dev_num) + { + if (cudaSetDevice (dev_num) != cudaSuccess) + { + fprintf (stderr, "cudaSetDevice failed.\n"); + abort (); + } + + printf("Checking device %d\n", dev_num); + + const char *vendor = "Nvidia"; + size_t free_mem; + size_t total_mem; + if (cudaMemGetInfo(&free_mem, &total_mem) != cudaSuccess) + { + fprintf (stderr, "cudaMemGetInfo failed.\n"); + abort (); + } + + struct cudaDeviceProp p; + if (cudaGetDeviceProperties(&p, dev_num) != cudaSuccess) + { + fprintf (stderr, "cudaGetDeviceProperties failed.\n"); + abort (); + } + + int driver_version; + if (cudaDriverGetVersion(&driver_version) != cudaSuccess) + { + fprintf (stderr, "cudaDriverGetVersion failed.\n"); + abort (); + } + /* The version string should contain the version of the CUDA Toolkit + in the same MAJOR.MINOR format that is used by Nvidia. + The format string below is the same that is used by the deviceQuery + program, which belongs to Nvidia's CUDA samples, to print the version. */ + char *driver = malloc(sizeof(char) * 40); + snprintf (driver, 40, "CUDA Driver %u.%u", driver_version / 1000, + driver_version % 1000 / 10); + + expect_device_properties(acc_device_nvidia, dev_num, + total_mem, free_mem, vendor, p.name, driver); + } +} diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property-3.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property-3.c new file mode 100644 index 000000000000..1a8dca6193b7 --- /dev/null +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property-3.c @@ -0,0 +1,19 @@ +/* Test the `acc_get_property' and '`acc_get_property_string' library + functions for the host device. */ +/* { dg-additional-sources acc-get-property-aux.c } */ +/* { dg-do run } */ + +#include <openacc.h> +#include <stdio.h> + +void expect_device_properties +(acc_device_t dev_type, int dev_num, + int expected_total_mem, int expected_free_mem, + const char* expected_vendor, const char* expected_name, + const char* expected_driver); + +int main() +{ + printf ("Checking acc_device_host device properties\n"); + expect_device_properties (acc_device_host, 0, 0, 0, "GNU", "GOMP", "1.0"); +} diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property-aux.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property-aux.c new file mode 100644 index 000000000000..5540b91f8427 --- /dev/null +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property-aux.c @@ -0,0 +1,60 @@ +/* Auxiliary functions for acc_get_property tests */ +/* { dg-do compile { target skip-all-targets } } */ + +#include <openacc.h> +#include <stdlib.h> +#include <stdio.h> +#include <string.h> + +void expect_device_properties +(acc_device_t dev_type, int dev_num, + int expected_total_mem, int expected_free_mem, + const char* expected_vendor, const char* expected_name, + const char* expected_driver) +{ + const char *vendor = acc_get_property_string (dev_num, dev_type, + acc_property_vendor); + if (strcmp (vendor, expected_vendor)) + { + fprintf (stderr, "Expected acc_property_vendor to equal \"%s\", " + "but was \"%s\".\n", expected_vendor, vendor); + abort (); + } + + int total_mem = acc_get_property (dev_num, dev_type, + acc_property_memory); + if (total_mem != expected_total_mem) + { + fprintf (stderr, "Expected acc_property_memory to equal %d, " + "but was %d.\n", expected_total_mem, total_mem); + abort (); + + } + + int free_mem = acc_get_property (dev_num, dev_type, + acc_property_free_memory); + if (free_mem != expected_free_mem) + { + fprintf (stderr, "Expected acc_property_free_memory to equal %d, " + "but was %d.\n", expected_free_mem, free_mem); + abort (); + } + + const char *name = acc_get_property_string (dev_num, dev_type, + acc_property_name); + if (strcmp (name, expected_name)) + { + fprintf(stderr, "Expected acc_property_name to equal \"%s\", " + "but was \"%s\".\n", expected_name, name); + abort (); + } + + const char *driver = acc_get_property_string (dev_num, dev_type, + acc_property_driver); + if (strcmp (expected_driver, driver)) + { + fprintf (stderr, "Expected acc_property_driver to equal %s, " + "but was %s.\n", expected_driver, driver); + abort (); + } +} diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property.c new file mode 100644 index 000000000000..e9a86860f45d --- /dev/null +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property.c @@ -0,0 +1,75 @@ +/* Test the `acc_get_property' and '`acc_get_property_string' library + functions by printing the results of those functions for all devices + of all device types mentioned in the OpenACC standard. + + See also acc-get-property.f90. */ +/* { dg-do run } */ + +#include <openacc.h> +#include <stdlib.h> +#include <stdio.h> +#include <string.h> + +/* Print the values of the properties of all devices of the given type + and do basic device independent validation. */ + +void +print_device_properties(acc_device_t type) +{ + const char *s; + size_t v; + + int dev_count = acc_get_num_devices(type); + + for (int i = 0; i < dev_count; ++i) + { + printf(" Device %d:\n", i+1); + + s = acc_get_property_string (i, type, acc_property_vendor); + printf (" Vendor: %s\n", s); + if (s == NULL || *s == 0) + { + fprintf (stderr, "acc_property_vendor should not be null or empty.\n"); + abort (); + } + + v = acc_get_property (i, type, acc_property_memory); + printf (" Total memory: %zd\n", v); + + v = acc_get_property (i, type, acc_property_free_memory); + printf (" Free memory: %zd\n", v); + + s = acc_get_property_string (i, type, acc_property_name); + printf (" Name: %s\n", s); + if (s == NULL || *s == 0) + { + fprintf (stderr, "acc_property_name should not be null or empty.\n"); + abort (); + } + + s = acc_get_property_string (i, type, acc_property_driver); + printf (" Driver: %s\n", s); + if (s == NULL || *s == 0) + { + fprintf (stderr, "acc_property_string should not be null or empty.\n"); + abort (); + } + } +} + +int main () +{ + printf("acc_device_none:\n"); + /* For completness; not expected to print anything since there + should be no devices of this type. */ + print_device_properties(acc_device_none); + + printf("acc_device_default:\n"); + print_device_properties(acc_device_default); + + printf("acc_device_host:\n"); + print_device_properties(acc_device_host); + + printf("acc_device_not_host:\n"); + print_device_properties(acc_device_not_host); +} diff --git a/libgomp/testsuite/libgomp.oacc-fortran/acc-get-property.f90 b/libgomp/testsuite/libgomp.oacc-fortran/acc-get-property.f90 new file mode 100644 index 000000000000..71239cd0c7c2 --- /dev/null +++ b/libgomp/testsuite/libgomp.oacc-fortran/acc-get-property.f90 @@ -0,0 +1,80 @@ +! Test the `acc_get_property' and '`acc_get_property_string' library +! functions by printing the results of those functions for all devices +! of all device types mentioned in the OpenACC standard. +! +! See also acc-get-property.c +! { dg-do run } + +program test + use openacc + implicit none + + print *, "acc_device_none:" + ! For completeness; not expected to print anything + call print_device_properties (acc_device_none) + + print *, "acc_device_default:" + call print_device_properties (acc_device_default) + + print *, "acc_device_host:" + call print_device_properties (acc_device_host) + + print *, "acc_device_not_host:" + call print_device_properties (acc_device_not_host) +end program test + +! Print the values of the properties of all devices of the given type +! and do basic device independent validation. +subroutine print_device_properties (device_type) + use openacc + implicit none + + integer, intent(in) :: device_type + + integer :: device_count + integer :: device + integer(acc_device_property) :: v + character*256 :: s + + device_count = acc_get_num_devices(device_type) + + do device = 0, device_count - 1 + print "(a, i0)", " Device ", device + + call acc_get_property_string (device, device_type, acc_property_vendor, s) + print "(a, a)", " Vendor: ", trim (s) + if (s == "") then + print *, "acc_property_vendor should not be empty." + stop 1 + end if + + v = acc_get_property (device, device_type, acc_property_memory) + print "(a, i0)", " Total memory: ", v + if (v < 0) then + print *, "acc_property_memory should not be negative." + stop 1 + end if + + v = acc_get_property (device, device_type, acc_property_free_memory) + print "(a, i0)", " Free memory: ", v + if (v < 0) then + print *, "acc_property_free_memory should not to be negative." + stop 1 + end if + + call acc_get_property_string (device, device_type, acc_property_name, s) + print "(a, a)", " Name: ", trim (s) + if (s == "") then + print *, "acc_property_name should not be empty." + stop 1 + end if + + call acc_get_property_string (device, device_type, acc_property_driver, s) + print "(a, a)", " Driver: ", trim (s) + if (s == "") then + print *, "acc_property_driver should not be empty." + stop 1 + end if + + end do +end subroutine print_device_properties diff --git a/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp b/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp index d1678d0514e9..e3c60ad7f1fe 100644 --- a/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp +++ b/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp @@ -174,6 +174,28 @@ 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: + /* TODO: "error: invalid conversion from 'const void*' to 'void*' [-fpermissive]" */ + return (union gomp_device_property_value) { .ptr = (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)