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

Add a test plan for the sycl_ext_oneapi_default_context extension #959

Closed
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 84 additions & 0 deletions test_plans/oneapi/default_context.asciidoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
:sectnums:
:xrefstyle: short

= Test plan for sycl_ext_oneapi_default_context

This is a test plan for the default SYCL context extension defined in
https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/supported/sycl_ext_oneapi_default_context.asciidoc[sycl_ext_oneapi_default_context.asciidoc].

== Testing scope

=== Device coverage

All of the tests described below are performed only on the default device that
is selected on the CTS command line.

=== Feature test macro

All of the tests should use `#ifdef SYCL_EXT_ONEAPI_DEFAULT_CONTEXT` so they can
be skipped if feature is not supported.

== Tests
The extension defines a default context that can be retrieved using the following new method for the platform class:

[source,c++]
----
context ext_oneapi_get_default_context()
----

It also updates the `queue` class to use this context instead of creating a new one.

=== Default Context Devices Test

The default context should contain all of a platform's device. The test should create a platform and get its default context. The test should then check that the context has all of the devices from the platform.

=== Queue Constructor Test

When constructing a `queue` without specifying a `context`, the `queue` should use the default context. The test should create a new `context` using the default constructor and retrieve the platform's default context. The test should then do the following:
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a hard time understanding this sentence:

The test should create a new context using the default constructor and retrieve the platform's default context.

Which default constructor are we talking about? The context default constructor? This is probably not a good choice. Reading the specification for the context default constructor, I think it would be valid for the implementation to construct a context that is a copy of the default context for the platform that contains the default device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which default constructor are we talking about? The context default constructor?

Yes.

Reading the specification for the context default constructor, I think it would be valid for the implementation to construct a context that is a copy of the default context for the platform that contains the default device.

I didn't realize that this was valid. I think I can change this test to construct a new context using only the default device or I can remove the check that the new context is not equal to the default context.

It seems confusing that a default-constructed context may or may not be the platform's default context. Is there any plan to clarify this in the SYCL specification or is this intentionally implementation-defined behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't realize that this was valid. I think I can change this test to construct a new context using only the default device or I can remove the check that the new context is not equal to the default context.

Actually, I take this back. I think it would not be valid for the default-constructed context to be the same object as the platform's default context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please clarify why it wouldn't be valid for a default-constructed context to be the same object as the platform's default context?

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 it comes from this text in Section 4.5.2. Common reference semantics:

T must be equality comparable in the host application. Equality between two instances of T (i.e. a == b) must be true if one instance is a copy of the other and non-equality between two instances of T (i.e. a != b) must be true if neither instance is a copy of the other

(Emphasis mine.)

In the case we're talking about, the application creates a context with the context's default constructor. This is not the copy constructor, so it is not making a copy. Therefore, the spec statement above requires this context object to compare unequal with other context objects.

Copy link
Member

Choose a reason for hiding this comment

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

I am puzzled. This is probably to be clarified in the spec (or in my brain if the spec is clear 😄).
Creating a default queue gives a different queue every time because this is the goal to express concurrency.
Creating a default device will probably give the same device every time, specially when there is only 1 device.
So what about the context? Is there a value to have a different context every time? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a topic to discuss in the WG. My comment above merely describes the current state of the specification. If the WG wants to allow the context constructors to return copies of existing context objects, I think they need to change the specification.

I suppose there might be an advantage to constructing separate context objects. We use context as a sort of container for USM memory allocations. USM memory allocated from context A can only be accessed from a kernel submitted to a queue that also uses context A. If an application has USM memory buffer B1 that is only used in kernel K1 and USM memory buffer B2 that is only used in kernel K2, there might be an advantage to using two different contexts. Perhaps the implementation could perform some sort of optimization when it sees the separate contexts?


1. Check that the new `context` differs from the default context.
2. Check that a `queue` created without specifying a `context` uses the default context. This check should include the following `queue` constructors:
+
[source,c++]
----
explicit queue(const property_list& propList = {});`

explicit queue(const async_handler& asyncHandler,
const property_list& propList = {});`

template <typename DeviceSelector>
explicit queue(const DeviceSelector& deviceSelector,
const property_list& propList = {});

template <typename DeviceSelector>
explicit queue(const DeviceSelector& deviceSelector,
const async_handler& asyncHandler,
const property_list& propList = {});

explicit queue(const device& syclDevice, const property_list& propList = {});

explicit queue(const device& syclDevice, const async_handler& asyncHandler,
const property_list& propList = {});
----
3. Check that a `queue` created with the new `context` uses that `context` and not the default context. This check should include the following `queue` constructors:
+
[source,c++]
----
template <typename DeviceSelector>
explicit queue(const context& syclContext,
const DeviceSelector& deviceSelector,
const property_list& propList = {});

template <typename DeviceSelector>
explicit queue(const context& syclContext,
const DeviceSelector& deviceSelector,
const async_handler& asyncHandler,
const property_list& propList = {});

explicit queue(const context& syclContext, const device& syclDevice,
const property_list& propList = {});

explicit queue(const context& syclContext, const device& syclDevice,
const async_handler& asyncHandler,
const property_list& propList = {});
----
Loading