[SYCL] Align get_info<info::device::version>() with the SYCL spec #2507

DenisBakhvalov · 2020-09-21T17:47:43Z

According to the SYCL spec, cl::sycl::info::device::version
should be returned in a form: <major_version>.<minor_version>

This patch trims the string returned from the piDeviceGetInfo call.
For example, for the string "OpenCL 2.1 (Build 0)",
it will return "2.1".

According to the SYCL spec, cl::sycl::info::device::version should be returned in a form: `<major_version>.<minor_version>` This patch trims the string returned from the piDeviceGetInfo call. For example, for the string "OpenCL 2.1 (Build 0)", it will return "2.1".

sycl/test/plugins/sycl-ls-gpu-opencl.cpp

dm-vodopyanov

Please update basic_tests/parallel_for_range, sub_group/broadcast and sub_group/broadcast_fp64 tests, and add test cases for L0, CUDA, OpenCL CPU, OpenCL ACC and host device.

This reverts commit 8d4876a.

sycl/test/plugins/sycl-ls-gpu-opencl.cpp

vladimirlaz · 2020-09-23T07:07:58Z

sycl/source/detail/device_info.hpp

+      leftPos++;
+
+    auto rightPos = result.find(' ', dotPos);
+    return result.substr(leftPos, rightPos - leftPos);


the function is not robust. If version string contains "bla X.Y.Z bla" it will report X.Y.Z which also violates spec.

Yes, I know. It will also violate the spec if the plugin call returns the string without a dot, like 'XYZ'. What we should do in this case? Throwing an exception seems too strict, isn't it?

can we make sure that if format of the version returned by BE does not fit our requirements (e.g. no dot, X.Y.Z, X.Y at the end of string) the user friendly string will be displayed (e.g. the whole string returned by BE, X.Y(.Z is ignored), X.Y).

Ok. This is what I'm doing if the string doesn't have the dot. E.g. For string "bla XdotY bla" I return "bla XdotY bla".
I'm not sure how you define "user friendly string".

sycl/test/basic_tests/info_ocl_version-cuda.cpp

sycl/test/basic_tests/info_ocl_version.cpp

sycl/test/basic_tests/parallel_for_range.cpp

vladimirlaz · 2020-09-24T03:38:59Z

you updated info::device::version but spec contains also info::platform::version with the same wording. Could you please ensure that is follows the spec as well.

DenisBakhvalov · 2020-09-24T03:50:15Z

you updated info::device::version but spec contains also info::platform::version with the same wording. Could you please ensure that is follows the spec as well.

Sure, I can do that. Don't you mind I do it in a separate patch?

vladimirlaz · 2020-09-24T03:57:34Z

you updated info::device::version but spec contains also info::platform::version with the same wording. Could you please ensure that is follows the spec as well.

Sure, I can do that. Don't you mind I do it in a separate patch?

ok for me to do that in separate patch

vladimirlaz · 2020-09-24T04:07:27Z

@DenisBakhvalov could you please put this change on hold until question below is resolved?

@bader I see that in SYCL 2020 spec there is no requirement for info::platform::version format (major_number.minor_number).
It stays only for device. Do we really need to apply this change? We can stay with the version format returned by BE to avoid confusion.

vladimirlaz · 2020-09-24T04:03:51Z

sycl/source/detail/device_info.hpp

+      leftPos++;
+
+    auto rightPos = result.find(' ', dotPos);
+    return result.substr(leftPos, rightPos - leftPos);


can we make sure that if format of the version returned by BE does not fit our requirements (e.g. no dot, X.Y.Z, X.Y at the end of string) the user friendly string will be displayed (e.g. the whole string returned by BE, X.Y(.Z is ignored), X.Y).

bader · 2020-09-24T09:46:21Z

@DenisBakhvalov could you please put this change on hold until question below is resolved?

@bader I see that in SYCL 2020 spec there is no requirement for info::platform::version format (major_number.minor_number).
It stays only for device. Do we really need to apply this change? We can stay with the version format returned by BE to avoid confusion.

Good question. @intel/dpcpp-specification-reviewers, what do you think?

DenisBakhvalov · 2020-09-28T19:56:40Z

pinging @intel/dpcpp-specification-reviewers.

dm-vodopyanov · 2020-09-29T16:05:25Z

@jbrodman, @mkinsner, could you please answer the question above?

DenisBakhvalov · 2020-10-02T21:19:52Z

@jbrodman, @mkinsner, could you please answer the question above?

@jbrodman, @mkinsner, ping

bader · 2020-10-12T09:35:28Z

@vladimirlaz, SYCL-2020 defines info::platform::version as Returns the software driver version of the device., w/o restricting string format. I think we can keep the patch limited to info::device::version only.

