[SYCL] Deprecate SYCL 1.2.1 device selectors #6599
Conversation
Signed-off-by: Chris Perkins <[email protected]>
Signed-off-by: Larsen, Steffen <[email protected]>
bader
changed the title
bader
changed the title
| try { | ||
| sycl::default_selector device_selector; | ||
|
|
||
| sycl::range<1> BufSize{1}; | ||
| sycl::buffer<int, 1> Buf(BufSize); | ||
|
|
||
| TestQueue Queue(device_selector); | ||
| TestQueue Queue(sycl::default_selector{}); |
There was a problem hiding this comment.
Hm... I didn't catch: what has changed here?
Didn't you intend to use sycl::default_selector_v?
There was a problem hiding this comment.
At least an intermediate variable was removed so it is simpler.
There was a problem hiding this comment.
Right, but I suppose compiler will emit a warning about using a deprecated API here. Right?
| try { | ||
| sycl::default_selector device_selector; | ||
|
|
||
| sycl::range<1> BufSize{1}; | ||
| sycl::buffer<int, 1> Buf(BufSize); | ||
|
|
||
| TestQueue Queue(device_selector); | ||
| TestQueue Queue(sycl::default_selector{}); |
There was a problem hiding this comment.
At least an intermediate variable was removed so it is simpler.
| @@ -22,7 +22,7 @@ like in example below: | |||
| sycl::queue Queue; | |||
|
|
|||
| int main() { | |||
| Queue = sycl::queue{sycl::default_selector{}.select_device()}; | |||
| Queue = sycl::queue{sycl::default_selector_v}; | |||
There was a problem hiding this comment.
This example might not be best suited here anymore. default_selector_v is a function in our implementation so there is no "race" between it and Queue object created at line 22.
Line 25 is also useless now as the Queue is already initialized to the default device.
There was a problem hiding this comment.
Agree about Line 25, but I think we should leave it. The whole text is about how you get undefined behavior when using global objects because the C++ standard does not specify creation/destruction order, and how the example seems syntactically correct, yet is still undefined behavior. If we remove 25 then the surrounding text isn't really making a point.
There was a problem hiding this comment.
Where is UB here now?
There was a problem hiding this comment.
the queue is created outside main. The "seems syntactically correct" part is declaring the queue outside main but initializing it inside main. If we remove line 25 there is no "seems syntactically correct" part. The example is intended to be intentionally wrong. Read the entire section and you'll see what I mean.
There was a problem hiding this comment.
probably the whole thing should be re-written, but that's outside the scope of this PR. I'm just trying to bring the example up to date, not address its overall point.
There was a problem hiding this comment.
part is declaring the queue outside main but initializing it inside main
I don't think it's true. We do create and define a queue outside main. Then we override the value inside the variable with another one, while the original is being destroyed.
There was a problem hiding this comment.
correct. but to make that point, we need to keep line 25. This isn't real code, this is documentation talking about global object usage.
There was a problem hiding this comment.
Still, where is UB?
There was a problem hiding this comment.
First sentence. And first sentence after the code snippet.
Signed-off-by: Chris Perkins <[email protected]>
|
ping to reviewers. Addressed the latest feedback. This PR is getting old now and I'd like to get it approved and merged before it is forgotten about entirely. |
Now that we have added the SYCL 2020 callable device selectors, we need to prepare for the removal of the older SYCL 1.2.1
device_selectorclass. The first step is to add the deprecation message to the 1.2.1 style device selectors, which this PR does. It also removes the usage of those from our own codebase so as to not trip on our own messages in the future.