diff mbox series

[committed] libgomp: Fix OMP_TARGET_OFFLOAD=mandatory

Message ID 91bb9136-f8a4-e516-3f42-ed6d66dc8ce0@codesourcery.com
State New
Headers show
Series [committed] libgomp: Fix OMP_TARGET_OFFLOAD=mandatory | expand

Commit Message

Tobias Burnus June 16, 2023, 3:57 p.m. UTC
Found an order problem caused by my r14-1801-g18c8b56c7d67a9 due to
ordering issues related to the offloading initialization
(gomp_init_targets_once).

The testsuite did test various ways but only code such paths that
initialized the library before ...

Committed as Rev. r14-1893-g8216ca85037be9.

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

Comments

Thomas Schwinge June 16, 2023, 8:42 p.m. UTC | #1
Hi Tobias!

On 2023-06-16T17:57:10+0200, Tobias Burnus <tobias@codesourcery.com> wrote:
> Found an order problem caused by my r14-1801-g18c8b56c7d67a9 due to
> ordering issues related to the offloading initialization
> (gomp_init_targets_once).
>
> The testsuite did test various ways but only code such paths that
> initialized the library before ...
>
> Committed as Rev. r14-1893-g8216ca85037be9.

> commit 8216ca85037be9f4d5c20540522a22a4a93b660e
> Author: Tobias Burnus <tobias@codesourcery.com>
> Date:   Fri Jun 16 17:21:59 2023 +0200
>
>     libgomp: Fix OMP_TARGET_OFFLOAD=mandatory
>
>     It turned out that gomp_init_targets_once() was not run when directly
>     calling 'omp target' or 'omp target (enter/exit) data' causing an
>     abort with OMP_TARGET_OFFLOAD=mandatory wrongly claiming that no
>     device is available. It was called a tiny bit later but few lines too
>     late for updating the default-device-var.
>
>     libgomp/ChangeLog:
>
>             * target.c (resolve_device): Call gomp_get_num_devices early to ensure
>             gomp_init_targets_once was called before using default-device-var.

> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -138,6 +138,10 @@ gomp_get_num_devices (void)
>  static struct gomp_device_descr *
>  resolve_device (int device_id, bool remapped)
>  {
> +  /* Get number of devices and thus ensure that 'gomp_init_targets_once' was
> +     called, which must be done before using default_device_var.  */
> +  int num_devices = gomp_get_num_devices ();
> +
>    if (remapped && device_id == GOMP_DEVICE_ICV)
>      {
>        struct gomp_task_icv *icv = gomp_icv (false);
> @@ -151,7 +155,7 @@ resolve_device (int device_id, bool remapped)
>                                : omp_initial_device))
>       return NULL;
>        if (gomp_target_offload_var == GOMP_TARGET_OFFLOAD_MANDATORY
> -       && gomp_get_num_devices () == 0)
> +       && num_devices == 0)
>       gomp_fatal ("OMP_TARGET_OFFLOAD is set to MANDATORY, "
>                   "but only the host device is available");
>        else if (device_id == omp_invalid_device)
> @@ -162,10 +166,10 @@ resolve_device (int device_id, bool remapped)
>
>        return NULL;
>      }
> -  else if (device_id >= gomp_get_num_devices ())
> +  else if (device_id >= num_devices)
>      {
>        if (gomp_target_offload_var == GOMP_TARGET_OFFLOAD_MANDATORY
> -       && device_id != num_devices_openmp)
> +       && device_id != num_devices)
>       gomp_fatal ("OMP_TARGET_OFFLOAD is set to MANDATORY, "
>                   "but device not found");

I see the new tests PASS, but with offloading enabled (nvptx) also see:

    PASS: libgomp.c/target-51.c (test for excess errors)
    PASS: libgomp.c/target-51.c execution test
    [-PASS:-]{+FAIL:+} libgomp.c/target-51.c output pattern test

... due to:

    Output was:

    libgomp: OMP_TARGET_OFFLOAD is set to MANDATORY, but device cannot be used for offloading

    Should match:
    .*libgomp: OMP_TARGET_OFFLOAD is set to MANDATORY, but device not found.*


Grüße
 Thomas


> diff --git a/libgomp/testsuite/libgomp.c/target-55.c b/libgomp/testsuite/libgomp.c/target-55.c
> new file mode 100644
> index 00000000000..1314b3c6963
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.c/target-55.c
> @@ -0,0 +1,20 @@
> +/* { dg-do run { target { offload_device } } } */
> +/* { dg-set-target-env-var OMP_TARGET_OFFLOAD "mandatory" } */
> +
> +/* Should pass - see target-55a.c for !offload_device */
> +
> +/* Check OMP_TARGET_OFFLOAD - it shall run on systems with offloading
> +   devices available and fail otherwise.  Note that this did always
> +   fail - as the device handling wasn't initialized before doing the
> +   mandatory checking.  */
> +
> +int
> +main ()
> +{
> +  int x = 1;
> +  #pragma omp target map(tofrom: x)
> +    x = 5;
> +  if (x != 5)
> +    __builtin_abort ();
> +  return 0;
> +}
> diff --git a/libgomp/testsuite/libgomp.c/target-55a.c b/libgomp/testsuite/libgomp.c/target-55a.c
> new file mode 100644
> index 00000000000..53978c3f405
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.c/target-55a.c
> @@ -0,0 +1,23 @@
> +/* { dg-do run { target { ! offload_device } } } */
> +/* { dg-set-target-env-var OMP_TARGET_OFFLOAD "mandatory" } */
> +
> +/* Should fail - see target-55a.c for offload_device */
> +
> +/* { dg-shouldfail "omp_invalid_device" } */
> +/* { dg-output ".*libgomp: OMP_TARGET_OFFLOAD is set to MANDATORY, but only the host device is available.*" } */
> +
> +/* Check OMP_TARGET_OFFLOAD - it shall run on systems with offloading
> +   devices available and fail otherwise.  Note that this did always
> +   fail - as the device handling wasn't initialized before doing the
> +   mandatory checking.  */
> +
> +int
> +main ()
> +{
> +  int x = 1;
> +  #pragma omp target map(tofrom: x)
> +    x = 5;
> +  if (x != 5)
> +    __builtin_abort ();
> +  return 0;
> +}
-----------------
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
diff mbox series

