On Wed, 6 May 2015 10:32:56 +0200
Thomas Schwinge <thomas@codesourcery.com> wrote:

> Hi!
> 
> On Fri, 1 May 2015 10:47:19 +0100, Julian Brown
> <julian@codesourcery.com> wrote:
> > The patch also fixes a thinko that was revealed in image unloading
> > in the NVPTX backend. Tested for libgomp with PTX offloading.
> 
> Confirming that both nvptx (PR65904 fixed) and (emulated) intelmic (no
> changes) testing look fine.

Thanks for testing!

> By the way, do we need to lock ptx_devices in
> libgomp/plugin/plugin-nvptx.c:nvptx_attach_host_thread_to_device and
> libgomp/plugin/plugin-nvptx.c:GOMP_OFFLOAD_openacc_create_thread_data?
> (ptx_dev_lock?  If yes, its definition as well as instantiated_devices
> should be moved to a more appropriate place, probably?)

Probably yes (though I'm not sure what you mean about moving the
instantiated_devices and ptx_dev_lock to a more appropriate place?).

> Also, several
> accesses to instantiated_devices are not locked by ptx_dev_lock but
> should be, from my cursory review.

I'm not sure about that.

> > --- a/libgomp/target.c
> > +++ b/libgomp/target.c
> > @@ -797,32 +797,79 @@ GOMP_offload_register (void *host_table, enum
> > offload_target_type target_type, gomp_mutex_unlock (&register_lock);
> >  }
> >  
> > -/* This function should be called from every offload image while
> > unloading.
> > -   It gets the descriptor of the host func and var tables
> > HOST_TABLE, TYPE of
> > -   the target, and TARGET_DATA needed by target plugin.  */
> > +/* DEVICEP should be locked on entry, and remains locked on exit.
> > */
> 
> (I'm not a native speaker, but would use what I consider to be more
> explicit/precise language: »must be locked« instead of »should be«.
> I'll be happy to learn should they mean the same thing?)

I've changed the wording in a couple of comments.

> >  
> > -void
> > -GOMP_offload_unregister (void *host_table, enum
> > offload_target_type target_type,
> > -			 void *target_data)
> > +static void
> > +gomp_deoffload_image_from_device (struct gomp_device_descr
> > *devicep,
> > +				  void *host_table, void
> > *target_data) {
> 
> > +/* This function should be called from every offload image while
> > unloading.
> 
> s%from%for%, I think?  (And, s%should%must%, again?)

No, this really is "from" -- this comment wasn't actually added by my
patch, just moved. I'm also not sure about "should" in this instance --
unloading an image is already a corner-case, and maybe there are
circumstances in which it'd be impossible for some given object to call
the function?

> > +   It gets the descriptor of the host func and var tables
> > HOST_TABLE, TYPE of
> > +   the target, and TARGET_DATA needed by target plugin.  */
> > +
> > +void
> > +GOMP_offload_unregister (void *host_table, enum
> > offload_target_type target_type,
> > +			 void *target_data)
> > +{
> 
> > -/* Free address mapping tables.  MM must be locked on entry, and
> > remains locked
> > -   on return.  */
> > +/* Free address mapping tables for an active device DEVICEP.  This
> > includes
> > +   both mapped offload functions/variables, and mapped user data
> > regions.
> > +   To be used before shutting a device down: subsequently
> > reinitialising the
> > +   device will repopulate the offload image mappings.  */
> >  
> >  attribute_hidden void
> > -gomp_free_memmap (struct splay_tree_s *mem_map)
> > +gomp_free_memmap (struct gomp_device_descr *devicep)
> >  {
> > +  int i;
> > +  struct splay_tree_s *mem_map = &devicep->mem_map;
> > +
> > +  assert (devicep->is_initialized);
> > +
> > +  gomp_mutex_lock (&devicep->lock);
> 
> Need to lock before first access to *devicep?

Fixed.

> > +  
> > +  /* Unmap offload images that are registered to this device.  */
> > +  for (i = 0; i < num_offload_images; i++)
> > +    {
> > +      struct offload_image_descr *image = &offload_images[i];
> 
> Need to take register_lock when accessing offload_images?

This too. Retested for libgomp/NVPTX.

OK for trunk now?

Thanks,

Julian

ChangeLog

    PR libgomp/65904

    libgomp/
    * libgomp.h (gomp_free_memmap): Update prototype.
    * oacc-init.c (acc_shutdown_1): Pass device descriptor to
    gomp_free_memmap. Don't lock device around call.
    * target.c (gomp_map_vars): Initialise tgt->array to NULL before
    early exit.
    (GOMP_offload_unregister): Split out and call...
    (gomp_deoffload_image_from_device): This new function.
    (gomp_free_memmap): Call gomp_deoffload_image_from_device.
    * plugin/nvptx.c (struct ptx_image_data): Add ord, fn_descs fields.
    (nvptx_init): Tweak comment.
    (nvptx_attach_host_thread_to_device): Add locking with ptx_dev_lock
    around ptx_devices accesses.
    (GOMP_OFFLOAD_load_image): Populate new ptx_image_data fields.
    (GOMP_OFFLOAD_unload_image): Switch to ORD'th device before freeing
    images, and use fn_descs field from ptx_image_data instead of
    incorrectly using a pointer derived from target_data.
    (GOMP_OFFLOAD_openacc_create_thread_data): Add locking around
    ptx_devices accesses.