Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[SYCL][Doc] Add spec to get device image backend content #14811
[SYCL][Doc] Add spec to get device image backend content #14811
Changes from 5 commits
f09c9d6
2cd6478
793b739
bf6b734
a548105
5f17499
ef53b68
acd6b94
d288119
236c32e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think we should drop this, and rely on the
ifdef
in the synopsis. This is really saying "Available only when the library provides an implementation ofstd::span
", and that's implied by usage of the type.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.
Let's try to agree on standard wording for cases like this because I think it will come up more and more.
I feel like we should say somewhere that this API is tied to C++20. How do you feel about the wording I added in d288119?
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.
I would have said this is good enough, but if you'd like to settle on standard wording I'd like to talk about it a bit more.
I think it would be more accurate to say that the implementation defines the macro. ISO C++ says the implementation only defines the macro if you include
<version>
or<span>
.Given the above, do you want to say anything about who is responsible for including
<span>
somewhere? It probably shouldn't be part of the function synopsis, but we should make it clear that if this extension is implemented thensycl.hpp
is expected to include<span>
if it's available (see__has_include
), and that this isn't a user's responsibility. For the actual SYCL specification, I imagine we'd have a single dedicated section to explain how compilation with different versions of C++ works, and we could cover this there instead of in each extension.Finally, I think we could improve the formatting. We should add a title to the paragraph (like we do with Constraints, etc) and consider rephrasing the "Available only when" (to further differentiate from constraints). How about something like:
I deliberately put "C++" in the title in an attempt to be future-proof. Even if ISO C++ decided to introduce a similar concept with the same name, it would probably just be called "Required Features" (since the C++ would be implied).
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.
Hmm, this is making me think that my original wording was better. There are plenty of C++ feature-test macros even for features that exist in C++17. For example,
__cpp_lib_byte
tells whetherstd::byte
is available. Are we going to add similar wording for every API that usesstd::byte
or every API that uses any C++ feature with a corresponding feature-test macro? This seems unnecessarily complex to me.I think the SYCL spec should assume a C++ compiler that is fully conformant to whatever C++ version it claims to be. Implementations can then decide to make some of these APIs conditionally available for the benefit of partially conformant C++ compilers if they so desire. But, this would be a quality of implementation thing, not part of the specification.
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.
I agree it would be unnecessary to go back and add the features for C++17, but I think that's because SYCL currently requires C++17 as a base and so we can assume that all C++17 features are available. If we were to change that and say that SYCL is compatible with every version of C++, but that different subsets of features are available based on the capabilities of your C++ implementation, then I absolutely think we should clearly label the requirements of each SYCL feature.
What I like about separating the macros from the C++ version is that it allows for a world where developers can opt-in to certain things, which I think we (and other implementations) might want.
For example, let's say that we design a feature (like a replacement for accessors) that uses
std::mdspan
. I think we'd want to make that available to as many developers as possible as quickly as possible. But there may be a lot of developers who are not willing/able to adopt C++23, or there might only be incomplete support for C++23 in their compiler/library. In such a situation, we could add a compiler option to makestd::mdspan
available with earlier versions of C++: DPC++ would provide<mdspan>
,__has_include(<mdspan>)
would evaluate to true, and__cpp_lib_mdspan
would be defined appropriately, but the__cplusplus
macro would still evaluate to C++17.I believe my thinking here is aligned with the reasoning behind introducing these macros in the first place (e.g., see https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations#explanation-and-rationale-for-the-approach).
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.
It looks like this is going to be a longer conversation, and I'd like to get implementation started on this extension. Would you be OK approving what we have now, and I'll open an issue in the Khronos repo to continue discussion about the precise wording that we'll use going forward?
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.
Yes, absolutely!
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.
Issue opened: KhronosGroup/SYCL-Docs#667