[SYCL][Docs] Add sycl_ext_oneapi_weak_object extension and implementation #7508

steffenlarsen · 2022-11-23T13:55:07Z

This commit add the sycl_ext_oneapi_weak_object extension together with its implementation. This extension intendeds to allow users to weak variants of SYCL objects with common reference semantics, similar to how std::weak_ptr works with std::shared_ptr. Among other benefits, this allows user code to store these SYCL objects without worrying about the storage keeping the objects alive for longer than needed.

Additionally the extension adds the notion of owner ordering of SYCL objects. This can be used as less-than comparison between SYCL objects and their corresponding weak variants.

…tion This commit add the sycl_ext_oneapi_weak_object extension together with its implementation. This extension intendeds to allow users to weak variants of SYCL objects with common reference semantics, similar to how std::weak_ptr works with std::shared_ptr. Among other benefits, this allows user code to store these SYCL objects without worrying about the storage keeping the objects alive for longer than needed. Additionally the extension adds the notion of owner ordering of SYCL objects. This can be used as less-than comparison between SYCL objects and their corresponding weak variants. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

aelovikov-intel · 2022-11-23T17:41:27Z

I've briefly looked through some files where I'd expect to find the implementation details documentation (like weak_object[_base}.hpp, etc.) but haven't found anything looking close enough. Can you please add some high-level description of how that is supposed to work (i.e., where the counters are located, what happens when new objects are created, etc.)?

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen · 2022-11-24T09:58:47Z

I've briefly looked through some files where I'd expect to find the implementation details documentation (like weak_object[_base}.hpp, etc.) but haven't found anything looking close enough. Can you please add some high-level description of how that is supposed to work (i.e., where the counters are located, what happens when new objects are created, etc.)?

I have added some notes. Since we use std::shared_ptr with implementation classes to do common reference semantics, the implementation of weak_object is mostly just a wrapper around a std::weak_ptr created from these. There are some deviations from the behavior of std::weak_ptr as we do not have the notion of empty SYCL objects, so lock will throw instead.

sycl/include/sycl/accessor.hpp

sycl/include/sycl/ext/oneapi/weak_object_base.hpp

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

sycl/doc/extensions/supported/sycl_ext_oneapi_weak_object.asciidoc

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

gmlueck

Spec LGTM.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

aelovikov-intel · 2022-12-01T17:21:10Z

sycl/unittests/Extensions/WeakObject.cpp

+    sycl::accessor<int, 1, sycl::access::mode::read> DAcc1D1{Buf1D1, CGH};
+    sycl::accessor<int, 1, sycl::access::mode::read> DAcc1D2{Buf1D2, CGH};
+    sycl::accessor<int, 2, sycl::access::mode::read> DAcc2D1{Buf2D1, CGH};
+    sycl::accessor<int, 2, sycl::access::mode::read> DAcc2D2{Buf2D2, CGH};
+    sycl::accessor<int, 3, sycl::access::mode::read> DAcc3D1{Buf3D1, CGH};
+    sycl::accessor<int, 3, sycl::access::mode::read> DAcc3D2{Buf3D2, CGH};


Suggested change

sycl::accessor<int, 1, sycl::access::mode::read> DAcc1D1{Buf1D1, CGH};

sycl::accessor<int, 1, sycl::access::mode::read> DAcc1D2{Buf1D2, CGH};

sycl::accessor<int, 2, sycl::access::mode::read> DAcc2D1{Buf2D1, CGH};

sycl::accessor<int, 2, sycl::access::mode::read> DAcc2D2{Buf2D2, CGH};

sycl::accessor<int, 3, sycl::access::mode::read> DAcc3D1{Buf3D1, CGH};

sycl::accessor<int, 3, sycl::access::mode::read> DAcc3D2{Buf3D2, CGH};

sycl::accessor DAcc1D1{Buf1D1, CGH};

sycl::accessor DAcc1D2{Buf1D2, CGH};

sycl::accessor DAcc2D1{Buf2D1, CGH};

