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:
> > > > 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;
}
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
