Re: [Patch] libgomp: Handle OpenMP's reverse offloads

[Tobias Burnus <tobias@codesourcery.com>]

Hi,

I have not fully tried to understand it, yet.

(A) Regarding the issue of stalling, see als Andrew's patch and the
discussion about it in

"[PATCH] libgomp: fix hang on fatal error",
https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603616.html

and in particular Jakub's two replies.

(b) I think you want to remove this:

On 15.12.22 18:34, Thomas Schwinge wrote:
> --- a/libgomp/plugin/plugin-nvptx.c
> +++ b/libgomp/plugin/plugin-nvptx.c
> @@ -1,3 +1,5 @@
> +#pragma GCC optimize "O0"
> +
>   /* Plugin for NVPTX execution.

(c)

> If the libgomp plugin doesn't request special
> 'host_to_dev_cpy'/'dev_to_host_cpy' for 'gomp_target_rev', then standard
> 'gomp_copy_host2dev'/'gomp_copy_dev2host' are used, which use
> 'gomp_device_copy', which expects the device to be locked.  (As can be
> told by the unconditional 'gomp_mutex_unlock (&devicep->lock);' before
> 'gomp_fatal'.)  However, in a number of the
> 'gomp_copy_host2dev'/'gomp_copy_dev2host' calls from 'gomp_target_rev',
> the device definitely is not locked; see the calls adjacent to the TODO

The question is what unlocks the device – it is surely locked in gomp_target_rev by:

   if (!(devicep->capabilities & GOMP_OFFLOAD_CAP_SHARED_MEM))
     ...
       gomp_mutex_lock (&devicep->lock);
       for (uint64_t i = 0; i < mapnum; i++)
...
         }
       gomp_mutex_unlock (&devicep->lock);
     }

Except for code like:
                 gomp_mutex_unlock (&devicep->lock);
                 gomp_fatal ("gomp_target_rev unhandled kind 0x%.4x", kinds[i]);

The only functions that know about the pointer and get called are those behind
the dev_to_host_cpy and host_to_dev_cpy - thus, they seemingly mess about with the
unlocking?!?

  * * *

Regarding your patch, I do not understand why you call twice unlock and
have trice TODO unlock; that does not seem to make any sense.

I think it is worthwhile to understand why plugin-nvptx.c unlocks the lock in
the non-error case - as you observe that it is not locked in the error case.

Additionally, it seems to make more sense to look into a revised patch of
Andrew's patch, your patch looks like a rather bad band aid.

Tobias

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955