sycl::accessor DAcc2D2{Buf2D2, CGH};

sycl::accessor DAcc3D1{Buf3D1, CGH};

sycl::accessor DAcc3D2{Buf3D2, CGH};

Maybe with an extra sycl::read_only (is that the right spelling?) param.

It looks like unittests don't currently like the deduction guides, seemingly due to a problem with IsValidTag and IsSameAsBuffer. I cannot reproduce with an actual SYCL program, so I suspect it may have something to do with how the unittests are built and/or potentially a bug in the host compiler. I have a fix but I think it would be better to do in isolation as a follow-up patch.

aelovikov-intel · 2022-12-01T17:23:34Z

sycl/unittests/Extensions/WeakObject.cpp

+  sycl::accessor<int, 1, sycl::access::mode::read_write,
+                 sycl::access::target::device,
+                 sycl::access::placeholder::true_t>
+      PAcc1D1{Buf1D1};


AFAIK sycl::access::placeholder::true_t is deprecated, we can use normal CTAD like

sycl::accessor PAcc1D1{Buf1D1};

to create a placeholder accessor.

This has the same problem as #7508 (comment). I will address it separately.

sycl/include/sycl/detail/owner_less_base.hpp

aelovikov-intel · 2022-12-01T18:48:09Z

sycl/include/sycl/ext/oneapi/owner_less.hpp

+template <>
+struct owner_less<context> : public detail::owner_less_base<context> {};
+template <>
+struct owner_less<device> : public detail::owner_less_base<device> {};
+template <> struct owner_less<event> : public detail::owner_less_base<event> {};
+template <>
+struct owner_less<kernel> : public detail::owner_less_base<kernel> {};
+template <>
+struct owner_less<kernel_id> : public detail::owner_less_base<kernel_id> {};
+template <>
+struct owner_less<platform> : public detail::owner_less_base<platform> {};
+template <> struct owner_less<queue> : public detail::owner_less_base<queue> {};
+
+template <bundle_state State>
+struct owner_less<device_image<State>>
+    : public detail::owner_less_base<device_image<State>> {};
+
+template <bundle_state State>
+struct owner_less<kernel_bundle<State>>
+    : public detail::owner_less_base<kernel_bundle<State>> {};
+
+template <typename DataT, int Dimensions, typename AllocatorT>
+struct owner_less<buffer<DataT, Dimensions, AllocatorT>>
+    : public detail::owner_less_base<buffer<DataT, Dimensions, AllocatorT>> {};
+
+template <typename DataT, int Dimensions, access_mode AccessMode,
+          target AccessTarget, access::placeholder isPlaceholder>
+struct owner_less<
+    accessor<DataT, Dimensions, AccessMode, AccessTarget, isPlaceholder>>
+    : public detail::owner_less_base<accessor<DataT, Dimensions, AccessMode,
+                                              AccessTarget, isPlaceholder>> {};
+
+template <typename DataT, int Dimensions, access_mode AccessMode>
+struct owner_less<host_accessor<DataT, Dimensions, AccessMode>>
+    : public detail::owner_less_base<
+          host_accessor<DataT, Dimensions, AccessMode>> {};


nit, feel free to ignore.

I'd rather see something like

namespace detail { template <typename T> struct owner_less_unsupport { static_assert(always_false_v<T>); } } template <typename T> struct owner_less : public std::conditional<...., detail::owner_less_base<T>, detail::owner_less_unsupported<T>> {};

using some utilities like

llvm/sycl/include/sycl/properties/accessor_properties.hpp

Lines 106 to 115 in ff59e41

template <typename T> struct is_accessor : std::false_type {};

template <typename DataT, int Dimensions, access::mode AccessMode,

access::target AccessTarget, access::placeholder IsPlaceholder,

typename PropertyListT>

struct is_accessor<accessor<DataT, Dimensions, AccessMode, AccessTarget,

IsPlaceholder, PropertyListT>> : std::true_type {};

template <typename T> struct is_host_accessor : std::false_type {};

template <typename DataT, int Dimensions, access::mode AccessMode>

