Hi Sandra,

looks quite okay, but I have a couple of remarks:

Sandra Loosemore wrote:
From: Kwok Cheung Yeung<k...@codesourcery.com>

This patch implements the libgomp runtime support for the dynamic
target_device selector via the GOMP_evaluate_target_device function.

...
--- /dev/null
+++ b/libgomp/config/gcn/selector.c
...
+GOMP_evaluate_current_device (const char *kind, const char *arch,
+                             const char *isa)
+{
+  if (kind && strcmp (kind, "gpu") != 0)
+    return false;

This should also match: kind == nohost.

+
+  if (arch && strcmp (arch, "gcn") != 0)
+    return false;

"amdgcn" missing - we support both for better compatibility with LLVM.

+  if (!isa)
+    return true;
+
+#ifdef __GCN3__
+  if (strcmp (isa, "fiji") == 0 || strcmp (isa, "gfx803") == 0)
+    return true;
+#endif
+
+#ifdef __GCN5__
+  if (strcmp (isa, "gfx900") == 0 || strcmp (isa, "gfx906") != 0
+      || strcmp (isa, "gfx908") == 0)
+    return true;
+#endif

This misses gfx90a and gfx1030. Additionally, the last conditions matches too much. Can you use

#ifdef __fiji__
#ifdef __gfx900__
etc.

instead?

--- /dev/null
+++ b/libgomp/config/linux/selector.c
...
+bool
+GOMP_evaluate_current_device (const char *kind, const char *arch,
+                             const char *isa)
+{
+  if (kind && strcmp (kind, "cpu") != 0)
+    return false;

You also need to match "host".

diff --git a/libgomp/config/linux/x86/selector.c 
b/libgomp/config/linux/x86/selector.c
new file mode 100644
index 00000000000..2b6c2ba165b
--- /dev/null
+++ b/libgomp/config/linux/x86/selector.c
...

+bool
+GOMP_evaluate_current_device (const char *kind, const char *arch,
+                             const char *isa)
+{
+  if (kind && strcmp (kind, "cpu") != 0)
+    return false;

This misses "host" as well.

+  if (arch
+      && strcmp (arch, "x86") != 0
+      && strcmp (arch, "ia32") != 0
+#ifdef __x86_64__
+      && strcmp (arch, "x86_64") != 0
+#endif
+#ifdef __ILP32__
+      && strcmp (arch, "x32") != 0
+#endif
+      && strcmp (arch, "i386") != 0
+      && strcmp (arch, "i486") != 0
+#ifndef __i486__
+      && strcmp (arch, "i586") != 0
+#endif
+#if !defined (__i486__) && !defined (__i586__)
+      && strcmp (arch, "i686") != 0
+#endif

The 'i486' seems to lack a #ifdef __i486__ check.
And it seems to be such that
  i486 implies i386
  i586 implies i486 and i386
  etc.
if I understand ix86_omp_device_kind_arch_isa in
gcc/config/i386/i386-options.cc correctly.


There is of course the problem that the compilation flags used for libgomp are very likely different to the compilation flags of the user program, which in term can differ between files.

Thus, I think we should update
  https://gcc.gnu.org/onlinedocs/libgomp/OpenMP-Context-Selectors.html
(a) the host compiler always also matches "cpu"
(b) We probably should state somewhere that:
    * on x86, both the arch = i486 to i686 and the isa flags depend on
      the command line arguments more than on the actual hardware.
    * that's especially true for dynamic selectors as the flags used
      can differ between 'compilation units' and also the flags used
      for the run-time library.
    * For nvptx: on the device side, the -march= implies that all
      sm_* lower than that value is set.
      For target_device, the actual hardware is checked at run time,
      implying the highest of the gcc-manual listed -march= values is
      selected that the hardware actually supports at runtime.

For (b) we should have to find some better wording and possibly be less precise but I think some kind of warning/note is needed here.

+  if (!isa)
+    return true;
+
+#ifdef __WBNOINVD__
+  if (strcmp (isa, "wbnoinvd") == 0) return true;
+#endif

I think at least the following are missing:

-mavx10.1-256 and -mavx10.1-512
do not seem to have a #define
→ Maybe we should file a PR given that those
seem to be the only missing ones.

otherwise:

__AVX10_512BIT__ and "avx10-max-512bit"
__AVX10_1__ and "avx10.1"
__AMX_FP16__ and -mamx-fp16
__CMPCCXADD__ and "cmpccxadd"
__AVXNECONVERT__ and "avxneconvert"
__RAOINT__ and "raoint"
__PREFETCHI__ and "refetchi"
__USER_MSR__ and "usermsr".
__EVEX256__ and "evex512".
__AVXVNNIINT8__ and "avxvnniint8"
__SM4__ and "sm4"
__SHA512__ and "sha512"
__SM3__ and "sm3"
__AVXVNNIINT16__ and "avxvnniint16"
__AMX_COMPLEX__ and "amx-complex"
__AVXIFMA__ and avxifma"

and possibly some more but it might be also be complete.


+++ b/libgomp/config/nvptx/selector.c
+bool
+GOMP_evaluate_current_device (const char *kind, const char *arch,
+                             const char *isa)
+{
+  if (kind && strcmp (kind, "gpu") != 0)
+    return false;
"nohost" missing.

--- a/libgomp/libgomp.map
+++ b/libgomp/libgomp.map
@@ -414,6 +414,7 @@ GOMP_5.1 {
        GOMP_scope_start;
        GOMP_warning;
        GOMP_teams4;
+       GOMP_evaluate_target_device;
  } GOMP_5.0.1;

This looks wrong. In my understanding you cannot just randomly
add entries to old map entries but it needs to be a new group
in a new compiler release. In any case, I believe for GCC 14
it should be added to GOMP_5.1.2.

But in doubt ask Jakub, who knows this inside out.

--- a/libgomp/plugin/plugin-gcn.c
+++ b/libgomp/plugin/plugin-gcn.c
@@ -3984,6 +3984,20 @@ GOMP_OFFLOAD_async_run (int device, void *tgt_fn, void 
*tgt_vars,
                       GOMP_PLUGIN_target_task_completion, async_data);
  }
+bool
+GOMP_OFFLOAD_evaluate_device (int device_num, const char *kind,
+                             const char *arch, const char *isa)
+{
+  struct agent_info *agent = get_agent_info (device_num);
+
+  if (kind && strcmp (kind, "gpu") != 0)
+    return false;

"nohost" missing

+  if (arch && strcmp (arch, "gcn") != 0)
+    return false;

"amdgcn" missing.

index c04c3acd679..9dcd8a6f6eb 100644
--- a/libgomp/plugin/plugin-nvptx.c
+++ b/libgomp/plugin/plugin-nvptx.c
...
+bool
+GOMP_OFFLOAD_evaluate_device (int device_num, const char *kind,
+                             const char *arch, const char *isa)
+{
+  if (kind && strcmp (kind, "gpu") != 0)
+    return false;

"nohost" missing.

+++ b/libgomp/selector.c
@@ -0,0 +1,36 @@
+/* Copyright (C) 2022 Free Software Foundation, Inc.
+   Contributed by Mentor, a Siemens Business.
+
+   This file is part of the GNU Offloading and Multi Processing Library
+   (libgomp).
+
+   Libgomp is free software; you can redistribute it and/or modify it
+   under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3, or (at your option)
+   any later version.
+
+   Libgomp is distributed in the hope that it will be useful, but WITHOUT ANY
+   WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+   FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+   more details.
+
+   Under Section 7 of GPL version 3, you are granted additional
+   permissions described in the GCC Runtime Library Exception, version
+   3.1, as published by the Free Software Foundation.
+
+   You should have received a copy of the GNU General Public License and
+   a copy of the GCC Runtime Library Exception along with this program;
+   see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
+<http://www.gnu.org/licenses/>.  */
+
+/* This file contains a placeholder implementation of
+   GOMP_evaluate_current_device.  */
+
+#include "libgomp.h"
+
+bool
+GOMP_evaluate_current_device (const char *kind, const char *arch,
+                             const char *isa)
+{
+  return false;
+}

Isn't this called in some cases on the host? If so, it should
support kind == "host" and kind == "cpu".

diff --git a/libgomp/target.c b/libgomp/target.c
index 1367e9cce6c..206987953dc 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -5088,6 +5088,43 @@ omp_pause_resource_all (omp_pause_resource_t kind)
  ialias (omp_pause_resource)
  ialias (omp_pause_resource_all)
+bool
+GOMP_evaluate_target_device (int device_num, const char *kind,
+                            const char *arch, const char *isa)
+{
+  bool result = true;
+
+  if (device_num < 0)
+    device_num = omp_get_default_device ();

As mentioned with regards to 1/8, 'omp_initial_device == -1' according
to the OpenMP standard and there is additionally 'omp_invalid_device'.

Thanks,

Tobias

Reply via email to