[SYCL] Adding caching when using interop constructor #3327

maximdimakov · 2021-03-09T09:24:54Z

When interop constructor is used it creates new impl every time. This patch allows to use already created impls in the constructor.

Signed-off-by: mdimakov <[email protected]>

alexbatashev · 2021-03-09T09:47:37Z

sycl/include/CL/sycl/platform.hpp

+  static std::unordered_map<cl_platform_id,
+                            std::weak_ptr<detail::platform_impl>>
+      platform_impls;
+  static std::mutex platform_mutex;


This isn't going to work properly. Both unordered_map and mutex are non-trivial classes. See this document for more info.

alexbatashev · 2021-03-09T09:49:24Z

sycl/include/CL/sycl/platform.hpp

+    auto cl_platform =
+        reinterpret_cast<typename interop<BackendName, platform>::type>(
+            getNative());
+    std::lock_guard<std::mutex> lock(platform_mutex);
+    platform_impls[cl_platform] = impl;
+    return cl_platform;


Nit

Suggested change

auto cl_platform =

reinterpret_cast<typename interop<BackendName, platform>::type>(

getNative());

std::lock_guard<std::mutex> lock(platform_mutex);

platform_impls[cl_platform] = impl;

return cl_platform;

auto ClPlatform =

reinterpret_cast<typename interop<BackendName, platform>::type>(

getNative());

std::lock_guard<std::mutex> Lock(platform_mutex);

platform_impls[ClPlatform] = impl;

return ClPlatform;

alexbatashev · 2021-03-09T09:50:12Z

sycl/include/CL/sycl/device.hpp

+  static std::unordered_map<cl_device_id, std::weak_ptr<detail::device_impl>>
+      device_impls;
+  static std::mutex device_mutex;


See my comment for the same piece in platform class.

alexbatashev · 2021-03-09T09:52:28Z

sycl/source/device.cpp

+    std::lock_guard<std::mutex> lock(device_mutex);
+    auto it = device_impls.find(deviceId);
+    if (it != device_impls.end() && !it->second.expired())
+      impl = it->second.lock();
+    else {
+      impl = std::make_shared<detail::device_impl>(
+          detail::pi::cast<pi_native_handle>(deviceId),
+          RT::getPlugin<backend::opencl>());
+      if (it == device_impls.end())
+        device_impls[deviceId] = impl;
+      else
+        it->second = impl;
+    }


Nit

Suggested change

std::lock_guard<std::mutex> lock(device_mutex);

auto it = device_impls.find(deviceId);

if (it != device_impls.end() && !it->second.expired())

impl = it->second.lock();

else {

impl = std::make_shared<detail::device_impl>(

detail::pi::cast<pi_native_handle>(deviceId),

RT::getPlugin<backend::opencl>());

if (it == device_impls.end())

device_impls[deviceId] = impl;

else

it->second = impl;

}

std::lock_guard<std::mutex> Lock(device_mutex);

auto It = DeviceImpls.find(DeviceId);

if (It != DeviceImpls.end() && !It->second.expired())

impl = It->second.lock();

else {

impl = std::make_shared<detail::device_impl>(

detail::pi::cast<pi_native_handle>(DeviceId),

RT::getPlugin<backend::opencl>());

if (it == DeviceImpls.end())

DeviceImpls[DeviceId] = impl;

else

It->second = impl;

}

romanovvlad · 2021-03-09T10:45:12Z

sycl/include/CL/sycl/device.hpp

