Add a test plan for the sycl_ext_oneapi_default_context extension#959
Add a test plan for the sycl_ext_oneapi_default_context extension#9590x12CC wants to merge 1 commit intoKhronosGroup:SYCL-2020from
sycl_ext_oneapi_default_context extension#959Conversation
Signed-off-by: Michael Aziz <michael.aziz@intel.com>
gmlueck
left a comment
gmlueck
left a comment
There was a problem hiding this comment.
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:
-
KhronosGroup/SYCL-Docs#623 changes the core SYCL specification to say that all the
queueconstructors that don't take an explicit context use the platform's default context. We should add CTS tests for this behavior that are just part of the core test suite, not tied to any extension. I created a brief test plan in #943, which we can use as a starting point. -
KhronosGroup/SYCL-Docs#624 adds a KHR extension to retrieve the platform's default context. We should add CTS tests for this behavior that are tied to the KHR. I created a brief test plan in #944, which we can use as a starting point.
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: |
There was a problem hiding this comment.
I had a hard time understanding this sentence:
The test should create a new
contextusing 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
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?
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. |
…ce_num Set timelimit for device_selector_aspect based on devices available
3 participants
No description provided.