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

[Sri Hari Krishna Narayanan via Phabricator via Openmp-commits]

sriharikrishna marked 17 inline comments as done.
sriharikrishna added inline comments.


================
Comment at: openmp/libomptarget/include/interop.h:19
+
+/// TODO: Include the `omp.h` of the current build
+/* OpenMP 5.1 interop */
----------------
tianshilei1992 wrote:
> Probably not an option.
Okay. Any suggestions on how to overcome this?


================
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
----------------
jdoerfert wrote:
> 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.
Okay. While linking, if we use the order -lomptarget -lomp then we get the values from libomptarget. If we use the order -lomp -lomptarget we get the values from libomp. This seems fragile. 


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


================
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,
----------------
jdoerfert wrote:
> 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.
It is not clear how to fix this. In the user code's interop directive, the interop variable must be of type 'omp_interop_t'. So getting 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106674