[SYCL] Fix return type of the accessor::get_pointer and local_accessor::get_pointer. #8493

mmoadeli · 2023-02-28T17:51:13Z

accessor::get_pointer and local_accessor::get_pointer return std::add_pointer_t<value_type>
Modifies multi_ptr ctors accepting local_accessor.
Improves existing test to check the return type of get_pointer.

Update failing tests due to adding diagnostic for const qualified DataT only allowed for non readonly accessor.

… data.

- Use -Xclang -verify for testing

Having it requires handling over 20 expected-erros.

…o inheritance from base class hitting assert.

…pointer.

…ification.

steffenlarsen · 2023-03-20T21:44:50Z

sycl/include/sycl/multi_ptr.hpp

@@ -1148,6 +1154,9 @@ class multi_ptr<const void, Space, access::decorated::legacy> {

  static constexpr access::address_space address_space = Space;

+  using decorated_type =
+      typename detail::DecoratedType<const void, Space>::type;
+


This should be removed. It is defined publicly, but is not part of the specification.

@steffenlarsen , thanks, done.

steffenlarsen · 2023-03-20T21:45:47Z

sycl/include/sycl/multi_ptr.hpp

@@ -1006,6 +1009,9 @@ class multi_ptr<void, Space, access::decorated::legacy> {

  static constexpr access::address_space address_space = Space;

+  using decorated_type =
+      typename detail::DecoratedType<const void, Space>::type;


Same applies to this. The places in this class that uses it should be able to use pointer_t instead.

@steffenlarsen , thanks, done.

…multi_ptr.

sycl/include/sycl/multi_ptr.hpp

Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>

steffenlarsen · 2023-03-21T11:00:21Z

sycl/test/basic_tests/accessor/accessor_multi_ptr.cpp

               access::placeholder::true_t>;
  using local_accessor_t = local_accessor<int, size>;

-  auto ptr = buffer.get_access<access_mode::read_write, target::device>(cgh);
+  auto ptr = buffer.get_host_access(cgh);


I am not sure I understand this change. ptr would be a host_accessor. Why do we compare its get_multi_ptr result to a host_task accessor's accessor_t?

Also, if you would like to check the type of the access::target::host_task it would be best to add it here rather than replace the one for access::target::device, just to avoid reducing our test coverage.

thanks @steffenlarsen, I reverted those changes and added a new test for get_pointer.

steffenlarsen

Tiny nit, otherwise LGTM!

steffenlarsen · 2023-03-21T12:49:58Z

sycl/test/basic_tests/accessor/accessor_get_pointer.cpp

+  static_assert(std::is_same_v<decltype(target_local_ptr), local_ptr<int>>);
+  static_assert(
+      std::is_same_v<decltype(local_pointer), std::add_pointer_t<int>>);
+}


Suggested change

}

}

mmoadeli · 2023-03-21T13:59:47Z

/verify with intel/llvm-test-suite#1670

Wanzizhu · 2023-04-06T08:38:34Z

hi, @mmoadeli, as this is a change that breaking backward compatibility, kernel code using local_accssor will not be backward compatible, do you have any idea/advice for this issue?

steffenlarsen · 2023-04-06T09:04:37Z

Hi @Wanzizhu ! The get_pointer interface on local_accessor was not in line with the SYCL 2020 specification. The appropriate alternative to the old non-conformant behavior is to call the get_multi_ptr<sycl::access::decorated::legacy>() member function which returns the same as the old non-conformant API did.

Wanzizhu · 2023-04-06T09:20:54Z

hi， @steffenlarsen , i tried get_multi_ptr, and found code still run into issue when using compiler 2023.1.0. Git commit show it's a [feature|https://github.com//pull/8249] that added on Feb 27. Could you please provided a solution both working on old/new compiler ?

error: no member named 'get_multi_ptr' in 'sycl::local_accessor<float, 1>'
    T *lmem = scratch_.get_multi_ptr().get();

mmoadeli · 2023-04-06T09:28:41Z

@Wanzizhu
get_pointer' is a bit tricky when it comes to local_accessors and accessor specialization with target::local`

Accessor specialization with target::local have below signature
local_ptr<DataT> get_pointer() const noexcept;

while get_pointer in local_accessor: has below signature
std::add_pointer_t<value_type> get_pointer() const noexcept;

Depending on which you use, you may need to update your code accordingly.

steffenlarsen · 2023-04-06T09:38:51Z

@Wanzizhu - The get_multi_ptr member is new to the next release. An alternative that should work for 2023.1 and current open-source is to change it to sycl::local_ptr<T>(LocalPtrVar.get_pointer()). In your given case, that would be something like:

T *lmem = sycl::local_ptr<T>(scratch_.get_pointer()).get();

This is slightly less performant than get_multi_ptr() but should work for both. Eventually, you can simplify the mentioned code as just

T *lmem = scratch_.get_pointer();

If you wanted to use the more performant paths, you could have a function like:

// Getting a multi_ptr from a local_accessor.
template <typename T, int Dims> sycl::local_ptr<T> getMultiPtr(sycl::local_accessor<T, Dims> &LAcc) {
  if constexpr (std::is_same_v<decltype(LAcc.get_pointer()), sycl::local_ptr<T>>)
    return LAcc.get_pointer();
  else
    return LAcc.get_multi_ptr<sycl::access::decorated::legacy>();
}

// Getting a raw pointer from a local_accessor
template <typename T, int Dims> T *getPtr(sycl::local_accessor<T, Dims> &LAcc) {
  if constexpr (std::is_same_v<decltype(LAcc.get_pointer()), sycl::local_ptr<T>>)
    return LAcc.get_pointer().get();
  else
    return LAcc.get_pointer();
}

