[SYCL] Implement multi_ptr default to be legacy to avoid code break with SYCL 1.2.1 #10174
Conversation
…to the access::decorated::legacy specialization of multi_ptr
…decorated::legacy
…ck to 1.2.1 version. - enable_if async_work_group_copy in group.hpp and nd_item.hpp when Dst and Src have the same type.
- Update tests to reflect deprecating access::decorated::legacy
| @@ -662,6 +662,7 @@ class multi_ptr<void, Space, DecorateAddress> { | |||
| template <typename ElementType, access::address_space Space> | |||
| class multi_ptr<ElementType, Space, access::decorated::legacy> { | |||
| public: | |||
| using value_type = ElementType; | |||
There was a problem hiding this comment.
This is not actually part of the spec, even after the changes from the mentioned spec PR. I have made a comment on it: https://github.com/KhronosGroup/SYCL-Docs/pull/432/files#r1251095640, but we should not add interfaces that aren't part of the spec.
There was a problem hiding this comment.
thanks @steffenlarsen. I agree with you on above.
I would add it to spec for consistency with others.
There was a problem hiding this comment.
This is a good catch, the PR used value_type and it wasn't defined, I've created a new PR to make a change to the spec to introduce this (KhronosGroup/SYCL-Docs#437), so I think this change will be okay after that is merged.
| @@ -662,6 +662,7 @@ class multi_ptr<void, Space, DecorateAddress> { | |||
| template <typename ElementType, access::address_space Space> | |||
| class multi_ptr<ElementType, Space, access::decorated::legacy> { | |||
| public: | |||
| using value_type = ElementType; | |||
There was a problem hiding this comment.
This is a good catch, the PR used value_type and it wasn't defined, I've created a new PR to make a change to the spec to introduce this (KhronosGroup/SYCL-Docs#437), so I think this change will be okay after that is merged.
| @@ -1,12 +1,12 @@ | |||
| // RUN: %clangxx %fsycl-host-only -fsyntax-only -Xclang -verify %s -o %t.out | |||
| // expected-no-diagnostics | |||
| // RUN: %clangxx %fsycl-host-only -fsyntax-only -Xclang -verify -Xclang -verify-ignore-unexpected=note %s | |||
There was a problem hiding this comment.
I assume that deprecation decorated::legacy deprecation warnings are not the main thing we want to test here. Therefore, we can consider adding -Wno-deprecated and leave the test as-is, i.e. expecting no diagnostics
There was a problem hiding this comment.
Thanks @AlexeySachkov , good point.
…ation. - Add get_raw / get_decorated in multi_ptr void/const void/ legacy specialisation.
- Use `const_cast` in `read_only` accessors to handle above update/
| "accessor::get_pointer() is deprecated, please use get_multi_ptr()") | ||
| global_ptr<DataT> get_pointer() const noexcept { | ||
| return global_ptr<DataT>( | ||
| const_cast<typename detail::DecoratedType<DataT, AS>::type *>( |
There was a problem hiding this comment.
Note: Discussed offline. The deprecated API tries to be in line with SYCL 1.2.1, which means returning a DataT pointer. However, with SYCL 2020 the value-type is changed to const when it is read-only. Instead of changing the internal representation, const_cast for this deprecated API seemed like the lesser evil.
|
a friendly request for review please. @intel/dpcpp-esimd-reviewers |
|
friendly request for review please. https://github.com/orgs/intel/teams/dpcpp-esimd-reviewers |
mmoadeli
changed the title
get_pointerinterface for device, host and local_accessor back to 1.2.1 version.enable_ifasync_work_group_copyingroup.hppandnd_item.hppto ensure Dst and Src(const / non-const) have the same type.