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

[SYClomatic-test] Add help function for Block Load header #619

Merged
merged 38 commits into from
May 13, 2024

Conversation

abhilash1910
Copy link
Contributor

Add block load test Following PR #1640
oneapi-src/SYCLomatic#1640 (Merge after 1640 is merged)
cc @yihanwg

Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

Needs some more checks as described. Until this is improved, I don't think we can be confident in the PR.

help_function/help_function.xml Outdated Show resolved Hide resolved
help_function/src/onedpl_test_group_load.cpp Show resolved Hide resolved
help_function/src/onedpl_test_group_load.cpp Outdated Show resolved Hide resolved
help_function/src/onedpl_test_group_load.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@mmichel11 mmichel11 left a comment

Choose a reason for hiding this comment

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

A few comments from my side. The most important I think are testing the remaining free functions and verifying the correctness of the data that has been loaded.

help_function/src/onedpl_test_group_load.cpp Show resolved Hide resolved
help_function/src/onedpl_test_group_load.cpp Outdated Show resolved Hide resolved
help_function/src/onedpl_test_group_load.cpp Outdated Show resolved Hide resolved
q.wait_and_throw();

sycl::host_accessor data_accessor(buffer, sycl::read_only);
const int *ptr = data_accessor.get_multi_ptr<sycl::access::decorated::yes>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal from my side, but host_accessor overloads operator[], so we don't really need a pointer.

help_function/src/onedpl_test_group_load.cpp Outdated Show resolved Hide resolved
help_function/src/onedpl_test_group_load.cpp Outdated Show resolved Hide resolved
help_function/src/onedpl_test_group_load.cpp Outdated Show resolved Hide resolved
help_function/help_function.xml Outdated Show resolved Hide resolved
help_function/src/onedpl_test_group_load.cpp Outdated Show resolved Hide resolved
help_function/src/onedpl_test_group_load.cpp Outdated Show resolved Hide resolved
help_function/src/onedpl_test_group_load.cpp Outdated Show resolved Hide resolved
help_function/src/onedpl_test_group_load.cpp Outdated Show resolved Hide resolved
help_function/src/onedpl_test_group_load.cpp Outdated Show resolved Hide resolved
@abhilash1910 abhilash1910 requested a review from danhoeflinger May 7, 2024 13:19
@abhilash1910
Copy link
Contributor Author

Also requesting review from @ShengchenJ @yihanwg . thanks

@@ -144,6 +144,7 @@
<!-- <test testName="onedpl_test_transform" configFile="config/TEMPLATE_help_function_skip_cuda_backend.xml" /> -->
<test testName="onedpl_test_transform_output_iterator" configFile="config/TEMPLATE_help_function_skip_cuda_backend.xml" />
<test testName="onedpl_test_group_sort" configFile="config/TEMPLATE_help_function.xml" />
<test testName="util_group_load_test" configFile="config/TEMPLATE_help_function.xml" />
Copy link
Contributor

Choose a reason for hiding this comment

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

The test name should be onedpl_test_group_load.

@@ -0,0 +1,213 @@
// ====------ util_group_load_test.cpp------------ *- C++ -* ----===//
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep the filename same as other onedpl test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the other similar filenames for sort etc should also change when being migrated to the group_util.hpp file.
This was changed to util_group_load_test post @danhoeflinger 's review.

Copy link
Contributor

@yihanwg yihanwg May 10, 2024

Choose a reason for hiding this comment

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

This shouldn't be done in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not testing oneDPL or oneDPL-like functionality. It is currently placed in the dpcpp extensions header with the intention to move it to a new header which is targeted at sycl compat.

This code tests a helper function called from within a kernel, which is a lower level than the rest of oneDPL or oneDPL compatibility headers.

If we must have the name as onedpl test until the code is moved, that is fine, but it is just more that needs to be changed later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes noted, will update the file paths when writing linking tests for oneapi-src/SYCLomatic#1784 .

Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for all the work on this.

@abhilash1910
Copy link
Contributor Author

Thanks @danhoeflinger @mmichel11 @yihanwg for review .

Copy link
Contributor

@yihanwg yihanwg left a comment

Choose a reason for hiding this comment

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

LGTM

@ShengchenJ ShengchenJ merged commit 9fbcaf8 into oneapi-src:SYCLomatic May 13, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants