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 2020] Add new device selector API #531

Merged
merged 1 commit into from
Apr 21, 2021

Conversation

illuhad
Copy link
Collaborator

@illuhad illuhad commented Apr 20, 2021

This adds the new device selector API from SYCL 2020

@illuhad illuhad merged commit ae93ae9 into develop Apr 21, 2021
@illuhad illuhad deleted the feature/sycl-2020/add-new-device-selector-api branch April 21, 2021 14:36
@psalz
Copy link
Member

psalz commented May 11, 2021

While the 2020 spec (curiously!) doesn't say how the old-style selectors should be implemented, I think the fact that they exist for backwards compatibility is a strong indicator that they should be implemented the same way as described in the 1.2.1 spec -- otherwise, what's the point? One could debate whether that includes the fact that e.g. gpu_selector must be derived from device_selector, but in practice I doubt it's going to matter to anyone. However I just ran into the problem that because it is currently no longer derived from device_selector, I cannot do this:

sycl::gpu_selector selector;
auto device = selector.select_device();

Is this intentional, or a bug?

@illuhad
Copy link
Collaborator Author

illuhad commented May 11, 2021

As you say the spec is very vague with respect to what is supposed to work with the old-style selectors. The reason they are no longer derived from device_selector is that then gpu_selector_v etc. could no longer be implemented as an inline constexpr instance of gpu_selector, since it would contain virtual functions.

You could get the same semantics as your example code by just passing your selector object into the device constructor.

@psalz
Copy link
Member

psalz commented May 11, 2021

Sure the problem is not that hard to fix, but it's always jarring to have an existing program suddenly no longer compile. It's an interesting question though. Given this vagueness, should the CTS test for select_device?

The reason they are no longer derived from device_selector is that then gpu_selector_v etc. could no longer be implemented as an inline constexpr instance of gpu_selector, since it would contain virtual functions.

Okay, but why can't it be inline const?

@illuhad
Copy link
Collaborator Author

illuhad commented May 11, 2021

Given this vagueness, should the CTS test for select_device?

IMO given the current spec the CTS should not test for select_device as there's no requirement in the spec to support it. If this is not intentional, the spec would have to be changed first.

Okay, but why can't it be inline const?

I think it probably could, but not making something constexpr when we could just because of some (as of now) vague and ill-defined requirements for backwards compatibility does not seem very plausible to me.

@gogo2
Copy link
Contributor

gogo2 commented May 20, 2021

I also have a comment on this. First of all this change stopped my software from compiling without any changes made to it. And that is okay as we are moving to the new standard.
But I worked in an environment with possibility to switch SYCL compiler between hipSYCL/oneapi/computecpp
and I even do some tests with Xilinx FPGA on trisycl. Now I can't do that without some macro spaghetti as they apparently does not support SYCL 2020 selectors yet (or I am missing something, sorry if I do).

EDIT Ok, I managed to overcome this in an appealing way.

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