From 2ec2ffb33281df888a19216bd487e56cdeef7b8c Mon Sep 17 00:00:00 2001 From: "Larsen, Steffen" Date: Tue, 21 Mar 2023 07:58:21 -0700 Subject: [PATCH 1/3] [SYCL] Fix weak_object and owner_less for device objects This commit fixes an issue where weak_object and in turn owner_less would fail to construct for SYCL object types that are usable on device. This was due to these object types only having an impl on host. As a result the kernel compilation would fail to get the impl of these. Since weak_object isn't intended for use inside kernels, this was fixed by using a dummy weak_ptr during kernel compilation to make it act like it is containing a weak pointer. Previously this was not found in unittests because they are not compiled with the SYCL compiler. To avoid this breaking in the future, these tests are moved to the test-suite. Signed-off-by: Larsen, Steffen --- sycl/include/sycl/accessor.hpp | 8 +- sycl/include/sycl/detail/owner_less_base.hpp | 7 + sycl/include/sycl/ext/oneapi/weak_object.hpp | 18 +- .../sycl/ext/oneapi/weak_object_base.hpp | 21 +- sycl/unittests/Extensions/CMakeLists.txt | 1 - sycl/unittests/Extensions/WeakObject.cpp | 430 ------------------ 6 files changed, 49 insertions(+), 436 deletions(-) delete mode 100644 sycl/unittests/Extensions/WeakObject.cpp diff --git a/sycl/include/sycl/accessor.hpp b/sycl/include/sycl/accessor.hpp index a98bc611a5c84..f31a020256a16 100644 --- a/sycl/include/sycl/accessor.hpp +++ b/sycl/include/sycl/accessor.hpp @@ -535,7 +535,7 @@ class __SYCL_EXPORT LocalAccessorBaseHost { protected: template - friend decltype(Obj::impl) getSyclObjImpl(const Obj &SyclObject); + friend decltype(Obj::impl) detail::getSyclObjImpl(const Obj &SyclObject); template friend T detail::createSyclObjFromImpl(decltype(T::impl) ImplObj); @@ -1209,6 +1209,9 @@ class __SYCL_EBO __SYCL_SPECIAL_CLASS __SYCL_TYPE(accessor) accessor : friend class sycl::stream; friend class sycl::ext::intel::esimd::detail::AccessorPrivateProxy; + template + friend decltype(Obj::impl) detail::getSyclObjImpl(const Obj &SyclObject); + template friend T detail::createSyclObjFromImpl(decltype(T::impl) ImplObj); @@ -2527,6 +2530,9 @@ class __SYCL_SPECIAL_CLASS local_accessor_base : return Result; } + template + friend decltype(Obj::impl) detail::getSyclObjImpl(const Obj &SyclObject); + template friend T detail::createSyclObjFromImpl(decltype(T::impl) ImplObj); diff --git a/sycl/include/sycl/detail/owner_less_base.hpp b/sycl/include/sycl/detail/owner_less_base.hpp index 026fe10c04d42..d082afd954d1c 100644 --- a/sycl/include/sycl/detail/owner_less_base.hpp +++ b/sycl/include/sycl/detail/owner_less_base.hpp @@ -42,6 +42,13 @@ template class OwnerLessBase { return getSyclObjImpl(*static_cast(this)) .owner_before(getSyclObjImpl(Other)); } +#else + // On device calls to these functions are disallowed, so declare them but + // don't define them to avoid compilation failures. + bool ext_oneapi_owner_before( + const ext::oneapi::detail::weak_object_base &Other) + const noexcept; + bool ext_oneapi_owner_before(const SyclObjT &Other) const noexcept; #endif }; diff --git a/sycl/include/sycl/ext/oneapi/weak_object.hpp b/sycl/include/sycl/ext/oneapi/weak_object.hpp index 33d797e77f57a..7d74486ac6d46 100644 --- a/sycl/include/sycl/ext/oneapi/weak_object.hpp +++ b/sycl/include/sycl/ext/oneapi/weak_object.hpp @@ -50,12 +50,13 @@ class weak_object : public detail::weak_object_base { weak_object &operator=(const SYCLObjT &SYCLObj) noexcept { // Create weak_ptr from the shared_ptr to SYCLObj's implementation object. - this->MObjWeakPtr = sycl::detail::getSyclObjImpl(SYCLObj); + this->MObjWeakPtr = GetWeakImpl(SYCLObj); return *this; } weak_object &operator=(const weak_object &Other) noexcept = default; weak_object &operator=(weak_object &&Other) noexcept = default; +#ifndef __SYCL_DEVICE_ONLY__ std::optional try_lock() const noexcept { auto MObjImplPtr = this->MObjWeakPtr.lock(); if (!MObjImplPtr) @@ -69,6 +70,12 @@ class weak_object : public detail::weak_object_base { "Referenced object has expired."); return *OptionalObj; } +#else + // On device calls to these functions are disallowed, so declare them but + // don't define them to avoid compilation failures. + std::optional try_lock() const noexcept; + SYCLObjT lock() const; +#endif // __SYCL_DEVICE_ONLY__ }; // Specialization of weak_object for buffer as it needs additional members @@ -96,7 +103,7 @@ class weak_object> weak_object &operator=(const buffer_type &SYCLObj) noexcept { // Create weak_ptr from the shared_ptr to SYCLObj's implementation object. - this->MObjWeakPtr = sycl::detail::getSyclObjImpl(SYCLObj); + this->MObjWeakPtr = GetWeakImpl(SYCLObj); this->MRange = SYCLObj.Range; this->MOffsetInBytes = SYCLObj.OffsetInBytes; this->MIsSubBuffer = SYCLObj.IsSubBuffer; @@ -105,6 +112,7 @@ class weak_object> weak_object &operator=(const weak_object &Other) noexcept = default; weak_object &operator=(weak_object &&Other) noexcept = default; +#ifndef __SYCL_DEVICE_ONLY__ std::optional try_lock() const noexcept { auto MObjImplPtr = this->MObjWeakPtr.lock(); if (!MObjImplPtr) @@ -119,6 +127,12 @@ class weak_object> "Referenced object has expired."); return *OptionalObj; } +#else + // On device calls to these functions are disallowed, so declare them but + // don't define them to avoid compilation failures. + std::optional try_lock() const noexcept; + buffer_type lock() const; +#endif // __SYCL_DEVICE_ONLY__ private: // Additional members required for recreating buffers. diff --git a/sycl/include/sycl/ext/oneapi/weak_object_base.hpp b/sycl/include/sycl/ext/oneapi/weak_object_base.hpp index 7dc10e7e86e1a..71efcf2fe23b1 100644 --- a/sycl/include/sycl/ext/oneapi/weak_object_base.hpp +++ b/sycl/include/sycl/ext/oneapi/weak_object_base.hpp @@ -29,7 +29,7 @@ template class weak_object_base { constexpr weak_object_base() noexcept : MObjWeakPtr() {} weak_object_base(const SYCLObjT &SYCLObj) noexcept - : MObjWeakPtr(sycl::detail::getSyclObjImpl(SYCLObj)) {} + : MObjWeakPtr(GetWeakImpl(SYCLObj)) {} weak_object_base(const weak_object_base &Other) noexcept = default; weak_object_base(weak_object_base &&Other) noexcept = default; @@ -43,19 +43,36 @@ template class weak_object_base { bool expired() const noexcept { return MObjWeakPtr.expired(); } +#ifndef __SYCL_DEVICE_ONLY__ bool owner_before(const SYCLObjT &Other) const noexcept { - return MObjWeakPtr.owner_before(sycl::detail::getSyclObjImpl(Other)); + return MObjWeakPtr.owner_before(GetWeakImpl(Other)); } bool owner_before(const weak_object_base &Other) const noexcept { return MObjWeakPtr.owner_before(Other.MObjWeakPtr); } +#else + // On device calls to these functions are disallowed, so declare them but + // don't define them to avoid compilation failures. + bool owner_before(const SYCLObjT &Other) const noexcept; + bool owner_before(const weak_object_base &Other) const noexcept; +#endif // __SYCL_DEVICE_ONLY__ protected: +#ifndef __SYCL_DEVICE_ONLY__ // Store a weak variant of the impl in the SYCLObjT. typename std::invoke_result_t< decltype(sycl::detail::getSyclObjImpl), SYCLObjT>::weak_type MObjWeakPtr; + static decltype(MObjWeakPtr) GetWeakImpl(const SYCLObjT &SYCLObj) { + return sycl::detail::getSyclObjImpl(SYCLObj); + } +#else + // On device we may not have an impl, so we pad with an unused void pointer. + std::weak_ptr MObjWeakPtr; + static std::weak_ptr GetWeakImpl(const SYCLObjT &) { return {}; } +#endif // __SYCL_DEVICE_ONLY__ + template friend decltype(weak_object_base::MObjWeakPtr) detail::getSyclWeakObjImpl(const weak_object_base &WeakObj); diff --git a/sycl/unittests/Extensions/CMakeLists.txt b/sycl/unittests/Extensions/CMakeLists.txt index d144d3641b1db..ebe99f15ec84d 100644 --- a/sycl/unittests/Extensions/CMakeLists.txt +++ b/sycl/unittests/Extensions/CMakeLists.txt @@ -4,7 +4,6 @@ add_sycl_unittest(ExtensionsTests OBJECT DefaultContext.cpp FPGADeviceSelectors.cpp DeviceArchitecture.cpp - WeakObject.cpp USMMemcpy2D.cpp DeviceGlobal.cpp OneAPISubGroupMask.cpp diff --git a/sycl/unittests/Extensions/WeakObject.cpp b/sycl/unittests/Extensions/WeakObject.cpp deleted file mode 100644 index 8af9974bdba4a..0000000000000 --- a/sycl/unittests/Extensions/WeakObject.cpp +++ /dev/null @@ -1,430 +0,0 @@ -//==------------------------- WeakObject.cpp -------------------------------==// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// - -#include - -#include -#include - -#include - -template struct WeakObjectCheckExpired { - void operator()(SyclObjT Obj) { - sycl::ext::oneapi::weak_object WeakObj{Obj}; - sycl::ext::oneapi::weak_object NullWeakObj; - - EXPECT_FALSE(WeakObj.expired()); - EXPECT_TRUE(NullWeakObj.expired()); - } -}; - -template struct WeakObjectCheckTryLock { - void operator()(SyclObjT Obj) { - sycl::ext::oneapi::weak_object WeakObj{Obj}; - sycl::ext::oneapi::weak_object NullWeakObj; - - std::optional TLObj = WeakObj.try_lock(); - std::optional TLNull = NullWeakObj.try_lock(); - - EXPECT_TRUE(TLObj.has_value()); - EXPECT_FALSE(TLNull.has_value()); - - EXPECT_TRUE(TLObj.value() == Obj); - } -}; - -template struct WeakObjectCheckLock { - void operator()(SyclObjT Obj) { - sycl::ext::oneapi::weak_object WeakObj{Obj}; - sycl::ext::oneapi::weak_object NullWeakObj; - - SyclObjT LObj = WeakObj.lock(); - EXPECT_TRUE(LObj == Obj); - - try { - SyclObjT LNull = NullWeakObj.lock(); - FAIL() << "Locking empty weak object did not throw."; - } catch (sycl::exception &E) { - EXPECT_EQ(E.code(), sycl::make_error_code(sycl::errc::invalid)) - << "Unexpected thrown error code."; - } - } -}; - -template struct WeakObjectCheckOwnerBefore { - void operator()(SyclObjT Obj) { - sycl::ext::oneapi::weak_object WeakObj{Obj}; - sycl::ext::oneapi::weak_object NullWeakObj; - - EXPECT_TRUE((WeakObj.owner_before(NullWeakObj) && - !NullWeakObj.owner_before(WeakObj)) || - (NullWeakObj.owner_before(WeakObj) && - !WeakObj.owner_before(NullWeakObj))); - - EXPECT_FALSE(WeakObj.owner_before(Obj)); - EXPECT_FALSE(Obj.ext_oneapi_owner_before(WeakObj)); - - EXPECT_FALSE(Obj.ext_oneapi_owner_before(Obj)); - } -}; - -template struct WeakObjectCheckOwnerLess { - void operator()(SyclObjT Obj) { - sycl::ext::oneapi::weak_object WeakObj{Obj}; - sycl::ext::oneapi::weak_object NullWeakObj; - sycl::ext::oneapi::owner_less Comparator; - - EXPECT_TRUE((Comparator(WeakObj, NullWeakObj) && - !Comparator(NullWeakObj, WeakObj)) || - (Comparator(NullWeakObj, WeakObj) && - !Comparator(WeakObj, NullWeakObj))); - - EXPECT_FALSE(Comparator(WeakObj, Obj)); - EXPECT_FALSE(Comparator(Obj, WeakObj)); - } -}; - -template struct WeakObjectCheckReset { - void operator()(SyclObjT Obj) { - sycl::ext::oneapi::weak_object WeakObj{Obj}; - sycl::ext::oneapi::weak_object NullWeakObj; - - WeakObj.reset(); - EXPECT_TRUE(WeakObj.expired()); - EXPECT_FALSE(WeakObj.owner_before(NullWeakObj)); - EXPECT_FALSE(NullWeakObj.owner_before(WeakObj)); - - std::optional TLObj = WeakObj.try_lock(); - EXPECT_FALSE(TLObj.has_value()); - - try { - SyclObjT LObj = WeakObj.lock(); - FAIL() << "Locking reset weak object did not throw."; - } catch (sycl::exception &E) { - EXPECT_EQ(E.code(), sycl::make_error_code(sycl::errc::invalid)) - << "Unexpected thrown error code."; - } - } -}; - -template struct WeakObjectCheckOwnerLessMulti { - void operator()(SyclObjT Obj1, SyclObjT Obj2) { - sycl::ext::oneapi::weak_object WeakObj1{Obj1}; - sycl::ext::oneapi::weak_object WeakObj2{Obj2}; - sycl::ext::oneapi::owner_less Comparator; - - EXPECT_TRUE( - (Comparator(WeakObj1, WeakObj2) && !Comparator(WeakObj2, WeakObj1)) || - (Comparator(WeakObj2, WeakObj1) && !Comparator(WeakObj1, WeakObj2))); - - EXPECT_FALSE(Comparator(WeakObj1, Obj1)); - EXPECT_FALSE(Comparator(Obj1, WeakObj1)); - - EXPECT_FALSE(Comparator(WeakObj2, Obj2)); - EXPECT_FALSE(Comparator(Obj2, WeakObj2)); - } -}; - -template struct WeakObjectCheckOwnerBeforeMulti { - void operator()(SyclObjT Obj1, SyclObjT Obj2) { - sycl::ext::oneapi::weak_object WeakObj1{Obj1}; - sycl::ext::oneapi::weak_object WeakObj2{Obj2}; - - EXPECT_TRUE( - (WeakObj1.owner_before(WeakObj2) && !WeakObj2.owner_before(WeakObj1)) || - (WeakObj2.owner_before(WeakObj1) && !WeakObj1.owner_before(WeakObj2))); - - EXPECT_FALSE(WeakObj1.owner_before(Obj1)); - EXPECT_FALSE(Obj1.ext_oneapi_owner_before(WeakObj1)); - - EXPECT_FALSE(WeakObj2.owner_before(Obj2)); - EXPECT_FALSE(Obj2.ext_oneapi_owner_before(WeakObj2)); - - EXPECT_TRUE((Obj1.ext_oneapi_owner_before(Obj2) && - !Obj2.ext_oneapi_owner_before(Obj1)) || - (Obj2.ext_oneapi_owner_before(Obj1) && - !Obj1.ext_oneapi_owner_before(Obj2))); - } -}; - -template struct WeakObjectCheckOwnerLessMap { - void operator()(SyclObjT Obj1, SyclObjT Obj2) { - sycl::ext::oneapi::weak_object WeakObj1{Obj1}; - sycl::ext::oneapi::weak_object WeakObj2{Obj2}; - - std::map, int, - sycl::ext::oneapi::owner_less> - Map; - Map[WeakObj1] = 1; - Map[WeakObj2] = 2; - - EXPECT_EQ(Map.size(), (size_t)2); - EXPECT_EQ(Map[WeakObj1], 1); - EXPECT_EQ(Map[WeakObj2], 2); - EXPECT_EQ(Map[Obj1], 1); - EXPECT_EQ(Map[Obj2], 2); - - Map[WeakObj1] = 2; - Map[WeakObj2] = 3; - - EXPECT_EQ(Map.size(), (size_t)2); - EXPECT_EQ(Map[WeakObj1], 2); - EXPECT_EQ(Map[WeakObj2], 3); - EXPECT_EQ(Map[Obj1], 2); - EXPECT_EQ(Map[Obj2], 3); - - Map[Obj1] = 5; - Map[Obj2] = 6; - - EXPECT_EQ(Map.size(), (size_t)2); - EXPECT_EQ(Map[WeakObj1], 5); - EXPECT_EQ(Map[WeakObj2], 6); - EXPECT_EQ(Map[Obj1], 5); - EXPECT_EQ(Map[Obj2], 6); - - Map[sycl::ext::oneapi::weak_object{Obj1}] = 10; - Map[sycl::ext::oneapi::weak_object{Obj2}] = 13; - - EXPECT_EQ(Map.size(), (size_t)2); - EXPECT_EQ(Map[WeakObj1], 10); - EXPECT_EQ(Map[WeakObj2], 13); - EXPECT_EQ(Map[Obj1], 10); - EXPECT_EQ(Map[Obj2], 13); - } -}; - -template struct WeakObjectCheckCopy { - void operator()(SyclObjT Obj) { - sycl::ext::oneapi::weak_object WeakObj{Obj}; - - sycl::ext::oneapi::weak_object WeakObjCopyCtor{WeakObj}; - sycl::ext::oneapi::weak_object WeakObjCopyAssign = WeakObj; - - EXPECT_FALSE(WeakObjCopyCtor.expired()); - EXPECT_FALSE(WeakObjCopyAssign.expired()); - - EXPECT_TRUE(WeakObjCopyCtor.lock() == Obj); - EXPECT_TRUE(WeakObjCopyAssign.lock() == Obj); - } -}; - -template struct WeakObjectCheckMove { - void operator()(SyclObjT Obj) { - sycl::ext::oneapi::weak_object WeakObj1{Obj}; - sycl::ext::oneapi::weak_object WeakObj2{Obj}; - - sycl::ext::oneapi::weak_object WeakObjMoveCtor{ - std::move(WeakObj1)}; - sycl::ext::oneapi::weak_object WeakObjMoveAssign = - std::move(WeakObj2); - - EXPECT_FALSE(WeakObjMoveCtor.expired()); - EXPECT_FALSE(WeakObjMoveAssign.expired()); - - EXPECT_TRUE(WeakObjMoveCtor.lock() == Obj); - EXPECT_TRUE(WeakObjMoveAssign.lock() == Obj); - } -}; - -template