[SYCL][XPTI] Pass plugin information to subscribers

[alexbatashev]

This patch makes the following additional information available to XPTI subscribers.
All streams:

• Actual major and minor versions of SYCL runtime (instead of dummy values) as well as their string variant.

sycl.pi.debug stream:

• Backend type, which is defined as a uint8_t value of sycl::backend enum.
• Pointer to PI plugin to provide some degree of application flow variance (e.g. query additional info about device, USM pointers, memory, etc).

[smaslov-intel]

Backend type, which is defined as a uint8_t value of sycl::backend enum.

This creates new interface dependency between components, e.g. you can't reorder backend enum values now.
Why is this needed? Can you instead pass a string that identifies the backend?

Pointer to PI plugin to provide some degree of application flow variance (e.g. query additional info about device, USM pointers, memory, etc).

Is this normal for XPTI model to cross shared objects boundaries with some internal pointers? Shouldn't that come at least with some pointer lifetime management, such that it is not released before XPTI readers are done? Again, what is the motivation of this change?

[alexbatashev]

This creates new interface dependency between components, e.g. you can't reorder backend enum values now.
Why is this needed? Can you instead pass a string that identifies the backend?

It’s part of SYCL ABI, you can’t reorder them anyway. As for strings, they can be expensive, if you need to do comparisons.

Is this normal for XPTI model to cross shared objects boundaries with some internal pointers? Shouldn't that come at least with some pointer lifetime management, such that it is not released before XPTI readers are done? Again, what is the motivation of this change?

I don’t see why you’d need to save pointer to plugin somewhere. And it is guaranteed to be valid throughout callback lifetime, because they’re synchronous. Same thing is true about any other callback argument.

My motivation is that memory transfers are asynchronous, and I need them to be synchronous in order to be able to store memory content. Thus, I put extra waits in callbacks.

[smaslov-intel]

It’s part of SYCL ABI, you can’t reorder them anyway. As for strings, they can be expensive, if you need to do comparisons.

Why do you need backend ID at all in XPTI stream receiver?

And it is guaranteed to be valid throughout callback lifetime, because they’re synchronous. Same thing is true about any other callback argument.

Good point about other pointer arguments, so at least you aren't making things worse than they are already.

My motivation is that memory transfers are asynchronous, and I need them to be synchronous in order to be able to store memory content. Thus, I put extra waits in callbacks.

Can you point me to where you are doing so? I am curious how do you identify what to wait for, and avoid deadlocks (e.g. when some copies are dependent on some others, and waiting for them may cause no forward progress, I think).

[alexbatashev]

Why do you need backend ID at all in XPTI stream receiver?

Seems like a useful information for debugging purpose. Applications may use more than one platform. Knowledge of particular backend, that executes PI call may help here.

Can you point me to where you are doing so? I am curious how do you identify what to wait for, and avoid deadlocks (e.g. when some copies are dependent on some others, and waiting for them may cause no forward progress, I think).

https://github.com/alexbatashev/dpcpp_trace/blob/c676824b3a5ad4da091dd639ae070d63c15bb3cb/lib/plugin_record/record_handler.cpp#L268

Scheduler wouldn't enqueue a command, unless all dependencies are satisfied. Thus, I don't expect deadlocks.

[smaslov-intel]

Knowledge of particular backend, that executes PI call may help here.

Why not defer adding it until we know how to use it? Also, if we wanted it, I think we should have this info in the "pi_plugin" structure, pointer to which you are passing already.

Scheduler wouldn't enqueue a command, unless all dependencies are satisfied.

Really? Aren't we using events wait-lists for drivers to track/resolve dependencies?
Most PI enqueue calls have them:

llvm/sycl/include/CL/sycl/detail/pi.h

Line 1380 in a4165b3

const size_t *local_work_size, pi_uint32 num_events_in_wait_list,

[alexbatashev]

Why not defer adding it until we know how to use it? Also, if we wanted it, I think we should have this info in the "pi_plugin" structure, pointer to which you are passing already.

I can remove it if nobody sees any use for it. Right now all I do with that argument is print it on the screen for debugging purpose. Maybe @tovinkere can suggest other usages here.

What I'd like to avoid, though, is messing with pi_plugin structure. Since it's part of cross-library interface, it would be great to have some degree of stability here.

Really? Aren't we using events wait-lists for drivers to track/resolve dependencies?
Most PI enqueue calls have them:

I don't see much use for those arguments. What if our dependency is a host task? Or an event from another plugin? So, we have to wait for particular events anyway. And even if we use this parameter somewhere, waiting on the output event will also wait on this list of events. So, it should be safe from that point of view.

Besides, there're other usages for additional PI calls. For example, one may want to query pointer type to figure out data flow direction in memcpy.

[alexbatashev]

@smaslov-intel what would be the final decision here? Should I remove backend field? If yes, should I simply pass pi_plugin as user data?

[smaslov-intel]

@smaslov-intel what would be the final decision here? Should I remove backend field? If yes, should I simply pass pi_plugin as user data?

Yes, please remove it and just pass the pi_plugin.