[SYCL] Add local_accessor and deprecate target::local #6341

AidanBeltonS · 2022-06-22T09:05:52Z

This PR creates the local_accessor class by aliasing the accessor class with target::local.
The motivation behind this is that target::local has been deprecated in favour of local_accessor in SYCL2020.
The approach of aliasing is taken as the spec states that local_access has the same semantics and restrictions as accessor with target::local.

Related issue: #4713
llvm-test-suite: intel/llvm-test-suite#1063

againull

I'm sorry for the delayed review.

againull · 2022-06-28T21:08:56Z

sycl/include/CL/sycl/accessor.hpp

+class __SYCL_SPECIAL_CLASS __SYCL2020_DEPRECATED(
+    "'accessor' with 'target::local' is deprecated: use 'local_accessor' "
+    "instead.") accessor<DataT, Dimensions, AccessMode, access::target::local,
+                         IsPlaceholder> :


Unfortunately [[deprecated]] attribute doesn't work on template specializations: https://godbolt.org/z/nx56bsfjz
Deprecation message is not generated.

Another problem is that even if you deprecate this template specialization then I believe usage of local_accessor alias will result in deprecated message as well, isn't it?

Okay, that is a bit of an issue. I think a possible solution could be to deprecate the class constructors instead of the class itself.
The only way I can think of to resolve the alias of local_accessor producing a deprecated message is to create a new class just for local_accessor.

Indeed this seems to be very unpleasant issue, I'm sorry.

Other possible option is just to not add [[deprecated]] attribute in this patch if we are ok with absence of deprecation message. @steffenlarsen Do you think it is ok to not emit deprecation warning in this case? Since emitting a deprecation warning in this case causes too much headache and it seems to be only usability issue.

I would definitely prefer that we deprecate the API explicitly, but you are right that we cannot deprecate specializations.

That said, SYCL 2020 deprecated select members of target, local included, so what I suggest is:

Deprecate access::target::local.

Move the implementation in this specialization into a new local_accessor class.

Make local_accessor a base class of this accessor specialization.

This has the added benefit of making it easy to remove the deprecated specialization when we remove (access::)target::local in the future. 😄

@steffenlarsen Sounds great to me, thanks a lot for the help!

I have updated this PR to follow the suggested approach. It takes a slightly different structure by using a local_accessor_base class to implement both local_accessor and accessor. This is necessary to handle accessors template arguments.

Additionally, some changes to Clang's SemaSYCL were applied so the new local_accessor is identified and correctly handled.

…l_accessor

…sor children

…l_accessor

clang/lib/Sema/SemaSYCL.cpp

premanandrao

FE changes look good to me.

smanna12

FE changes LGTM.

clang/lib/Sema/SemaSYCL.cpp

sycl/include/sycl/accessor.hpp

sycl/test/check_device_code/kernel_arguments_as.cpp

sycl/test/basic_tests/set_arg_error.cpp

remove unnecessary cl namespace Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>

…l_accessor

steffenlarsen · 2022-08-16T09:26:01Z

Friendly ping, @intel/dpcpp-esimd-reviewers

v-klochkov

ESIMD related changes have only minor differences in 1 LIT test.
Ok for ESIMD. Thank you.

steffenlarsen · 2022-08-25T21:49:15Z

@AidanBeltonS - Commit fails in post-commit. Seems like MSVC is having trouble with the default template argument in AccessorSubscript. Could you please investigate?

AidanBeltonS · 2022-08-26T09:07:16Z

@AidanBeltonS - Commit fails in post-commit. Seems like MSVC is having trouble with the default template argument in AccessorSubscript. Could you please investigate?

Okay I am currently looking into it.

AidanBeltonS · 2022-08-26T09:56:01Z

I have proposed a fix for this issue with #6648.

This PR fixes an issue caused when building on Windows with MSVC. The problem was introduced in #6341. MSVC does not correctly infer the default template for its return type. This is fixed by setting the return type to auto and letting the compiler infer the return type.

steffenlarsen · 2022-08-26T14:01:53Z

I have proposed a fix for this issue with #6648.

Post-commit is back to green. Thank you, @AidanBeltonS !

This PR updates tests that use `target::local` to local_accessor. In all cases the change should not functionally change the test. The goal is to move from the deprecated `target::local` accessor to `local_accessor`. Depends on: intel/llvm#6341

