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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

We need a runtime test.

Synchronizing the asyn info object and signal out dependences will happen in the next revision.
So will minor edits as requested.

I commented below on some things.



================
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) {
----------------
tianshilei1992 wrote:
> tianshilei1992 wrote:
> > tianshilei1992 wrote:
> > > 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.
> > I mean, this function doesn't have to start with `__tgt` because it is internal. Functions starting with `__tgt` are usually interface functions. >From my perspective, it is better to name it as `getPropertyErrorType`, a.k.a. to use LLVM code style.
> Looks better. Can you check https://llvm.org/docs/CodingStandards.html for the code style?
It's unclear given the mixed nature. Let's go with this and figure out if we need to rename vars later or not.


================
Comment at: openmp/libomptarget/src/interop.cpp:198-201
+  if (interop_type == kmp_interop_type_tasksync) {
+    __kmpc_omp_wait_deps(loc_ref, gtid, ndeps, dep_list, ndeps_noalias,
+                         noalias_dep_list);
+  }
----------------
RaviNarayanaswamy wrote:
> Interop object does not wait for any task from libomp.  
> Init just initializes the interop object.
> The initialization of interop object should  be done by the plugin as only the plugin knows what properties are supported
> Interop object does not wait for any task from libomp. 

I don't know why you think we would not wait for libomp tasks. If we have dependences we need to wait for them.

> The initialization of interop object should be done by the plugin as only the plugin knows what properties are supported.

It is, below. This is the generic part that then redirects to the plugin.


================
Comment at: openmp/libomptarget/src/interop.cpp:260-263
+  if (interop_val->interop_type == kmp_interop_type_tasksync) {
+    __kmpc_omp_wait_deps(loc_ref, gtid, ndeps, dep_list, ndeps_noalias,
+                         noalias_dep_list);
+  }
----------------
RaviNarayanaswamy wrote:
> You don't wait for the omp tasks.  Need to flush the queue associated with the interop through the plugin
Waiting for omp task is necessary and the above should do it. Flushing and signaling out dependences will be added in the next revision.


================
Comment at: openmp/runtime/src/kmp_ftn_entry.h:1449-1494
+/// TODO: Include the `omp.h` of the current build
+/* OpenMP 5.1 interop */
+typedef intptr_t omp_intptr_t;
+
+/* 0..omp_get_num_interop_properties()-1 are reserved for implementation-defined
+ * properties */
+typedef enum omp_interop_property {
----------------
RaviNarayanaswamy wrote:
> Why do you have all this in openmp/runtime.  Openmp should call libomptarget to get interop properties. if libomptarget is not loaded it should return 0
That is exactly what this code does, no? Look for libomptarget and redirect to that one, otherwise return 0.
Unsure I see your point. FWIW, this is copied from other functions we duplicate in libomp but that actually are part of libomptarget.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106674