-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add x, y, and z shorthands to id and range classes #679
base: main
Are you sure you want to change the base?
Conversation
The numbering of dimensions in SYCL is aligned with the numbering of dimensions in ISO C++, such that the highest-numbered dimension is the fastest-moving. This numbering is inconvenient when working with generic functions compatible with one-, two-, or three-dimensional ranges, since developers must account somehow for differences in the numbering of the dimensions. One common solution is to define helper functions called x(), y() and z() to encapsulate this logic. This solution can also assist with the migration of code from other languages (e.g., OpenCL and CUDA), and may provide a simpler mental model for SYCL developers working with images or other forms of graphics interop. This commit adds x(), y() and z() functions directly to the id and range classes, providing a consistent way for SYCL developers to use this indexing pattern, rather than relying on each SYCL code base to define and maintain compatible index abstractions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't id
default to returning 0 instead of 1? Otherwise the linearization equation for some index idx
inside a 1D kernel of range rg
doesn't work:
linear_idx = idx.z() * rg.y() * rg.x() + idx.y() * rg.x() + idx.x()
= 1 * 1 * rg.x() + 1 * rg.x() + idx.x()
= 2 * rg.x() + id.x()
instead of idx.x()
.
In general I agree that working with ranges/ids of different dimensionalities can be annoying, particularly in generic contexts. That being said, the letters x/y/z/
may introduce some unwanted semantic associations; at least in 2D, I'd say x
is generally considered to be the horizontal axis, and y
the vertical. In my experience things can get pretty confusing if the code one is working with has a different notion of what x/y/z
means (for example when processing an image in column-major order). Perhaps i() / j() / k()
could serve as a more neutral alternative?
First, I agree it's one of the differences with SYCL that people struggle with. I personally hate I will prefer |
In order to produce the correct result from the linearization equation, id components should default to 0 while range components should default to 1.
Yes, it should. Good catch! Fixed in d642a37.
Why do you say this is unwanted? I might have misunderstood how images work in SYCL (I've not used them), but I think the situation we're in today is that the coordinates passed to functions like
I'm not strongly opposed to this, but I do think I don't think other names are as intuitive: for example, it may not be obvious to everybody that |
I wouldn't have a problem with introducing But if we were to move beyond 4 dimensions, I would propose that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you intentionally not adding these to item
?
Yes, this is deliberate. As the specification is currently written, the intended use would be |
Sorry, I meant plain image data, not
If I have a 2D SYCL buffer, to get coalesced memory access in a 2D kernel I need to index into the buffer like so:
If I for example wanted to do vertical edge detection on an image by computing the horizontal gradient, in a row-major buffer I would do it like so:
If the data is stored in column-major format however,
I suddenly need to do the offset computation on Because the letters |
I can see the argument, but I think it's also possible to flip it: if Before I ask this next question, I want to emphasize that I'm not proposing this change to try and turn SYCL into CUDA -- my goal is to make it easier for developers who struggle with the re-numbering of dimensions. But since we know that CUDA uses the |
Okay sure, if they can freely choose the format, that might be true.
The issue with the reverse ordering ( To be clear, this is not a major issue for me, I just wanted to point out potential downsides of this proposal. To me the most intuitive way of expressing memory order is always in terms of "fastest/slowest changing index", and I like that so far SYCL has steered clear of using terminology like "row-major", which is rooted in conventions. If we look to C++, as far as I can tell, the C++23 specification also doesn't use the terms "row-major" or "column-major"; The super descriptive but probably too verbose option would be to go with something like |
I appreciate the feedback! I'm sorry if I'm coming across as confrontational.
I agree that bringing row-major vs column-major into this would be even more confusing. I like how
I had a similar thought along these lines: I wondered whether I'm also worried that it could be more confusing to define Finally, I'm worried that introducing two different numeric ways to reference the same dimensions could get really confusing. Today, if we say "The second dimension of a 3D kernel" it's unambiguous; if we had |
Yes I agree with your concerns. Unfortunately I'm out of ideas; we should probably discuss this with the WG! |
Speaking personally here, the choice of defaulting to 1/0 for the range/id is sensible given how these APIs behave elsewhere (e.g., OpenCL I can see the value of providing this directly, rather than asking user code to write the template magic needed to pull out the dimension from the template parameter of these objects. Another aspect here is thanks to the CTAD stuff we make use of in SYCL, for a lot of codes you don't really have to know all the C++ mechanics that are going on under the hood, and so if this is a way to reduce what would otherwise be friction to adoption when migrating to SYCL, then that is probably a net win. I think this is just a symptom of aligning the fastest-moving dimension with C++ which means picking out the "last" one is painful, and C++ doesn't give us a solution there either. |
The numbering of dimensions in SYCL is aligned with the numbering of dimensions in ISO C++, such that the highest-numbered dimension is the fastest-moving.
This numbering is inconvenient when working with generic functions compatible with one-, two-, or three-dimensional ranges, since developers must account somehow for differences in the numbering of the dimensions. One common solution is to define helper functions called
x()
,y()
andz()
to encapsulate this logic. This solution can also assist with the migration of code from other languages (e.g., OpenCL and CUDA), and may provide a simpler mental model for SYCL developers working with images or other forms of graphics interop.This commit adds
x()
,y()
andz()
functions directly to theid
andrange
classes, providing a consistent way for SYCL developers to use this indexing pattern, rather than relying on each SYCL code base to define and maintain compatible index abstractions.I've proposed this as a SYCL-Next feature because, like #633, this is merely introducing a shorthand for something that is already supported by all SYCL implementations. Going through the full KHR process for this feature would require either function names like
khr_x()
or clones ofrange
andid
in thekhr::
namespace, which both feel like bad solutions to the problem.I'd also like to point out that use of these shorthands could be combined with the improved group interface being discussed in #638, allowing developers to write things like
item.id().x()
to access the fastest-moving component of an item'sid
.Finally, since there may be some confusion and I want to be very clear: I am not suggesting that we revisit the linearization equation for multi-dimensional SYCL quantities, and this is in no way a breaking change.
x()
,y()
andz()
are defined in terms of the existing dimension numbering and are thus fully backwards-compatible.