struct is_host_accessor<host_accessor<DataT, Dimensions, AccessMode>>

for the condition.

So instead of having a handful specializations we would have a generic class with checks for all the different classes we should support? Since we would still need to add new is_X traits for new classes and add them here, I am not sure I see the benefit to that approach.

I have a feeling that I see the same pattern in multiple places. As such, new is_X cost would be spread over multiple places (maybe not now, but soon enough). For example, is_*access* already has other usage - the link above.

a generic class with checks for all the different classes we should support

That somehow sounds much heavier than it would be like.

I suppose for the ones with multiple template arguments this approach would make it cleaner, but I am reluctant to add these helpers as part of this PR as it is already fairly big. I will try and do some refactoring after this is merged.

sycl/include/sycl/ext/oneapi/weak_object.hpp

aelovikov-intel · 2022-12-01T18:54:35Z

sycl/include/sycl/ext/oneapi/weak_object_base.hpp

@@ -0,0 +1,66 @@
+//==------- weak_object_base.hpp --- SYCL weak objects base class ----------==//


nit: I'd prefer not having this separate file, but up to you.

It is needed because we specialize weak_object for buffer and that would either require us to create a source file or create a circular dependency, so this seemed like the simpler path.

sycl/unittests/Extensions/WeakObject.cpp

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

sycl/include/sycl/ext/oneapi/weak_object.hpp

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

aelovikov-intel

All major concerns have been addressed! Remaining nits are up to the author's judgement.

keryell

Interesting concept.
Perhaps weak_ref is more precise?
In the C++ standard there is also reference_wrapper but it seems less suitable for weak_wrapper for SYCL.

steffenlarsen · 2022-12-02T07:51:58Z

Interesting concept. Perhaps weak_ref is more precise? In the C++ standard there is also reference_wrapper but it seems less suitable for weak_wrapper for SYCL.

I might worry that the name ref could make it look like it should act like a reference and make the underlying object directly accessible. weak_object fits the bill because it is a "weak" copy of the SYCL object, which do to the common reference semantics makes it a reference to the same object. That said, it's not a strong preference for one or the other. @gmlueck - Do you have a preference?

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

gmlueck · 2022-12-02T19:08:23Z

Do you have a preference?

I do not have a strong preference. I think I have a weak preference for weak_object, but it is a very weak preference.

keryell · 2022-12-02T20:27:33Z

It works like a weak_ptr but you do not use a * with it. So this is why this makes me think to a reference. :-)
But whatever is fine, since it is an extension.
@gmlueck Do you propose weak_preference? ;-)

steffenlarsen · 2022-12-05T09:06:55Z

In that case I will make the executive decision and stick with weak_object.

