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] Provide extension to query for unsupported platforms #15368

Closed

Conversation

jchlanda
Copy link
Contributor

Avoid extending a public facing interface of sycl::platform by wrapping a call to getting unsupported platforms in an oneAPI extension.

@jchlanda
Copy link
Contributor Author

cc: @steffenlarsen this should address your comment, thank you :)

@jchlanda
Copy link
Contributor Author

This is a follow up to: #15166 addressing the pubic interface change.

Copy link
Contributor

@bso-intel bso-intel left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 58 to 59
available SYCL platforms in the system (the resulting vector always contains a
single SYCL host platform instance). This extension adds an API entry to list
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Is "host" a supported platform?
I thought it was deprecated long ago.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was, that's a comment that I've lifted from source code without thinking too much about it. I'll rephrase it and remove the original one as well.

Done in 413dd39

@@ -0,0 +1,123 @@
= sycl_ext_oneapi_get_unsupported_platforms
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a general comment here so it can be threaded.

I think the goal is to allow sycl-ls to list platforms that could potentially be supported by DPC++ even if those platforms are not actually available on the current system. Is that right? How would we decide what platforms are potentially supported? Will there be a hard-coded list someplace? For example, will we list CUDA as a potential platform even if there is no Nvidia card installed on the system?

I'm also curious if there is user request for this feature.

If we do want to support this feature, I think this extension is not the right way. The Khronos WG discussed this a while ago and agreed to a different direction. Some WG members wanted to expose "potential" platforms from their implementations, so the WG clarified the SYCL specification to allow a platform object to have no devices. An implementation can use this to expose platforms that could potentially be supported, even if there is no support in the current installation.

Intel was weakly opposed because we did not see a reason to do this. However, we did not feel strongly because we did not expect DPC++ to make use of this feature. There is no requirement in the spec to expose empty platforms like this, so we did not expect DPC++ to do so.

If DPC++ does want to expose "potential" platforms, it should do so by exposing empty platform objects rather than adding a new oneAPI extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the goal is to allow sycl-ls to list platforms that could potentially be supported by DPC++ even if those platforms are not actually available on the current system. Is that right? How would we decide what platforms are potentially supported? Will there be a hard-coded list someplace? For example, will we list CUDA as a potential platform even if there is no Nvidia card installed on the system?

I guess I owe a bit of an explanation here; the goal of this API is not to introduce logic to decide which platforms to mark as unsupported, this is already handled in IsBannedPlatform helper, the sole purpose of the new entry is to provide a way of communicating which platforms have been banned.

I'm also curious if there is user request for this feature.

I suppose it's a quality of life kind of a thing more than a strict user requirement. FWIW it came my way through a discussion here.

I'm not sure if the ability to list banned platforms is worth going through the motions of WG and discussions with other vendors, I'm OK with closing this PR (and removing the entry that required this extension in the first place).

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I owe a bit of an explanation here; the goal of this API is not to introduce logic to decide which platforms to mark as unsupported, this is already handled in IsBannedPlatform helper, the sole purpose of the new entry is to provide a way of communicating which platforms have been banned.

The comment there indicates that the UR intentionally does not expose certain OpenCL platforms because they don't work with DPC++. Does this feature expose them anyway via a new API? That does not make sense to me.

If there is no user request for this feature, I think we should not add any SYCL API that exposes platforms for OpenCL installations that are known to not work. If this means they are not displayed in sycl-ls, I think that is OK.

If we really want to list these in sycl-ls (or some other tool), I'd rather expose some API via the UR and call that API directly. However, it's not clear to me why sycl-ls should show this information. It's also not clear to me how we would decide which "banned" things to list here. For example, some implementations of SYCL layer on top of OpenMP as a backend. Should sycl-ls list all OpenMP installations as "banned"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment there indicates that the UR intentionally does not expose certain OpenCL platforms because they don't work with DPC++. Does this feature expose them anyway via a new API? That does not make sense to me.

I never though about the "exposing" aspect of this API; if the question is: can this API provide us with an instance of sycl::platform that otherwise would not be accessible and is not valid, than the answer is yes. I am not in a position to judge if this is acceptable or not, fwiw, the entry clearly states that the returned platforms are unsupported (perhaps the wording in the doc could be stronger).

If we really want to list these in sycl-ls (or some other tool), I'd rather expose some API via the UR and call that API directly. It's also not clear to me how we would decide which "banned" things to list here.

I'm not sure if moving this functionality to UR would work, the logic of which platforms are marked as banned is currently handled by the platform (IsBannedPlatform) and that seems to be well placed.

However, it's not clear to me why sycl-ls should show this information.

I could imagine it being useful from the infrastructure perspective, seeing what the system has installed, for instance, on my machine, along 4 supported platforms, I have an unsupported OpenCL 3.0 from cuda:

Unsupported Platforms: 1
Platform [#1]:
    Version  : OpenCL 3.0 CUDA 12.5.85
    Name     : NVIDIA CUDA
    Vendor   : NVIDIA Corporation
    Devices  : 1
        Device [#0]:
        Type              : gpu
        Version           : OpenCL 3.0 CUDA
        Name              : NVIDIA GeForce GTX 1650
        Vendor            : NVIDIA Corporation
        Driver            : 555.58.02
        UUID              : 2073250284612179411136216771208211227
        Num SubDevices    : 0
        Num SubSubDevices : 0

As this has always been a developer request, I'm happy to follow your advice and close it, if adding an extension is not suitable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the idea of adding an API that returns a platform object that is not supported. Since there is no user request for this feature, let's close this PR and also remove get_unsupported_platforms that was added in #15166.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done in: #15415

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.

4 participants