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

[Ravi Narayanaswamy via Phabricator via Openmp-commits]

RaviNarayanaswamy added inline comments.


================
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);
+  }
----------------
jdoerfert wrote:
> RaviNarayanaswamy wrote:
> > jdoerfert wrote:
> > > 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.
> > Libomp would have not invoked the task which calls this routine if there are dependences.   They must be executed before the task containing  the interop creation is scheduled.
> > 
> > The interop_type should be passed to plugin and let it initialize the common for all interop-types and then add based on the interop_type 
> > Libomp would have not invoked the task which calls this routine if there are dependences. They must be executed before the task containing the interop creation is scheduled.
> 
> To me it seems you are assuming that we create a task in which this routine is called. We do not, as far as I can tell. See D105876.
> 
> > The interop_type should be passed to plugin and let it initialize the common for all interop-types and then add based on the interop_type
> 
> So what you are trying to say is that `init_device_info` should take the `interop_type` too? That makes sense to me. But as discussed in other reviews recently, we should not extend the API for "future use cases" but extend it as use cases become relevant. For now it seems we can simply set up the `tgt_device_info` part of the `omp_interop_val_t` without knowing the `interop_type`.
Then need to change this code since the interop_type  can be both target_sync and  target which will not be handled correctly.  target_sync and target have common initialization + additional  property base don the interop_type requested


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106674