Skip to content

[SYCL] No error when using wrong kind of accessor in explicit copy op #3109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
al42and opened this issue Jan 27, 2021 · 1 comment · Fixed by #3111
Closed

[SYCL] No error when using wrong kind of accessor in explicit copy op #3109

al42and opened this issue Jan 27, 2021 · 1 comment · Fixed by #3111

Comments

@al42and
Copy link
Contributor

al42and commented Jan 27, 2021

This issue is not about a bug, but imperfect handling of bad user code.

Per both SYCL-1.2.1 and SYCL-2020 (provisional), the source accessor to handler::copy must be either read or read_write.

However, the code below compiles and runs fine with the mode set to discard_write or write.

#include <CL/sycl.hpp>
#include <vector>

static constexpr int size = 32;

int main() {
  cl::sycl::device dev{cl::sycl::gpu_selector()};
  cl::sycl::queue q{dev};
  
  cl::sycl::buffer<int> d_x(cl::sycl::range<1>{size});
  std::vector<int> h_x(size, 0);

  q.submit([&](cl::sycl::handler &cgh) {
          auto accessor = d_x.get_access<cl::sycl::access::mode::write>(cgh);
          cgh.fill(accessor, 1);
  });

  q.submit([&](cl::sycl::handler &cgh) {
          auto accessor = d_x.get_access<cl::sycl::access::mode::write>(cgh); // <<<
          cgh.copy(accessor, h_x.data());
  });

  q.wait_and_throw();

  return 0;
}

An example where such a diagnostic would have been helpful (besides just being pedantic):

A kernel that only reads the buffer is enqueued after the cgh.copy. If the code had the correct access mode for copying, it would have been possible to execute the kernel and copy the data simultaneously. With the wrong access mode as in the example above, the code works, but the kernel is scheduled to wait for the data copy to complete. A simple static_assert here would have warned the user of their mistake and of missed optimization opportunity.

@keryell
Copy link
Contributor

keryell commented Jan 27, 2021

I like the straight-forward solution with a static_assert or something like that.

al42and added a commit to al42and/llvm that referenced this issue Jan 28, 2021
This patch introduces compile-time checks to cl::sycl::handler::copy
functions that ensure the correctness of accessors' access mode
(section 4.8.6 of SYCL-1.2.1 specification).
v-klochkov pushed a commit that referenced this issue Jan 28, 2021
This patch introduces compile-time checks to cl::sycl::handler::copy
functions that ensure the correctness of accessors' access mode
(section 4.8.6 of SYCL-1.2.1 specification).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants