From 48d6393024b74f5eaf442a7e6bc851fbb452d826 Mon Sep 17 00:00:00 2001 From: Sergey V Maslov Date: Thu, 18 Feb 2021 14:30:30 -0800 Subject: [PATCH 1/7] [SYCL] Add Level-Zero interop with specification of ownership Signed-off-by: Sergey V Maslov --- sycl/include/CL/sycl/backend/level_zero.hpp | 22 +++++++++++++++++---- sycl/include/CL/sycl/detail/pi.h | 4 +++- sycl/plugins/cuda/pi_cuda.cpp | 1 + sycl/plugins/level_zero/pi_level_zero.cpp | 9 ++++++--- sycl/plugins/level_zero/pi_level_zero.hpp | 9 +++++++-- sycl/plugins/opencl/pi_opencl.cpp | 2 ++ sycl/source/backend/level_zero.cpp | 6 ++++-- sycl/source/backend/opencl.cpp | 2 +- sycl/source/detail/context_impl.cpp | 14 +++++++++---- 9 files changed, 52 insertions(+), 17 deletions(-) diff --git a/sycl/include/CL/sycl/backend/level_zero.hpp b/sycl/include/CL/sycl/backend/level_zero.hpp index 70a04aa57f724..e46d597e0d8e9 100644 --- a/sycl/include/CL/sycl/backend/level_zero.hpp +++ b/sycl/include/CL/sycl/backend/level_zero.hpp @@ -51,12 +51,20 @@ struct interop &DeviceList, - pi_native_handle NativeHandle); + pi_native_handle NativeHandle, + bool keep_ownership = false); __SYCL_EXPORT program make_program(const context &Context, pi_native_handle NativeHandle); __SYCL_EXPORT queue make_queue(const context &Context, @@ -82,11 +90,17 @@ T make(const platform &Platform, /// created SYCL context. Provided devices and native context handle must /// be associated with the same platform. /// \param Interop is a Level Zero native context handle. +/// \param Ownership (optional) specifies who will assume ownership of the +/// native context handle. Default is that SYCL RT does, so it destroys +/// the native handle when the created SYCL object goes out of life. +/// template ::value>::type * = nullptr> T make(const vector_class &DeviceList, - typename interop::type Interop) { - return make_context(DeviceList, detail::pi::cast(Interop)); + typename interop::type Interop, + ownership Ownership = ownership::transfer) { + return make_context(DeviceList, detail::pi::cast(Interop), + Ownership == ownership::keep); } // Construction of SYCL program. diff --git a/sycl/include/CL/sycl/detail/pi.h b/sycl/include/CL/sycl/detail/pi.h index da4a71c03aba0..0b4cbadaff9ed 100644 --- a/sycl/include/CL/sycl/detail/pi.h +++ b/sycl/include/CL/sycl/detail/pi.h @@ -983,6 +983,8 @@ piextContextGetNativeHandle(pi_context context, pi_native_handle *nativeHandle); /// \param devices is the list of devices in the context. Parameter is ignored /// if devices can be queried from the context native handle for a /// backend. +/// \param ownNativeHandle tells if SYCL RT should assume the ownership of +/// the native handle, if it can. /// \param context is the PI context created from the native handle. /// \return PI_SUCCESS if successfully created pi_context from the handle. /// PI_OUT_OF_HOST_MEMORY if can't allocate memory for the pi_context @@ -991,7 +993,7 @@ piextContextGetNativeHandle(pi_context context, pi_native_handle *nativeHandle); /// native handle. PI_UNKNOWN_ERROR in case of another error. __SYCL_EXPORT pi_result piextContextCreateWithNativeHandle( pi_native_handle nativeHandle, pi_uint32 numDevices, - const pi_device *devices, pi_context *context); + const pi_device *devices, bool ownNativeHandle, pi_context *context); // // Queue diff --git a/sycl/plugins/cuda/pi_cuda.cpp b/sycl/plugins/cuda/pi_cuda.cpp index bced54a14d56f..3b77736b95c1b 100644 --- a/sycl/plugins/cuda/pi_cuda.cpp +++ b/sycl/plugins/cuda/pi_cuda.cpp @@ -1698,6 +1698,7 @@ pi_result cuda_piextContextGetNativeHandle(pi_context context, pi_result cuda_piextContextCreateWithNativeHandle(pi_native_handle nativeHandle, pi_uint32 num_devices, const pi_device *devices, + bool ownNativeHandle, pi_context *context) { cl::sycl::detail::pi::die( "Creation of PI context from native handle not implemented"); diff --git a/sycl/plugins/level_zero/pi_level_zero.cpp b/sycl/plugins/level_zero/pi_level_zero.cpp index cffac6745653c..6b04b4d835751 100644 --- a/sycl/plugins/level_zero/pi_level_zero.cpp +++ b/sycl/plugins/level_zero/pi_level_zero.cpp @@ -1954,7 +1954,7 @@ pi_result piContextCreate(const pi_context_properties *Properties, ZE_CALL(zeContextCreate((*Devices)->Platform->ZeDriver, &ContextDesc, &ZeContext)); try { - *RetContext = new _pi_context(ZeContext, NumDevices, Devices); + *RetContext = new _pi_context(ZeContext, NumDevices, Devices, true); (*RetContext)->initialize(); } catch (const std::bad_alloc &) { return PI_OUT_OF_HOST_MEMORY; @@ -2013,6 +2013,7 @@ pi_result piextContextGetNativeHandle(pi_context Context, pi_result piextContextCreateWithNativeHandle(pi_native_handle NativeHandle, pi_uint32 NumDevices, const pi_device *Devices, + bool OwnNativeHandle, pi_context *RetContext) { PI_ASSERT(NativeHandle, PI_INVALID_VALUE); PI_ASSERT(Devices, PI_INVALID_DEVICE); @@ -2021,7 +2022,7 @@ pi_result piextContextCreateWithNativeHandle(pi_native_handle NativeHandle, try { *RetContext = new _pi_context(pi_cast(NativeHandle), - NumDevices, Devices); + NumDevices, Devices, OwnNativeHandle); (*RetContext)->initialize(); } catch (const std::bad_alloc &) { return PI_OUT_OF_HOST_MEMORY; @@ -2059,7 +2060,9 @@ pi_result piContextRelease(pi_context Context) { // and therefore it must be valid at that point. // Technically it should be placed to the destructor of pi_context // but this makes API error handling more complex. - ZE_CALL(zeContextDestroy(ZeContext)); + if (Context->OwnZeContext) { + ZE_CALL(zeContextDestroy(ZeContext)); + } return Result; } diff --git a/sycl/plugins/level_zero/pi_level_zero.hpp b/sycl/plugins/level_zero/pi_level_zero.hpp index 0430a1bfb1df4..0a121429655ca 100644 --- a/sycl/plugins/level_zero/pi_level_zero.hpp +++ b/sycl/plugins/level_zero/pi_level_zero.hpp @@ -171,8 +171,9 @@ struct _pi_device : _pi_object { struct _pi_context : _pi_object { _pi_context(ze_context_handle_t ZeContext, pi_uint32 NumDevices, - const pi_device *Devs) - : ZeContext{ZeContext}, Devices{Devs, Devs + NumDevices}, + const pi_device *Devs, bool OwnZeContext) + : ZeContext{ZeContext}, + OwnZeContext{OwnZeContext}, Devices{Devs, Devs + NumDevices}, ZeCommandListInit{nullptr}, ZeEventPool{nullptr}, NumEventsAvailableInEventPool{}, NumEventsLiveInEventPool{} { // Create USM allocator context for each pair (device, context). @@ -201,6 +202,10 @@ struct _pi_context : _pi_object { // resources that may be used by multiple devices. ze_context_handle_t ZeContext; + // Indicates if we own the ZeContext or it came from interop that + // asked to not transfer the ownership to SYCL RT. + bool OwnZeContext; + // Keep the PI devices this PI context was created for. std::vector Devices; diff --git a/sycl/plugins/opencl/pi_opencl.cpp b/sycl/plugins/opencl/pi_opencl.cpp index 99d687724d74b..a77924a211081 100644 --- a/sycl/plugins/opencl/pi_opencl.cpp +++ b/sycl/plugins/opencl/pi_opencl.cpp @@ -536,10 +536,12 @@ pi_result piContextCreate(const pi_context_properties *properties, pi_result piextContextCreateWithNativeHandle(pi_native_handle nativeHandle, pi_uint32 num_devices, const pi_device *devices, + bool ownNativeHandle, pi_context *piContext) { (void)num_devices; (void)devices; assert(piContext != nullptr); + assert(ownNativeHandle == false); *piContext = reinterpret_cast(nativeHandle); return PI_SUCCESS; } diff --git a/sycl/source/backend/level_zero.cpp b/sycl/source/backend/level_zero.cpp index 935583c7d0532..5ab1712729e19 100644 --- a/sycl/source/backend/level_zero.cpp +++ b/sycl/source/backend/level_zero.cpp @@ -48,7 +48,8 @@ __SYCL_EXPORT device make_device(const platform &Platform, //---------------------------------------------------------------------------- // Implementation of level_zero::make __SYCL_EXPORT context make_context(const vector_class &DeviceList, - pi_native_handle NativeHandle) { + pi_native_handle NativeHandle, + bool KeepOwnership) { const auto &Plugin = pi::getPlugin(); // Create PI context first. pi_context PiContext; @@ -57,7 +58,8 @@ __SYCL_EXPORT context make_context(const vector_class &DeviceList, DeviceHandles.push_back(detail::getSyclObjImpl(Dev)->getHandleRef()); } Plugin.call( - NativeHandle, DeviceHandles.size(), DeviceHandles.data(), &PiContext); + NativeHandle, DeviceHandles.size(), DeviceHandles.data(), !KeepOwnership, + &PiContext); // Construct the SYCL context from PI context. return detail::createSyclObjFromImpl( std::make_shared(PiContext, async_handler{}, Plugin)); diff --git a/sycl/source/backend/opencl.cpp b/sycl/source/backend/opencl.cpp index aff0c89789aea..d5dae4b1805e1 100644 --- a/sycl/source/backend/opencl.cpp +++ b/sycl/source/backend/opencl.cpp @@ -51,7 +51,7 @@ __SYCL_EXPORT context make_context(pi_native_handle NativeHandle) { // Create PI context first. pi::PiContext PiContext; Plugin.call( - NativeHandle, 0, nullptr, &PiContext); + NativeHandle, 0, nullptr, false, &PiContext); // Construct the SYCL context from PI context. return detail::createSyclObjFromImpl( std::make_shared(PiContext, async_handler{}, Plugin)); diff --git a/sycl/source/detail/context_impl.cpp b/sycl/source/detail/context_impl.cpp index 666f12b226e61..3dbcc18d94fb3 100644 --- a/sycl/source/detail/context_impl.cpp +++ b/sycl/source/detail/context_impl.cpp @@ -56,7 +56,8 @@ context_impl::context_impl(const vector_class Devices, getPlugin().call( Props, DeviceIds.size(), DeviceIds.data(), nullptr, nullptr, &MContext); #else - cl::sycl::detail::pi::die("CUDA support was not enabled at compilation time"); + cl::sycl::detail::pi::die( + "CUDA support was not enabled at compilation time"); #endif } else { getPlugin().call(nullptr, DeviceIds.size(), @@ -96,7 +97,12 @@ context_impl::context_impl(RT::PiContext PiContext, async_handler AsyncHandler, // TODO catch an exception and put it to list of asynchronous exceptions // getPlugin() will be the same as the Plugin passed. This should be taken // care of when creating device object. - getPlugin().call(MContext); + // + // TODO: Move this backend-specific retain of the context to SYCL-2020 style + // make_context interop, when that is created. + if (getPlugin().getBackend() == cl::sycl::backend::opencl) { + getPlugin().call(MContext); + } MKernelProgramCache.setContextPtr(this); } @@ -153,8 +159,8 @@ KernelProgramCache &context_impl::getKernelProgramCache() const { return MKernelProgramCache; } -bool -context_impl::hasDevice(shared_ptr_class Device) const { +bool context_impl::hasDevice( + shared_ptr_class Device) const { for (auto D : MDevices) if (getSyclObjImpl(D) == Device) return true; From 1ae971564a4a5aa822f01c47b443b3958d62fc03 Mon Sep 17 00:00:00 2001 From: Sergey V Maslov Date: Sun, 21 Feb 2021 17:30:14 -0800 Subject: [PATCH 2/7] Review comments #1 Signed-off-by: Sergey V Maslov --- .../LevelZeroBackend/LevelZeroBackend.md | 37 +++++++++++++------ sycl/include/CL/sycl/detail/pi.h | 5 ++- sycl/test/abi/sycl_symbols_linux.dump | 8 ++-- 3 files changed, 33 insertions(+), 17 deletions(-) diff --git a/sycl/doc/extensions/LevelZeroBackend/LevelZeroBackend.md b/sycl/doc/extensions/LevelZeroBackend/LevelZeroBackend.md index 1c1322bb397dd..1a184a25a6324 100755 --- a/sycl/doc/extensions/LevelZeroBackend/LevelZeroBackend.md +++ b/sycl/doc/extensions/LevelZeroBackend/LevelZeroBackend.md @@ -87,7 +87,7 @@ a SYCL object that encapsulates a corresponding Level-Zero object: |-------------|:------------| |``` make(ze_driver_handle_t);```|Constructs a SYCL platform instance from a Level-Zero ```ze_driver_handle_t```.| |``` make(const platform &, ze_device_handle_t);```|Constructs a SYCL device instance from a Level-Zero ```ze_device_handle_t```. The platform argument gives a SYCL platform, encapsulating a Level-Zero driver supporting the passed Level-Zero device.| -|``` make(const vector_class &, ze_context_handle_t);```| Constructs a SYCL context instance from a Level-Zero ```ze_context_handle_t```. The context is created against the devices passed in. There must be at least one device given and all the devices must be from the same SYCL platform and thus from the same Level-Zero driver.| +|``` make(const vector_class &, ze_context_handle_t, ownership = transfer);```| Constructs a SYCL context instance from a Level-Zero ```ze_context_handle_t```. The context is created against the devices passed in. There must be at least one device given and all the devices must be from the same SYCL platform and thus from the same Level-Zero driver. The ```ownership``` argument specifies if SYCL RT should take ownership of the passed native handle. The default behavior is to transfer the ownership to SYCL RT. See section 4.4 for details.| |``` make(const context &, ze_command_queue_handle_t);```| Constructs a SYCL queue instance from a Level-Zero ```ze_command_queue_handle_t```. The context argument must be a valid SYCL context encapsulating a Level-Zero context. The queue is attached to the first device in the passed SYCL context.| |``` make(const context &, ze_module_handle_t);```| Constructs a SYCL program instance from a Level-Zero ```ze_module_handle_t```. The context argument must be a valid SYCL context encapsulating a Level-Zero context. The Level-Zero module must be fully linked (i.e. not require further linking through [```zeModuleDynamicLink```](https://spec.oneapi.com/level-zero/latest/core/api.html?highlight=zemoduledynamiclink#_CPPv419zeModuleDynamicLink8uint32_tP18ze_module_handle_tP28ze_module_build_log_handle_t)), and thus the SYCL program is created in the "linked" state.| @@ -96,23 +96,37 @@ NOTE: We shall consider adding other interoperability as needed, if possible. ### 4.4 Level-Zero handles' ownership and thread-safety The Level-Zero runtime doesn't do reference-counting of its objects, so it is crucial to adhere to these -practices of how Level-Zero handles are manged. +practices of how Level-Zero handles are manged. By default, the ownership us transferred to SYCL RT, but +some interoparability API supports overriding this behavior and keep the ownership in the application. +Use this enumeration for explicit specification of the ownership: +``` C++ +enum ownership { transfer, keep }; +``` -#### 4.4.1 SYCL runtime takes ownership +#### 4.4.1 SYCL runtime takes ownership (default) Whenever the application creates a SYCL object from the corresponding Level-Zero handle via one of the ```make()``` functions, -the SYCL runtime takes ownership of the Level-Zero handle. The application must not use the Level-Zero handle after -the last host copy of the SYCL object is destroyed (as described in the core SYCL specification under -"Common reference semantics"), and the application must not destroy the Level-Zero handle itself. +the SYCL runtime takes ownership of the Level-Zero handle, if no explicit ```ownership::keep``` was specified. +The application must not use the Level-Zero handle after the last host copy of the SYCL object is destroyed ( +as described in the core SYCL specification under "Common reference semantics"), and the application must not +destroy the Level-Zero handle itself. -#### 4.4.2 SYCL runtime assumes ownership +#### 4.4.2 SYCL runtime assumes ownership (default) The application may call the ```get_native()``` member function of a SYCL object to retrieve the underlying Level-Zero handle, -however, the SYCL runtime continues to retain ownership of this handle. The application must not use this handle after -the last host copy of the SYCL object is destroyed (as described in the core SYCL specification under -"Common reference semantics"), and the application must not destroy the Level-Zero handle. +however, the SYCL runtime continues to retain ownership of this handle, unless SYCL object was created with interoperability API that asked +to keep the ownership by the application with ```ownership::keep```. The application must not use this handle after the last host copy of +the SYCL object is destroyed (as described in the core SYCL specification under "Common reference semantics"), and the application must +not destroy the Level-Zero handle. + +#### 4.4.3 Application keeps ownership (explicit) + +If SYCL object is created with an interoperability API explicitly asking to keep the native handle ownership in the application with +```ownership::keep``` then SYCL RT does not take the ownership and will not destroy the Level-Zero handle at the destruction of the SYCL object. +Application is responsible for destroying the native handle when it no longer needs it, but not earlier than the SYCL object created with that +handle is EOL. -#### 4.4.3 Considerations for multi-threaded environment +#### 4.4.4 Considerations for multi-threaded environment The Level-Zero API is not thread-safe, refer to . Applications must make sure that the Level-Zero handles themselves aren't used simultaneously from different threads. @@ -123,4 +137,5 @@ the application should not attempt further direct use of those handles. |Rev|Date|Author|Changes| |-------------|:------------|:------------|:------------| |1|2021-01-26|Sergey Maslov|Initial public working draft +|2|2021-02-21|Sergey Maslov|Introduced explicit ownership for context diff --git a/sycl/include/CL/sycl/detail/pi.h b/sycl/include/CL/sycl/detail/pi.h index 0b4cbadaff9ed..b1003954f7a00 100644 --- a/sycl/include/CL/sycl/detail/pi.h +++ b/sycl/include/CL/sycl/detail/pi.h @@ -34,9 +34,10 @@ // pi_device_binary_property_set PropertySetsBegin; // pi_device_binary_property_set PropertySetsEnd; // 2. A number of types needed to define pi_device_binary_property_set added. +// 3. Added new ownership argument to piextContextCreateWithNativeHandle. // -#define _PI_H_VERSION_MAJOR 2 -#define _PI_H_VERSION_MINOR 3 +#define _PI_H_VERSION_MAJOR 3 +#define _PI_H_VERSION_MINOR 4 #define _PI_STRING_HELPER(a) #a #define _PI_CONCAT(a, b) _PI_STRING_HELPER(a.b) diff --git a/sycl/test/abi/sycl_symbols_linux.dump b/sycl/test/abi/sycl_symbols_linux.dump index 472f460b94c79..3c789f870fab6 100644 --- a/sycl/test/abi/sycl_symbols_linux.dump +++ b/sycl/test/abi/sycl_symbols_linux.dump @@ -3592,7 +3592,7 @@ _ZN2cl10__host_std9u_sub_satEmm _ZN2cl10__host_std9u_sub_satEtt _ZN2cl4sycl10level_zero10make_queueERKNS0_7contextEm _ZN2cl4sycl10level_zero11make_deviceERKNS0_8platformEm -_ZN2cl4sycl10level_zero12make_contextERKSt6vectorINS0_6deviceESaIS3_EEm +_ZN2cl4sycl10level_zero12make_contextERKSt6vectorINS0_6deviceESaIS3_EEmb _ZN2cl4sycl10level_zero12make_programERKNS0_7contextEm _ZN2cl4sycl10level_zero13make_platformEm _ZN2cl4sycl11malloc_hostEmRKNS0_5queueE @@ -3616,6 +3616,8 @@ _ZN2cl4sycl20aligned_alloc_sharedEmmRKNS0_5queueE _ZN2cl4sycl20aligned_alloc_sharedEmmRKNS0_6deviceERKNS0_7contextE _ZN2cl4sycl4freeEPvRKNS0_5queueE _ZN2cl4sycl4freeEPvRKNS0_7contextE +_ZN2cl4sycl5INTEL15online_compilerILNS1_15source_languageE0EE7compileIJSt6vectorINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESaISC_EEEEES6_IhSaIhEERKSC_DpRKT_ +_ZN2cl4sycl5INTEL15online_compilerILNS1_15source_languageE1EE7compileIJSt6vectorINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESaISC_EEEEES6_IhSaIhEERKSC_DpRKT_ _ZN2cl4sycl5event13get_wait_listEv _ZN2cl4sycl5event14wait_and_throwERKSt6vectorIS1_SaIS1_EE _ZN2cl4sycl5event14wait_and_throwEv @@ -3627,8 +3629,6 @@ _ZN2cl4sycl5eventC1Ev _ZN2cl4sycl5eventC2EP9_cl_eventRKNS0_7contextE _ZN2cl4sycl5eventC2ESt10shared_ptrINS0_6detail10event_implEE _ZN2cl4sycl5eventC2Ev -_ZN2cl4sycl5INTEL15online_compilerILNS1_15source_languageE0EE7compileIJSt6vectorINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESaISC_EEEEES6_IhSaIhEERKSC_DpRKT_ -_ZN2cl4sycl5INTEL15online_compilerILNS1_15source_languageE1EE7compileIJSt6vectorINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESaISC_EEEEES6_IhSaIhEERKSC_DpRKT_ _ZN2cl4sycl5queue10mem_adviseEPKvm14_pi_mem_advice _ZN2cl4sycl5queue10wait_proxyERKNS0_6detail13code_locationE _ZN2cl4sycl5queue11submit_implESt8functionIFvRNS0_7handlerEEERKNS0_6detail13code_locationE @@ -3964,6 +3964,7 @@ _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE16785EEENS3_12param_traitsIS4_XT_ _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE16786EEENS3_12param_traitsIS4_XT_EE11return_typeEv _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE16787EEENS3_12param_traitsIS4_XT_EE11return_typeEv _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE16788EEENS3_12param_traitsIS4_XT_EE11return_typeEv +_ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE16915EEENS3_12param_traitsIS4_XT_EE11return_typeEv _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE4096EEENS3_12param_traitsIS4_XT_EE11return_typeEv _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE4097EEENS3_12param_traitsIS4_XT_EE11return_typeEv _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE4098EEENS3_12param_traitsIS4_XT_EE11return_typeEv @@ -4039,7 +4040,6 @@ _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE4168EEENS3_12param_traitsIS4_XT_E _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE4169EEENS3_12param_traitsIS4_XT_EE11return_typeEv _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE4188EEENS3_12param_traitsIS4_XT_EE11return_typeEv _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE4189EEENS3_12param_traitsIS4_XT_EE11return_typeEv -_ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE16915EEENS3_12param_traitsIS4_XT_EE11return_typeEv _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE65568EEENS3_12param_traitsIS4_XT_EE11return_typeEv _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE65569EEENS3_12param_traitsIS4_XT_EE11return_typeEv _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE65570EEENS3_12param_traitsIS4_XT_EE11return_typeEv From c14a67b54f8edecbffc50c836aefa0cf596bbfa0 Mon Sep 17 00:00:00 2001 From: Sergey V Maslov Date: Sun, 21 Feb 2021 20:12:01 -0800 Subject: [PATCH 3/7] [SYCL] remember the context to destroy Signed-off-by: Sergey V Maslov --- sycl/plugins/level_zero/pi_level_zero.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sycl/plugins/level_zero/pi_level_zero.cpp b/sycl/plugins/level_zero/pi_level_zero.cpp index 6b04b4d835751..e3b5e29e2506d 100644 --- a/sycl/plugins/level_zero/pi_level_zero.cpp +++ b/sycl/plugins/level_zero/pi_level_zero.cpp @@ -2046,7 +2046,8 @@ pi_result piContextRelease(pi_context Context) { PI_ASSERT(Context, PI_INVALID_CONTEXT); if (--(Context->RefCount) == 0) { - auto ZeContext = Context->ZeContext; + ze_context_handle_t DestoryZeContext = + Context->OwnZeContext ? Context->ZeContext : nullptr; // Clean up any live memory associated with Context pi_result Result = Context->finalize(); @@ -2060,9 +2061,8 @@ pi_result piContextRelease(pi_context Context) { // and therefore it must be valid at that point. // Technically it should be placed to the destructor of pi_context // but this makes API error handling more complex. - if (Context->OwnZeContext) { - ZE_CALL(zeContextDestroy(ZeContext)); - } + if (DestoryZeContext) + ZE_CALL(zeContextDestroy(DestoryZeContext)); return Result; } From 3b97d3cafcb3c7b24e16462aa35ff5ee2c23493b Mon Sep 17 00:00:00 2001 From: Sergey V Maslov Date: Mon, 22 Feb 2021 10:05:17 -0800 Subject: [PATCH 4/7] review comments #2 Signed-off-by: Sergey V Maslov --- .../LevelZeroBackend/LevelZeroBackend.md | 38 +++++++++++-------- sycl/include/CL/sycl/backend/level_zero.hpp | 2 +- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/sycl/doc/extensions/LevelZeroBackend/LevelZeroBackend.md b/sycl/doc/extensions/LevelZeroBackend/LevelZeroBackend.md index 1a184a25a6324..60cc4fbe9890c 100755 --- a/sycl/doc/extensions/LevelZeroBackend/LevelZeroBackend.md +++ b/sycl/doc/extensions/LevelZeroBackend/LevelZeroBackend.md @@ -87,7 +87,7 @@ a SYCL object that encapsulates a corresponding Level-Zero object: |-------------|:------------| |``` make(ze_driver_handle_t);```|Constructs a SYCL platform instance from a Level-Zero ```ze_driver_handle_t```.| |``` make(const platform &, ze_device_handle_t);```|Constructs a SYCL device instance from a Level-Zero ```ze_device_handle_t```. The platform argument gives a SYCL platform, encapsulating a Level-Zero driver supporting the passed Level-Zero device.| -|``` make(const vector_class &, ze_context_handle_t, ownership = transfer);```| Constructs a SYCL context instance from a Level-Zero ```ze_context_handle_t```. The context is created against the devices passed in. There must be at least one device given and all the devices must be from the same SYCL platform and thus from the same Level-Zero driver. The ```ownership``` argument specifies if SYCL RT should take ownership of the passed native handle. The default behavior is to transfer the ownership to SYCL RT. See section 4.4 for details.| +|``` make(const vector_class &, ze_context_handle_t, ownership = transfer);```| Constructs a SYCL context instance from a Level-Zero ```ze_context_handle_t```. The context is created against the devices passed in. There must be at least one device given and all the devices must be from the same SYCL platform and thus from the same Level-Zero driver. The ```ownership``` argument specifies if the SYCL runtime should take ownership of the passed native handle. The default behavior is to transfer the ownership to the SYCL runtime. See section 4.4 for details.| |``` make(const context &, ze_command_queue_handle_t);```| Constructs a SYCL queue instance from a Level-Zero ```ze_command_queue_handle_t```. The context argument must be a valid SYCL context encapsulating a Level-Zero context. The queue is attached to the first device in the passed SYCL context.| |``` make(const context &, ze_module_handle_t);```| Constructs a SYCL program instance from a Level-Zero ```ze_module_handle_t```. The context argument must be a valid SYCL context encapsulating a Level-Zero context. The Level-Zero module must be fully linked (i.e. not require further linking through [```zeModuleDynamicLink```](https://spec.oneapi.com/level-zero/latest/core/api.html?highlight=zemoduledynamiclink#_CPPv419zeModuleDynamicLink8uint32_tP18ze_module_handle_tP28ze_module_build_log_handle_t)), and thus the SYCL program is created in the "linked" state.| @@ -96,11 +96,17 @@ NOTE: We shall consider adding other interoperability as needed, if possible. ### 4.4 Level-Zero handles' ownership and thread-safety The Level-Zero runtime doesn't do reference-counting of its objects, so it is crucial to adhere to these -practices of how Level-Zero handles are manged. By default, the ownership us transferred to SYCL RT, but +practices of how Level-Zero handles are managed. By default, the ownership is transferred to the SYCL runtime, but some interoparability API supports overriding this behavior and keep the ownership in the application. Use this enumeration for explicit specification of the ownership: ``` C++ -enum ownership { transfer, keep }; +namespace sycl { +namespace level_zero { + +enum class ownership { transfer, keep }; + +} // namesace level_zero +} // namespace sycl ``` #### 4.4.1 SYCL runtime takes ownership (default) @@ -110,21 +116,21 @@ the SYCL runtime takes ownership of the Level-Zero handle, if no explicit ```own The application must not use the Level-Zero handle after the last host copy of the SYCL object is destroyed ( as described in the core SYCL specification under "Common reference semantics"), and the application must not destroy the Level-Zero handle itself. - -#### 4.4.2 SYCL runtime assumes ownership (default) -The application may call the ```get_native()``` member function of a SYCL object to retrieve the underlying Level-Zero handle, -however, the SYCL runtime continues to retain ownership of this handle, unless SYCL object was created with interoperability API that asked -to keep the ownership by the application with ```ownership::keep```. The application must not use this handle after the last host copy of -the SYCL object is destroyed (as described in the core SYCL specification under "Common reference semantics"), and the application must -not destroy the Level-Zero handle. - -#### 4.4.3 Application keeps ownership (explicit) +#### 4.4.2 Application keeps ownership (explicit) If SYCL object is created with an interoperability API explicitly asking to keep the native handle ownership in the application with -```ownership::keep``` then SYCL RT does not take the ownership and will not destroy the Level-Zero handle at the destruction of the SYCL object. -Application is responsible for destroying the native handle when it no longer needs it, but not earlier than the SYCL object created with that -handle is EOL. +```ownership::keep``` then the SYCL runtime does not take the ownership and will not destroy the Level-Zero handle at the destruction of the SYCL object. +The application is responsible for destroying the native handle when it no longer needs it, but it must not destroy the +handle before the last host copy of the SYCL object is destroyed (as described in the core SYCL specification under +"Common reference semantics"). + +#### 4.4.3 Obtaining native handle does not change ownership + +The application may call the ```get_native()``` member function of a SYCL object to retrieve the underlying Level-Zero handle. +Doing so does not change the ownership of the the Level-Zero handle. Therefore, the application may not use this +handle after the last host copy of the SYCL object is destroyed (as described in the core SYCL specification under +"Common reference semantics") unless the SYCL object was created by the application with ```ownership::keep```. #### 4.4.4 Considerations for multi-threaded environment @@ -137,5 +143,5 @@ the application should not attempt further direct use of those handles. |Rev|Date|Author|Changes| |-------------|:------------|:------------|:------------| |1|2021-01-26|Sergey Maslov|Initial public working draft -|2|2021-02-21|Sergey Maslov|Introduced explicit ownership for context +|2|2021-02-22|Sergey Maslov|Introduced explicit ownership for context diff --git a/sycl/include/CL/sycl/backend/level_zero.hpp b/sycl/include/CL/sycl/backend/level_zero.hpp index e46d597e0d8e9..e7b8be77bba93 100644 --- a/sycl/include/CL/sycl/backend/level_zero.hpp +++ b/sycl/include/CL/sycl/backend/level_zero.hpp @@ -55,7 +55,7 @@ namespace level_zero { // be explicit about the ownership of the native handles used in the // interop functions below. // -enum ownership { transfer, keep }; +enum class ownership { transfer, keep }; // Implementation of various "make" functions resides in libsycl.so and thus // their interface needs to be backend agnostic. From 068a1f1d7c69dc335ec4c0c2c7f42469ffc38784 Mon Sep 17 00:00:00 2001 From: Sergey V Maslov Date: Wed, 24 Feb 2021 16:05:29 -0800 Subject: [PATCH 5/7] [SYCL] keep old version until can break ABI Signed-off-by: Sergey V Maslov --- sycl/source/backend/level_zero.cpp | 18 ++++++++++++++++++ sycl/test/abi/sycl_symbols_linux.dump | 1 + 2 files changed, 19 insertions(+) diff --git a/sycl/source/backend/level_zero.cpp b/sycl/source/backend/level_zero.cpp index 5ab1712729e19..46e3f7bd11ffa 100644 --- a/sycl/source/backend/level_zero.cpp +++ b/sycl/source/backend/level_zero.cpp @@ -47,6 +47,24 @@ __SYCL_EXPORT device make_device(const platform &Platform, //---------------------------------------------------------------------------- // Implementation of level_zero::make +// TODO: remove this version (without ownership) when allowed to break ABI. +__SYCL_EXPORT context make_context(const vector_class &DeviceList, + pi_native_handle NativeHandle) { + const auto &Plugin = pi::getPlugin(); + // Create PI context first. + pi_context PiContext; + vector_class DeviceHandles; + for (auto Dev : DeviceList) { + DeviceHandles.push_back(detail::getSyclObjImpl(Dev)->getHandleRef()); + } + Plugin.call( + NativeHandle, DeviceHandles.size(), DeviceHandles.data(), false, + &PiContext); + // Construct the SYCL context from PI context. + return detail::createSyclObjFromImpl( + std::make_shared(PiContext, async_handler{}, Plugin)); +} + __SYCL_EXPORT context make_context(const vector_class &DeviceList, pi_native_handle NativeHandle, bool KeepOwnership) { diff --git a/sycl/test/abi/sycl_symbols_linux.dump b/sycl/test/abi/sycl_symbols_linux.dump index 3c789f870fab6..f652421aaaab1 100644 --- a/sycl/test/abi/sycl_symbols_linux.dump +++ b/sycl/test/abi/sycl_symbols_linux.dump @@ -3592,6 +3592,7 @@ _ZN2cl10__host_std9u_sub_satEmm _ZN2cl10__host_std9u_sub_satEtt _ZN2cl4sycl10level_zero10make_queueERKNS0_7contextEm _ZN2cl4sycl10level_zero11make_deviceERKNS0_8platformEm +_ZN2cl4sycl10level_zero12make_contextERKSt6vectorINS0_6deviceESaIS3_EEm _ZN2cl4sycl10level_zero12make_contextERKSt6vectorINS0_6deviceESaIS3_EEmb _ZN2cl4sycl10level_zero12make_programERKNS0_7contextEm _ZN2cl4sycl10level_zero13make_platformEm From 11b340eea2270456e9d18f19a6cbb60e8846bcb5 Mon Sep 17 00:00:00 2001 From: Sergey V Maslov Date: Fri, 26 Feb 2021 12:11:53 -0800 Subject: [PATCH 6/7] review comments #3 Signed-off-by: Sergey V Maslov --- sycl/source/backend/level_zero.cpp | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/sycl/source/backend/level_zero.cpp b/sycl/source/backend/level_zero.cpp index 46e3f7bd11ffa..39ac1a28284c0 100644 --- a/sycl/source/backend/level_zero.cpp +++ b/sycl/source/backend/level_zero.cpp @@ -50,19 +50,7 @@ __SYCL_EXPORT device make_device(const platform &Platform, // TODO: remove this version (without ownership) when allowed to break ABI. __SYCL_EXPORT context make_context(const vector_class &DeviceList, pi_native_handle NativeHandle) { - const auto &Plugin = pi::getPlugin(); - // Create PI context first. - pi_context PiContext; - vector_class DeviceHandles; - for (auto Dev : DeviceList) { - DeviceHandles.push_back(detail::getSyclObjImpl(Dev)->getHandleRef()); - } - Plugin.call( - NativeHandle, DeviceHandles.size(), DeviceHandles.data(), false, - &PiContext); - // Construct the SYCL context from PI context. - return detail::createSyclObjFromImpl( - std::make_shared(PiContext, async_handler{}, Plugin)); + return make_context(DeviceList, NativeHandle, false); } __SYCL_EXPORT context make_context(const vector_class &DeviceList, From b6c76996b2e736904d1cd1bddd58d58582bb46a2 Mon Sep 17 00:00:00 2001 From: Sergey V Maslov Date: Fri, 26 Feb 2021 12:36:39 -0800 Subject: [PATCH 7/7] review comments #4 Signed-off-by: Sergey V Maslov --- sycl/source/backend/level_zero.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/sycl/source/backend/level_zero.cpp b/sycl/source/backend/level_zero.cpp index 39ac1a28284c0..22ceec88b799f 100644 --- a/sycl/source/backend/level_zero.cpp +++ b/sycl/source/backend/level_zero.cpp @@ -47,12 +47,6 @@ __SYCL_EXPORT device make_device(const platform &Platform, //---------------------------------------------------------------------------- // Implementation of level_zero::make -// TODO: remove this version (without ownership) when allowed to break ABI. -__SYCL_EXPORT context make_context(const vector_class &DeviceList, - pi_native_handle NativeHandle) { - return make_context(DeviceList, NativeHandle, false); -} - __SYCL_EXPORT context make_context(const vector_class &DeviceList, pi_native_handle NativeHandle, bool KeepOwnership) { @@ -71,6 +65,12 @@ __SYCL_EXPORT context make_context(const vector_class &DeviceList, std::make_shared(PiContext, async_handler{}, Plugin)); } +// TODO: remove this version (without ownership) when allowed to break ABI. +__SYCL_EXPORT context make_context(const vector_class &DeviceList, + pi_native_handle NativeHandle) { + return make_context(DeviceList, NativeHandle, false); +} + //---------------------------------------------------------------------------- // Implementation of level_zero::make __SYCL_EXPORT program make_program(const context &Context,