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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
