Compiler error when indexing sycl::accessor with dimensions > 1

[bluebear94]

When compiling this code:

$ /opt/hipSYCL/bin/syclcc-clang mandelbrot.cpp -o mandelbrot --hipsycl-platform=cuda --hipsycl-gpu-arch=sm_62 --std=c++17 -Wall -Wextra -Wpedantic

[snip]

In file included from mandelbrot.cpp:4:
In file included from /opt/hipSYCL/bin/../include/CL/sycl.hpp:42:
In file included from /opt/hipSYCL/bin/../include/CL/sycl/queue.hpp:41:
In file included from /opt/hipSYCL/bin/../include/CL/sycl/handler.hpp:37:
/opt/hipSYCL/bin/../include/CL/sycl/accessor.hpp:528:76: error: calling a private constructor of class 'cl::sycl::accessor<unsigned char, 1, cl::sycl::access::mode::read,
      cl::sycl::access::target::host_buffer, cl::sycl::access::placeholder::false_t>'
    accessor<dataT, dimensions-1, accessmode, accessTarget, isPlaceholder> sub_accessor;
                                                                           ^
mandelbrot.cpp:74:19: note: in instantiation of function template specialization 'cl::sycl::accessor<unsigned char, 2, cl::sycl::access::mode::read, cl::sycl::access::target::host_buffer,
      cl::sycl::access::placeholder::false_t>::operator[]<2, void>' requested here
                        uint8_t idx = I[y][x];
                                       ^
/opt/hipSYCL/bin/../include/CL/sycl/accessor.hpp:598:3: note: declared private here
  accessor(){}
  ^
/opt/hipSYCL/bin/../include/CL/sycl/accessor.hpp:529:18: error: '_range' is a private member of 'cl::sycl::accessor<unsigned char, 1, cl::sycl::access::mode::read,
      cl::sycl::access::target::host_buffer, cl::sycl::access::placeholder::false_t>'
    sub_accessor._range = detail::range::omit_first_dimension(this->_range);
                 ^
/opt/hipSYCL/bin/../include/CL/sycl/accessor.hpp:602:21: note: declared private here
  range<dimensions> _range;
                    ^
/opt/hipSYCL/bin/../include/CL/sycl/accessor.hpp:530:18: error: '_buffer_range' is a private member of 'cl::sycl::accessor<unsigned char, 1, cl::sycl::access::mode::read,
      cl::sycl::access::target::host_buffer, cl::sycl::access::placeholder::false_t>'
    sub_accessor._buffer_range = _buffer_range;
                 ^
/opt/hipSYCL/bin/../include/CL/sycl/accessor.hpp:601:21: note: declared private here
  range<dimensions> _buffer_range;
                    ^
/opt/hipSYCL/bin/../include/CL/sycl/accessor.hpp:530:32: error: no viable overloaded '='
    sub_accessor._buffer_range = _buffer_range;
    ~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~
/opt/hipSYCL/bin/../include/CL/sycl/detail/../id.hpp:42:7: note: candidate function (the implicit move assignment operator) not viable: no known conversion from 'const range<2 aka 2>'
      to 'range<1>' for 1st argument
class range;
      ^
/opt/hipSYCL/bin/../include/CL/sycl/detail/../range.hpp:42:7: note: candidate function (the implicit copy assignment operator) not viable: no known conversion from 'const range<2 aka 2>'
      to 'const range<1>' for 1st argument
