[SYCL] Implement local_accessor begin/end #6692

KornevNikita · 2022-09-02T13:10:06Z

SYCL2020 introduces various accessor begin/end member functions that
allow user to iterate through the underlying buffer.
llvm-test-suite patch: intel/llvm-test-suite#1225

SYCL2020 introduces various accessor begin/end member functions that allow user to iterate through the underlying buffer.

…erators

KornevNikita · 2022-09-02T15:13:17Z

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

KornevNikita · 2022-09-02T16:55:54Z

Looks like CI & jenkins/llvm-test-suite failures are not related to this patch. @steffenlarsen @AlexeySachkov take a look please

steffenlarsen · 2022-09-05T15:55:10Z

Looks like the same test fails on #6666

KornevNikita · 2022-09-05T15:59:45Z

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

steffenlarsen

LGTM, but how is this an ABI-break?

KornevNikita · 2022-09-06T12:00:05Z

Re-trigger tests

KornevNikita · 2022-09-06T13:59:26Z

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

KornevNikita · 2022-09-07T08:51:26Z

@AlexeySachkov @dm-vodopyanov could you please take a look?

KornevNikita · 2022-09-07T10:22:47Z

Re-trigger tests

…erators

KornevNikita · 2022-09-07T13:08:59Z

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

Fznamznon · 2022-09-07T13:24:54Z

sycl/include/sycl/accessor.hpp

  using const_reference = const DataT &;
+  using iterator = value_type *;


Please do not consider my comments as meaningful or blocking since I'm not an expert in this.
But, I'm just curious, will iterator that defined as a pointer work for some weird test cases like 2D sub-acccessor. See Ranged accessors in SYCL 2020 to understand what I mean - https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#sec:accessors.ranged . The phrase that concerns me most is:

If the ranged accessor is multi-dimensional, the sub-range is allowed to describe a region of memory in the underlying buffer that is not contiguous in the linear address space.

Make sense. Need to investigate, thank you

Removed this part of changes because it will be done in scope of #6815

KornevNikita · 2022-09-08T09:34:52Z

Converting to draft since this interface should also be implemented for local_accessor

…erators

KornevNikita · 2022-09-19T16:35:34Z

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

aelovikov-intel · 2022-09-19T16:38:12Z

How does it work without #6815 ?

KornevNikita · 2022-09-19T16:46:36Z

@aelovikov-intel Hi, we need a new special iterator class from #6815 only for the accessor, because accessor can be ranged hence it's underlying buffer may not be continuous in memory (please check Maria's comment above). For the local_accessor we can just use basic pointer to data, because his buffer always continuous.

aelovikov-intel · 2022-09-19T16:57:31Z

That makes sense, thanks! One more question - where does the guarantee that

local_accessor<double, 2> a{cgh, range{3, 3}};

would be consecutive comes from? Is it the standard or the implementation?

KornevNikita · 2022-09-20T11:43:02Z

@aelovikov-intel I can't find any confirmative statement about this moment in the spec. But, according to the same quote from the Maria's comment - If the ranged accessor is multi-dimensional, the sub-range is allowed to describe a region of memory in the underlying buffer that is not contiguous in the linear address space - I believe the only sub-range of the ranged accessor is allowed to describe a non-contiguous region. And since the local_accessor hasn't an offset - it can't be ranged, so it's ensured by implementation, isn't it?

aelovikov-intel · 2022-09-20T16:04:26Z

I don't think it does, because, despite similarities, accessor and local_accessor are different entities after all.

I'm fine if our implementation ensures that and it would be enough for me.

aelovikov-intel

LGTM, I'll let others to take a look/approve.

Fznamznon

No more tricky questions from my side :)

steffenlarsen

LGTM!

KornevNikita · 2022-09-21T12:22:53Z

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

AlexeySachkov · 2022-09-22T12:47:59Z

@KornevNikita, even though failures look unrelated, we need to have them fixed or at least covered by a tracker - could you please create such?

sycl patch: intel/llvm#6692

sycl patch: intel#6692

[SYCL][ABI-BREAK] Implement accessor begin/end

f31f26e

SYCL2020 introduces various accessor begin/end member functions that allow user to iterate through the underlying buffer.

KornevNikita requested a review from a team as a code owner September 2, 2022 13:10

KornevNikita requested a review from steffenlarsen September 2, 2022 13:10

KornevNikita mentioned this pull request Sep 2, 2022