@@ -192,6 +195,10 @@ class __SYCL_EXPORT device {
  shared_ptr_class<detail::device_impl> impl;
  device(shared_ptr_class<detail::device_impl> impl) : impl(impl) {}

+  static std::unordered_map<cl_device_id, std::weak_ptr<detail::device_impl>>
+      device_impls;


Is it possible to reuse already existing cache, see platform_impl.hpp:199 (MDeviceCache) ?

Yes, it is possible, testing it. I didn't see that when trying to implement caching with map

I pushed another solution with reusing an existing code

Signed-off-by: mdimakov <[email protected]>

alexbatashev

Otherwise LGTM

alexbatashev · 2021-03-12T06:15:00Z

sycl/include/CL/sycl/device.hpp

@@ -16,7 +16,6 @@
 #include <CL/sycl/platform.hpp>
 #include <CL/sycl/stl.hpp>

-#include <memory>


memory was actually used for shared_ptr.

It was deleted when I tested map. But memory is included in stl.hpp, that included higher. Do I need to return it?

Yes. You can't rely on CL/sycl/stl.hpp to include memory header for you.

Returned it

Signed-off-by: mdimakov <[email protected]>

* upstream/sycl: (1804 commits) [SYCL] SYCL 2020 backend interoperability part 1 (intel#3354) [Driver][SYCL] Address issue when unbundling for non-FPGA archive (intel#3366) [SYCL][Doc] Update compiler options descriptions (intel#3340) [SYCL] Update ABI dump tool to disable checks with libcxx by default (intel#3370) [SYCL] Update the way we handle -sycl-std= based on community review feedback (intel#3371) [SYCL] Move tests to llvm-test-suite (intel#3359) [SYCL][PI][L0] Revert copy batching from intel#3232 (intel#3363) [SYCL] Remove unsupported option. [SYCL] Fix pragma setting in sycl-post-link (intel#3358) [SYCL] Add ITT annotation instructions (intel#3299) [SYCL] Propagate attributes of original kernel to wrapper kernel generated for range-rounding (intel#3306) [BuildBot] Uplift GPU RT version for Linux to 21.09.19150 (intel#3316) [SYCL] Retain PI events until they have signaled (intel#3350) [SYCL] Add caching when using interop constructor (intel#3327) Add ITT stubs and wrappers for SPIR-V devices (intel#3279) [SYCL] Add zero argument version of buffer::reinterpret() for SYCL 2020 (intel#3333) [SYCL] Restore old behavior of get() method (intel#3356) [Driver][SYCL][FPGA] Improve FPGA AOT when using Triple (intel#3330) [SYCL] Revert support for pinned_host_memory extension in Level-Zero backend. Make it a NOP (intel#3349) [SYCL] Remove redundant build options processing (intel#3342) ...

maximdimakov added 3 commits March 5, 2021 11:54

[SYCL] Add caching into device

45e9dd8

Signed-off-by: mdimakov <[email protected]>

[SYCL] Add checking for weak_ptr into device

a20af7e

Signed-off-by: mdimakov <[email protected]>

[SYCL] Add caching for device and platform

b5e45fc

Signed-off-by: mdimakov <[email protected]>

maximdimakov requested a review from a team as a code owner March 9, 2021 09:24

maximdimakov requested a review from alexbatashev March 9, 2021 09:24

maximdimakov added 2 commits March 9, 2021 12:34

[SYCL] Clang-format fix

1088259

Signed-off-by: mdimakov <[email protected]>

[SYCL] Clang-format fix

6fdff7e

Signed-off-by: mdimakov <[email protected]>

alexbatashev suggested changes Mar 9, 2021

View reviewed changes

alexbatashev requested a review from romanovvlad March 9, 2021 09:53

romanovvlad reviewed Mar 9, 2021

View reviewed changes

maximdimakov added 3 commits March 10, 2021 22:16

Resolve merge conflict

a88ccbd

Merge sycl into caching_interop

5c014bd

[SYCL] Use existing caching implementation

b72b4cf

Signed-off-by: mdimakov <[email protected]>

maximdimakov requested review from elizabethandrews and premanandrao as code owners March 11, 2021 18:48

maximdimakov added 2 commits March 11, 2021 21:50

Remove problems

e010395

Clang-format fix

89179d0

Signed-off-by: mdimakov <[email protected]>

alexbatashev previously approved these changes Mar 12, 2021

View reviewed changes

Adress review comment

3221fc2

Signed-off-by: mdimakov <[email protected]>

maximdimakov dismissed alexbatashev’s stale review via 3221fc2 March 12, 2021 08:34

alexbatashev approved these changes Mar 12, 2021

View reviewed changes

romanovvlad approved these changes Mar 12, 2021

View reviewed changes

bader merged commit d3aeb4a into intel:sycl Mar 15, 2021

maximdimakov mentioned this pull request Mar 16, 2021

[SYCL] Test for device/platform interop constructor intel/llvm-test-suite#183

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Adding caching when using interop constructor #3327

[SYCL] Adding caching when using interop constructor #3327

maximdimakov commented Mar 9, 2021

alexbatashev Mar 9, 2021

alexbatashev Mar 9, 2021

alexbatashev Mar 9, 2021

alexbatashev Mar 9, 2021

romanovvlad Mar 9, 2021

maximdimakov Mar 10, 2021

maximdimakov Mar 11, 2021

alexbatashev left a comment

alexbatashev Mar 12, 2021

maximdimakov Mar 12, 2021

bader Mar 12, 2021

maximdimakov Mar 12, 2021

[SYCL] Adding caching when using interop constructor #3327

[SYCL] Adding caching when using interop constructor #3327

Conversation

maximdimakov commented Mar 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexbatashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment