* [PATCH][OpenMP] Fix resolve_device with -foffload=disable
@ 2015-04-20 14:16 Ilya Verbin
2015-04-27 16:38 ` Ilya Verbin
2015-06-01 14:04 ` Ilya Verbin
0 siblings, 2 replies; 4+ messages in thread
From: Ilya Verbin @ 2015-04-20 14:16 UTC (permalink / raw)
To: Jakub Jelinek, Thomas Schwinge; +Cc: gcc-patches, Kirill Yukhin, Julian Brown
Hi!
Currently if a compiler is configured with enabled offloading, the 'devices'
array in libgomp is filled properly with a number of available devices.
However, if a program is compiled with -foffload=disable, the resolve_device
function returns a pointer to the device, and host-fallback is not happening.
The patch below fixes this issue.
make check-target-libgomp passed. OK for trunk?
libgomp/
* libgomp.h (struct gomp_device_descr): Add num_images.
* target.c (resolve_device): Call gomp_init_device. Return NULL if
there is no image loaded to the device.
(gomp_offload_image_to_device): Increase num_images.
(GOMP_offload_unregister): Decrease num_images.
(GOMP_target): Don't call gomp_init_device.
(GOMP_target_data): Ditto.
(GOMP_target_update): Ditto.
(gomp_target_init): Set num_images to 0.
* testsuite/libgomp.c/target-1-disable.c: New test.
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index 5272f01..47a064a 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -762,6 +762,9 @@ struct gomp_device_descr
/* Set to true when device is initialized. */
bool is_initialized;
+ /* Number of images offloaded to the device. */
+ int num_images;
+
/* OpenACC-specific data and functions. */
/* This is mutable because of its mutable data_environ and target_data
members. */
diff --git a/libgomp/target.c b/libgomp/target.c
index d8da783..f5126b9 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -132,6 +132,14 @@ resolve_device (int device_id)
if (device_id < 0 || device_id >= gomp_get_num_devices ())
return NULL;
+ gomp_mutex_lock (&devices[device_id].lock);
+ if (!devices[device_id].is_initialized)
+ gomp_init_device (&devices[device_id]);
+ gomp_mutex_unlock (&devices[device_id].lock);
+
+ if (devices[device_id].num_images <= 0)
+ return NULL;
+
return &devices[device_id];
}
@@ -697,6 +705,7 @@ gomp_offload_image_to_device (struct gomp_device_descr *devicep,
struct addr_pair *target_table = NULL;
int i, num_target_entries
= devicep->load_image_func (devicep->target_id, target_data, &target_table);
+ devicep->num_images++;
if (num_target_entries != num_funcs + num_vars)
{
@@ -831,6 +840,7 @@ GOMP_offload_unregister (void *host_table, enum offload_target_type target_type,
}
devicep->unload_image_func (devicep->target_id, target_data);
+ devicep->num_images--;
/* Remove mapping from splay tree. */
struct splay_tree_key_s k;
@@ -966,11 +976,6 @@ GOMP_target (int device, void (*fn) (void *), const void *unused,
return;
}
- gomp_mutex_lock (&devicep->lock);
- if (!devicep->is_initialized)
- gomp_init_device (devicep);
- gomp_mutex_unlock (&devicep->lock);
-
void *fn_addr;
if (devicep->capabilities & GOMP_OFFLOAD_CAP_NATIVE_EXEC)
@@ -1034,11 +1039,6 @@ GOMP_target_data (int device, const void *unused, size_t mapnum,
return;
}
- gomp_mutex_lock (&devicep->lock);
- if (!devicep->is_initialized)
- gomp_init_device (devicep);
- gomp_mutex_unlock (&devicep->lock);
-
struct target_mem_desc *tgt
= gomp_map_vars (devicep, mapnum, hostaddrs, NULL, sizes, kinds, false,
false);
@@ -1069,11 +1069,6 @@ GOMP_target_update (int device, const void *unused, size_t mapnum,
|| !(devicep->capabilities & GOMP_OFFLOAD_CAP_OPENMP_400))
return;
- gomp_mutex_lock (&devicep->lock);
- if (!devicep->is_initialized)
- gomp_init_device (devicep);
- gomp_mutex_unlock (&devicep->lock);
-
gomp_update (devicep, mapnum, hostaddrs, sizes, kinds, false);
}
@@ -1265,6 +1260,7 @@ gomp_target_init (void)
current_device.type = current_device.get_type_func ();
current_device.mem_map.root = NULL;
current_device.is_initialized = false;
+ current_device.num_images = 0;
current_device.openacc.data_environ = NULL;
for (i = 0; i < new_num_devices; i++)
{
diff --git a/libgomp/testsuite/libgomp.c/target-1-disable.c b/libgomp/testsuite/libgomp.c/target-1-disable.c
new file mode 100644
index 0000000..00ea143
--- /dev/null
+++ b/libgomp/testsuite/libgomp.c/target-1-disable.c
@@ -0,0 +1,4 @@
+/* { dg-options "-foffload=disable" } */
+/* { dg-require-effective-target offload_device } */
+
+#include "target-1.c"
-- Ilya
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][OpenMP] Fix resolve_device with -foffload=disable
2015-04-20 14:16 [PATCH][OpenMP] Fix resolve_device with -foffload=disable Ilya Verbin
@ 2015-04-27 16:38 ` Ilya Verbin
2015-06-01 14:04 ` Ilya Verbin
1 sibling, 0 replies; 4+ messages in thread
From: Ilya Verbin @ 2015-04-27 16:38 UTC (permalink / raw)
To: Jakub Jelinek, Thomas Schwinge; +Cc: gcc-patches, Kirill Yukhin, Julian Brown
On Mon, Apr 20, 2015 at 17:16:03 +0300, Ilya Verbin wrote:
> Hi!
>
> Currently if a compiler is configured with enabled offloading, the 'devices'
> array in libgomp is filled properly with a number of available devices.
> However, if a program is compiled with -foffload=disable, the resolve_device
> function returns a pointer to the device, and host-fallback is not happening.
> The patch below fixes this issue.
> make check-target-libgomp passed. OK for trunk?
>
>
> libgomp/
> * libgomp.h (struct gomp_device_descr): Add num_images.
> * target.c (resolve_device): Call gomp_init_device. Return NULL if
> there is no image loaded to the device.
> (gomp_offload_image_to_device): Increase num_images.
> (GOMP_offload_unregister): Decrease num_images.
> (GOMP_target): Don't call gomp_init_device.
> (GOMP_target_data): Ditto.
> (GOMP_target_update): Ditto.
> (gomp_target_init): Set num_images to 0.
> * testsuite/libgomp.c/target-1-disable.c: New test.
Ping.
-- Ilya
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][OpenMP] Fix resolve_device with -foffload=disable
2015-04-20 14:16 [PATCH][OpenMP] Fix resolve_device with -foffload=disable Ilya Verbin
2015-04-27 16:38 ` Ilya Verbin
@ 2015-06-01 14:04 ` Ilya Verbin
2015-06-17 11:04 ` Thomas Schwinge
1 sibling, 1 reply; 4+ messages in thread
From: Ilya Verbin @ 2015-06-01 14:04 UTC (permalink / raw)
To: Thomas Schwinge; +Cc: gcc-patches, Kirill Yukhin, Julian Brown, Jakub Jelinek
Hi, Thomas!
Is this change ok for OpenACC/PTX?
On Mon, Apr 20, 2015 at 17:16:03 +0300, Ilya Verbin wrote:
> Currently if a compiler is configured with enabled offloading, the 'devices'
> array in libgomp is filled properly with a number of available devices.
> However, if a program is compiled with -foffload=disable, the resolve_device
> function returns a pointer to the device, and host-fallback is not happening.
> The patch below fixes this issue.
> make check-target-libgomp passed. OK for trunk?
>
>
> libgomp/
> * libgomp.h (struct gomp_device_descr): Add num_images.
> * target.c (resolve_device): Call gomp_init_device. Return NULL if
> there is no image loaded to the device.
> (gomp_offload_image_to_device): Increase num_images.
> (GOMP_offload_unregister): Decrease num_images.
> (GOMP_target): Don't call gomp_init_device.
> (GOMP_target_data): Ditto.
> (GOMP_target_update): Ditto.
> (gomp_target_init): Set num_images to 0.
> * testsuite/libgomp.c/target-1-disable.c: New test.
>
>
> diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
> index 5272f01..47a064a 100644
> --- a/libgomp/libgomp.h
> +++ b/libgomp/libgomp.h
> @@ -762,6 +762,9 @@ struct gomp_device_descr
> /* Set to true when device is initialized. */
> bool is_initialized;
>
> + /* Number of images offloaded to the device. */
> + int num_images;
> +
> /* OpenACC-specific data and functions. */
> /* This is mutable because of its mutable data_environ and target_data
> members. */
> diff --git a/libgomp/target.c b/libgomp/target.c
> index d8da783..f5126b9 100644
> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -132,6 +132,14 @@ resolve_device (int device_id)
> if (device_id < 0 || device_id >= gomp_get_num_devices ())
> return NULL;
>
> + gomp_mutex_lock (&devices[device_id].lock);
> + if (!devices[device_id].is_initialized)
> + gomp_init_device (&devices[device_id]);
> + gomp_mutex_unlock (&devices[device_id].lock);
> +
> + if (devices[device_id].num_images <= 0)
> + return NULL;
> +
> return &devices[device_id];
> }
>
> @@ -697,6 +705,7 @@ gomp_offload_image_to_device (struct gomp_device_descr *devicep,
> struct addr_pair *target_table = NULL;
> int i, num_target_entries
> = devicep->load_image_func (devicep->target_id, target_data, &target_table);
> + devicep->num_images++;
>
> if (num_target_entries != num_funcs + num_vars)
> {
> @@ -831,6 +840,7 @@ GOMP_offload_unregister (void *host_table, enum offload_target_type target_type,
> }
>
> devicep->unload_image_func (devicep->target_id, target_data);
> + devicep->num_images--;
>
> /* Remove mapping from splay tree. */
> struct splay_tree_key_s k;
> @@ -966,11 +976,6 @@ GOMP_target (int device, void (*fn) (void *), const void *unused,
> return;
> }
>
> - gomp_mutex_lock (&devicep->lock);
> - if (!devicep->is_initialized)
> - gomp_init_device (devicep);
> - gomp_mutex_unlock (&devicep->lock);
> -
> void *fn_addr;
>
> if (devicep->capabilities & GOMP_OFFLOAD_CAP_NATIVE_EXEC)
> @@ -1034,11 +1039,6 @@ GOMP_target_data (int device, const void *unused, size_t mapnum,
> return;
> }
>
> - gomp_mutex_lock (&devicep->lock);
> - if (!devicep->is_initialized)
> - gomp_init_device (devicep);
> - gomp_mutex_unlock (&devicep->lock);
> -
> struct target_mem_desc *tgt
> = gomp_map_vars (devicep, mapnum, hostaddrs, NULL, sizes, kinds, false,
> false);
> @@ -1069,11 +1069,6 @@ GOMP_target_update (int device, const void *unused, size_t mapnum,
> || !(devicep->capabilities & GOMP_OFFLOAD_CAP_OPENMP_400))
> return;
>
> - gomp_mutex_lock (&devicep->lock);
> - if (!devicep->is_initialized)
> - gomp_init_device (devicep);
> - gomp_mutex_unlock (&devicep->lock);
> -
> gomp_update (devicep, mapnum, hostaddrs, sizes, kinds, false);
> }
>
> @@ -1265,6 +1260,7 @@ gomp_target_init (void)
> current_device.type = current_device.get_type_func ();
> current_device.mem_map.root = NULL;
> current_device.is_initialized = false;
> + current_device.num_images = 0;
> current_device.openacc.data_environ = NULL;
> for (i = 0; i < new_num_devices; i++)
> {
> diff --git a/libgomp/testsuite/libgomp.c/target-1-disable.c b/libgomp/testsuite/libgomp.c/target-1-disable.c
> new file mode 100644
> index 0000000..00ea143
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.c/target-1-disable.c
> @@ -0,0 +1,4 @@
> +/* { dg-options "-foffload=disable" } */
> +/* { dg-require-effective-target offload_device } */
> +
> +#include "target-1.c"
-- Ilya
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][OpenMP] Fix resolve_device with -foffload=disable
2015-06-01 14:04 ` Ilya Verbin
@ 2015-06-17 11:04 ` Thomas Schwinge
0 siblings, 0 replies; 4+ messages in thread
From: Thomas Schwinge @ 2015-06-17 11:04 UTC (permalink / raw)
To: Ilya Verbin, Jakub Jelinek; +Cc: gcc-patches, Kirill Yukhin, Julian Brown
[-- Attachment #1: Type: text/plain, Size: 3252 bytes --]
Hi!
On Mon, 1 Jun 2015 17:04:03 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
> Is this change ok for OpenACC/PTX?
Well, it doesn't cause any visible regressions in libgomp testing for
OpenACC, so OK from that point of view.
The code that you're changing is not actually used for OpenACC; I first
though it was, until I found that there is -- rather confusingly -- a
separate resolve_device function in libgomp/oacc-init.c in addition to
the one you changed in libgomp/target.c...
That said, OpenACC offloading exhibits the same problem, so that also
needs to be fixed, but this can happen in a separate patch.
> On Mon, Apr 20, 2015 at 17:16:03 +0300, Ilya Verbin wrote:
> > Currently if a compiler is configured with enabled offloading, the 'devices'
> > array in libgomp is filled properly with a number of available devices.
> > However, if a program is compiled with -foffload=disable, the resolve_device
> > function returns a pointer to the device, and host-fallback is not happening.
(Heh, indeed.)
> > The patch below fixes this issue.
> > make check-target-libgomp passed. OK for trunk?
I have not reviewed the locking requirements in detail, but wondered
whether:
> > --- a/libgomp/libgomp.h
> > +++ b/libgomp/libgomp.h
> > @@ -762,6 +762,9 @@ struct gomp_device_descr
> > /* Set to true when device is initialized. */
> > bool is_initialized;
> >
> > + /* Number of images offloaded to the device. */
> > + int num_images;
> > +
> > /* OpenACC-specific data and functions. */
> > /* This is mutable because of its mutable data_environ and target_data
> > members. */
... any access to this new member also needs to be locked:
> > --- a/libgomp/target.c
> > +++ b/libgomp/target.c
> > @@ -132,6 +132,14 @@ resolve_device (int device_id)
> > if (device_id < 0 || device_id >= gomp_get_num_devices ())
> > return NULL;
> >
> > + gomp_mutex_lock (&devices[device_id].lock);
> > + if (!devices[device_id].is_initialized)
> > + gomp_init_device (&devices[device_id]);
> > + gomp_mutex_unlock (&devices[device_id].lock);
> > +
> > + if (devices[device_id].num_images <= 0)
> > + return NULL;
> > +
> > return &devices[device_id];
> > }
> >
> > @@ -697,6 +705,7 @@ gomp_offload_image_to_device (struct gomp_device_descr *devicep,
> > struct addr_pair *target_table = NULL;
> > int i, num_target_entries
> > = devicep->load_image_func (devicep->target_id, target_data, &target_table);
> > + devicep->num_images++;
> >
> > if (num_target_entries != num_funcs + num_vars)
> > {
> > @@ -831,6 +840,7 @@ GOMP_offload_unregister (void *host_table, enum offload_target_type target_type,
> > }
> >
> > devicep->unload_image_func (devicep->target_id, target_data);
> > + devicep->num_images--;
> >
> > /* Remove mapping from splay tree. */
> > struct splay_tree_key_s k;
Also, how "defensive" should the code in libgomp be -- should asserts be
added at the places where num_images is modified, to make sure that it
doesn't overflow/wrap around (gomp_offload_image_to_device), or run below
zero (GOMP_offload_unregister)? (Jakub?)
Grüße,
Thomas
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-06-17 11:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-20 14:16 [PATCH][OpenMP] Fix resolve_device with -foffload=disable Ilya Verbin
2015-04-27 16:38 ` Ilya Verbin
2015-06-01 14:04 ` Ilya Verbin
2015-06-17 11:04 ` Thomas Schwinge
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).