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

[Jakub Jelinek <jakub@redhat.com>]

On Thu, Dec 15, 2022 at 06:34:30PM +0100, Thomas Schwinge wrote:
> --- a/libgomp/libgomp-plugin.c
> +++ b/libgomp/libgomp-plugin.c
> @@ -82,9 +82,9 @@ GOMP_PLUGIN_fatal (const char *msg, ...)
>  void
>  GOMP_PLUGIN_target_rev (uint64_t fn_ptr, uint64_t mapnum, uint64_t devaddrs_ptr,
>  			uint64_t sizes_ptr, uint64_t kinds_ptr, int dev_num,
> -			void (*dev_to_host_cpy) (void *, const void *, size_t,
> +			bool (*dev_to_host_cpy) (void *, const void *, size_t,
>  						 void *),
> -			void (*host_to_dev_cpy) (void *, const void *, size_t,
> +			bool (*host_to_dev_cpy) (void *, const void *, size_t,
>  						 void *), void *token)
>  {
>    gomp_target_rev (fn_ptr, mapnum, devaddrs_ptr, sizes_ptr, kinds_ptr, dev_num,
> diff --git a/libgomp/libgomp-plugin.h b/libgomp/libgomp-plugin.h
> index ac3878289506..fb533164bf9b 100644
> --- a/libgomp/libgomp-plugin.h
> +++ b/libgomp/libgomp-plugin.h
> @@ -122,9 +122,9 @@ extern void GOMP_PLUGIN_fatal (const char *, ...)
>  
>  extern void GOMP_PLUGIN_target_rev (uint64_t, uint64_t, uint64_t, uint64_t,
>  				    uint64_t, int,
> -				    void (*) (void *, const void *, size_t,
> +				    bool (*) (void *, const void *, size_t,
>  					      void *),
> -				    void (*) (void *, const void *, size_t,
> +				    bool (*) (void *, const void *, size_t,
>  					      void *), void *);
>  
>  /* Prototypes for functions implemented by libgomp plugins.  */
> --- a/libgomp/libgomp.h
> +++ b/libgomp/libgomp.h
> @@ -1130,8 +1130,8 @@ extern int gomp_get_num_devices (void);
>  extern bool gomp_target_task_fn (void *);
>  extern void gomp_target_rev (uint64_t, uint64_t, uint64_t, uint64_t, uint64_t,
>  			     int,
> -			     void (*) (void *, const void *, size_t, void *),
> -			     void (*) (void *, const void *, size_t, void *),
> +			     bool (*) (void *, const void *, size_t, void *),
> +			     bool (*) (void *, const void *, size_t, void *),
>  			     void *);
>  
>  /* Splay tree definitions.  */

I think returning bool from those is fine.

> --- a/libgomp/plugin/plugin-nvptx.c
> +++ b/libgomp/plugin/plugin-nvptx.c
> @@ -1,3 +1,5 @@
> +#pragma GCC optimize "O0"

But the pragmas are not.

> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -1,3 +1,5 @@
> +#pragma GCC optimize "O0"

Neither here.

> @@ -3340,12 +3342,21 @@ gomp_target_rev (uint64_t fn_ptr, uint64_t mapnum, uint64_t devaddrs_ptr,
>        kinds = (unsigned short *) gomp_malloc (mapnum * sizeof (unsigned short));
>        if (dev_to_host_cpy)
>         {
> -         dev_to_host_cpy (devaddrs, (const void *) (uintptr_t) devaddrs_ptr,
> -                          mapnum * sizeof (uint64_t), token);
> -         dev_to_host_cpy (sizes, (const void *) (uintptr_t) sizes_ptr,
> -                          mapnum * sizeof (uint64_t), token);
> -         dev_to_host_cpy (kinds, (const void *) (uintptr_t) kinds_ptr,
> -                          mapnum * sizeof (unsigned short), token);
> +         bool ok = true;
> +         ok = ok && dev_to_host_cpy (devaddrs,
> +                                     (const void *) (uintptr_t) devaddrs_ptr,
> +                                     mapnum * sizeof (uint64_t), token);
> +         ok = ok && dev_to_host_cpy (sizes,
> +                                     (const void *) (uintptr_t) sizes_ptr,
> +                                     mapnum * sizeof (uint64_t), token);
> +         ok = ok && dev_to_host_cpy (kinds,
> +                                     (const void *) (uintptr_t) kinds_ptr,
> +                                     mapnum * sizeof (unsigned short), token);

Why not just
	if (!dev_to_host_cpy (...)
	    || !dev_to_host_cpy (...)
	    || !dev_to_host_cpy (...))
?
	
> +         if (!ok)
> +           {
> +             /*TODO gomp_mutex_unlock (&devicep->lock); */

Why the comment?  That makes no sense, devicep->lock isn't locked here.

>  	    else if (dev_to_host_cpy)
> -	      dev_to_host_cpy (tgt + tgt_size, (void *) (uintptr_t) devaddrs[i],
> -			       (size_t) sizes[i], token);
> +	      {
> +		if (!dev_to_host_cpy (tgt + tgt_size,
> +				      (void *) (uintptr_t) devaddrs[i],
> +				      (size_t) sizes[i], token))
> +		  {
> +		    /*TODO gomp_mutex_unlock (&devicep->lock); */

Neither here.
> @@ -3662,9 +3692,15 @@ gomp_target_rev (uint64_t fn_ptr, uint64_t mapnum, uint64_t devaddrs_ptr,
>  	      case GOMP_MAP_FROM:
>  	      case GOMP_MAP_TOFROM:
>  		if (copy && host_to_dev_cpy)
> -		  host_to_dev_cpy ((void *) (uintptr_t) cdata[i].devaddr,
> -				   (void *) (uintptr_t) devaddrs[i],
> -				   sizes[i], token);
> +		  {
> +		    if (!host_to_dev_cpy ((void *) (uintptr_t) cdata[i].devaddr,
> +					  (void *) (uintptr_t) devaddrs[i],
> +					  sizes[i], token))
> +		      {
> +			/*TODO gomp_mutex_unlock (&devicep->lock); */

And neither here.
> +			exit (EXIT_FAILURE);
> +		      }
> +		  }

	Jakub