Update device and platform version descriptions #231
Conversation
This patch updates: * `device::version` description to use the new SYCL versioning scheme. * Update wording of `platform::version`, a platform has multiple devices so referencing the device doesn't seem sensible. * Remove mentions of OpenCL backend in descriptors table * Update OpenCL backend doc to describe the meaning of the version numbers.
| @@ -2432,7 +2430,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>#. | |||
| [code]#<version>.<revision>#, with [code]#<version># being a year based | |||
| number, for example [code]#2020.4#. | |||
There was a problem hiding this comment.
This query doesn't make sense to me. The implementation already exposes the SYCL version via the SYCL_LANGUAGE_VERSION macro. Since the entire implementation is conformant to that version, won't every device return the same information via info::device::version?
I'm also not sure we should return the minor version number here (or in SYCL_LANGUAGE_VERSION). Do we really expect implementations to change each time we make a minor version release of the spec? Will we add a conformance test to validate that the implementation returns the most up-to-date minor version? Will vendors need to re-validate each time there is a new minor release of the spec?
Is it too late to mark this info descriptor as "deprecated"? We still need to define the semantic even if it is deprecated. We could either say that it returns "2020", or say something more generic like:
Returns a vendor-defined string for the device.
There was a problem hiding this comment.
Perhaps it is a left-over of SYCL 2.2 when we had the idea of having SYCL 1.2 and 2.2 compatible devices in the same program.
There was a problem hiding this comment.
Yeah having the specification revision in there doesn't really make sense.
We could just return say "2020" and mark this property as deprecated, I think that would make sense.
It would just be a little bit of a shame to "lose" a simple property like that, that could easily be used by backends to report more informative information on the device such as OpenCL device version, architecture, compute capabilities and such.
So ideally I think we should aim to re-purpose this property, but it makes sense to deprecate it first.
There was a problem hiding this comment.
Updated the PR to mark info::device::version as deprecated and make it return just the year based SYCL version.
There was a problem hiding this comment.
It would just be a little bit of a shame to "lose" a simple property like that, that could easily be used by backends to report more informative information on the device such as OpenCL device version, architecture, compute capabilities and such.
If you feel this way, then we should not add the wording you proposed in 2ecbb67. Instead, we should say something like:
Returns a vendor-defined string for the device
This gives vendors the flexibility to return information like you suggest.
There was a problem hiding this comment.
Updated with what I suggested, although, note that I was thinking of backend-defined rather than vendor-defined, so what the value should be would still be defined by the SYCL spec but just on a per backend basis.
I feel like that gives us enough flexibility to report useful information for the specific backend types while still giving some consistency across vendors.
|
defined meaning to string is a concern |
adoc/chapters/opencl_backend.adoc
Outdated
| [code]#sycl::device::get_info()# for the attributes | ||
| [code]#info::platform::driver_version# and [code]#info::device::driver_version# | ||
| respectively, represents the OpenCL software driver version in the form: | ||
| [code]#major_number.minor_number#. |
There was a problem hiding this comment.
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.
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:
OpenCL software driver version string in the form major_number.minor_number
Which, looking at a couple other OpenCL implementation seems to be a vendor specific number but still supposed to be a major_number.minor_number string, 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.
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.
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.
| @@ -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. | |||
There was a problem hiding this comment.
I see that the OpenCL backend interop spec says this corresponds to CL_DRIVER_VERSION. OpenCL 3.0 defines that as:
OpenCL software driver version string. Follows a vendor-specific format.
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:
A vendor-defined string describing the version of the underlying backend software driver.
There was a problem hiding this comment.
Oh it got updated in 3.0! That's a better definition for sure.
A vendor-defined string describing the version of the underlying backend software driver.
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 CL_DRIVER_VERSION defers it to the OpenCL spec which ends up being vendor defined anyway.
But then for say a CUDA backend we could define it to the compute capability in the format major.minor for example rather than having it vendor defined.
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.
There was a problem hiding this comment.
What is the motivation for specifying this query as backend-defined, though? Most of our queries are not backend-defined. If the intent of driver_version is to return the version string of the vendor's device driver, it's hard to see why a backend would mandate any specific format. Wouldn't we want each vendor to have the freedom to decide it's own format?
There was a problem hiding this comment.
Oh right! I was still thinking about the version rather than the driver_version, since the driver_version was already backend specific before my patch.
But now that I think about it, I do think you're right that it makes more sense to have the driver_version simply be vendor specific, I'll change it as well.
There was a problem hiding this comment.
Updated the driver_version as suggested and removed the CL_DRIVER_VERSION part from the OpenCL backend spec.
Update device and platform version descriptions
This PR is a draft in attempt to resolve #222
Any further feedback on these is appreciated.
This patch updates:
device::versiondescription to use the new SYCL versioning scheme.platform::version, a platform has multiple devicesso referencing the device doesn't seem sensible.
numbers.