This PR updates tests that use `target::local` to local_accessor. In all cases the change should not functionally change the test. The goal is to move from the deprecated `target::local` accessor to `local_accessor`. Depends on: intel#6341

Add local_accessor and deprecate target::local accessor

534f667

AidanBeltonS requested review from a team as code owners June 22, 2022 09:05

AidanBeltonS requested a review from againull June 22, 2022 09:05

AidanBeltonS mentioned this pull request Jun 22, 2022

[SYCL] Update tests to use local_accessor intel/llvm-test-suite#1063

Merged

Update simd_copy_to_copy_from.cpp line numbers

aec26e8

againull requested changes Jun 28, 2022

View reviewed changes

aidan.belton added 3 commits July 29, 2022 11:03

Merge branch 'sycl' of https://github.com/AidanBeltonS/llvm into loca…

42fd917

…l_accessor

Create local_accessor_base, make access target::local and local_acces…

712664e

…sor children

Update SemaSYCL to recognize and handle local_accessor

3dbfd0e

AidanBeltonS requested a review from a team as a code owner August 10, 2022 09:42

aidan.belton added 2 commits August 10, 2022 14:14

Merge branch 'sycl' of https://github.com/AidanBeltonS/llvm into loca…

7a13bb8

…l_accessor

Update kernel_arguments_as.cpp

8d0f764

elizabethandrews reviewed Aug 10, 2022

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

aidan.belton added 4 commits August 11, 2022 09:55

Remove isReadOnly

dec6b1f

Clang format

7009076

Clang-format accessor

3648d5c

Format with more upto date version

626a101

premanandrao previously approved these changes Aug 11, 2022

View reviewed changes

smanna12 previously approved these changes Aug 11, 2022

View reviewed changes

elizabethandrews reviewed Aug 11, 2022

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

steffenlarsen reviewed Aug 12, 2022

View reviewed changes

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

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

sycl/test/check_device_code/kernel_arguments_as.cpp Outdated Show resolved Hide resolved

sycl/test/basic_tests/set_arg_error.cpp Outdated Show resolved Hide resolved

Remove unneccessary parameter in isSyclAccessorType

e662262

AidanBeltonS dismissed stale reviews from smanna12 and premanandrao via e662262 August 12, 2022 14:17

AidanBeltonS temporarily deployed to aws August 12, 2022 14:20 Inactive

Apply suggestions from code review

69820f4

remove unnecessary cl namespace Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>

AidanBeltonS temporarily deployed to aws August 12, 2022 14:28 Inactive

aidan.belton added 2 commits August 12, 2022 17:38

Merge branch 'sycl' of https://github.com/AidanBeltonS/llvm into loca…

4bdfa84

…l_accessor

Add TODO

627e2f7

Update test namespace

f1f82cd

steffenlarsen approved these changes Aug 15, 2022

View reviewed changes

againull self-requested a review August 15, 2022 17:15

againull approved these changes Aug 15, 2022

View reviewed changes

smanna12 approved these changes Aug 15, 2022

View reviewed changes

v-klochkov approved these changes Aug 25, 2022

View reviewed changes

steffenlarsen merged commit e4423ef into intel:sycl Aug 25, 2022

AidanBeltonS mentioned this pull request Aug 26, 2022

[SYCL] Fix AccessorSubscript windows build issue #6648

Merged

AidanBeltonS deleted the local_accessor branch August 26, 2022 09:56

[SYCL] Add local_accessor and deprecate target::local #6341

[SYCL] Add local_accessor and deprecate target::local #6341

Uh oh!

Conversation

AidanBeltonS commented Jun 22, 2022

Uh oh!

againull left a comment

Choose a reason for hiding this comment

Uh oh!

againull Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

AidanBeltonS Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

againull Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

steffenlarsen Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

againull Jun 29, 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!

AidanBeltonS Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

smanna12 left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

steffenlarsen commented Aug 16, 2022

Uh oh!

v-klochkov left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen commented Aug 25, 2022

Uh oh!

AidanBeltonS commented Aug 26, 2022

Uh oh!

AidanBeltonS commented Aug 26, 2022

Uh oh!

steffenlarsen commented Aug 26, 2022

Uh oh!

Uh oh!

againull Jun 29, 2022 •

edited

Loading

smanna12 left a comment •

edited

Loading