* sycl: (566 commits) [SYCL] Fix explicit copy operation for host device (intel#2627) [SYCL] Fix initialization issue on Windows (intel#2632) [SYCL][CUDA] Disable image_write test on CUDA devices (intel#2630) [SYCL] Removes any knowledge of specific memory advices from PI API. (intel#2607) [BuildBot] Uplift GPU RT version for Linux to 20.40.18075 (intel#2626) [SYCL] Wrap complex global objects to control lifetime (intel#2516) [SYCL][CUDA] Image Basic Test (intel#1970) [SYCL] Align get_info<info::device::version>() with the SYCL spec (intel#2507) [Driver][SYCL] Correct optimization disabling option for gen (intel#2622) [SYCL][LIT] Add deleter func for test in buffer.cpp to avoid potential SegFault (intel#2616) [SYCL] Remove half type alias causing name conflicts (intel#2624) [BuildBot] OpenCL CPU/FPGAEMU driver uplift (intel#2620) [SYCL][Doc] Add overview of kernel-program caching (intel#2514) [SYCL] Remove two-input sub-group shuffles (intel#2614) [SYCL] Add support for new spelling of FPGA kernel attribute scheduler_target_fmax_mhz (intel#2618) [SYCL] Align image class constructors with the SYCL spec (intel#2603) [SYCL] Remove tests migrated to llvm-test-suite (intel#2611) [SYCL][NFC] Fix dependency for SYCL add_sycl_executable macro (intel#2613) [SYCL][PI][L0] Update environment variables from LEVEL0 to LEVEL_ZERO (intel#2612) [SYCL] Add KernelNameTypeVisitor validation check (intel#2596) ...

-This reduces the number of required type casts.- This unifies all the version methods used in translator. Original commit: KhronosGroup/SPIRV-LLVM-Translator@a3d840ec247c5cb

DenisBakhvalov requested a review from a team as a code owner September 21, 2020 17:47

DenisBakhvalov requested a review from dm-vodopyanov September 21, 2020 17:47

DenisBakhvalov force-pushed the private/dbakhval-device-get_info branch from fe4a8d8 to e39575e Compare September 21, 2020 19:24

vladimirlaz suggested changes Sep 22, 2020

View reviewed changes

sycl/test/plugins/sycl-ls-gpu-opencl.cpp Outdated Show resolved Hide resolved

dm-vodopyanov requested changes Sep 22, 2020

View reviewed changes

DenisBakhvalov added 4 commits September 22, 2020 14:42

Fixed review comments

8d4876a

Revert "Fixed review comments"

7c6b856

This reverts commit 8d4876a.

Fixed review comments

db2b321

Run clang-format on sycl/test/basic_tests/parallel_for_range.cpp

1b40b9f

DenisBakhvalov force-pushed the private/dbakhval-device-get_info branch from e9b312a to 1b40b9f Compare September 22, 2020 22:25

DenisBakhvalov added 3 commits September 22, 2020 16:10

Added more tests for get_info<info::device::version>()

da074db

Fixed basic_tests/parallel_for_range.cpp

2da0428

Updated sub_group/broadcast tests

b66ec6e

vladimirlaz suggested changes Sep 23, 2020

View reviewed changes

Fixed review comments

5bd0760

vladimirlaz suggested changes Sep 24, 2020

View reviewed changes

bader changed the title ~~[SYCL] - Align get_info<info::device::version>() with the SYCL spec~~ [SYCL] Align get_info<info::device::version>() with the SYCL spec Sep 24, 2020

bader requested review from vladimirlaz and dm-vodopyanov October 12, 2020 09:35

vladimirlaz approved these changes Oct 12, 2020

View reviewed changes

dm-vodopyanov approved these changes Oct 12, 2020

View reviewed changes

bader merged commit 4644e63 into intel:sycl Oct 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Align get_info<info::device::version>() with the SYCL spec #2507

[SYCL] Align get_info<info::device::version>() with the SYCL spec #2507

DenisBakhvalov commented Sep 21, 2020

dm-vodopyanov left a comment

vladimirlaz Sep 23, 2020

DenisBakhvalov Sep 23, 2020

vladimirlaz Sep 24, 2020

DenisBakhvalov Sep 24, 2020

vladimirlaz commented Sep 24, 2020

DenisBakhvalov commented Sep 24, 2020

vladimirlaz commented Sep 24, 2020

vladimirlaz commented Sep 24, 2020

vladimirlaz Sep 24, 2020

bader commented Sep 24, 2020

DenisBakhvalov commented Sep 28, 2020

dm-vodopyanov commented Sep 29, 2020

DenisBakhvalov commented Oct 2, 2020

bader commented Oct 12, 2020

[SYCL] Align get_info<info::device::version>() with the SYCL spec #2507

[SYCL] Align get_info<info::device::version>() with the SYCL spec #2507

Conversation

DenisBakhvalov commented Sep 21, 2020

dm-vodopyanov left a comment

Choose a reason for hiding this comment

vladimirlaz Sep 23, 2020

Choose a reason for hiding this comment

DenisBakhvalov Sep 23, 2020

Choose a reason for hiding this comment

vladimirlaz Sep 24, 2020

Choose a reason for hiding this comment

DenisBakhvalov Sep 24, 2020

Choose a reason for hiding this comment

vladimirlaz commented Sep 24, 2020

DenisBakhvalov commented Sep 24, 2020

vladimirlaz commented Sep 24, 2020

vladimirlaz commented Sep 24, 2020

vladimirlaz Sep 24, 2020

Choose a reason for hiding this comment

bader commented Sep 24, 2020

DenisBakhvalov commented Sep 28, 2020

dm-vodopyanov commented Sep 29, 2020

DenisBakhvalov commented Oct 2, 2020

bader commented Oct 12, 2020