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
