[cuda][hip] Removed bad event wait constraint / use scopedContext correctly.

[JackAKirk]

There is no reason to not allow waits from native hip/cuda events in different sycl "contexts".
Even the sycl spec does not mandate such a thing. This constraint does not appear to be applied in L0 backend.
This also fixes scopedContext to set the per device (CU/HIP)context.

This is a fix for intel/llvm#6749 (comment)

[hdelan]

I don't think we need to set the context every time this lambda is run. It's valid to wait on an event that was recorded in another context, so setting the context just once at the start of the try catch should be enough

[JackAKirk]

Is it valid to wait for an event that was set in another CUcontext (on a different device)?
I assumed this was not the case: cuEventSynchronize can return "CUDA_ERROR_INVALID_CONTEXT".

[hdelan]

Think we should only have a context set once per urEventWait, not once per event

[JackAKirk]

Think we should only have a context set once per urEventWait, not once per event

urEventWait waits on a list of events which can be associated with different device CUcontexts. This is what the code was doing (using different devices) that led to the error that motivated this patch. If you only set the context once it could generally be set for a CUcontext that doesn't correspond to the CUcontext used for the stream that the event is associated with.

[hdelan]

urEventWait waits on a list of events which can be associated with different device CUcontexts. This is what the code was doing (using different devices) that led to the error that motivated this patch. If you only set the context once it could generally be set for a CUcontext that doesn't correspond to the CUcontext used for the stream that the event is associated with.

These are docs for the CUDA runtime which says we don't need to set a context to use cudaEventSynchronize.
https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#stream-and-event-behavior

I think it must be the same for the driver API as well since if we have a CUevent associated with context A and a stream associated with context B, since in the CUDA runtime it says we can successfully call cudaStreamWaitEvent, this must also be possible in the CUDA driver since there is no way to emulate having two active contexts to make this call.

However this is sort of on the fringes of documentation so I think we should run some tests to make sure that this is OK in cuda driver API.

[JackAKirk]

urEventWait waits on a list of events which can be associated with different device CUcontexts. This is what the code was doing (using different devices) that led to the error that motivated this patch. If you only set the context once it could generally be set for a CUcontext that doesn't correspond to the CUcontext used for the stream that the event is associated with.

These are docs for the CUDA runtime which says we don't need to set a context to use cudaEventSynchronize. https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#stream-and-event-behavior

I think it must be the same for the driver API as well since if we have a CUevent associated with context A and a stream associated with context B, since in the CUDA runtime it says we can successfully call cudaStreamWaitEvent, this must also be possible in the CUDA driver since there is no way to emulate having two active contexts to make this call.

However this is sort of on the fringes of documentation so I think we should run some tests to make sure that this is OK in cuda driver API.

Yeah makes sense, nice find. I'll run some tests.

[JackAKirk]

urEventWait waits on a list of events which can be associated with different device CUcontexts. This is what the code was doing (using different devices) that led to the error that motivated this patch. If you only set the context once it could generally be set for a CUcontext that doesn't correspond to the CUcontext used for the stream that the event is associated with.

These are docs for the CUDA runtime which says we don't need to set a context to use cudaEventSynchronize. https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#stream-and-event-behavior
I think it must be the same for the driver API as well since if we have a CUevent associated with context A and a stream associated with context B, since in the CUDA runtime it says we can successfully call cudaStreamWaitEvent, this must also be possible in the CUDA driver since there is no way to emulate having two active contexts to make this call.
However this is sort of on the fringes of documentation so I think we should run some tests to make sure that this is OK in cuda driver API.

Yeah makes sense, nice find. I'll run some tests.

Made some multidevice/multiqueue tests and they pass fine on nvidia/hip systems so I think you must be right. I've made the change you suggested.

[hdelan]

Made some multidevice/multiqueue tests and they pass fine on nvidia/hip systems so I think you must be right. I've made the change you suggested.

Great thanks

[JackAKirk]

This is P2P functionality requireing 2 gpus and doesn't have a test-e2e