[RFC]: C API

[rscohn2]

I formatted Gengbin's C APi proposal as an RFC.

[garzaran]

Looks good.

[blazej-smorawski]

What's the device? We should be much more specific here. I think we don't have to cover this API. My proposal is to handle all device handles in ...Config APIs, so for example. For example passing users device to onecclCommInit would look like:

onecclCommConfig_t config = ONECCL_COMM_CONFIG_INITIALIZER;
config.sycl.queue = &queue;

onecclCommInitConfig(rank, size, ..., &config);

Similar pattern could be applied to onecclStreamCreate, so we could get rid of onecclCreateSyclStream which looks like a workaround rather than an elegant solution:

onecclStream_t stream;
onecclStreamConfig_t stream_config = ONECCL_STREAM_SYCL_CONFIG_INITIALIZER;
// or onecclStreamConfig_t config = ONECCL_STREAM_CONFIG_INITIALIZER;
config.sycl.queue = &queue;

onecclStreamCreateConfig(&stream, &config);

[blazej-smorawski]

We agreed to use integer index for the devices temporarily, before SYCL releases API for device selection similar to cudaSetDevice.

For the streams, I think that onecclCreateStreamXpu(&onecclStream_t stream_ptr, void* args) should be enough for all communications backed we would like to support.

[zhenggb72]

fixed

[JackAKirk]

Have you considered this issue intel/llvm#15251
which is described more fully here: oneapi-src/unified-runtime#2077

Are there not similar implementation issues in the level_zero backend due to the implicit device setting nature of SYCL?
I looked into the l0 adapter implementation: I think it is possible that l0 has similar issues, depending on ipc usage: I think it possible that if intel/llvm#15251 could be built for PVC you will see similar issues

I do not see how implementing a wrapper function to sycl that matches the api of cudaSetDevice such as suggested here: https://github.com/intel/llvm/
pull/15382
can solve such issues.

I think that the only current solution is to rely on the assumption that no more than one gpu device will be used per MPI rank and that environment variables such as CUDA_VISIBLE_DEVICES/ intel vendor equivalent, or ONEAPI_DEVICE_SECTOR are used: as detailed in intel/llvm#15251

[garzaran]

Have you considered this issue intel/llvm#15251 which is described more fully here: oneapi-src/unified-runtime#2077

Are there not similar implementation issues in the level_zero backend due to the implicit device setting nature of SYCL? I looked into the l0 adapter implementation: I think it is possible that l0 has similar issues, depending on ipc usage: I think it possible that if intel/llvm#15251 could be built for PVC you will see similar issues

I do not see how implementing a wrapper function to sycl that matches the api of cudaSetDevice such as suggested here: https://github.com/intel/llvm/ pull/15382 can solve such issues.

I think that the only current solution is to rely on the assumption that no more than one gpu device will be used per MPI rank and that environment variables such as CUDA_VISIBLE_DEVICES/ intel vendor equivalent, or ONEAPI_DEVICE_SECTOR are used: as detailed in intel/llvm#15251

We do need to support the case where one MPI rank will open more than one GPU device. It is hard to understand all the details in the ticket you refer, but it appears more like a memory leak, which I assume can/has to be fixed. In any case, in general a MPI rank can open all the GPU devices.
Take a look to this example from CUDA: https://docs.nvidia.com/deeplearning/nccl/user-guide/docs/examples.html#example-3-multiple-devices-per-thread. Tensorflow uses/needs such a use-case where a single process opens all the devices.

[blazej-smorawski]

So onecclStreamDestroy is the final version right?

[blazej-smorawski]

cclStream_t -> onecclStream_t

[garzaran]

Yes, this is a typo. @zhenggb72, can you update this and we can approve and merge?

[JackAKirk]

Have you considered this issue intel/llvm#15251 which is described more fully here: oneapi-src/unified-runtime#2077
Are there not similar implementation issues in the level_zero backend due to the implicit device setting nature of SYCL? I looked into the l0 adapter implementation: I think it is possible that l0 has similar issues, depending on ipc usage: I think it possible that if intel/llvm#15251 could be built for PVC you will see similar issues
I do not see how implementing a wrapper function to sycl that matches the api of cudaSetDevice such as suggested here: https://github.com/intel/llvm/ pull/15382 can solve such issues.
I think that the only current solution is to rely on the assumption that no more than one gpu device will be used per MPI rank and that environment variables such as CUDA_VISIBLE_DEVICES/ intel vendor equivalent, or ONEAPI_DEVICE_SECTOR are used: as detailed in intel/llvm#15251

We do need to support the case where one MPI rank will open more than one GPU device. It is hard to understand all the details in the ticket you refer, but it appears more like a memory leak, which I assume can/has to be fixed. In any case, in general a MPI rank can open all the GPU devices. Take a look to this example from CUDA: https://docs.nvidia.com/deeplearning/nccl/user-guide/docs/examples.html#example-3-multiple-devices-per-thread. Tensorflow uses/needs such a use-case where a single process opens all the devices.

Thanks @garzaran that link was useful. I've opened a OPENMPI issue with a cuda reproducer that represents what happens in DPC++ here: open-mpi/ompi#12848

[garzaran]

Looks good