Wanzizhu · 2023-04-06T13:58:34Z

thanks, i wil take this solution.

mmoadeli and others added 18 commits January 31, 2023 09:49

Diagnostic for const qualified DataT with non read-only accessor

2503a62

Merge branch 'sycl' of https://github.com/mmoadeli/llvm into sycl

862b688

Add test for const qualified DataT with non readonly accessor.

1edb94f

Update failing tests due to adding diagnostic for const qualified DataT only allowed for non readonly accessor.

Assign valid access mode for local accessor, depending on the type of…

7ba64c6

… data.

Minor test update.

ef0038e

- Rename AccessMode to AccessModeFromConstness

36971f8

- Use -Xclang -verify for testing

- Revert test to be without -Xclang -verify.

29e3036

Having it requires handling over 20 expected-erros.

Refactors implementation of diagnostic to avoid compiler errors due t…

92debe8

…o inheritance from base class hitting assert.

Updates the test to use -Xclang -verify

7eac372

Fix style-checl

dcac706

Merge branch 'intel:sycl' into sycl

9d4b313

Merge branch 'intel:sycl' into sycl

ad92330

Merge branch 'intel:sycl' into sycl

ce1cbe8

Merge branch 'intel:sycl' into sycl

dc33cc4

Merge branch 'sycl' of https://github.com/mmoadeli/llvm into sycl

44b1f9e

Merge branch 'sycl' of https://github.com/mmoadeli/llvm into sycl

2803f25

Merge branch 'sycl' of https://github.com/mmoadeli/llvm into sycl

0b358a5

Fix rerurn type of the accessor::get_pointer and local_accessor::get_…

00c9329

…pointer.

mmoadeli requested a review from a team as a code owner February 28, 2023 17:51

mmoadeli requested a review from bso-intel February 28, 2023 17:51

mmoadeli marked this pull request as draft February 28, 2023 18:35

mmoadeli temporarily deployed to aws February 28, 2023 21:22 — with GitHub Actions Inactive

mmoadeli added 2 commits March 1, 2023 11:26

Add get_pointer for accessor with target::local according to the spec…

beabf81

…ification.

Remove commented out code.

29fadf4

mmoadeli marked this pull request as ready for review March 1, 2023 11:32

mmoadeli temporarily deployed to aws March 1, 2023 16:03 — with GitHub Actions Inactive

mmoadeli temporarily deployed to aws March 3, 2023 12:24 — with GitHub Actions Inactive

mmoadeli temporarily deployed to aws March 6, 2023 12:25 — with GitHub Actions Inactive

mmoadeli temporarily deployed to aws March 7, 2023 16:33 — with GitHub Actions Inactive

mmoadeli temporarily deployed to aws March 7, 2023 22:11 — with GitHub Actions Inactive

steffenlarsen reviewed Mar 20, 2023

View reviewed changes

Remove definition of decorated_type in decorated::legacy specialized …

c2ac5e2

…multi_ptr.

mmoadeli temporarily deployed to aws March 20, 2023 23:09 — with GitHub Actions Inactive

Merge branch 'sycl' into get_pointer

627a270

mmoadeli temporarily deployed to aws March 20, 2023 23:17 — with GitHub Actions Inactive

steffenlarsen reviewed Mar 21, 2023

View reviewed changes

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

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

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

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

mmoadeli and others added 4 commits March 21, 2023 10:42

Update sycl/include/sycl/multi_ptr.hpp

e5cf267

Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>

Update sycl/include/sycl/multi_ptr.hpp

4463651

Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>

Update sycl/include/sycl/multi_ptr.hpp

abafe2e

Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>

Update sycl/include/sycl/multi_ptr.hpp

4621dc4

Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>

steffenlarsen reviewed Mar 21, 2023

View reviewed changes

mmoadeli temporarily deployed to aws March 21, 2023 11:29 — with GitHub Actions Inactive

mmoadeli added 2 commits March 21, 2023 12:34

Add test for get_pointer and reverts earlier change to get_multi_ptr

057528a

Fix a minor style.

166b652

steffenlarsen approved these changes Mar 21, 2023

View reviewed changes

mmoadeli temporarily deployed to aws March 21, 2023 13:28 — with GitHub Actions Inactive

Remove unused alias definition

8d595bc

mmoadeli temporarily deployed to aws March 21, 2023 14:24 — with GitHub Actions Inactive

mmoadeli temporarily deployed to aws March 21, 2023 15:10 — with GitHub Actions Inactive

bader merged commit 3fdbfeb into intel:sycl Mar 22, 2023

AlexeySachkov mentioned this pull request Mar 28, 2023

Wrong return type for sycl::accessor::get_pointer() and sycl::local_accessor::get_pointer() #8301

Closed

mmoadeli deleted the get_pointer branch July 7, 2023 10:42

[SYCL] Fix return type of the accessor::get_pointer and local_accessor::get_pointer. #8493

[SYCL] Fix return type of the accessor::get_pointer and local_accessor::get_pointer. #8493

Uh oh!

Conversation

mmoadeli commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steffenlarsen Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

mmoadeli Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

steffenlarsen Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

mmoadeli Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steffenlarsen Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

mmoadeli Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

mmoadeli commented Mar 21, 2023

Uh oh!

Wanzizhu commented Apr 6, 2023

Uh oh!

steffenlarsen commented Apr 6, 2023

Uh oh!

Wanzizhu commented Apr 6, 2023

Uh oh!

mmoadeli commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steffenlarsen commented Apr 6, 2023

Uh oh!

Wanzizhu commented Apr 6, 2023

Uh oh!

Uh oh!

mmoadeli commented Feb 28, 2023 •

edited

Loading

mmoadeli commented Apr 6, 2023 •

edited

Loading