Hi! On Mon, 1 Jun 2015 17:04:03 +0300, Ilya Verbin <iver...@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
signature.asc
Description: PGP signature
