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

Sycl ext oneapi current device test plan #1033

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

Conversation

dklochkov-emb
Copy link
Contributor

Test plan to ext oneapi device feature. See docs

@dklochkov-emb dklochkov-emb requested review from gmlueck and a team as code owners February 6, 2025 10:08
Copy link
Contributor

@gmlueck gmlueck left a comment

Choose a reason for hiding this comment

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

Some specific comments below. At a higher level, I think there are three things to test:

  1. Call set_current_device(d) then call get_current_device() from the same thread. Make sure it returns d.

  2. Do not call set_current_device. Call get_current_device() and make sure it returns the default device.

  3. Create multiple threads. Each thread calls the following in a loop: set_current_device(d) followed by get_current_device(). Each thread uses a different device d (if the system has more than one device). Make sure the call to get_device returns the same device that was set in set_current_device. This tests that each thread is keeping a separate copy of the "current device".

=== Function `get_current_device`

1. Positive case. This test can be performed by seting default device, calling from a host thread the function sycl::ext::oneapi::experimental::this_thread::get_current_device
and compare function output with the default device which was set earlier, check thread ID
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you mean that the test will do:

set_current_device(dev);
// ...
d = get_current_devic();
assert(d == dev);

Correct?

What do you mean by "check thread ID"?

1. Positive case. This test can be performed by seting default device, calling from a host thread the function sycl::ext::oneapi::experimental::this_thread::get_current_device
and compare function output with the default device which was set earlier, check thread ID

2. Negative case 1. Implement the same as positive but call function from kernel(not host) code. Use XFAIL to detect compilation error
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't usually write tests in the CTS to detect compilation failures for illegal programs. In fact, we should not do this in the CTS because there is no requirement for a SYCL implementation to diagnose a compilation error in this case.


2. Negative case 1. Implement the same as positive but call function from kernel(not host) code. Use XFAIL to detect compilation error

3. Negative case 2. Create async task and call the function sycl::ext::oneapi::experimental::this_thread::get_current_device from async task, Use XFAIL to detect compilation error.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "async task" mean? Is this a SYCL host task?

Assuming you mean host task, we should not test this case. There is no requirement for the compiler to diagnose a compile-time error. In fact, I don't think DPC++ would generate a compilation error in this case because we can't tell at compile time whether the call comes from a host task.

=== Function `set_current_device`

1. Positive case. This test can be performed by checking default device, calling from a host thread the function sycl::ext::oneapi::experimental::this_thread::set_current_device,
getting default device and compare it with the previous value, check thread ID
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I can't follow this description. Is this test different from the one above for get_current_device?

@dklochkov-emb
Copy link
Contributor Author

Some specific comments below. At a higher level, I think there are three things to test:

  1. Call set_current_device(d) then call get_current_device() from the same thread. Make sure it returns d.
  2. Do not call set_current_device. Call get_current_device() and make sure it returns the default device.
  3. Create multiple threads. Each thread calls the following in a loop: set_current_device(d) followed by get_current_device(). Each thread uses a different device d (if the system has more than one device). Make sure the call to get_device returns the same device that was set in set_current_device. This tests that each thread is keeping a separate copy of the "current device".

@gmlueck What do you think about additional test: we should check if one thread sets device and another reads it? I.e. compare value(device name) read from thread 2 and value(device name) set earlier in thread 1.

@gmlueck
Copy link
Contributor

gmlueck commented Feb 7, 2025

What do you think about additional test: we should check if one thread sets device and another reads it? I.e. compare value(device name) read from thread 2 and value(device name) set earlier in thread 1.

This sounds like a wrong test. The spec for set_current_device says:

Sets the current default device to dev for the calling host thread.

Therefore, setting the current device in one thread should not be visible in another thread.


=== Functions `get_current_device` and `set_current_device`

1. Get default device name from environment variable. Call `get_current_device()` but do not call `set_current_device`.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can get the default device just by default-constructing a device object:

void test() {
  sycl::device d;  // d is the default device
}

There is no need to use an environment variable.

2. Call `set_current_device(d)` then call `get_current_device()` from the same thread.
Compare return value of `get_current_device()` with `d`.
3. Create multiple threads. Each thread calls the following in a loop: `set_current_device(d)` followed by `get_current_device()`. Each thread uses a different device `d` (if the system has more than one device).
Make sure the call to get_device returns the same device that was set in `set_current_device(d)`. This tests that each thread is keeping a separate copy of the "current device".
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about this more. A better test would be:

  • Thread 1 does:
    • Call set_current_device(d1)
    • Signal thread 2
    • Wait for thread 2 to signal
    • Call get_current_device() and make sure it returns d1.
  • Thread 2 does:
    • Call set_current_device(d2)
    • Signal thread 1
    • Wait for thread 1 to signal
    • Call get_current_device() and make sure it returns d2.

Here d1 and d2 should be different devices, if the system has more than one device. There is no need to run this in a loop.

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.

2 participants