On Fri, Dec 11, 2015 at 18:27:13 +0100, Jakub Jelinek wrote:
> On Tue, Dec 08, 2015 at 05:45:59PM +0300, Ilya Verbin wrote:
> > --- a/libgomp/oacc-init.c
> > +++ b/libgomp/oacc-init.c
> > @@ -306,10 +306,11 @@ acc_shutdown_1 (acc_device_t d)
> > {
> > struct gomp_device_descr *acc_dev = &base_dev[i];
> > gomp_mutex_lock (&acc_dev->lock);
> > - if (acc_dev->is_initialized)
> > + if (acc_dev->state == GOMP_DEVICE_INITIALIZED)
> > {
> > devices_active = true;
> > - gomp_fini_device (acc_dev);
> > + acc_dev->fini_device_func (acc_dev->target_id);
> > + acc_dev->state = GOMP_DEVICE_UNINITIALIZED;
> > }
> > gomp_mutex_unlock (&acc_dev->lock);
> > }
>
> I'd bet you want to set state here to GOMP_DEVICE_FINALIZED too,
> but I'd leave that to the OpenACC folks to do that incrementally
> once they test it and/or decide what to do.
libgomp/testsuite/libgomp.oacc-c-c++-common/lib-5.c contains a call to acc_init,
next acc_shutdown, and acc_init again, so I guess that OpenACC allows to
initialize the device again after acc_shutdown, but GOMP_DEVICE_FINALIZED means
that it's terminally finalized.
> > @@ -356,6 +361,11 @@ gomp_map_vars (struct gomp_device_descr *devicep,
> > size_t mapnum,
> > }
> >
> > gomp_mutex_lock (&devicep->lock);
> > + if (devicep->state == GOMP_DEVICE_FINALIZED)
> > + {
> > + gomp_mutex_unlock (&devicep->lock);
>
> You need to free (tgt); here I think to avoid leaking memory.
>
> > + return NULL;
> > + }
> >
> > for (i = 0; i < mapnum; i++)
> > {
> > @@ -834,6 +844,11 @@ gomp_unmap_vars (struct target_mem_desc *tgt, bool
> > do_copyfrom)
> > }
> >
> > gomp_mutex_lock (&devicep->lock);
> > + if (devicep->state == GOMP_DEVICE_FINALIZED)
> > + {
> > + gomp_mutex_unlock (&devicep->lock);
> > + return;
>
> Supposedly you want at least free (tgt->array); free (tgt); here.
> Plus the question is if the mappings shouldn't be removed from the splay tree
> before that.
>
> > +/* This function finalizes all initialized devices. */
> > +
> > +static void
> > +gomp_target_fini (void)
> > +{
> > + int i;
> > + for (i = 0; i < num_devices; i++)
> > + {
> > + struct gomp_device_descr *devicep = &devices[i];
> > + gomp_mutex_lock (&devicep->lock);
> > + if (devicep->state == GOMP_DEVICE_INITIALIZED)
> > + {
> > + devicep->fini_device_func (devicep->target_id);
> > + devicep->state = GOMP_DEVICE_FINALIZED;
> > + }
> > + gomp_mutex_unlock (&devicep->lock);
> > + }
> > +}
>
> The question is what will this do if there are async target tasks still
> running on some of the devices at this point (forgotten #pragma omp taskwait
> or similar if target nowait regions are started outside of parallel region,
> or exit inside of parallel, etc. But perhaps it can be handled incrementally.
> Also there is the question that the
> So I think the patch is ok with the above mentioned changes.
>
> What is the state of the link clause implementation patch? Does it depend
> on this?
It's ready, but it depends on this. I will retest and resend "link" patch after
checking-in "init/fini" patch.
-- Ilya
