Skip to content
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

[onedpl][ranges][test] Added std ranges to oneDPL Tested API #1571

Merged
merged 19 commits into from
Dec 17, 2024

Conversation

MikeDvorskiy
Copy link
Contributor

@MikeDvorskiy MikeDvorskiy commented May 8, 2024

[onedpl][ranges][test] Added std ranges to oneDPL Tested API (for parallel oneDPL algorithms):

C++ standard sized RA views (factories and adaptors), released with C++20:

  1. views::all
  2. subrange
  3. single_view
  4. iota_view
  5. take_view
  6. drop_view
  7. reverse_view
  8. transform_view

Copy link
Contributor

@akukanov akukanov left a comment

Choose a reason for hiding this comment

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

For other tested APIs we preferred to adjust libc++ tests, Have you looked at https://github.com/llvm/llvm-project/tree/main/libcxx/test/std/ranges? Does it make sense to use these for testing, and if not - why?

@@ -117,6 +117,8 @@
#endif
#endif //!defined(_ENABLE_RANGES_TESTING)

#define _ENABLE_STD_RANGES_TESTING (_ONEDPL___cplusplus >= 202002L)
Copy link
Contributor

Choose a reason for hiding this comment

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

_ONEDPL___cplusplus is an implementation detail. Please use __cplusplus and _MSVC_LANG here. See also #1568 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probbaly, does it make sense to use the following definition in onedpl_config.h ??
#define _ONEDPL_CPP20_RANGES_PRESENT (__cplusplus >= 202002L || _MSVC_LANG >= 202002L) && __cpp_lib_ranges >= 201911L
?
(https://en.cppreference.com/w/cpp/feature_test)

@MikeDvorskiy
Copy link
Contributor Author

For other tested APIs we preferred to adjust libc++ tests, Have you looked at https://github.com/llvm/llvm-project/tree/main/libcxx/test/std/ranges? Does it make sense to use these for testing, and if not - why?

No, I have not looked at ".../libcxx/test/std/ranges" before...
I've done it right now....
The first concern, that I have - a std test contains many RT assserts in a test body... It doesnt work in a kernel, as far as I understand.
As variant we can "invent" some macro which "converts" each assert condition to "b_res &&= condition" and write RT assert after kernel on the host side with global variable "b_res" check.
The second concern - there are many files on each std range. Each file checks just one part of functionality of the range. As far as we have some restrictions on std views/ranges (like sized RA views), probably we we need just the test files which test juts "sized RA views/ranges"

@MikeDvorskiy
Copy link
Contributor Author

RT asserts
Update: it seems that works, it leads to std::abort call... (https://intel.github.io/llvm-docs/design/Assert.html)

@MikeDvorskiy MikeDvorskiy marked this pull request as draft May 14, 2024 12:20
@MikeDvorskiy MikeDvorskiy marked this pull request as draft May 14, 2024 12:20
@MikeDvorskiy
Copy link
Contributor Author

The second concern - there are many files on each std range. Each file checks just one part of functionality of the range.

as an example, I have adapted a test for "all_view" (as is with minimal changes).
https://github.com/oneapi-src/oneDPL/pull/1571/files#diff-e827a6e11d65e046ef5e9e92d7c4302e71a32b3ac4557dd16f8a423762221701

Ok, for "all_view" there is one (two but we can take juts one) test file (from LLVM).
BUT, for the others views LLVM has about 10 test files for each testing view! Each file checks juts one method. I am not sure that we want to have so bulk tests for our needs in oneDPL.
In my opinion, checking the base functionality in 1-3 lines (see below) in one lambda is enough for oneDPL.

auto test = [](){
        auto res = std::ranges::views::iota(0, 4);
        return res.size() == 4 && res[0] == 0 && res[1] == 1 && res[2] == 2 && res[3] == 3 
                  && *(res.begin() + 2) == 2 && (res.end() - res.begin()) == 4;
    };

@SergeyKopienko
Copy link
Contributor

The second concern - there are many files on each std range. Each file checks just one part of functionality of the range.

as an example, I have adapted a test for "all_view" (as is with minimal changes). https://github.com/oneapi-src/oneDPL/pull/1571/files#diff-e827a6e11d65e046ef5e9e92d7c4302e71a32b3ac4557dd16f8a423762221701

Ok, for "all_view" there is one (two but we can take juts one) test file (from LLVM). BUT, for the others views LLVM has about 10 test files for each testing view! Each file checks juts one method. I am not sure that we want to have so bulk tests for our needs in oneDPL. In my opinion, checking the base functionality in 1-3 lines (see below) in one lambda is enough for oneDPL.

auto test = [](){
        auto res = std::ranges::views::iota(0, 4);
        return res.size() == 4 && res[0] == 0 && res[1] == 1 && res[2] == 2 && res[3] == 3 
                  && *(res.begin() + 2) == 2 && (res.end() - res.begin()) == 4;
    };

From point of view of other our test implementations, it's good way to have one test for one function.
We always able to se exactly what was broken.

@MikeDvorskiy MikeDvorskiy marked this pull request as ready for review May 22, 2024 12:51
@MikeDvorskiy MikeDvorskiy requested a review from rarutyun June 1, 2024 08:38
@akukanov akukanov added this to the 2022.7.0 milestone Aug 19, 2024
@danhoeflinger danhoeflinger added the test Test only Change label Sep 11, 2024
@MikeDvorskiy MikeDvorskiy force-pushed the dev/mdvorski/xpu_test_ranges branch from 2f60b40 to 905ca41 Compare September 11, 2024 16:09
@akukanov akukanov removed this from the 2022.7.0 milestone Sep 16, 2024
Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev left a comment

Choose a reason for hiding this comment

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

Overall, the PR looks good, but I've left some comments regarding the added test.

The PR covers only a portion of the c++20 ranges functionality and does not cover free range functions like ranges::size() or more advanced adaptors like join_view. Do you plan adding it in consequent PRs?

Functions such as std::ranges::size() and more adaptors such as ranges::join_view?

test/xpu_api/ranges/xpu_std_ranges_test.h Show resolved Hide resolved
test/xpu_api/ranges/xpu_std_iota_view.pass.cpp Outdated Show resolved Hide resolved
test/xpu_api/ranges/xpu_std_drop_view.pass.cpp Outdated Show resolved Hide resolved
test/xpu_api/ranges/xpu_std_single_view.pass.cpp Outdated Show resolved Hide resolved
Comment on lines 29 to 34
auto test = [](){
auto v = std::ranges::views::iota(0, 4);
auto res = std::ranges::subrange(v.begin() + 1, v.end());
return res.size() == 3 && res[0] == 0 && res[1] == 1 && res[2] == 2 &&
(*res.begin() + 2) == 2 && res.end() - res.begin() == 3;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also check prev(), next() and advance()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

due to within a kernel any range/view must be a random access view, testing of advance doesn't have much sense, I guess...

Copy link
Contributor

Choose a reason for hiding this comment

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

It must work, practically (I do not expect an implementer to rely on recursion here, for example). I would check it anyway as it is easy, however I would not insist if you do not do it.

@MikeDvorskiy MikeDvorskiy force-pushed the dev/mdvorski/xpu_test_ranges branch from ffab2b2 to f74c6dd Compare December 16, 2024 12:56
Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@SergeyKopienko SergeyKopienko left a comment

Choose a reason for hiding this comment

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

LGTM

@MikeDvorskiy MikeDvorskiy merged commit eb2471a into main Dec 17, 2024
22 checks passed
@MikeDvorskiy MikeDvorskiy deleted the dev/mdvorski/xpu_test_ranges branch December 17, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for test test Test only Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants