Skip to content
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

Allow "vec::load" / "vec::store" from/to an accessor #547

Open
gmlueck opened this issue Mar 25, 2024 · 1 comment
Open

Allow "vec::load" / "vec::store" from/to an accessor #547

gmlueck opened this issue Mar 25, 2024 · 1 comment

Comments

@gmlueck
Copy link
Contributor

gmlueck commented Mar 25, 2024

As KhronosGroup/SYCL-CTS#870 notes, the CTS seems to be testing that vec::load and vec::store can take an accessor as a parameter. See here.

This is not in the spec, but it seems generally useful. I wonder if it was just an oversight that we didn't add it to the spec. The fact that we included it in the CTS seems to indicate that it was our intent to allow vec::load / vec::store to an accessor.

If we decide to added these overloads, I think they would look like this:

class vec {
  // Available only when:
  //   (Mode == access_mode::read || Mode == access_mode::write) &&
  //   (std::is_same_v<DataT, std::remove_const_t<SrcDataT>> == true)
  template <typename SrcDataT, int Dimensions, access::mode Mode, access::placeholder IsPlaceholder>
  void load(size_t offset, accessor<SrcDataT, Dimensions, Mode, target::device, IsPlaceholder> acc);

  // Available only when:
  //   (Mode == access_mode::write || Mode == access_mode::read_write) &&
  //   (std::is_same_v<std::remove_const_t<DataT>, DestDataT> == true)
  template <typename DestDataT, int Dimensions, access::mode Mode, access::placeholder IsPlaceholder>
  void store(size_t offset, accessor<DestDataT, Dimensions, Mode, target::device, IsPlaceholder> acc)

I'm not sure if the CTS is testing with target::local or target::constant_buffer. If so, we could add deprecated overloads for these. (Those two targets are deprecated in SYCL 2020.)

Although the CTS does not test local_accessor, I presume we would want to allow that also. I think the only reason the CTS does not test local_accessor is because these tests were written in the SYCL 1.2.1 days, and we added local_accessor in SYCL 2020.

@gmlueck gmlueck added the Agenda To be discussed during a SYCL committee meeting label Mar 25, 2024
@gmlueck
Copy link
Contributor Author

gmlueck commented Mar 25, 2024

Thinking some more about this, we probably want to allow vec::load and vec::store to be used from a host task. In that case, we also need to allow target::host_task. Therefore, it might be better to add this as a template parameter with another constraint:

class vec {
  // Available only when:
  //   (Mode == access_mode::read || Mode == access_mode::write) &&
  //   (std::is_same_v<DataT, std::remove_const_t<SrcDataT>> == true) &&
  //   (Target == target::device || Target == target::host_task)
  template <typename SrcDataT, int Dimensions, access::mode Mode, target Target, access::placeholder IsPlaceholder>
  void load(size_t offset, accessor<SrcDataT, Dimensions, Mode, Target, IsPlaceholder> acc);

  // Available only when:
  //   (Mode == access_mode::write || Mode == access_mode::read_write) &&
  //   (std::is_same_v<std::remove_const_t<DataT>, DestDataT> == true) &&
  //   (Target == target::device || Target == target::host_task)
  template <typename DestDataT, int Dimensions, access::mode Mode, target Target, access::placeholder IsPlaceholder>
  void store(size_t offset, accessor<DestDataT, Dimensions, Mode, Target, IsPlaceholder> acc)

We could then allow target::local and target::constant_buffer by expanding the constraint.

@tomdeakin tomdeakin removed the Agenda To be discussed during a SYCL committee meeting label Mar 28, 2024
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

No branches or pull requests

2 participants