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

Simplify new_kernel_name implementation in tests #2128

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

SergeyKopienko
Copy link
Contributor

@SergeyKopienko SergeyKopienko commented Mar 13, 2025

In this PR we simplify new_kernel_name implementation in tests.
The goal - to make generated new Kernel names more short and simple.

Example

Let's take a look to the code (for example):

auto new_policy = make_new_policy<new_kernel_name<Policy, 0>>(exec);
  • before this PR, the type of new_policy is
oneapi::dpl::execution::__dpl::device_policy<TestUtils::unique_kernel_name<oneapi::dpl::execution::__dpl::device_policy<TestUtils::unique_kernel_name<test_binary_search<unsigned long long>,0>>,0>>
  • after this PR, the type of new_policy is
oneapi::dpl::execution::__dpl::device_policy<TestUtils::unique_kernel_name<TestUtils::unique_kernel_name<test_binary_search<unsigned long long>,0>,0>>

@SergeyKopienko SergeyKopienko added enhancement test Test only Change labels Mar 13, 2025
@SergeyKopienko SergeyKopienko added this to the 2022.9.0 milestone Mar 13, 2025
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/simplify_tests_new_kernel_name_impl branch from eae40da to fc30465 Compare March 13, 2025 15:08
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.

Can we take this opportunity to consolidate this to a single instance of this implementation?

@danhoeflinger
Copy link
Contributor

Is there a reason there are 2 nested copies of TestUtils::unique_kernel_name here?
Are they redundant? If we change the number from 0 to 1, in your code, do both copies of the TestUtils::unique_kernel_name change their number? If so, there is some other issue we can fix I think.

@SergeyKopienko
Copy link
Contributor Author

Is there a reason there are 2 nested copies of TestUtils::unique_kernel_name here? Are they redundant? If we change the number from 0 to 1, in your code, do both copies of the TestUtils::unique_kernel_name change their number? If so, there is some other issue we can fix I think.

In any case now new_kernel_name is implemented in this PR exactly as

template <typename Policy>
using __policy_kernel_name = typename ::std::decay_t<Policy>::kernel_name;

@danhoeflinger
Copy link
Contributor

In any case now new_kernel_name is implemented in this PR exactly as

template <typename Policy>
using __policy_kernel_name = typename ::std::decay_t<Policy>::kernel_name;

Yes, but in your "after" result, we still have 2 nested TestUtils::unique_kernel_name. Perhaps that is because of nesting in the test function you are using to generate the "before" and "after", and is required and intentional.

@SergeyKopienko
Copy link
Contributor Author

SergeyKopienko commented Mar 13, 2025

In any case now new_kernel_name is implemented in this PR exactly as

template <typename Policy>
using __policy_kernel_name = typename ::std::decay_t<Policy>::kernel_name;

Yes, but in your "after" result, we still have 2 nested TestUtils::unique_kernel_name. Perhaps that is because of nesting in the test function you are using to generate the "before" and "after", and is required and intentional.

As example I have used the policy creation at

auto new_policy = make_new_policy<new_kernel_name<Policy, 0>>(exec);
:

auto new_policy = make_new_policy<new_kernel_name<Policy, 0>>(exec);

at this point the type of exec is oneapi::dpl::execution::__dpl::device_policy<TestUtils::unique_kernel_name<test_binary_search<unsigned long long>,0>> &
and the type of new_policy is
oneapi::dpl::execution::__dpl::device_policy<TestUtils::unique_kernel_name<TestUtils::unique_kernel_name<test_binary_search<unsigned long long>,0>,0>>

So I think it's the question for test environment staff, not for new_kernel_name :

  • this type name starts from
template <::std::size_t CallNumber = 0>
struct invoke_on_all_hetero_policies::operator()(Op op, Args&&... rest)

@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/simplify_tests_new_kernel_name_impl branch from a4bdeee to 9ea0453 Compare March 19, 2025 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement test Test only Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants