[SYCL] Refactor address space casts functionality #15543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

aelovikov-intel merged 11 commits into intel:sycl from aelovikov-intel:refactor-as-casts

Oct 14, 2024

Contributor

aelovikov-intel commented Sep 27, 2024 •

edited

Loading

This follows the interfaces designed in https://github.com/intel/llvm/blob/3a1c3cb53566f904a73361d5c57b939d981564b5/sycl/doc/extensions/proposed/sycl_ext_oneapi_address_cast.asciidoc, but instead of operating on multi_ptr, these work on decorated C++ pointers (as that's what we need throughout our implementation, including multi_ptr implementation itself).

Basically, I've moved the implementation of the extension to the new detail::static|dynamic_address_cast functions and replaced all uses of the old detail::cast_AS (that had inconsistent static vs dynamic behavior depending on address spaces/backends) and also uses of direct SPIRV builtin/wrappers invocations.

This isn't NFC, because by doing that I've changed "dynamic" behavior to "static" whenever the spec allows that (e.g. if it's UB if runtime pointers doesn't point to a proper allocation).

aelovikov-intel temporarily deployed to WindowsCILock

September 27, 2024 22:27

— with

GitHub Actions Inactive

aelovikov-intel temporarily deployed to WindowsCILock

September 27, 2024 22:58

— with

GitHub Actions Inactive


          [NFCI][SYCL] Refactor address space casts functionality

a80cea1

aelovikov-intel force-pushed the refactor-as-casts branch from 72d1735 to 63667f0 Compare

October 8, 2024 23:14

aelovikov-intel had a problem deploying to WindowsCILock

October 8, 2024 23:15

— with

GitHub Actions Error

aelovikov-intel closed this

aelovikov-intel reopened this

aelovikov-intel had a problem deploying to WindowsCILock

October 9, 2024 15:42

— with

GitHub Actions Error

aelovikov-intel force-pushed the refactor-as-casts branch from 63667f0 to 2537fbc Compare

October 9, 2024 17:05

aelovikov-intel had a problem deploying to WindowsCILock

October 9, 2024 17:07

— with

GitHub Actions Error

aelovikov-intel added 6 commits

October 9, 2024 10:13


          group/nd_item changes

e912e9f


          cast_AS -> static_address_cast/static_cast in multi_ptr

25d51c5

multi_ptr(accessor) - correct AS is guaranteed by SFINAE
explicit operatr multi_ptr<...> - UB if mismatch

TODO: constant_space ?


          TODO: generic <-> constant

3dba4af

WIP

fc44e23


          Prefetch changes

3b318ca


          Remove now-unused

261dddf

aelovikov-intel force-pushed the refactor-as-casts branch from 2537fbc to 261dddf Compare

October 9, 2024 17:14

aelovikov-intel had a problem deploying to WindowsCILock

October 9, 2024 17:15

— with

GitHub Actions Error


          ctor invocation must be explicit

6a90fef

aelovikov-intel commented

View reviewed changes

sycl/include/sycl/ext/oneapi/experimental/prefetch.hpp

-                auto *ptrGlobalAS = __SYCL_GenericCastToPtrExplicit_ToGlobal<const char>(ptr);
+                auto *ptrGlobalAS =
+                    reinterpret_cast<__attribute__((opencl_global)) const char *>(
+                        detail::static_address_cast<access::address_space::global_space>(

Contributor Author

aelovikov-intel Oct 9, 2024

See https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/experimental/sycl_ext_oneapi_prefetch.asciidoc, it states (for non-accessor overloads):

Preconditions: ptr must point to an object in global memory.

sycl/include/sycl/group.hpp

                   using QualSrcT =
                       std::conditional_t<std::is_const_v<SrcT>, const uint8_t, uint8_t>;
                   auto DestP = multi_ptr<uint8_t, DestS, access::decorated::yes>(
-                      detail::cast_AS<typename multi_ptr<uint8_t, DestS,
-                                                         access::decorated::yes>::pointer>(
+                      reinterpret_cast<typename multi_ptr<uint8_t, DestS,

Contributor Author

aelovikov-intel Oct 9, 2024

We're changing element type of the decorated C++ pointer type here, no address space changes. Same throughout this file.

sycl/include/sycl/nd_item.hpp

@@ @@ -384,12 +384,12 @@ template <int Dimensions = 1> class nd_item { @@
                   using QualSrcT =
                       std::conditional_t<std::is_const_v<SrcT>, const uint8_t, uint8_t>;
                   auto DestP = multi_ptr<uint8_t, DestS, access::decorated::yes>(
-                      detail::cast_AS<typename multi_ptr<uint8_t, DestS,
-                                                         access::decorated::yes>::pointer>(
+                      reinterpret_cast<typename multi_ptr<uint8_t, DestS,

Contributor Author

aelovikov-intel Oct 9, 2024

We're changing element type of the decorated C++ pointer type here, no address space changes. Same throughout this file.

sycl/include/sycl/multi_ptr.hpp

@@ @@ -272,7 +272,7 @@ class __SYCL_TYPE(multi_ptr) multi_ptr { @@
                                          OtherSpace != access::address_space::constant_space>>
                 multi_ptr &
                 operator=(const multi_ptr<value_type, OtherSpace, OtherIsDecorated> &Other) {
-                  m_Pointer = detail::cast_AS<decorated_type *>(Other.get_decorated());
+                  m_Pointer = detail::static_address_cast<Space>(Other.get_decorated());

Contributor Author

aelovikov-intel Oct 9, 2024

"static" cast from non-constant_space to generic_space is always possible.

sycl/include/sycl/multi_ptr.hpp

@@ @@ -282,15 +282,18 @@ class __SYCL_TYPE(multi_ptr) multi_ptr { @@
                                          OtherSpace != access::address_space::constant_space>>
                 multi_ptr &
                 operator=(multi_ptr<value_type, OtherSpace, OtherIsDecorated> &&Other) {
-                  m_Pointer = detail::cast_AS<decorated_type *>(std::move(Other.m_Pointer));
+                  m_Pointer = detail::static_address_cast<Space>(std::move(Other.m_Pointer));

Contributor Author

aelovikov-intel Oct 9, 2024

Likewise.

sycl/include/sycl/multi_ptr.hpp

                 multi_ptr(accessor<ElementType, Dimensions, Mode, target::device,
                                    isPlaceholder, PropertyListT>
                               Accessor)
-                    : multi_ptr(detail::cast_AS<decorated_type *>(
+                    : multi_ptr(detail::static_address_cast<Space>(

Contributor Author

aelovikov-intel Oct 9, 2024

because accessor.

sycl/include/sycl/multi_ptr.hpp

-                pointer get() const { return detail::cast_AS<pointer>(m_Pointer); }
+                pointer get() const {
+                  return detail::static_address_cast<
+                      is_decorated ? Space : access::address_space::generic_space>(m_Pointer);

Contributor Author

aelovikov-intel Oct 9, 2024

See above - either no-op/identity or cast away decoration.

sycl/include/sycl/multi_ptr.hpp

Comment on lines +701 to +702

		static_cast<typename multi_ptr<ElementType, Space,
		access::decorated::yes>::pointer>(

Contributor Author

aelovikov-intel Oct 9, 2024

Changing element type from void to ElementType, no address space changes.

sycl/include/sycl/multi_ptr.hpp

                   return multi_ptr<void, GlobalSpace, DecorateAddress>(
-                      detail::cast_AS<global_pointer_t>(m_Pointer));
+                      detail::static_address_cast<GlobalSpace>(m_Pointer));

Contributor Author

aelovikov-intel Oct 9, 2024

Subsections of the global_space per definition, known to succeed.

sycl/include/sycl/multi_ptr.hpp

@@ @@ -782,14 +775,17 @@ class __SYCL2020_DEPRECATED( @@
               #endif
                 multi_ptr(ElementType *pointer)
-                    : m_Pointer(detail::cast_AS<pointer_t>(pointer)) {
+                    : m_Pointer(detail::dynamic_address_cast<

Contributor Author

aelovikov-intel Oct 9, 2024

See comment near the definition of detail::dynamic_address_cast, that part is mostly NFC. The exception is non-SPIRV backends when we know that address spaces are NOT compatible. Previously we were doing C-style cast anyway, now nullptr is returned.

Same for ctors below.

aelovikov-intel had a problem deploying to WindowsCILock

October 9, 2024 17:36

— with

GitHub Actions Error


          Resolve constant_space TODO

9ee5382

aelovikov-intel temporarily deployed to WindowsCILock

October 9, 2024 18:08

— with

GitHub Actions Inactive

aelovikov-intel commented

View reviewed changes

sycl/include/sycl/ext/oneapi/experimental/address_cast.hpp

                 if constexpr (Space == generic_space)
                   return Ptr;
                 else
-                  return {static_address_cast<Space>(Ptr.get_raw())};
+                  return {static_address_cast<Space>(Ptr.get_decorated())};

Contributor Author

aelovikov-intel Oct 9, 2024

We were limited by the extension interface before, so had to drop this bit of information in order to delegate. Now that we're calling something from detail:: we can keep decoration in place.

aelovikov-intel changed the title ~~[NFCI][SYCL] Refactor address space casts functionality~~ [SYCL] Refactor address space casts functionality

aelovikov-intel marked this pull request as ready for review

October 9, 2024 18:18

aelovikov-intel requested a review from a team as a code owner

October 9, 2024 18:18

aelovikov-intel requested a review from bso-intel

October 9, 2024 18:18

aelovikov-intel had a problem deploying to WindowsCILock

October 10, 2024 08:41

— with

GitHub Actions Failure


          Fix prefetch header

8b779b5

aelovikov-intel temporarily deployed to WindowsCILock

October 10, 2024 17:04

— with

GitHub Actions Inactive

aelovikov-intel temporarily deployed to WindowsCILock

October 10, 2024 21:10

— with

GitHub Actions Inactive

bso-intel approved these changes

View reviewed changes

Contributor

bso-intel left a comment

LGTM.
Thanks for extra comments about your code changes.


          Merge remote-tracking branch 'origin/sycl' into HEAD

ae76d5d

aelovikov-intel temporarily deployed to WindowsCILock

October 14, 2024 16:13

— with

GitHub Actions Inactive

aelovikov-intel temporarily deployed to WindowsCILock

October 14, 2024 16:44

— with

GitHub Actions Inactive

steffenlarsen approved these changes

View reviewed changes

Contributor

steffenlarsen left a comment

Very nice!

aelovikov-intel merged commit 9ea0f20 into intel:sycl

12 checks passed

aelovikov-intel deleted the refactor-as-casts branch

October 14, 2024 18:23

aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request


          [SYCL] Fix SYCL-CTS regression after intel#15543

931f427

Apparently, we perform some hacks around constant address space that I
wasn't aware of. Make sure new address cast helpers are called
consistently with the previous behavior.

aelovikov-intel added a commit that referenced this pull request


          [SYCL] Fix SYCL-CTS regression after #15543 (#15931)

ead1579

Apparently, we perform some hacks around constant address space that I
wasn't aware of. Make sure new address cast helpers are called
consistently with the previous behavior.

---------

Co-authored-by: Sergey Semenov <[email protected]>

aelovikov-intel mentioned this pull request

[SYCL][Doc] Release Notes for Nov'24 release #17179

Merged

aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request


          [SYCL] Unify SPIR/non-SPIR paths for atomic_ref ctor

a05b781

This became possible sometime after one/all of
 * intel#15394
 * intel#15543
 * intel#16604

aelovikov-intel mentioned this pull request

[SYCL] Unify SPIR/non-SPIR paths for atomic_ref ctor #17196

Merged

aelovikov-intel added a commit that referenced this pull request


          [SYCL] Unify SPIR/non-SPIR paths for atomic_ref ctor (#17196)

09032a4

This became possible sometime after one/all of
 * #15394
 * #15543
 * #16604

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet