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

Conversation

0x12CC
Copy link
Contributor

@0x12CC 0x12CC commented Oct 7, 2024

No description provided.

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.

I think it does not make sense to create CTS test for the "sycl_ext_oneapi_default_context" extension at this point. We have recently written a KHR extension for this same feature, so it makes more sense to create tests for that KHR.

In fact, we split the functionality into two parts:

I think these tests are all in line with what you have in #960, so I think most of that work can be reused. There will just be a few changes to the API names and a bit of restructuring.


=== 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?

@0x12CC
Copy link
Contributor Author

0x12CC commented Oct 7, 2024

I think these tests are all in line with what you have in #960, so I think most of that work can be reused. There will just be a few changes to the API names and a bit of restructuring.

Thanks for taking a look at this! Is your preference that I close this PR and just update #960 to include the changes you're describing?

@gmlueck
Copy link
Contributor

gmlueck commented Oct 7, 2024

I think these tests are all in line with what you have in #960, so I think most of that work can be reused. There will just be a few changes to the API names and a bit of restructuring.

Thanks for taking a look at this! Is your preference that I close this PR and just update #960 to include the changes you're describing?

I think this would be fine. We already have the test plans outlined in #943 and #944.

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.

3 participants