[SYCL] Implement accessor iterator #6815

AlexeySachkov · 2022-09-19T12:19:46Z

Added base type for implementing accessor iterators.
Implemented accessor::begin() and accessor::end() methods.

Added base type for implementing accessor iterators. Implemented `accessor::begin()` and `accessor::end()` methods.

sycl/include/sycl/detail/accessor_iterator.hpp

aelovikov-intel · 2022-09-19T15:56:17Z

sycl/include/sycl/detail/accessor_iterator.hpp

+      _MLinearCurrent = _MLinearEnd;
+    else
+      _MLinearCurrent += _N;
+    _MCurrent = __delinearizeIndex(_MLinearCurrent);


Can we have a test checking the generated LLVM IR and/or target assembly to ensure the compiler toolchain can generate it efficiently (i.e. without doing division).

I'm not entirely sure that we can add a test which checks for optimized LLVM IR, because some optimizations are done by low-level runtimes and I doubt that such test would be reliable.

We also synced offline and there is a generic comment that we shouldn't pay the price of handling ranged accessors with we are dealing with non-ranged accessors. The latter should be as much as possible close to a plain pointer. I have an idea how it can be done, I will post an update to the implementation.

I've rewritten the implementation, so it is now should be as cheap as possible for non-ranged accessors, see ba45577

sycl/include/sycl/detail/accessor_iterator.hpp

sycl/unittests/accessor/AccessorIterator.cpp

sycl/include/sycl/accessor.hpp

…ssor-iterator

Now iterators for non-ranged accessors should be almost equal to plain pointers and only iterator dereference for a ranged accessor is a costly operation which requires various calculations including division.

…r constructor

This is a prepartion commit for when the iterator won't hold a pointer to an accessor at all.

…ssor-iterator

steffenlarsen

Looks solid, well documented, and thoroughly tested. However, I am a little on the fence about the use of reserved identifiers before we conclude on it in #2981. Granted the SYCL subproject regrettably isn't very consistent in naming, having some parts using reserved names just adds to it.

steffenlarsen · 2022-10-06T11:18:06Z

sycl/include/sycl/detail/accessor_iterator.hpp

+      : _MDataPtr(_DataPtr) {
+    constexpr int _XIndex = _Dimensions - 1;
+    constexpr int _YIndex = _Dimensions - 2;
+    (void)_YIndex;


Very much a nit, but I would prefer we lean into using std::ignore for these situations.

So, looking at cppreference I don't quite understand how exactly should I use it in here.

From what I understand std::ignore simply defines a custom operator=, which allows to silence the warning, but those values here are not always ignored

Should just be something like

Suggested change

(void)_YIndex;

std::ignore = _YIndex;

keryell · 2022-10-07T08:51:40Z

sycl/include/sycl/detail/accessor_iterator.hpp

+  }
+
+  __accessor_iterator &operator++() {
+    if (_MLinearId < _MEnd)


What is the reason for having all these tests?
I guess that it will diminish the performance.

What is the reason for having all these tests?

See my reply to your other comment. Basically, the reasoning is quite weak and we should probably indeed remove those checks.

Simplified the implementation in 8e2db4e

keryell · 2022-10-07T08:53:39Z

sycl/unittests/accessor/AccessorIterator.cpp

+  }
+  {
+    auto It = accessor.end();
+    // Check that It can't be incremented past end


Is it in the SYCL specification?

It is neither a part of SYCL specification nor it is a part of C++ specification. This is a test for an implementation detail. Essentially, C++ says that decrementing past .begin() or incrementing past .end() is a UB. By some reason, I decided to make this UB more user-friendly.

I agree that it is not strictly necessary and it makes sense to remove associated checks from the implementation.

Removed the test and related parts of the implementation in 8e2db4e

I understand your motivation.
But UB in C/C++ are not there to annoy the user but to allow bare-metal speed when needed. At the end the goal is not to have UB anyway and less speed. :-)
Safe users or coding rules can avoid the use of pointers and iterators and use std::span, std::ranges, etc.

