[SYCL] remove dependencies from memcpy, memset and kernel launch

[AuroraPerego]

The requirements / dependencies were needed for the buffers, so now can be removed.
SYCL queues and events are thread-safe, the mutex in QueueGenericSyclBase.hpp has been removed as well.
When there is an event to wait for, instead of registering it as a dependency, the method ext_oneapi_submit_barrier(const std::vector<event> &) is used.

[j-stephan]

This looks dangerously like a race condition.

[fwyzard]

Do you mean between the call to enqueue() and a possible call to get_last_event() or empty() ?

[j-stephan]

I'm worried about the assignment here. What happens if two threads enqueue work concurrently? Do we have a guarantee that m_last_event will actually refer to the last event?

[fwyzard]

Ah, I see.

You are right, if multiple threads enqueue a task to the same queue, there is no guarantee about which of the two events would be stored in m_last_event.

Possible solutions:

1. add back the lock, and use it only in enqueue(), empty(), get_last_event() (and maybe wait()? probably not)
2. remove the m_last_event member and

• implement get_last_event() with a call to m_queue.ext_oneapi_submit_barrier()
• implement empty() with a call to m_queue.ext_oneapi_empty()
  Unfortunately according to the extension itself, ext_oneapi_empty() is not supported by the OpenCL back0end :-(

[j-stephan]

From the extension link:

Currently support for OpenCL backend is limited, API introduced by this extension can be called only for in-order queues which doesn’t have discard_events property. Exception is thrown if new API is called on other type of queue. OpenCL currently doesn’t have an API to get queue status.

We are using in-order SYCL queues internally, so I think we should be fine, right?

[fwyzard]

Good point.

I guess we can try and see if it works :-)

[j-stephan]

Okay, the checks are looking good. Have you executed the runtime tests?

[AuroraPerego]

Yes, i've built and run the tests for CPU and GPU with the usual result (all pass on GPU, 4 out of resources on CPU)

[AuroraPerego]

(I'll check also the impact on the performance)

[j-stephan]

From my point of view this is good to go. @fwyzard feel free to merge once the performance analysis is done.

[j-stephan]

Converting to draft until the performance analysis is ready.

[AuroraPerego]

The situation is worse than expected, apparently there are many more synchronization introduced with these changes.
I compared the baseline with this PR and with another version that has only the changes to the Copy.hpp and Set.hpp files.

nThreads	base@c9dbac0	base + PR	base + Copy/Set changes
1	148.44 +/- 3.48	142.81 +/- 1.71	148.94 +/- 2.58
2	172.73 +/- 0.94	162.27 +/- 2.48	171.95 +/- 1.05
4	181.79 +/- 0.16	167.61 +/- 1.06	182.29 +/- 0.31
8	182.89 +/- 0.06	166.67 +/- 0.54	183.21 +/- 0.06

Doing some profiling, these are the main differences:

	zeEventHostSynchronize	zeCommandListAppendBarrier	zeEventQueryStatus	zeEventHostReset
base	5	5	2	~60
pr	6	~48	~22	~103

[j-stephan]

What are those numbers? Runtime?

[AuroraPerego]

Yes, sorry.
The tests have been done on pixeltrack-standalone@dcf2898.
These numbers are the throughput, i.e. the number of concurrent events processed per second, computed processing 10000 events and nThreads is the number of CPU threads used

[j-stephan]

Ah, I see. So lower numbers are worse. That is indeed a change for the worse then. How did you ensure that the requirements handling is correct in your third approach (only changes to Copy and Set)?

[AuroraPerego]

Not sure the handling it's correct, it was done mostly to remove the ext_oneapi_submit_barrier() and ext_oneapi_empty() calls to check the impact.
At this point I'm not sure if it's worth to change something in the current implementation.

[j-stephan]

Hm, maybe open a bug report for Intel? I'm sure they would be interested to hear about this :-)