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

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

sriharikrishna marked an inline comment as not done.
sriharikrishna 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);
+  }
----------------
RaviNarayanaswamy wrote:
> jdoerfert wrote:
> > RaviNarayanaswamy wrote:
> > > 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
> > > 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
> > 
> > Could you please elaborate what needs to be changed exactly. What information is currently not available in the setup as is? What properties would be different?
> Should be something like this
> 
> // NEED to add _kmp_interop_type_target  to represent interop target
> //  interop_ptr->device_info would initialize the following: device handle, device_context, platform.
> if (interop_type == kmp_interop_type_target) { 
>  if (!Device.RTL || !Device.RTL->init_device_info ||
>         Device.RTL->init_device_info(device_id, &(interop_ptr)->device_info,
>                                      &(interop_ptr)->err_str)) {
>       delete interop_ptr;
>       interop_ptr = omp_interop_none;
>  }
> // Add target sync if  request.
> if (interop_type == kmp_interop_type_tasksync) {
>     if (!Device.RTL || !Device.RTL->init_async_info ||
>         Device.RTL->init_async_info(device_id, &(interop_ptr)->async_info)) {
>       delete interop_ptr;
>       interop_ptr = omp_interop_none;
>   }
> 
> 
> 
It looks like the code you have written for kmp_interop_type_target already exists in the else part. So I am still puzzled about this. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106674