[SYCL] Add local_accessor iterator tests intel/llvm-test-suite#1225

Merged

Merge branch 'sycl' of https://github.com/intel/llvm into accessor-it…

2813ac1

…erators

steffenlarsen previously approved these changes Sep 5, 2022

View reviewed changes

KornevNikita changed the title ~~[SYCL][ABI-BREAK] Implement accessor begin/end~~ [SYCL] Implement accessor begin/end Sep 5, 2022

KornevNikita closed this Sep 6, 2022

KornevNikita reopened this Sep 6, 2022

KornevNikita closed this Sep 7, 2022

KornevNikita reopened this Sep 7, 2022

Merge branch 'sycl' of https://github.com/intel/llvm into accessor-it…

7d6106d

…erators

Fznamznon reviewed Sep 7, 2022

View reviewed changes

KornevNikita marked this pull request as draft September 8, 2022 09:34

KornevNikita dismissed steffenlarsen’s stale review via 9441e5c September 9, 2022 15:01

KornevNikita added 4 commits September 9, 2022 08:01

Implement local_accessor iterator funcs

9441e5c

Fix clang-format

2cf0f22

Merge branch 'sycl' of https://github.com/intel/llvm into accessor-it…

faf32c0

…erators

Remove accessor changes

5f4315c

KornevNikita changed the title ~~[SYCL] Implement accessor begin/end~~ [SYCL] Implement local_accessor begin/end Sep 19, 2022

KornevNikita added 2 commits September 19, 2022 08:14

Merge branch 'sycl' of https://github.com/intel/llvm into accessor-it…

fe3ac6c

…erators

Typo

af5dd11

KornevNikita marked this pull request as ready for review September 19, 2022 16:35

aelovikov-intel reviewed Sep 20, 2022

View reviewed changes

KornevNikita requested review from Fznamznon and steffenlarsen September 20, 2022 16:49

Fznamznon reviewed Sep 20, 2022

View reviewed changes

steffenlarsen approved these changes Sep 20, 2022

View reviewed changes

KornevNikita requested a review from AlexeySachkov September 21, 2022 08:54

AlexeySachkov merged commit 5b9fd3c into intel:sycl Sep 22, 2022

KornevNikita deleted the accessor-iterators branch September 23, 2022 10:07

steffenlarsen pushed a commit to intel/llvm-test-suite that referenced this pull request Oct 13, 2022

[SYCL] Add local_accessor iterator tests (#1225)

119307e

sycl patch: intel/llvm#6692

AlexeySachkov mentioned this pull request Oct 19, 2022

Fix and enable local_accessor iterator requirements test for DPCPP implementation KhronosGroup/SYCL-CTS#390

Merged

aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023

[SYCL] Add local_accessor iterator tests (intel/llvm-test-suite#1225)

4026184

sycl patch: intel#6692

		using const_reference = const DataT &;
		using iterator = value_type *;

[SYCL] Implement local_accessor begin/end #6692

[SYCL] Implement local_accessor begin/end #6692

Uh oh!

Conversation

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

Uh oh!

Uh oh!

KornevNikita commented Sep 2, 2022

Uh oh!

KornevNikita commented Sep 2, 2022

Uh oh!

steffenlarsen commented Sep 5, 2022

Uh oh!

KornevNikita commented Sep 5, 2022

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

KornevNikita commented Sep 6, 2022

Uh oh!

KornevNikita commented Sep 6, 2022

Uh oh!

KornevNikita commented Sep 7, 2022

Uh oh!

KornevNikita commented Sep 7, 2022

Uh oh!

KornevNikita commented Sep 7, 2022

Uh oh!

Fznamznon Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

KornevNikita Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

KornevNikita Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

KornevNikita commented Sep 8, 2022

Uh oh!

KornevNikita commented Sep 19, 2022

Uh oh!

aelovikov-intel commented Sep 19, 2022

Uh oh!

KornevNikita commented Sep 19, 2022

Uh oh!

aelovikov-intel commented Sep 19, 2022

Uh oh!

KornevNikita commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aelovikov-intel commented Sep 20, 2022

Uh oh!

aelovikov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

KornevNikita commented Sep 21, 2022

Uh oh!

AlexeySachkov commented Sep 22, 2022

Uh oh!

Uh oh!

KornevNikita commented Sep 2, 2022 •

edited

Loading

KornevNikita commented Sep 20, 2022 •

edited

Loading