Re: [PATCH][OpenMP] Fix resolve_device with -foffload=disable

[Thomas Schwinge <thomas at codesourcery dot com>]

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

Attachment: signature.asc
Description: PGP signature