-
Notifications
You must be signed in to change notification settings - Fork 69
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
Clarify requirements for types in device code #733
base: main
Are you sure you want to change the base?
Conversation
Still exhausted from the WG21 meeting, so hopefully this is coherent: If a type is an extended integer type, is that guaranteed to be supported on both the host and device? Same question goes for extended floating point types. |
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.
Thanks, @Pennycook. Please see inline comments.
I'm not very familiar with these extended types. Wouldn't this be very difficult for us to guarantee? It looks like these types are implementation defined, so there might be types that are defined by "a C++ implementation" (i.e., a standard library or host compiler) but that can't be made compatible with "a SYCL implementation" (i.e., a device compiler) or a specific device. Could we say something to the effect of: "Whether extended integer and/or extended floating-point types are available in device code is implementation-defined. However, if these types are available, the size, alignment, etc must match the host." |
That makes sense to me. |
A list of types that cannot be used in device code is clearer than what we had before, and will help to ensure that we give due consideration to additional types introduced in future versions of ISO C++.
These are implementation-defined, so whether they are available in device code should also be implementation-defined.
This gives us a place to talk about things like: - Which C++ standard library features (e.g., type aliases) are guaranteed to work on the device; and - Whether there are any additional restrictions on C++ standard library behavior.
There is only one long double type. Co-authored-by: Tom Honermann <[email protected]>
Just to add a useless grain of salt, a few of our users are already using So I like the
|
This is entirely valid under "undefined behavior". If an implementation wants to say that it supports The reason I'm suggesting we don't go with "implementation-defined" is that all existing implementations would then have to update their documentation to say something about |
Oh yes, sorry, It was not clear. It was 100% comment to say that I liked your clarification of new wording to use "undefined behavior"! This is nitpicking, but as you know, IEEE doesn't require long double to be 128 bits (but users can always check their long double size on the host and then check to correct aspect on the GPU, and if the device has it be happy because that mean (by some SYCL rules that I'm sure exist but I don't know -- triviality copyable?) they can use long-double on the GPU. |
It would be nice in the future if we could narrow down the possible behaviors. I can think of three possibilities:
Is random results really a possibility? |
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.
This looks good to me, so I'm giving it my stamp of approval for whatever that is worth. Thanks @Pennycook!
I might be wrong about this, but I convinced myself that random results could really be a possibility with existing SYCL 2020 implementations. Prior to this change, we didn't say anything about I agree that we don't want this to be the behavior forever, but I think the right way to fix things will be via a KHR or SYCL-Next feature. There are a few things to work out (e.g., whether we'd need separate aspects for |
The goal of this PR is to clarify:
As part of this change I removed the table of fundamental types, which duplicated a lot of information defined in C++. Just saying that we support the types with the same meaning as C++ seems simpler. I kept the list of types, but long-term I'd prefer to be able to say that SYCL supports all the C++ fundamental types and leave it at that.
Closes #372 and closes #373.
I can't assign this to you, @tahonermann, but since you originally opened these issues I'd appreciate it if you could take a look at this proposed fix.
I've opened it as a draft because it currently doesn't build, due to a broken reference to the table I removed. That reference will ve removed if we merge #730, though, so I wanted to open a PR and get some feedback.