Skip to content

Conversation

fknorr
Copy link
Contributor

@fknorr fknorr commented Mar 14, 2023

This PR implements support for 0-dimensional buffers, 0-dimensional global and local accessors as well as 0-dimensional kernels as proposed by the SYCL standard without relying on the feature being present in the SYCL implementation.

Most of the changed lines simply replace sycl::{range,id} with celerity::{range,id}. We need our own implementation here to support the 0-dimensional case independently from SYCL (in fact the new Celerity coordinate types can be arbitrary-dimensional, even though buffers and kernels are limited to Dims <= 3 as before).

0-dimensional accessors do not take range mappers on construction since every access must be an all-access anyway. To support the different signatures in CTAD the accessor type deduction logic was re-written from scratch.

The PR also gets rid of the broken backwards compatibility layer for sycl::item kernels.

Open Question

In SYCL, 0-dimensional accessors do not have an operator[](id<0>), but only operator*(). Should we support this (and also maybe range-mappers on 0-dimensional accessors) as an extension to ease generic programming on buffer dimensionality?

Edit: Yes we should.

Edit 2: Actually SYCL does not have pointer semantics but implicit conversions to reference-type for 0-dim accessors. We are currently undecided if we want to go the same route.

Edit 3: We are going to implement both, our sane API and also SYCL's for porting compatibility.

github-actions[bot]

This comment was marked as off-topic.

Copy link
Member

@psalz psalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a first pass, looks good, thanks! Since you mentioned that you'll have to do the s/sycl::/celerity::/ replacement again anyway, could you maybe do it as a separate commit to make reviewing easier?

@fknorr fknorr force-pushed the zero-sized-buffers branch 2 times, most recently from 2a31dad to 1a8d067 Compare March 28, 2023 13:53
github-actions[bot]

This comment was marked as off-topic.

@fknorr fknorr force-pushed the zero-sized-buffers branch 3 times, most recently from 033bcd6 to b10c485 Compare March 28, 2023 16:53
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Copy link
Member

@psalz psalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more notes!

@fknorr
Copy link
Contributor Author

fknorr commented Mar 30, 2023

From live discussion with @PeterTh:

I didn't read the SYCL spec for 0-dimensional accessors properly. It defines a reference-conversion operator and an assignment operator instead of operator*:

  /* Available only when: (AccessMode != access_mode::atomic && Dimensions == 0) */
  operator reference() const;

  /* Available only when: (AccessMode != access_mode::atomic && Dimensions == 0) */
  const accessor& operator=(const value_type& other) const;

  /* Available only when: (AccessMode != access_mode::atomic && Dimensions == 0) */
  const accessor& operator=(value_type&& other) const;

This allows an accessor to be used like a T& reference in some contexts, but for a buffer of structs e.g. buffer<sycl::float4> would require a static cast before accessing a member, which is quite hideous. I like the pointer API (operator* and TODO operator->) much better in that regard.

We could now either copy what SYCL does or break with the standard, but IMO we should have some form of operator* or a get() function regardless for usability.

Regarding operator[] and range mappers for Dims == 0 we agree that we should have both. One minor weirdness would arise from this code being allowed since master-node host tasks are zero-dimensional, which is not apparent to the user otherwise:

buffer<int, 0> buf = 42;
q.submit([=](handler &cgh) {
    accessor acc(buf, cgh, one_to_one(), read_only);
    cgh.host_task(on_master_node, ...);
});

Edit: We're implementing both the SYCL API and our operator*/operator-> API.

github-actions[bot]

This comment was marked as off-topic.

@fknorr fknorr force-pushed the zero-sized-buffers branch 2 times, most recently from d275449 to 7d78046 Compare March 30, 2023 14:15
Copy link
Contributor

@PeterTh PeterTh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only looked at 22/44 files so far, but I'll probably not get to the rest soon so I'll post that part for now.

github-actions[bot]

This comment was marked as off-topic.

@fknorr fknorr force-pushed the zero-sized-buffers branch 2 times, most recently from 0e36e75 to a546198 Compare April 4, 2023 10:23
github-actions[bot]

This comment was marked as off-topic.

github-actions[bot]

This comment was marked as off-topic.

@celerity celerity deleted a comment from github-actions bot Apr 5, 2023
@fknorr fknorr force-pushed the zero-sized-buffers branch from 0632927 to 7465169 Compare April 5, 2023 10:10
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Copy link
Contributor

@PeterTh PeterTh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but maybe run the GPUC2 benchmarks before merging and update the result files.

fknorr and others added 24 commits April 12, 2023 14:26
Co-authored-by: Philip Salzmann <philip.salzmann@uibk.ac.at>
@fknorr fknorr force-pushed the zero-sized-buffers branch from 1a4bf42 to b3b9507 Compare April 12, 2023 12:32
@fknorr fknorr merged commit 7dfe65a into master Apr 12, 2023
@fknorr fknorr deleted the zero-sized-buffers branch April 12, 2023 15:59
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 this pull request may close these issues.

3 participants