[Openmp-commits] [PATCH] D106674: Runtime for Interop directive

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

There are a lot of minor things that should be easily addressable.



================
Comment at: openmp/libomptarget/include/interop.h:1
+#ifndef _INTEROP_H_
+#define _INTEROP_H_
----------------
We need the llvm license header, copied from another file.


================
Comment at: openmp/libomptarget/include/interop.h:65
+extern "C" {
+#endif
+
----------------
put the entire file content (after macros) into a single extern C.


================
Comment at: openmp/libomptarget/include/interop.h:109
+extern const char *__KAI_KMPC_CONVENTION
+omp_get_interop_rc_desc(const omp_interop_t, omp_interop_rc_t);
+#ifdef __cplusplus
----------------
The functions above need definitions in libomp and libomptarget.
For the former we need to edit the following two files, look for "get_device_num" (or "GET_DEVICE_NUM") in both
to see how to define it weak:
openmp/runtime/src/kmp_ftn_os.h
openmp/runtime/src/kmp_ftn_entry.h                                                                                                                                                                                               
just return `0` or `nullptr` for all of them in libomp.

In libomptarget already have the definitions and exports.


================
Comment at: openmp/libomptarget/include/interop.h:160
+    assert(!device_info.Context);
+  }
+  const char *err_str = nullptr;
----------------
Remove the asserts.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:76
+  GENERIC,      // everything else
+  SPMD_GENERIC, // Generic kernel with SPMD execution
   NONE
----------------
tianshilei1992 wrote:
> I think these series of changes regarding `SPMD_GENERIC` has nothing to do with `interop`. Please make a separate patch for them.
This is a rebase problem. These changes are already upstream but somehow they managed to get into this commit/revision. Rebase locally and this should hopefully be gone.


================
Comment at: openmp/libomptarget/src/exports:1
-VERS1.0 {
+VERSION {
   global:
----------------
undo this.


================
Comment at: openmp/libomptarget/src/interop.cpp:45
+static const char *__kmpc_interop_vendor_id_to_str(intptr_t vendor_id) {
+  return "CUDA_driver";
+}
----------------
We nee a switch and return all values in Table 2.1 (https://www.openmp.org/wp-content/uploads/OpenMP-API-Additional-Definitions-2-0.pdf) or "unknown" as default.


================
Comment at: openmp/libomptarget/src/interop.cpp:177
+void __kmpc_interop_init(ident_t *loc_ref, kmp_int32 gtid,
+                         omp_interop_val_t *&interop_ptr,
+                         kmp_interop_type_t interop_type, kmp_int32 device_id,
----------------
don't use references in this code with C bindings. Use a ** instead and change left-hand-side uses to `*interop_ptr`. This will then also match D105876 where all 3 functions are declared to take a `void**`. That means `_use` below should take a `void**` as well.


================
Comment at: openmp/libomptarget/src/interop.cpp:191
+  if (device_id == omp_get_initial_device()) {
+    assert(device_id != omp_get_initial_device());
+    return;
----------------
This assertion seems nonsensical, just return I guess.


================
Comment at: openmp/libomptarget/src/interop.cpp:204
+                         noalias_dep_list);
+  }
+
----------------
Move the wait stuff to the beginning.


================
Comment at: openmp/libomptarget/src/interop.cpp:216
+                                     &(interop_ptr)->err_str)) {
+      delete interop_ptr;
+    }
----------------
Again
`interop_ptr = omp_interop_none;`


================
Comment at: openmp/libomptarget/src/private.h:117
 int __kmpc_get_target_offload(void) __attribute__((weak));
+void __kmpc_omp_wait_deps(ident_t *loc_ref, kmp_int32 gtid, kmp_int32 ndeps,
+                          kmp_depend_info_t *dep_list, kmp_int32 ndeps_noalias,
----------------
tianshilei1992 wrote:
> If this function is not defined in `libomp`, please don't use `__kmpc`.
It is in libomp, this is a forward decl. All good here.


================
Comment at: openmp/runtime/src/dllexports:582
+__kmpc_target_task_yield 2902
+__kmpc_get_target_task_npredecessors 2903
+
----------------
Maybe remove all these for now. That should also allow to not define omp_get_interop_XXX in libomp for now and it will just fail if we do not link libomptarget as a first step.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106674/new/

https://reviews.llvm.org/D106674