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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/include/interop.h:19
+
+/// TODO: Include the `omp.h` of the current build
+/* OpenMP 5.1 interop */
----------------
Probably not an option.


================
Comment at: openmp/libomptarget/include/omptarget.h:21
 
-#include <SourceInfo.h>
+#include "SourceInfo.h"
 
----------------
Why is that?


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:76
+  GENERIC,      // everything else
+  SPMD_GENERIC, // Generic kernel with SPMD execution
   NONE
----------------
I think these series of changes regarding `SPMD_GENERIC` has nothing to do with `interop`. Please make a separate patch for them.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:1152-1153
+
+    __tgt_async_info *P = new __tgt_async_info;
+    *AsyncInfo = P;
+    getStream(DeviceId, *AsyncInfo);
----------------



================
Comment at: openmp/libomptarget/src/CMakeLists.txt:21
   ${CMAKE_CURRENT_SOURCE_DIR}/omptarget.cpp
+  ${CMAKE_CURRENT_SOURCE_DIR}/interop.cpp
 )
----------------
order please


================
Comment at: openmp/libomptarget/src/exports:49
+    omp_get_interop_rc_desc;
+    __kmpc_interop_init;
+    __kmpc_interop_use;
----------------
Like @RaviNarayanaswamy said, better to rename them to `__tgt_xxx`.


================
Comment at: openmp/libomptarget/src/interop.cpp:13
+static omp_interop_rc_t
+__kmpc_interop_get_property_err_type(omp_interop_property_t property) {
+  switch (property) {
----------------
There are some conventions in current `libomptarget`.
1. If a function is internal, use LLVM code style.
2. If a function is exported and exposed to compiler, it should be `extern "C"` and use code style similar to existing functions whose name prefix with `__tgt`.

So basically, if these functions are only visible to this file, please format it with LLVM code style, and use anonymous name space.


================
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,
----------------
If this function is not defined in `libomp`, please don't use `__kmpc`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106674