This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [gomp4.5] Handle #pragma omp declare target link
- From: Jakub Jelinek <jakub at redhat dot com>
- To: Ilya Verbin <iverbin at gmail dot com>, Thomas Schwinge <thomas at codesourcery dot com>
- Cc: gcc-patches at gcc dot gnu dot org, Kirill Yukhin <kirill dot yukhin at gmail dot com>, Thomas Schwinge <thomas at codesourcery dot com>
- Date: Fri, 11 Dec 2015 18:27:13 +0100
- Subject: Re: [gomp4.5] Handle #pragma omp declare target link
- Authentication-results: sourceware.org; auth=none
- References: <20151130120459 dot GB5675 at tucnak dot redhat dot com> <20151130202934 dot GA22962 at msticlxl57 dot ims dot intel dot com> <20151130204902 dot GJ5675 at tucnak dot redhat dot com> <20151130205520 dot GB22962 at msticlxl57 dot ims dot intel dot com> <20151201081800 dot GN5675 at tucnak dot redhat dot com> <C5AE729F-0BA3-4933-91E1-E4729107798B at gmail dot com> <20151201131559 dot GU5675 at tucnak dot redhat dot com> <20151201172927 dot GA7692 at msticlxl57 dot ims dot intel dot com> <20151201190504 dot GY5675 at tucnak dot redhat dot com> <20151208144559 dot GB14238 at msticlxl57 dot ims dot intel dot com>
- Reply-to: Jakub Jelinek <jakub at redhat dot com>
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.
> @@ -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?
Jakub