Remove workarounds to make device equality work. #338
Conversation
|
@oleksandr-pavlyk dpcpp 2021.2 has the device equality fixes that makes my workaround redundant. This PR removed the |
diptorupd
force-pushed
the
device_cache_changes
branch
2 times, most recently
from
a502879 to
7febae7
Compare
- dpcpp 2021.2 fixed device equality in the dpcpp runtime and we can
now remove several workarounds in dpctl:
- DPCTLDeviceMgr_GetDeviceAndContextPair is now renamed as
DPCTLDeviceMgr_GetCachedContext and only returns a
DPCTLSyclContextRef instead of a pair of DPCTLSyclContextRef
and DPCTLSyclDeviceRef.
- Remove the DPCTLDeviceMgr_AreEq function and related internal
helper functions for hashing SYCL devices.
- Remove DeviceWrapper class inside dpctl_sycl_device_manager.cpp.
- Remove DPCTL_DeviceAndContextPair type form C API.
- Make equivalent changes to the Python API.
diptorupd
force-pushed
the
device_cache_changes
branch
from
7febae7 to
6da77c9
Compare
| if (dev_ctx.CRef is NULL) or (dev_ctx.DRef is NULL): | ||
| CRef = DPCTLDeviceMgr_GetCachedContext(DRef) | ||
| if (CRef is NULL): | ||
| DPCTLDevice_Delete(DRef) |
There was a problem hiding this comment.
DRef is the input argument. We should not be deleting it.
There was a problem hiding this comment.
If the argument device is a sub-device the look-up will fail. I thought we discussed that it would be better to create a new one instead.
There was a problem hiding this comment.
_init_queue_from_DPCTLSyclDeviceRef is returning an error code if the context look up fails. I assume the logic to handle creation of a context for sub-devices is not yet implemented? Do your upcoming changes to SyclContext have that change?
As for freeing DRef, in the current code it does not cause an issue. Does it? Are you saying from a design style perspective we should let caller free the object?
Since, _init_queue_from_DPCTLSyclDeviceRef is an internal helper that is invoked only by other internal helpers: _init_queue_from_filter_string, _init_queue_from_device, and _init_queue_default, freeing the DRef in _init_queue_from_DPCTLSyclDeviceRef when we know queue creation failed is fine IMO. But, I can change the code to free it in the callers on returning from _init_queue_from_DPCTLSyclDeviceRef at the cost of adding a check on the return code. Do note that I have a copy in _init_queue_from_device, the only place where deleting the DRef can have an external side-effect.
There was a problem hiding this comment.
As for freeing
DRef, in the current code it does not cause an issue. Does it? Are you saying from a design style perspective we should let caller free the object?
I see the issue now. DRef will leak when queue creation succeeded. I will fix that.
Nope, still do not see it. If queue creation succeeds then the DRef is stored in the SyclDevice member of the queue and freed at that time.
There was a problem hiding this comment.
_init_queue_from_DPCTLSyclDeviceRefis returning an error code if the context look up fails. I assume the logic to handle creation of a context for sub-devices is not yet implemented? Do your upcoming changes toSyclContexthave that change?
Ok #334 has the changes.
There was a problem hiding this comment.
@oleksandr-pavlyk in _init_queue_from_device we are taking the DRef from the SyclDevice argument and storing it in the SyclQueue if queue creation succeeded. I think we need a DPCTLDevice_Copy as I have done here. What happens when the SyclDevice is deleted thus deallocating the DPCTLSyclDeviceRef?
The changes implement convention that most |
- dpcpp 2021.2 fixed device equality in the dpcpp runtime and we can
now remove several workarounds in dpctl:
- DPCTLDeviceMgr_GetDeviceAndContextPair is now renamed as
DPCTLDeviceMgr_GetCachedContext and only returns a
DPCTLSyclContextRef instead of a pair of DPCTLSyclContextRef
and DPCTLSyclDeviceRef.
- Remove the DPCTLDeviceMgr_AreEq function and related internal
helper functions for hashing SYCL devices.
- Remove DeviceWrapper class inside dpctl_sycl_device_manager.cpp.
- Remove DPCTL_DeviceAndContextPair type form C API.
- Make equivalent changes to the Python API.
- dpcpp 2021.2 fixed device equality in the dpcpp runtime and we can
now remove several workarounds in dpctl:
- DPCTLDeviceMgr_GetDeviceAndContextPair is now renamed as
DPCTLDeviceMgr_GetCachedContext and only returns a
DPCTLSyclContextRef instead of a pair of DPCTLSyclContextRef
and DPCTLSyclDeviceRef.
- Remove the DPCTLDeviceMgr_AreEq function and related internal
helper functions for hashing SYCL devices.
- Remove DeviceWrapper class inside dpctl_sycl_device_manager.cpp.
- Remove DPCTL_DeviceAndContextPair type form C API.
- Make equivalent changes to the Python API.
dpcpp 2021.2 fixed device equality in the dpcpp runtime and we can
now remove several workarounds in dpctl:
DPCTLDeviceMgr_GetCachedContext and only returns a
DPCTLSyclContextRef instead of a pair of DPCTLSyclContextRef
and DPCTLSyclDeviceRef.
helper functions for hashing SYCL devices.
Make equivalent changes to the Python API.