class range {
      ^
In file included from mandelbrot.cpp:4:
In file included from /opt/hipSYCL/bin/../include/CL/sycl.hpp:42:
In file included from /opt/hipSYCL/bin/../include/CL/sycl/queue.hpp:41:
In file included from /opt/hipSYCL/bin/../include/CL/sycl/handler.hpp:37:
/opt/hipSYCL/bin/../include/CL/sycl/accessor.hpp:531:18: error: '_ptr' is a private member of 'cl::sycl::accessor<unsigned char, 1, cl::sycl::access::mode::read,
      cl::sycl::access::target::host_buffer, cl::sycl::access::placeholder::false_t>'
    sub_accessor._ptr = this->_ptr + index * sub_accessor._range.size();
                 ^
/opt/hipSYCL/bin/../include/CL/sycl/accessor.hpp:600:16: note: declared private here
  pointer_type _ptr;
               ^
/opt/hipSYCL/bin/../include/CL/sycl/accessor.hpp:531:59: error: '_range' is a private member of 'cl::sycl::accessor<unsigned char, 1, cl::sycl::access::mode::read,
      cl::sycl::access::target::host_buffer, cl::sycl::access::placeholder::false_t>'
    sub_accessor._ptr = this->_ptr + index * sub_accessor._range.size();
                                                          ^
/opt/hipSYCL/bin/../include/CL/sycl/accessor.hpp:602:21: note: declared private here
  range<dimensions> _range;
                    ^
/opt/hipSYCL/bin/../include/CL/sycl/accessor.hpp:598:3: error: constructor for 'cl::sycl::accessor<unsigned char, 1, cl::sycl::access::mode::read, cl::sycl::access::target::host_buffer,
      cl::sycl::access::placeholder::false_t>' must explicitly initialize the base class 'detail::accessor_base<(target)2018, (placeholder)0>' which does not have a default constructor
  accessor(){}
  ^
/opt/hipSYCL/bin/../include/CL/sycl/accessor.hpp:528:76: note: in instantiation of member function 'cl::sycl::accessor<unsigned char, 1, cl::sycl::access::mode::read,
      cl::sycl::access::target::host_buffer, cl::sycl::access::placeholder::false_t>::accessor' requested here
    accessor<dataT, dimensions-1, accessmode, accessTarget, isPlaceholder> sub_accessor;
                                                                           ^
mandelbrot.cpp:74:19: note: in instantiation of function template specialization 'cl::sycl::accessor<unsigned char, 2, cl::sycl::access::mode::read, cl::sycl::access::target::host_buffer,
      cl::sycl::access::placeholder::false_t>::operator[]<2, void>' requested here
                        uint8_t idx = I[y][x];
                                       ^
/opt/hipSYCL/bin/../include/CL/sycl/accessor.hpp:197:7: note: 'cl::sycl::detail::accessor_base<cl::sycl::access::target::host_buffer, cl::sycl::access::placeholder::false_t>' declared here
class accessor_base<access::target::host_buffer, IsPlaceholder>
      ^

For the errors regarding private members, I think adding a templated friend declaration inside the definition of accessor is enough:

template<typename dataT2, int dimensions2, access::mode accessmode2, access::target accessTarget2, access::placeholder isPlaceholder2>
     friend class accessor;

The assigment on line 530 of accessor.hpp also needs to explicitly convert _buffer_range into the appropriate type. I'm not sure how to fix the error about the base class initialization, though.

[illuhad]

Thanks for reporting. There are no unit tests yet to cover this slice API, so it's not yet really expected to work ;)
Personally, I would like to focus on fixing bugs and packaging in order to get the 0.8 release out as soon as possible. Since transitioning from "there's some code related to that feature" to "that feature is officially supported and routinely tested" is a bit more involved than fixing a bug, I would like to postpone that after the 0.8 release.

In the meantime, if feasible for your code base, I would propose to index with sycl::id<2>, as you did in the kernel, e.g.

png_image[sycl::id<2>{x,y}] = ...

[tomdeakin]

I'm also running into this issue.

It's very convenient for stencil codes to index the access as buf[x][y] += buf[x-1][y] + buf[x+1][y] + ....

It's tricky because the return type of this accessor is unspecified in the spec, which is probably sensible as it's an internal implementation detail. Ultimately when everything is specified you'd want this to come out as Equation 4.3, but I'm not totally convinced this is possible.

[illuhad]

So far I have no concerns that it is possible to implement equation 4.3 in that way (I assume this is just the linear id equation, I don't know the equation numbering from the top of my head ;) ). After all, the linear id equation is just the regular linearization equation that C and C++ use for constructs like array[x][y]. But I am curious to know why you are skeptical?

Is it an acceptable workaround for your stencil code as well to just use sycl::id for indexing?

[tomdeakin]

Sorry, yes that's the linear id equation!

Do we not have to worry about the intermediate accessor[x]. Can the compiler go directly from accessor[x][y] to accessor[x*n+y] by definition rather doing (accessor[x])[y] and hoping for the inliner to fix it up? The accessor isn't just a pointer type so it's really a operator[] that we have to define.

[illuhad]

Ah, so you are effectively asking about various implementation choices (compiler vs library).

I suppose this could probably also be implemented with some compiler trickery to replace the nested operator[] calls with a direct calculation of the linear id. But in hipSYCL, this was never really an option because it requires much, much more engineering on the compiler front (which is also much harder to maintain) and the performance benefit is unclear. As I am the only regular hipSYCL developer, I don't really have the resources to implement something in such an expensive way if it's not unavoidable for performance (I also don't know if the other implementations have decided that this is worth it? triSYCL seems to just rely on boost::multi_array_ref for this feature, so no compiler support either)

Because of this, the current implementation in hipSYCL (which is not yet working) returns a proxy object from operator[], which is actually just an accessor of dimension-1 that has a modified access offset to account for the offset of the index from the current dimension.
There is some overhead associated with this because the accessor class is very complex, so creating multiple accessors is not ideal and also a bit convoluted (which is probably why it's broken now) - back when this code was written the implementation was much simpler ;)
However, I don't think that an implementation with a proxy object necessarily has to be inferior. For example, the proxy object could just store the pointer to the actual accessor and collect the access indices in an id<dim>. Once the id is complete, it could just invoke the regular accessor operator[id<dim>]. I don't think this will be any slower than an implementation relying on compiler magic.
Thinking about it, I suspect that this would also be a much cleaner solution compared to the current implementation attempt, so rather than fixing the current code maybe we should just switch to this approach...

[illuhad]

Houston, we have a fix! I took some time yesterday evening to implement the idea outlined above in PR #186.

[illuhad]

Fixed by PR #186.

[tomdeakin]

Thanks!!