HIP failures are known (#7602 (comment)).

steffenlarsen · 2022-12-05T09:43:43Z

I am investigating the Windows post-commit failure.

steffenlarsen · 2022-12-05T16:30:19Z

Windows post-commit fix: #7642

…plementation (intel#7508)" This reverts commit d948427.

…n and implementation (intel#7508)"" This reverts commit 9f5ba51.

steffenlarsen requested a review from a team November 23, 2022 13:55

steffenlarsen requested review from a team as code owners November 23, 2022 13:55

steffenlarsen requested a review from bso-intel November 23, 2022 13:55

Fix formatting

46b0096

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Add implementation comments

491f2ab

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen mentioned this pull request Nov 24, 2022

[SYCL][Docs] Bump CI Sphinx version to 4.2.0 #7524

Merged

aelovikov-intel reviewed Nov 28, 2022

View reviewed changes

sycl/include/sycl/accessor.hpp Outdated Show resolved Hide resolved

aelovikov-intel reviewed Nov 28, 2022

View reviewed changes

sycl/include/sycl/ext/oneapi/weak_object_base.hpp Outdated Show resolved Hide resolved

steffenlarsen added 4 commits November 29, 2022 02:10

Merge remote-tracking branch 'intel/sycl' into weak_obj

46f0991

Make copy/move ctors and assignment operators default

ac5c334

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Use CRTP for accessor owner-based ordering

56589dd

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Improve test formatting

e07ba71

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

gmlueck reviewed Nov 29, 2022

View reviewed changes

steffenlarsen added 2 commits December 1, 2022 00:32

Address extension spec comments

adaf194

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Remove remaining lock noexcept

fc589db

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

gmlueck approved these changes Dec 1, 2022

View reviewed changes

steffenlarsen added 4 commits December 1, 2022 05:40

Merge remote-tracking branch 'intel/sycl' into weak_obj

bfb344e

Make common base class for owner-less methods

ff59e41

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Fix owner_less_base for two SYCL objects

a1289bd

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Remove device-side owner-less methods

219fe8d

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

aelovikov-intel reviewed Dec 1, 2022

View reviewed changes

sycl/unittests/Extensions/WeakObject.cpp Outdated Show resolved Hide resolved

sycl/unittests/Extensions/WeakObject.cpp Outdated Show resolved Hide resolved

sycl/unittests/Extensions/WeakObject.cpp Outdated Show resolved Hide resolved

steffenlarsen added 2 commits December 1, 2022 11:19

Allow 0-dim dummy range

c1086f9

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Add comment about assignment operator

02f10af

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

aelovikov-intel reviewed Dec 1, 2022

View reviewed changes

sycl/include/sycl/ext/oneapi/weak_object.hpp Show resolved Hide resolved

Add comment for other assignment operator too

705ecf8

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

aelovikov-intel approved these changes Dec 1, 2022

View reviewed changes

keryell reviewed Dec 2, 2022

View reviewed changes

Change owner-less tests

acde508

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen merged commit d948427 into intel:sycl Dec 5, 2022

whitneywhtsang added a commit to whitneywhtsang/llvm that referenced this pull request Dec 5, 2022

Revert "[SYCL][Docs] Add sycl_ext_oneapi_weak_object extension and im…

9f5ba51

…plementation (intel#7508)" This reverts commit d948427.

whitneywhtsang added a commit to whitneywhtsang/llvm that referenced this pull request Dec 6, 2022

Revert "Revert "[SYCL][Docs] Add sycl_ext_oneapi_weak_object extensio…

c0ee730

…n and implementation (intel#7508)"" This reverts commit 9f5ba51.

	template <typename T> struct is_accessor : std::false_type {};
	template <typename DataT, int Dimensions, access::mode AccessMode,
	access::target AccessTarget, access::placeholder IsPlaceholder,
	typename PropertyListT>
	struct is_accessor<accessor<DataT, Dimensions, AccessMode, AccessTarget,
	IsPlaceholder, PropertyListT>> : std::true_type {};

	template <typename T> struct is_host_accessor : std::false_type {};
	template <typename DataT, int Dimensions, access::mode AccessMode>
	struct is_host_accessor<host_accessor<DataT, Dimensions, AccessMode>>

		@@ -0,0 +1,66 @@
		//==------- weak_object_base.hpp --- SYCL weak objects base class ----------==//

[SYCL][Docs] Add sycl_ext_oneapi_weak_object extension and implementation #7508

[SYCL][Docs] Add sycl_ext_oneapi_weak_object extension and implementation #7508

Uh oh!

Conversation

steffenlarsen commented Nov 23, 2022

Uh oh!

aelovikov-intel commented Nov 23, 2022

Uh oh!

steffenlarsen commented Nov 24, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gmlueck left a comment

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!

steffenlarsen Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aelovikov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

keryell left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen commented Dec 2, 2022

Uh oh!

gmlueck commented Dec 2, 2022

Uh oh!

keryell commented Dec 2, 2022

Uh oh!

steffenlarsen commented Dec 5, 2022

Uh oh!

steffenlarsen commented Dec 5, 2022

Uh oh!

steffenlarsen commented Dec 5, 2022

Uh oh!

Uh oh!

steffenlarsen Dec 2, 2022 •

edited

Loading

aelovikov-intel Dec 2, 2022 •

edited

Loading