| Message ID | 20150420141603.GA46200@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Apr 20, 2015 at 17:16:03 +0300, Ilya Verbin wrote: > Hi! > > Currently if a compiler is configured with enabled offloading, the 'devices' > array in libgomp is filled properly with a number of available devices. > However, if a program is compiled with -foffload=disable, the resolve_device > function returns a pointer to the device, and host-fallback is not happening. > The patch below fixes this issue. > make check-target-libgomp passed. OK for trunk? > > > libgomp/ > * libgomp.h (struct gomp_device_descr): Add num_images. > * target.c (resolve_device): Call gomp_init_device. Return NULL if > there is no image loaded to the device. > (gomp_offload_image_to_device): Increase num_images. > (GOMP_offload_unregister): Decrease num_images. > (GOMP_target): Don't call gomp_init_device. > (GOMP_target_data): Ditto. > (GOMP_target_update): Ditto. > (gomp_target_init): Set num_images to 0. > * testsuite/libgomp.c/target-1-disable.c: New test. Ping. -- Ilya
Hi, Thomas! Is this change ok for OpenACC/PTX? On Mon, Apr 20, 2015 at 17:16:03 +0300, Ilya Verbin wrote: > Currently if a compiler is configured with enabled offloading, the 'devices' > array in libgomp is filled properly with a number of available devices. > However, if a program is compiled with -foffload=disable, the resolve_device > function returns a pointer to the device, and host-fallback is not happening. > The patch below fixes this issue. > make check-target-libgomp passed. OK for trunk? > > > libgomp/ > * libgomp.h (struct gomp_device_descr): Add num_images. > * target.c (resolve_device): Call gomp_init_device. Return NULL if > there is no image loaded to the device. > (gomp_offload_image_to_device): Increase num_images. > (GOMP_offload_unregister): Decrease num_images. > (GOMP_target): Don't call gomp_init_device. > (GOMP_target_data): Ditto. > (GOMP_target_update): Ditto. > (gomp_target_init): Set num_images to 0. > * testsuite/libgomp.c/target-1-disable.c: New test. > > > diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h > index 5272f01..47a064a 100644 > --- a/libgomp/libgomp.h > +++ b/libgomp/libgomp.h > @@ -762,6 +762,9 @@ struct gomp_device_descr > /* Set to true when device is initialized. */ > bool is_initialized; > > + /* Number of images offloaded to the device. */ > + int num_images; > + > /* OpenACC-specific data and functions. */ > /* This is mutable because of its mutable data_environ and target_data > members. */ > diff --git a/libgomp/target.c b/libgomp/target.c > index d8da783..f5126b9 100644 > --- a/libgomp/target.c > +++ b/libgomp/target.c > @@ -132,6 +132,14 @@ resolve_device (int device_id) > if (device_id < 0 || device_id >= gomp_get_num_devices ()) > return NULL; > > + gomp_mutex_lock (&devices[device_id].lock); > + if (!devices[device_id].is_initialized) > + gomp_init_device (&devices[device_id]); > + gomp_mutex_unlock (&devices[device_id].lock); > + > + if (devices[device_id].num_images <= 0) > + return NULL; > + > return &devices[device_id]; > } > > @@ -697,6 +705,7 @@ gomp_offload_image_to_device (struct gomp_device_descr *devicep, > struct addr_pair *target_table = NULL; > int i, num_target_entries > = devicep->load_image_func (devicep->target_id, target_data, &target_table); > + devicep->num_images++; > > if (num_target_entries != num_funcs + num_vars) > { > @@ -831,6 +840,7 @@ GOMP_offload_unregister (void *host_table, enum offload_target_type target_type, > } > > devicep->unload_image_func (devicep->target_id, target_data); > + devicep->num_images--; > > /* Remove mapping from splay tree. */ > struct splay_tree_key_s k; > @@ -966,11 +976,6 @@ GOMP_target (int device, void (*fn) (void *), const void *unused, > return; > } > > - gomp_mutex_lock (&devicep->lock); > - if (!devicep->is_initialized) > - gomp_init_device (devicep); > - gomp_mutex_unlock (&devicep->lock); > - > void *fn_addr; > > if (devicep->capabilities & GOMP_OFFLOAD_CAP_NATIVE_EXEC) > @@ -1034,11 +1039,6 @@ GOMP_target_data (int device, const void *unused, size_t mapnum, > return; > } > > - gomp_mutex_lock (&devicep->lock); > - if (!devicep->is_initialized) > - gomp_init_device (devicep); > - gomp_mutex_unlock (&devicep->lock); > - > struct target_mem_desc *tgt > = gomp_map_vars (devicep, mapnum, hostaddrs, NULL, sizes, kinds, false, > false); > @@ -1069,11 +1069,6 @@ GOMP_target_update (int device, const void *unused, size_t mapnum, > || !(devicep->capabilities & GOMP_OFFLOAD_CAP_OPENMP_400)) > return; > > - gomp_mutex_lock (&devicep->lock); > - if (!devicep->is_initialized) > - gomp_init_device (devicep); > - gomp_mutex_unlock (&devicep->lock); > - > gomp_update (devicep, mapnum, hostaddrs, sizes, kinds, false); > } > > @@ -1265,6 +1260,7 @@ gomp_target_init (void) > current_device.type = current_device.get_type_func (); > current_device.mem_map.root = NULL; > current_device.is_initialized = false; > + current_device.num_images = 0; > current_device.openacc.data_environ = NULL; > for (i = 0; i < new_num_devices; i++) > { > diff --git a/libgomp/testsuite/libgomp.c/target-1-disable.c b/libgomp/testsuite/libgomp.c/target-1-disable.c > new file mode 100644 > index 0000000..00ea143 > --- /dev/null > +++ b/libgomp/testsuite/libgomp.c/target-1-disable.c > @@ -0,0 +1,4 @@ > +/* { dg-options "-foffload=disable" } */ > +/* { dg-require-effective-target offload_device } */ > + > +#include "target-1.c" -- Ilya
Hi! On Mon, 1 Jun 2015 17:04:03 +0300, Ilya Verbin <iverbin@gmail.com> wrote: > Is this change ok for OpenACC/PTX? Well, it doesn't cause any visible regressions in libgomp testing for OpenACC, so OK from that point of view. The code that you're changing is not actually used for OpenACC; I first though it was, until I found that there is -- rather confusingly -- a separate resolve_device function in libgomp/oacc-init.c in addition to the one you changed in libgomp/target.c... That said, OpenACC offloading exhibits the same problem, so that also needs to be fixed, but this can happen in a separate patch. > On Mon, Apr 20, 2015 at 17:16:03 +0300, Ilya Verbin wrote: > > Currently if a compiler is configured with enabled offloading, the 'devices' > > array in libgomp is filled properly with a number of available devices. > > However, if a program is compiled with -foffload=disable, the resolve_device > > function returns a pointer to the device, and host-fallback is not happening. (Heh, indeed.) > > The patch below fixes this issue. > > make check-target-libgomp passed. OK for trunk? I have not reviewed the locking requirements in detail, but wondered whether: > > --- a/libgomp/libgomp.h > > +++ b/libgomp/libgomp.h > > @@ -762,6 +762,9 @@ struct gomp_device_descr > > /* Set to true when device is initialized. */ > > bool is_initialized; > > > > + /* Number of images offloaded to the device. */ > > + int num_images; > > + > > /* OpenACC-specific data and functions. */ > > /* This is mutable because of its mutable data_environ and target_data > > members. */ ... any access to this new member also needs to be locked: > > --- a/libgomp/target.c > > +++ b/libgomp/target.c > > @@ -132,6 +132,14 @@ resolve_device (int device_id) > > if (device_id < 0 || device_id >= gomp_get_num_devices ()) > > return NULL; > > > > + gomp_mutex_lock (&devices[device_id].lock); > > + if (!devices[device_id].is_initialized) > > + gomp_init_device (&devices[device_id]); > > + gomp_mutex_unlock (&devices[device_id].lock); > > + > > + if (devices[device_id].num_images <= 0) > > + return NULL; > > + > > return &devices[device_id]; > > } > > > > @@ -697,6 +705,7 @@ gomp_offload_image_to_device (struct gomp_device_descr *devicep, > > struct addr_pair *target_table = NULL; > > int i, num_target_entries > > = devicep->load_image_func (devicep->target_id, target_data, &target_table); > > + devicep->num_images++; > > > > if (num_target_entries != num_funcs + num_vars) > > { > > @@ -831,6 +840,7 @@ GOMP_offload_unregister (void *host_table, enum offload_target_type target_type, > > } > > > > devicep->unload_image_func (devicep->target_id, target_data); > > + devicep->num_images--; > > > > /* Remove mapping from splay tree. */ > > struct splay_tree_key_s k; Also, how "defensive" should the code in libgomp be -- should asserts be added at the places where num_images is modified, to make sure that it doesn't overflow/wrap around (gomp_offload_image_to_device), or run below zero (GOMP_offload_unregister)? (Jakub?) Grüße, Thomas
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h index 5272f01..47a064a 100644 --- a/libgomp/libgomp.h +++ b/libgomp/libgomp.h @@ -762,6 +762,9 @@ struct gomp_device_descr /* Set to true when device is initialized. */ bool is_initialized; + /* Number of images offloaded to the device. */ + int num_images; + /* OpenACC-specific data and functions. */ /* This is mutable because of its mutable data_environ and target_data members. */ diff --git a/libgomp/target.c b/libgomp/target.c index d8da783..f5126b9 100644 --- a/libgomp/target.c +++ b/libgomp/target.c @@ -132,6 +132,14 @@ resolve_device (int device_id) if (device_id < 0 || device_id >= gomp_get_num_devices ()) return NULL; + gomp_mutex_lock (&devices[device_id].lock); + if (!devices[device_id].is_initialized) + gomp_init_device (&devices[device_id]); + gomp_mutex_unlock (&devices[device_id].lock); + + if (devices[device_id].num_images <= 0) + return NULL; + return &devices[device_id]; } @@ -697,6 +705,7 @@ gomp_offload_image_to_device (struct gomp_device_descr *devicep, struct addr_pair *target_table = NULL; int i, num_target_entries = devicep->load_image_func (devicep->target_id, target_data, &target_table); + devicep->num_images++; if (num_target_entries != num_funcs + num_vars) { @@ -831,6 +840,7 @@ GOMP_offload_unregister (void *host_table, enum offload_target_type target_type, } devicep->unload_image_func (devicep->target_id, target_data); + devicep->num_images--; /* Remove mapping from splay tree. */ struct splay_tree_key_s k; @@ -966,11 +976,6 @@ GOMP_target (int device, void (*fn) (void *), const void *unused, return; } - gomp_mutex_lock (&devicep->lock); - if (!devicep->is_initialized) - gomp_init_device (devicep); - gomp_mutex_unlock (&devicep->lock); - void *fn_addr; if (devicep->capabilities & GOMP_OFFLOAD_CAP_NATIVE_EXEC) @@ -1034,11 +1039,6 @@ GOMP_target_data (int device, const void *unused, size_t mapnum, return; } - gomp_mutex_lock (&devicep->lock); - if (!devicep->is_initialized) - gomp_init_device (devicep); - gomp_mutex_unlock (&devicep->lock); - struct target_mem_desc *tgt = gomp_map_vars (devicep, mapnum, hostaddrs, NULL, sizes, kinds, false, false); @@ -1069,11 +1069,6 @@ GOMP_target_update (int device, const void *unused, size_t mapnum, || !(devicep->capabilities & GOMP_OFFLOAD_CAP_OPENMP_400)) return; - gomp_mutex_lock (&devicep->lock); - if (!devicep->is_initialized) - gomp_init_device (devicep); - gomp_mutex_unlock (&devicep->lock); - gomp_update (devicep, mapnum, hostaddrs, sizes, kinds, false); } @@ -1265,6 +1260,7 @@ gomp_target_init (void) current_device.type = current_device.get_type_func (); current_device.mem_map.root = NULL; current_device.is_initialized = false; + current_device.num_images = 0; current_device.openacc.data_environ = NULL; for (i = 0; i < new_num_devices; i++) { diff --git a/libgomp/testsuite/libgomp.c/target-1-disable.c b/libgomp/testsuite/libgomp.c/target-1-disable.c new file mode 100644 index 0000000..00ea143 --- /dev/null +++ b/libgomp/testsuite/libgomp.c/target-1-disable.c @@ -0,0 +1,4 @@ +/* { dg-options "-foffload=disable" } */ +/* { dg-require-effective-target offload_device } */ + +#include "target-1.c"