[SYCL] Fix get() method for non-opencl backends (#3070)

alexanderfle · web-flow · commit 5e7cbefb4d25 · 2021-01-29T12:01:01.000+03:00
Before this patch, if you use non-opencl backend, then get() method returns casted wrapper of the native handle and SYCL user can’t use it in his own code. As the user can't release the returned object, it used to lead to a memory leak.

Since, get() method is applicable only for OpenCL backend, this patch adds an exception for cases when get() method is called for non-opencl backend.

Signed-off-by: Alexander Flegontov &lt;alexander.flegontov@intel.com&gt;
diff --git a/sycl/source/context.cpp b/sycl/source/context.cpp
@@ -63,12 +63,14 @@ context::context(const vector_class<device> &DeviceList,
                                                   PropList);
   else {
     const device &NonHostDevice = *NonHostDeviceIter;
-    const auto &NonHostPlatform = NonHostDevice.get_platform().get();
+    const auto &NonHostPlatform =
+        detail::getSyclObjImpl(NonHostDevice.get_platform())->getHandleRef();
     if (std::any_of(DeviceList.begin(), DeviceList.end(),
                     [&](const device &CurrentDevice) {
-                        return (CurrentDevice.is_host() ||
-                                (CurrentDevice.get_platform().get() !=
-                                 NonHostPlatform));
+                      return (
+                          CurrentDevice.is_host() ||
+                          (detail::getSyclObjImpl(CurrentDevice.get_platform())
+                               ->getHandleRef() != NonHostPlatform));
                     }))
       throw invalid_parameter_error(
           "Can't add devices across platforms to a single context.",
diff --git a/sycl/source/detail/context_impl.cpp b/sycl/source/detail/context_impl.cpp
@@ -101,14 +101,14 @@ context_impl::context_impl(RT::PiContext PiContext, async_handler AsyncHandler,
 }
 
 cl_context context_impl::get() const {
-  if (!MHostContext) {
-    // TODO catch an exception and put it to list of asynchronous exceptions
-    getPlugin().call<PiApiKind::piContextRetain>(MContext);
-    return pi::cast<cl_context>(MContext);
+  if (MHostContext || getPlugin().getBackend() != cl::sycl::backend::opencl) {
+    throw invalid_object_error(
+        "This instance of context doesn't support OpenCL interoperability.",
+        PI_INVALID_CONTEXT);
   }
-  throw invalid_object_error(
-      "This instance of context doesn't support OpenCL interoperability.",
-      PI_INVALID_CONTEXT);
+  // TODO catch an exception and put it to list of asynchronous exceptions
+  getPlugin().call<PiApiKind::piContextRetain>(MContext);
+  return pi::cast<cl_context>(MContext);
 }
 
 bool context_impl::is_host() const { return MHostContext; }
diff --git a/sycl/source/detail/device_impl.cpp b/sycl/source/detail/device_impl.cpp
@@ -89,14 +89,13 @@ bool device_impl::is_affinity_supported(
 }
 
 cl_device_id device_impl::get() const {
-  if (MIsHostDevice)
-    throw invalid_object_error("This instance of device is a host instance",
-                               PI_INVALID_DEVICE);
-
-  const detail::plugin &Plugin = getPlugin();
-
+  if (MIsHostDevice || getPlugin().getBackend() != cl::sycl::backend::opencl) {
+    throw invalid_object_error(
+        "This instance of device doesn't support OpenCL interoperability.",
+        PI_INVALID_DEVICE);
+  }
   // TODO catch an exception and put it to list of asynchronous exceptions
-  Plugin.call<PiApiKind::piDeviceRetain>(MDevice);
+  getPlugin().call<PiApiKind::piDeviceRetain>(MDevice);
   return pi::cast<cl_device_id>(getNative());
 }
 
diff --git a/sycl/source/detail/event_impl.cpp b/sycl/source/detail/event_impl.cpp
@@ -35,13 +35,14 @@ extern xpti::trace_event_data_t *GSYCLGraphEvent;
 bool event_impl::is_host() const { return MHostEvent || !MOpenCLInterop; }
 
 cl_event event_impl::get() const {
-  if (MOpenCLInterop) {
-    getPlugin().call<PiApiKind::piEventRetain>(MEvent);
-    return pi::cast<cl_event>(MEvent);
+  if (!MOpenCLInterop ||
+      getPlugin().getBackend() != cl::sycl::backend::opencl) {
+    throw invalid_object_error(
+        "This instance of event doesn't support OpenCL interoperability.",
+        PI_INVALID_EVENT);
   }
-  throw invalid_object_error(
-      "This instance of event doesn't support OpenCL interoperability.",
-      PI_INVALID_EVENT);
+  getPlugin().call<PiApiKind::piEventRetain>(MEvent);
+  return pi::cast<cl_event>(MEvent);
 }
 
 event_impl::~event_impl() {
diff --git a/sycl/source/detail/kernel_impl.hpp b/sycl/source/detail/kernel_impl.hpp
@@ -80,9 +80,11 @@ class kernel_impl {
   ///
   /// \return a valid cl_kernel instance
   cl_kernel get() const {
-    if (is_host())
-      throw invalid_object_error("This instance of kernel is a host instance",
-                                 PI_INVALID_KERNEL);
+    if (is_host() || getPlugin().getBackend() != cl::sycl::backend::opencl) {
+      throw invalid_object_error(
+          "This instance of kernel doesn't support OpenCL interoperability.",
+          PI_INVALID_KERNEL);
+    }
     getPlugin().call<PiApiKind::piKernelRetain>(MKernel);
     return pi::cast<cl_kernel>(MKernel);
   }
diff --git a/sycl/source/detail/platform_impl.hpp b/sycl/source/detail/platform_impl.hpp
@@ -76,10 +76,11 @@ class platform_impl {
 
   /// \return an instance of OpenCL cl_platform_id.
   cl_platform_id get() const {
-    if (is_host())
-      throw invalid_object_error("This instance of platform is a host instance",
-                                 PI_INVALID_PLATFORM);
-
+    if (is_host() || getPlugin().getBackend() != cl::sycl::backend::opencl) {
+      throw invalid_object_error(
+          "This instance of platform doesn't support OpenCL interoperability.",
+          PI_INVALID_PLATFORM);
+    }
     return pi::cast<cl_platform_id>(MPlatform);
   }
 
diff --git a/sycl/source/detail/program_impl.cpp b/sycl/source/detail/program_impl.cpp
@@ -212,12 +212,12 @@ program_impl::~program_impl() {
 
 cl_program program_impl::get() const {
   throw_if_state_is(program_state::none);
-  if (is_host()) {
-    throw invalid_object_error("This instance of program is a host instance",
-                               PI_INVALID_PROGRAM);
+  if (is_host() || getPlugin().getBackend() != cl::sycl::backend::opencl) {
+    throw invalid_object_error(
+        "This instance of program doesn't support OpenCL interoperability.",
+        PI_INVALID_PROGRAM);
   }
-  const detail::plugin &Plugin = getPlugin();
-  Plugin.call<PiApiKind::piProgramRetain>(MProgram);
+  getPlugin().call<PiApiKind::piProgramRetain>(MProgram);
   return pi::cast<cl_program>(MProgram);
 }
 
diff --git a/sycl/source/detail/queue_impl.hpp b/sycl/source/detail/queue_impl.hpp
@@ -126,13 +126,13 @@ class queue_impl {
 
   /// \return an OpenCL interoperability queue handle.
   cl_command_queue get() {
-    if (!MHostQueue) {
-      getPlugin().call<PiApiKind::piQueueRetain>(MQueues[0]);
-      return pi::cast<cl_command_queue>(MQueues[0]);
+    if (MHostQueue || getPlugin().getBackend() != cl::sycl::backend::opencl) {
+      throw invalid_object_error(
+          "This instance of queue doesn't support OpenCL interoperability",
+          PI_INVALID_QUEUE);
     }
-    throw invalid_object_error(
-        "This instance of queue doesn't support OpenCL interoperability",
-        PI_INVALID_QUEUE);
+    getPlugin().call<PiApiKind::piQueueRetain>(MQueues[0]);
+    return pi::cast<cl_command_queue>(MQueues[0]);
   }
 
   /// \return an associated SYCL context.
diff --git a/sycl/source/detail/scheduler/commands.cpp b/sycl/source/detail/scheduler/commands.cpp
@@ -2010,8 +2010,6 @@ cl_int ExecCGCommand::enqueueImp() {
     ExecInterop->MInteropTask->call(InteropHandler);
     Plugin.call<PiApiKind::piEnqueueEventsWait>(MQueue->getHandleRef(), 0,
                                                 nullptr, &Event);
-    Plugin.call<PiApiKind::piQueueRelease>(
-        reinterpret_cast<pi_queue>(MQueue->get()));
 
     return CL_SUCCESS;
   }
diff --git a/sycl/test/basic_tests/platform.cpp b/sycl/test/basic_tests/platform.cpp
@@ -20,7 +20,11 @@ int main() {
   for (const auto &plt : platform::get_platforms()) {
     std::cout << "Platform " << i++
               << " is available: " << ((plt.is_host()) ? "host: " : "OpenCL: ")
-              << std::hex << ((plt.is_host()) ? nullptr : plt.get())
+              << std::hex
+              << ((plt.is_host() ||
+                   plt.get_backend() != cl::sycl::backend::opencl)
+                      ? nullptr
+                      : plt.get())
               << std::endl;
   }
 
@@ -34,7 +38,8 @@ int main() {
     platform MovedPlatform(std::move(Platform));
     assert(hash == hash_class<platform>()(MovedPlatform));
     assert(platformA.is_host() == MovedPlatform.is_host());
-    if (!platformA.is_host()) {
+    if (!platformA.is_host() &&
+        platformA.get_backend() == cl::sycl::backend::opencl) {
       assert(MovedPlatform.get() != nullptr);
     }
   }
@@ -46,7 +51,8 @@ int main() {
     WillMovedPlatform = std::move(Platform);
     assert(hash == hash_class<platform>()(WillMovedPlatform));
     assert(platformA.is_host() == WillMovedPlatform.is_host());
-    if (!platformA.is_host()) {
+    if (!platformA.is_host() &&
+        platformA.get_backend() == cl::sycl::backend::opencl) {
       assert(WillMovedPlatform.get() != nullptr);
     }
   }
diff --git a/sycl/unittests/scheduler/CommandsWaitForEvents.cpp b/sycl/unittests/scheduler/CommandsWaitForEvents.cpp
@@ -56,10 +56,10 @@ pi_result getEventInfoFunc(pi_event Event, pi_event_info PName, size_t PVSize,
 
   if (Event == TestContext->EventCtx1)
     *reinterpret_cast<pi_context *>(PV) =
-        reinterpret_cast<pi_context>(TestContext->Ctx1->get());
+        reinterpret_cast<pi_context>(TestContext->Ctx1->getHandleRef());
   else if (Event == TestContext->EventCtx2)
     *reinterpret_cast<pi_context *>(PV) =
-        reinterpret_cast<pi_context>(TestContext->Ctx2->get());
+        reinterpret_cast<pi_context>(TestContext->Ctx2->getHandleRef());
 
   return PI_SUCCESS;
 }
@@ -109,4 +109,7 @@ TEST_F(SchedulerTest, CommandsWaitForEvents) {
   ASSERT_TRUE(TestContext->EventCtx1WasWaited &&
               TestContext->EventCtx2WasWaited)
       << "Not all events were waited for";
+  delete TestContext.release(); // explicitly delete here is important for CUDA
+                                // BE to ensure that cuda driver is still in
+                                // memory while cuda objects are being freed.
 }

Original file line number	Diff line number	Diff line change
`@@ -2010,8 +2010,6 @@ cl_int ExecCGCommand::enqueueImp() {`
`2010`	`2010`	`ExecInterop->MInteropTask->call(InteropHandler);`
`2011`	`2011`	`Plugin.call<PiApiKind::piEnqueueEventsWait>(MQueue->getHandleRef(), 0,`
`2012`	`2012`	`nullptr, &Event);`
`2013`		`- Plugin.call<PiApiKind::piQueueRelease>(`
`2014`		`- reinterpret_cast<pi_queue>(MQueue->get()));`
`2015`	`2013`
`2016`	`2014`	`return CL_SUCCESS;`
`2017`	`2015`	`}`