Patch

commit 8216ca85037be9f4d5c20540522a22a4a93b660e
Author: Tobias Burnus <tobias@codesourcery.com>
Date:   Fri Jun 16 17:21:59 2023 +0200

    libgomp: Fix OMP_TARGET_OFFLOAD=mandatory
    
    It turned out that gomp_init_targets_once() was not run when directly
    calling 'omp target' or 'omp target (enter/exit) data' causing an
    abort with OMP_TARGET_OFFLOAD=mandatory wrongly claiming that no
    device is available. It was called a tiny bit later but few lines too
    late for updating the default-device-var.
    
    libgomp/ChangeLog:
    
            * target.c (resolve_device): Call gomp_get_num_devices early to ensure
            gomp_init_targets_once was called before using default-device-var.
            * testsuite/libgomp.c/target-55.c: New test.
            * testsuite/libgomp.c/target-55a.c: New test.
---
 libgomp/target.c                         | 10 +++++++---
 libgomp/testsuite/libgomp.c/target-55.c  | 20 ++++++++++++++++++++
 libgomp/testsuite/libgomp.c/target-55a.c | 23 +++++++++++++++++++++++
 3 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/libgomp/target.c b/libgomp/target.c
index e39ef8f6e82..b6a7214ab4f 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -138,6 +138,10 @@  gomp_get_num_devices (void)
 static struct gomp_device_descr *
 resolve_device (int device_id, bool remapped)
 {
+  /* Get number of devices and thus ensure that 'gomp_init_targets_once' was
+     called, which must be done before using default_device_var.  */
+  int num_devices = gomp_get_num_devices ();
+
   if (remapped && device_id == GOMP_DEVICE_ICV)
     {
       struct gomp_task_icv *icv = gomp_icv (false);
@@ -151,7 +155,7 @@  resolve_device (int device_id, bool remapped)
 				 : omp_initial_device))
 	return NULL;
       if (gomp_target_offload_var == GOMP_TARGET_OFFLOAD_MANDATORY
-	  && gomp_get_num_devices () == 0)
+	  && num_devices == 0)
 	gomp_fatal ("OMP_TARGET_OFFLOAD is set to MANDATORY, "
 		    "but only the host device is available");
       else if (device_id == omp_invalid_device)
@@ -162,10 +166,10 @@  resolve_device (int device_id, bool remapped)
 
       return NULL;
     }
-  else if (device_id >= gomp_get_num_devices ())
+  else if (device_id >= num_devices)
     {
       if (gomp_target_offload_var == GOMP_TARGET_OFFLOAD_MANDATORY
-	  && device_id != num_devices_openmp)
+	  && device_id != num_devices)
 	gomp_fatal ("OMP_TARGET_OFFLOAD is set to MANDATORY, "
 		    "but device not found");
 
diff --git a/libgomp/testsuite/libgomp.c/target-55.c b/libgomp/testsuite/libgomp.c/target-55.c
new file mode 100644
index 00000000000..1314b3c6963
--- /dev/null
+++ b/libgomp/testsuite/libgomp.c/target-55.c
@@ -0,0 +1,20 @@ 
+/* { dg-do run { target { offload_device } } } */
+/* { dg-set-target-env-var OMP_TARGET_OFFLOAD "mandatory" } */
+
+/* Should pass - see target-55a.c for !offload_device */
+
+/* Check OMP_TARGET_OFFLOAD - it shall run on systems with offloading
+   devices available and fail otherwise.  Note that this did always
+   fail - as the device handling wasn't initialized before doing the
+   mandatory checking.  */
+
+int
+main ()
+{
+  int x = 1;
+  #pragma omp target map(tofrom: x)
+    x = 5;
+  if (x != 5)
+    __builtin_abort ();
+  return 0;
+}
diff --git a/libgomp/testsuite/libgomp.c/target-55a.c b/libgomp/testsuite/libgomp.c/target-55a.c
new file mode 100644
index 00000000000..53978c3f405
--- /dev/null
+++ b/libgomp/testsuite/libgomp.c/target-55a.c
@@ -0,0 +1,23 @@ 
+/* { dg-do run { target { ! offload_device } } } */
+/* { dg-set-target-env-var OMP_TARGET_OFFLOAD "mandatory" } */
+
+/* Should fail - see target-55a.c for offload_device */
+
+/* { dg-shouldfail "omp_invalid_device" } */
+/* { dg-output ".*libgomp: OMP_TARGET_OFFLOAD is set to MANDATORY, but only the host device is available.*" } */
+
+/* Check OMP_TARGET_OFFLOAD - it shall run on systems with offloading
+   devices available and fail otherwise.  Note that this did always
+   fail - as the device handling wasn't initialized before doing the
+   mandatory checking.  */
+
+int
+main ()
+{
+  int x = 1;
+  #pragma omp target map(tofrom: x)
+    x = 5;
+  if (x != 5)
+    __builtin_abort ();
+  return 0;
+}