[SYCL] Cache devices so they actually compare == when they should. #2092

jbrodman · 2020-07-10T18:59:13Z

Platform device enumeration was creating new device impls every time for the same devices. This was making it impossible for devices that really should compare equal to compare equal.

Example:
queue q1;
queue q2;

Each q uses the default selector to select the same device - yet they would not compare equal.

Signed-off-by: James Brodman [email protected]

Signed-off-by: James Brodman <[email protected]>

…e it involves control characters Signed-off-by: James Brodman <[email protected]>

Signed-off-by: James Brodman <[email protected]>

sycl/source/detail/platform_impl.cpp

keryell

This semantics is simpler to the user.
Note that triSYCL uses weak pointers instead of shared pointers because otherwise the devices might never been released...
https://github.com/triSYCL/triSYCL/blob/master/include/triSYCL/detail/cache.hpp

jbrodman · 2020-07-17T20:21:32Z

Note to self: Update this to also fix platforms (same place, same issue).

Signed-off-by: James Brodman <[email protected]>

sycl/source/detail/context_impl.cpp

sycl/source/detail/device_impl.cpp

sycl/source/detail/platform_impl.cpp

Signed-off-by: James Brodman <[email protected]>

jbrodman · 2020-08-20T20:26:44Z

I think I've addressed all your points, @romanovvlad. Good suggestions.

romanovvlad · 2020-08-21T09:49:39Z

sycl/source/detail/context_impl.cpp

