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] test load and store headers #680

Open
wants to merge 12 commits into
base: SYCLomatic
Choose a base branch
from

Conversation

abhilash1910
Copy link
Contributor

@abhilash1910 abhilash1910 commented Apr 14, 2024

This is linked to composite load and store tests (PRs oneapi-src/SYCLomatic#1819 , oneapi-src/SYCLomatic#1784) from group_utils.hpp
Another standalone "store-only" test is at : #725 (WIP)
cc @yihanwg @zhimingwang36 @danhoeflinger @mmichel11

@abhilash1910 abhilash1910 requested a review from a team as a code owner April 14, 2024 07:09
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.

Same as 1784

@abhilash1910 abhilash1910 changed the title [SYCLomatic-test] link group_utils headers [SYCLomatic-test] test store headers May 31, 2024
@abhilash1910 abhilash1910 changed the title [SYCLomatic-test] test store headers [SYCLomatic-test] test load and store headers Jun 5, 2024
@abhilash1910
Copy link
Contributor Author

Completed testing with pass results with PR : oneapi-src/SYCLomatic#1819
@danhoeflinger @yihanwg @mmichel11 please review when available

@@ -0,0 +1,199 @@
// ====------ onedpl_test_group_load.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.

Suggested change
// ====------ onedpl_test_group_load.cpp------------ *- C++ -* ----===//
// ====------ util_group_load_store_test.cpp------------ *- C++ -* ----===//


template <dpct::group::load_algorithm T>
bool helper_validation_function(const int *ptr, const char *func_name) {
if constexpr (T == dpct::group::load_algorithm::BLOCK_LOAD_DIRECT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the only difference between verification for "direct" and "striped" load algorithms is the name. This makes sense because we should "unmap" any "map" via the store operation as long as they match.

However, it doesn't make sense to just have a branch and repeat the code. You can branch for the name, and run the same verification, or just embed the info you want in func_name.

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 that makes sense, thanks

return true;
}

bool subgroup_helper_validation_function(const int *ptr, const uint32_t *sg_sz,
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this has the same verification as well (as it should), but it can be consolidated into a single routine.

@mmichel11
Copy link
Contributor

Can we make some of the kernels launch multiple work-groups to test for issues mentioned in the review?

sycl::local_accessor<uint8_t, 1> tacc(sycl::range<1>(temp_storage_size), h);
sycl::accessor data_accessor_read_write(buffer, h, sycl::read_write);
h.parallel_for(
sycl::nd_range<3>(sycl::range<3>(2, 2, 64), sycl::range<3>(1, 1, 64)),
Copy link
Contributor

Choose a reason for hiding this comment

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

All that seems to be different between the single and multi work group cases seems to be the range. Could we just add an nd_range parameter to a single test function and use that in the kernel submission to unify the single work-group / multi work-group functions?

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 will add the change


sycl::host_accessor data_accessor(buffer, sycl::read_write);
const int *ptr = data_accessor.get_multi_ptr<sycl::access::decorated::yes>();
return helper_validation_function(ptr, "test_group_load_store");
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 add if it is the single work-group or multi work-group to the test string for more descriptive debug info?

Copy link
Contributor Author

@abhilash1910 abhilash1910 Jul 1, 2024

Choose a reason for hiding this comment

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

Added, thanks

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.

4 participants