-
Notifications
You must be signed in to change notification settings - Fork 78
Update device and platform version descriptions #231
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
Changes from 2 commits
663c3e4
2ecbb67
c499eee
2806ab0
aa1a98a
694a51a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1187,7 +1187,7 @@ info::platform::version | |
| ---- | ||
|
|
||
| @ [.code]#std::string# | ||
| a@ Returns the software driver version of the <<device>>. | ||
| a@ Returns a backend-defined <<platform>> version. | ||
|
|
||
| a@ | ||
| [source] | ||
|
|
@@ -2401,8 +2401,6 @@ info::device::driver_version | |
|
|
||
| @ [.code]#std::string# | ||
| a@ Returns a backend-defined driver version as a [code]#std::string#. | ||
| If using the OpenCL backend, the returned value represents the | ||
| OpenCL software driver version in the form: major_number.minor_number. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that the OpenCL backend interop spec says this corresponds to
Since OpenCL doesn't mandate any specific format, should we just say this in the SYCL spec directly? The SYCL definition would then be something like:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh it got updated in 3.0! That's a better definition for sure.
I feel like this a bit in conflict with the idea of having it backend-defined, the way I see it we say it's backend-defined in the main spec, then in the backend spec we can define it further to vendor-defined if we want, for the OpenCL plugin saying it's But then for say a CUDA backend we could define it to the compute capability in the format But if we just say it's vendor defined in the main spec I feel like we can't really give more details on what it's supposed to be in the backend spec.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the motivation for specifying this query as backend-defined, though? Most of our queries are not backend-defined. If the intent of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh right! I was still thinking about the But now that I think about it, I do think you're right that it makes more sense to have the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the |
||
|
|
||
| a@ | ||
| [source] | ||
|
|
@@ -2431,8 +2429,8 @@ info::device::version | |
| ---- | ||
|
|
||
| @ [.code]#std::string# | ||
| a@ Returns the SYCL version as a [code]#std::string# in the form: | ||
| [code]#<major_version>.<minor_version>#. | ||
| a@ Returns the year based SYCL version as a [code]#std::string#. This | ||
| property is deprecated. | ||
|
|
||
| a@ | ||
| [source] | ||
|
|
||
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.
Is it the intent that this number corresponds to the OpenCL specification version? For example, "1.2" would indicate that the backend conforms to OpenCL 1.2 and "3.0" would indicate that it conforms to OpenCL 3.0?
When I read this before, I though the intent was for each vendor to return some vendor-specific version number of the device driver. This is what DPC++ does currently. For example, we currently return this for one of our OpenCL devices:
"2021.13.11.0.23_160000"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 believe the intent for the device driver version is to report whatever the OpenCL implementation reports for
CL_DRIVER_VERSION, and that's defined in the OpenCL 1.2 spec as:Which, looking at a couple other OpenCL implementation seems to be a vendor specific number but still supposed to be a
major_number.minor_numberstring, which is fairly arbitrary.Since this part is for the OpenCL backend maybe we should just spell out what OpenCL properties these are intended to map to rather than copy-paste the definition from the OpenCL spec.
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've re-worked this paragraph into a table referring to the OpenCL properties which I think should be much clearer.
However I noticed that in the comments this section was referring to another one from the main spec: Platform mixed version support.
But I can't really make sense of what this section is trying to say or how this part of the OpenCL backend spec was addressing it, does it have something to do with the abandoned idea of mixing SYCL 1.2 and 2.2 devices as @keryell suggested?
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, I think this is to suggest that while the top-level supported version has some SYCL versions but using some platform might supporting only an older SYCL version. But since we no longer have true feature inclusion, I am not sure it still makes sense.