-    MDevices.emplace_back(createSyclObjFromImpl<device>(
-        std::make_shared<device_impl>(Dev, Plugin)));
+  if (!DeviceIds.empty()) {
+    auto Platform =


Suggested change

auto Platform =

std::shared_ptr<detail::platform_impl> Platform =

romanovvlad · 2020-08-21T09:50:03Z

sycl/source/detail/context_impl.cpp

+  if (!DeviceIds.empty()) {
+    auto Platform =
+        platform_impl::getPlatformFromPiDevice(DeviceIds[0], Plugin);
+    for (auto Dev : DeviceIds) {


Suggested change

for (auto Dev : DeviceIds) {

for (RT::PiDevice Dev : DeviceIds) {

romanovvlad · 2020-08-21T09:51:13Z

sycl/source/detail/device_info.hpp

@@ -406,8 +407,10 @@ template <> struct get_device_info<device, info::device::parent_device> {
          "No parent for device because it is not a subdevice",
          PI_INVALID_DEVICE);

+    // Get the platform of this device
+    auto Platform = platform_impl::getPlatformFromPiDevice(dev, Plugin);


Suggested change

auto Platform = platform_impl::getPlatformFromPiDevice(dev, Plugin);

std::shared_ptr<detail::platform_impl> Platform = platform_impl::getPlatformFromPiDevice(dev, Plugin);

romanovvlad · 2020-08-21T09:52:07Z

sycl/source/detail/platform_impl.cpp

+  static PlatformImplPtr HostImpl;
+  static std::once_flag HostImplInit;
+
+  std::call_once(HostImplInit,
+                 []() { HostImpl = std::make_shared<platform_impl>(); });


Suggested change

static PlatformImplPtr HostImpl;

static std::once_flag HostImplInit;

std::call_once(HostImplInit,

[]() { HostImpl = std::make_shared<platform_impl>(); });

static PlatformImplPtr HostImpl = std::make_shared<platform_impl>();;

romanovvlad · 2020-08-21T09:52:47Z

sycl/source/detail/platform_impl.cpp

+
+  PlatformImplPtr Result;
+  {
+    const std::lock_guard<std::mutex> guard(PlatformMapMutex);


Suggested change

const std::lock_guard<std::mutex> guard(PlatformMapMutex);

const std::lock_guard<std::mutex> Guard(PlatformMapMutex);

romanovvlad · 2020-08-21T09:56:34Z

sycl/source/detail/platform_impl.cpp

+  const std::lock_guard<std::mutex> Guard(MDeviceMapMutex);
+
+  // If we've already seen this device, return the impl
+  for (const auto &Device : MDeviceCache) {


Suggested change

for (const auto &Device : MDeviceCache) {

for (const std::shared_ptr<device_impl>&Device : MDeviceCache) {

romanovvlad · 2020-08-21T09:59:08Z

sycl/source/detail/platform_impl.cpp

+  }
+
+  // Otherwise make the impl
+  auto Result = std::make_shared<device_impl>(PiDevice, PlatformImpl);


Suggested change

auto Result = std::make_shared<device_impl>(PiDevice, PlatformImpl);

std::shared_ptr<detail::device_impl> Result = std::make_shared<device_impl>(PiDevice, PlatformImpl);

romanovvlad · 2020-08-21T09:59:44Z

sycl/source/detail/platform_impl.cpp

-                       std::make_shared<device_impl>(
-                           PiDevice, std::make_shared<platform_impl>(*this)));
-                 });
+  auto PlatformImpl = getOrMakePlatformImpl(MPlatform, *MPlugin);


Suggested change

auto PlatformImpl = getOrMakePlatformImpl(MPlatform, *MPlugin);

PlatformImplPtr PlatformImpl = getOrMakePlatformImpl(MPlatform, *MPlugin);

romanovvlad · 2020-08-21T10:00:24Z

sycl/source/detail/platform_impl.hpp

@@ -14,6 +14,8 @@
 #include <detail/platform_info.hpp>
 #include <detail/plugin.hpp>

+#include <map>


It seems you do not need this header anymore.

romanovvlad · 2020-08-21T10:05:04Z

sycl/unittests/pi/PiMock.cpp

@@ -39,8 +39,8 @@ TEST(PiMockTest, ConstructFromQueue) {
      detail::getSyclObjImpl(Mock.getPlatform())->getPlugin().getPiPlugin();
  EXPECT_EQ(&MockedQueuePiPlugin, &PiMockPlugin)
      << "The mocked object and the PiMock instance must share the same plugin";
-  ASSERT_FALSE(&NormalPiPlugin == &MockedQueuePiPlugin)
-      << "Normal and mock platforms must not share the same plugin";
+  EXPECT_EQ(&NormalPiPlugin, &MockedQueuePiPlugin)


I guess it breaks design of the plugin mock.
@AGindinson Could you please comment?

Signed-off-by: James Brodman <[email protected]>

jbrodman · 2020-08-21T17:43:22Z

No clue what's up with the interop task test on windows

bader · 2020-08-21T18:10:00Z

No clue what's up with the interop task test on windows

@alexbatashev, I see this failure second time. Please, revert #2342 and investigate this failure.

The -opaque-pointers flag is no longer supported. Original commit: KhronosGroup/SPIRV-LLVM-Translator@ff8c3e7

Cache devices so they actually compare == when they should.

9c5ef98

Signed-off-by: James Brodman <[email protected]>

jbrodman requested a review from AGindinson July 10, 2020 18:59

jbrodman requested a review from a team as a code owner July 10, 2020 18:59

jbrodman requested review from rbegam and removed request for rbegam July 10, 2020 18:59

jbrodman added 3 commits July 10, 2020 15:03

Add test.

0aa8627

Signed-off-by: James Brodman <[email protected]>

Clang formatting changes where I can't tell the difference so I assum…

bddfd12

…e it involves control characters Signed-off-by: James Brodman <[email protected]>

Add header

e3ed628

Signed-off-by: James Brodman <[email protected]>

romanovvlad requested changes Jul 13, 2020

View reviewed changes

sycl/source/detail/platform_impl.cpp Outdated Show resolved Hide resolved

keryell reviewed Jul 14, 2020

View reviewed changes

jbrodman added 2 commits August 12, 2020 13:39

Merge branch 'sycl' into deviceref

3a76864

Add platform cache. Change device cache to be per platform.

a8fd94d

Signed-off-by: James Brodman <[email protected]>

jbrodman requested a review from romanovvlad August 13, 2020 20:02

jbrodman added 9 commits August 13, 2020 16:04

clang-format

a15c0fa

Signed-off-by: James Brodman <[email protected]>

clang-format

4bbad36

Signed-off-by: James Brodman <[email protected]>

Add support for host device and platform

b4de638

Signed-off-by: James Brodman <[email protected]>

Add documentation

f9cf973

Signed-off-by: James Brodman <[email protected]>

clang-format

cd8f3df

Signed-off-by: James Brodman <[email protected]>

clang-format 2

d4941b0

Signed-off-by: James Brodman <[email protected]>

clang-format 3

d962e6b

Signed-off-by: James Brodman <[email protected]>

clang-format 4 - seriously clang-format - WHAT DO YOU WANT FROM ME

67ff7a6

Signed-off-by: James Brodman <[email protected]>

replacing unintentionally removed pragma once

c1f736f

Signed-off-by: James Brodman <[email protected]>

romanovvlad requested changes Aug 20, 2020

View reviewed changes

Apply suggested changes. Use vectors instead of maps.

a42dbf4

Signed-off-by: James Brodman <[email protected]>

romanovvlad requested changes Aug 21, 2020

View reviewed changes

jbrodman added 3 commits August 21, 2020 10:48

Requested changes. De-auto.

500d6ba

Signed-off-by: James Brodman <[email protected]>

Remove unused var

db9f1b4

Signed-off-by: James Brodman <[email protected]>

Merge branch 'sycl' into deviceref

b970c2a

romanovvlad approved these changes Aug 24, 2020

View reviewed changes

romanovvlad merged commit d392b51 into intel:sycl Aug 24, 2020

jsji pushed a commit that referenced this pull request Aug 11, 2023

Remove last -opaque-pointers flag from test (#2092)

8ff51cd

The -opaque-pointers flag is no longer supported. Original commit: KhronosGroup/SPIRV-LLVM-Translator@ff8c3e7

	auto Platform =
	std::shared_ptr<detail::platform_impl> Platform =

	for (auto Dev : DeviceIds) {
	for (RT::PiDevice Dev : DeviceIds) {

	auto Platform = platform_impl::getPlatformFromPiDevice(dev, Plugin);
	std::shared_ptr<detail::platform_impl> Platform = platform_impl::getPlatformFromPiDevice(dev, Plugin);

	const std::lock_guard<std::mutex> guard(PlatformMapMutex);
	const std::lock_guard<std::mutex> Guard(PlatformMapMutex);

	for (const auto &Device : MDeviceCache) {
	for (const std::shared_ptr<device_impl>&Device : MDeviceCache) {

	auto Result = std::make_shared<device_impl>(PiDevice, PlatformImpl);
	std::shared_ptr<detail::device_impl> Result = std::make_shared<device_impl>(PiDevice, PlatformImpl);

	auto PlatformImpl = getOrMakePlatformImpl(MPlatform, *MPlugin);
	PlatformImplPtr PlatformImpl = getOrMakePlatformImpl(MPlatform, *MPlugin);

[SYCL] Cache devices so they actually compare == when they should. #2092

[SYCL] Cache devices so they actually compare == when they should. #2092

Uh oh!

Conversation

jbrodman commented Jul 10, 2020

Uh oh!

Uh oh!

keryell left a comment

Choose a reason for hiding this comment

Uh oh!

jbrodman commented Jul 17, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbrodman commented Aug 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbrodman commented Aug 21, 2020

Uh oh!

bader commented Aug 21, 2020

Uh oh!

Uh oh!