Skip to content

[SYCL] Addition of get_range() method #1050

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 5, 2020
Merged

[SYCL] Addition of get_range() method #1050

merged 2 commits into from
Feb 5, 2020

Conversation

garimagu
Copy link
Contributor

@garimagu garimagu commented Jan 23, 2020 •
edited by bader
Loading

The change in the spec @
KhronosGroup/SYCL-Docs#31
has added the following functional changes to accessors:
Image Accessors: Removed get_size() method and added get_range() method.
Local Accessors: Added get_range() method.

Fixes #1023.

Signed-off-by: Garima Gupta [email protected]

@garimagu garimagu mentioned this pull request Jan 23, 2020
Closed
@garimagu garimagu requested review from romanovvlad and bader January 23, 2020 18:44
@garimagu garimagu force-pushed the private/garimagu/image_getrange branch from 4a422b8 to 4fe72b3 Compare January 23, 2020 19:03
romanovvlad
romanovvlad reviewed Jan 27, 2020
View reviewed changes
Copy link
Contributor

@romanovvlad romanovvlad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

bader
bader reviewed Jan 27, 2020
View reviewed changes
sycl/include/CL/sycl/accessor.hpp Outdated
@@ -1092,6 +1122,11 @@ class accessor<DataT, Dimensions, AccessMode, access::target::local,

size_t get_count() const { return getSize().size(); }