AlexeySachkov · 2022-10-07T14:09:06Z

However, I am a little on the fence about the use of reserved identifiers before we conclude on it in #2981. Granted the SYCL subproject regrettably isn't very consistent in naming, having some parts using reserved names just adds to it.

Ok, unless that discussion is concluded I reverted back to using regular non-reserved identifiers in a6effbb

steffenlarsen

LGTM!

[SYCL] Implement accessor iterator

f20318c

Added base type for implementing accessor iterators. Implemented `accessor::begin()` and `accessor::end()` methods.

AlexeySachkov requested review from dm-vodopyanov, KornevNikita and steffenlarsen September 19, 2022 12:19

Add more tests and fix operator+

2af129d

KornevNikita mentioned this pull request Sep 19, 2022

[SYCL] Implement local_accessor begin/end #6692

Merged

aelovikov-intel reviewed Sep 19, 2022

View reviewed changes

KornevNikita reviewed Sep 20, 2022

View reviewed changes

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

KornevNikita mentioned this pull request Sep 20, 2022

begin, end, cbegin and cend operations are not available for accessors #6788

Closed

AlexeySachkov added 6 commits September 20, 2022 11:16

Merge remote-tracking branch 'origin/sycl' into private/asachkov/acce…

a3c98fb

…ssor-iterator

Mark begin/end methods as const

d684185

Generalize test code a bit

2dff81d

Fixes and tests for offset + range accessors

87e771f

Remove LIT which was used for local experiments

36364a0

Completely refactor accessor::iterator

ba45577

Now iterators for non-ranged accessors should be almost equal to plain pointers and only iterator dereference for a ranged accessor is a costly operation which requires various calculations including division.

AlexeySachkov mentioned this pull request Sep 28, 2022

buffer::get_host_access() can't be compiled with gcc 8.4 #6892

Closed

AlexeySachkov added 10 commits September 28, 2022 09:15

Some fixes and a couple more tests

53d5799

Refactor tests to prepare for their generalization

90004cd

Do not store accessor memory range in iterator

7ff5db5

Remove uses of accessor::get_offset() from methods other than iterato…

d785db4

…r constructor

Do not use accessor APIs in the iterator constructor

7e5789b

This is a prepartion commit for when the iterator won't hold a pointer to an accessor at all.

Do not store a reference to an accessor within iterator

a4a1817

Add cbegin/cend and some tests for those methods

47e6c46

Merge remote-tracking branch 'origin/sycl' into private/asachkov/acce…

ef83061

…ssor-iterator

Fix build after merge

9ef84c2

Fix Wreorder warning coming from AccessorSubscript constructor

91f0d30

AlexeySachkov changed the title ~~[WIP] [SYCL] Implement accessor iterator~~ [SYCL] Implement accessor iterator Oct 4, 2022

AlexeySachkov marked this pull request as ready for review October 4, 2022 13:38

AlexeySachkov requested a review from a team as a code owner October 4, 2022 13:38

steffenlarsen reviewed Oct 6, 2022

View reviewed changes

steffenlarsen mentioned this pull request Oct 6, 2022

Compilation with -DVL=... crashes in type_list.hpp #2981

Open

keryell reviewed Oct 7, 2022

View reviewed changes

AlexeySachkov added 3 commits October 7, 2022 08:34

Allow iterators to be decremented past begin and incremented past end

8e2db4e

Add tests for writing through an iterator

769ab13

Revert back from using reserved identifiers

a6effbb

Fix clang-format

600916b

steffenlarsen approved these changes Oct 7, 2022

View reviewed changes

Return some undescores from comments back

78215dc

AlexeySachkov merged commit c7b1a00 into intel:sycl Oct 7, 2022

AlexeySachkov deleted the private/asachkov/accessor-iterator branch May 22, 2024 09:51

[SYCL] Implement accessor iterator #6815

[SYCL] Implement accessor iterator #6815

Uh oh!

Conversation

AlexeySachkov commented Sep 19, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov commented Oct 7, 2022

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!