template <int Dims = Dimensions, typename = detail::enable_if_t<(Dims > 0)>>
range<Dimensions> get_range() const {
return getSize();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@garimagu, please, make sure that this PR successfully passes latest version of CTS.

getSize() return type is range<AdjustedDim>.

Line 828 of http://ci.llvm.intel.com:8010/#/builders/29/builds/144/steps/18/logs/stdio.

[ 80%] Building CXX object tests/accessor/CMakeFiles/test_accessor_objects.dir/accessor_api_local_fp16.cpp.o
In file included from /localdisk2/sycl_ci/buildbot/worker/Regression_CTS_CPU/SYCL-CTS/tests/accessor/accessor_api_local.cpp:9:
In file included from /localdisk2/sycl_ci/buildbot/worker/Regression_CTS_CPU/SYCL-CTS/tests/accessor/../common/common.h:13:
In file included from /localdisk2/sycl_ci/buildbot/worker/Regression_CTS_CPU/SYCL-CTS/tests/accessor/../common/../../util/../tests/common/sycl.h:21:
In file included from /localdisk2/sycl_ci/buildbot/worker/Regression_CTS_CPU/llvm.obj/lib/clang/10.0.0/include/CL/sycl.hpp:11:
/localdisk2/sycl_ci/buildbot/worker/Regression_CTS_CPU/llvm.obj/lib/clang/10.0.0/include/CL/sycl/accessor.hpp:1127:12: error: no viable conversion from returned value of type 'const range<3>' to function return type 'range<1 aka 1>'
    return getSize();
           ^~~~~~~~~
/localdisk2/sycl_ci/buildbot/worker/Regression_CTS_CPU/SYCL-CTS/tests/accessor/accessor_api_local_common.h:76:30: note: in instantiation of function template specialization 'cl::sycl::accessor<int, 1, cl::sycl::access::mode::read_write, cl::sycl::access::target::local, cl::sycl::access::placeholder::false_t>::get_range<1, void>' requested here
    auto accessorRange = acc.get_range();
                             ^
/localdisk2/sycl_ci/buildbot/worker/Regression_CTS_CPU/SYCL-CTS/tests/accessor/accessor_api_local_common.h:112:7: note: in instantiation of member function '(anonymous namespace)::check_local_accessor_api_methods<int, 1>::check_get_range' requested here
      check_get_range(log, acc, range, is_zero_dim<dims>{});
      ^
/localdisk2/sycl_ci/buildbot/worker/Regression_CTS_CPU/SYCL-CTS/tests/accessor/accessor_api_local_common.h:209:3: note: in instantiation of member function '(anonymous namespace)::check_local_accessor_api_methods<int, 1>::operator()' requested here
  check_local_accessor_api_methods<T, dims>{count, size}(log, queue, range);
  ^
/localdisk2/sycl_ci/buildbot/worker/Regression_CTS_CPU/SYCL-CTS/tests/accessor/accessor_api_local_common.h:234:5: note: in instantiation of function template specialization '(anonymous namespace)::check_local_accessor_api_dim<int, 1>' requested here
    check_local_accessor_api_dim<T, 1>(log, count, size, queue, range1d);
    ^
/localdisk2/sycl_ci/buildbot/worker/Regression_CTS_CPU/SYCL-CTS/tests/accessor/accessor_api_local.cpp:43:43: note: in instantiation of member function '(anonymous namespace)::check_local_accessor_api_type<int>::operator()' requested here
      check_local_accessor_api_type<int>()(log, queue);
                                          ^
/localdisk2/sycl_ci/buildbot/worker/Regression_CTS_CPU/llvm.obj/lib/clang/10.0.0/include/CL/sycl/range.hpp:27:3: note: candidate constructor [with N = 1] not viable: no known conversion from 'const sycl::range<3>' to 'typename std::enable_if<(1 == 1), size_t>::type' (aka 'unsigned long') for 1st argument
  range(typename std::enable_if<(N == 1), size_t>::type dim0) : base(dim0) {}
  ^
/localdisk2/sycl_ci/buildbot/worker/Regression_CTS_CPU/llvm.obj/lib/clang/10.0.0/include/CL/sycl/range.hpp:58:3: note: candidate constructor not viable: no known conversion from 'const sycl::range<3>' to 'const range<1> &' for 1st argument
  range(const range<dimensions> &rhs) = default;
  ^
/localdisk2/sycl_ci/buildbot/worker/Regression_CTS_CPU/llvm.obj/lib/clang/10.0.0/include/CL/sycl/range.hpp:59:3: note: candidate constructor not viable: no known conversion from 'const sycl::range<3>' to 'range<1> &&' for 1st argument
  range(range<dimensions> &&rhs) = default;
  ^
/localdisk2/sycl_ci/buildbot/worker/Regression_CTS_CPU/llvm.obj/lib/clang/10.0.0/include/CL/sycl/detail/array.hpp:59:3: note: candidate function
  operator cl::sycl::range<dimensions>() const {
  ^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the error.
Sorry about missing to check the output. I somehow mentally thought that passing buildbot was good in my case. But it should have rather shown an error as we had discussed earlier.

@garimagu
Copy link
Contributor Author

@bader : From the traces at
http://ci.llvm.intel.com:8010/#/builders/29/builds/177/steps/18/logs/stdio : Line number 830
http://ci.llvm.intel.com:8010/#/builders/35/builds/176/steps/18/logs/stdio : Line number 830
It seems like they are still accessing get_size() method for image_accessors. But from the update in https://github.com/KhronosGroup/SYCL-Docs/pull/31/files ; the get_size() method is deleted.
Is the CTS not updated well?

@bader
Copy link
Contributor

bader commented Jan 28, 2020

Indeed. I still see get_size usages in CTS (e.g. https://github.com/KhronosGroup/SYCL-CTS/blob/SYCL-1.2.1/master/tests/accessor/accessor_constructors_image_utility.h#L150).
It looks like KhronosGroup/SYCL-CTS#35 haven't clean all test cases.
Could you try to fix the accessor test by updating the member function name, please?
Once we have a patch passing verification, we can contribute it back to Khronos SYCL-CTS.

@garimagu
Copy link
Contributor Author

garimagu commented Jan 28, 2020 •
edited
Loading

Indeed. I still see get_size usages in CTS (e.g. https://github.com/KhronosGroup/SYCL-CTS/blob/SYCL-1.2.1/master/tests/accessor/accessor_constructors_image_utility.h#L150).
It looks like KhronosGroup/SYCL-CTS#35 haven't clean all test cases.
Could you try to fix the accessor test by updating the member function name, please?
Once we have a patch passing verification, we can contribute it back to Khronos SYCL-CTS.

I can do that. It may take me a little more time since I haven't done changes to CTS yet.

@garimagu
[SYCL] Addition of get_range() method
6ccaa83 
The change in the spec @
 KhronosGroup/SYCL-Docs#31
has added the following functional changes to accessors:
Image Accessors: Removed get_size() method and added get_range() method.
Local Accessors: Added get_range() method.

Signed-off-by: Garima Gupta <[email protected]>
@garimagu
Correction of error in get_range API for local_accessors.
5417473 
Also optimized other changes done in previous patch to use the
converttoArrayofN(Dims, Default_value) method.

Signed-off-by: Garima Gupta <[email protected]>
@garimagu garimagu force-pushed the private/garimagu/image_getrange branch from 6176d54 to 5417473 Compare February 4, 2020 18:54
@garimagu
Copy link
Contributor Author

garimagu commented Feb 4, 2020 •
edited
Loading

http://ci.llvm.intel.com:8010/#/builders/29/builds/286/steps/18/logs/stdio : Line # 1444 shows that accessor test is passing on opencl CPU.
Same can be seen in http://ci.llvm.intel.com:8010/#/builders/24/builds/283/steps/18/logs/stdio : Line # 1222 for FPGA.

@bader bader merged commit 8ed5566 into intel:sycl Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romanovvlad romanovvlad romanovvlad left review comments

@bader bader bader approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Latest Khronos SYCL CTS build is broken
3 participants
@garimagu @bader @romanovvlad