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 test plan for sycl_ext_oneapi_composite_device #865

Conversation

AlexeySachkov
Copy link
Contributor

No description provided.

@AlexeySachkov AlexeySachkov requested review from gmlueck and a team as code owners February 5, 2024 18:46
==== Composite devices are not considered as root devices

The test should ensure that no composite devices are returned through the
`platform::get_device()` method for a platform the selected device belongs to.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also check for device::get_devices()?

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 that this is related to #865 (comment) - CTS is currently launches tests for a single selected device and I'm hesitant to move too away from that. device::get_devices() is not limited by a backend or platform and it may pull in some unwanted things from the environment which are unrelated to one device/platform you are trying to test.

Current wording allows to test devices other than the selected one, but it still limits the list to devices which belong to the same platform taken from the selected device.

A good implementation should probably ensure that it supports all the functionality (support here may mean some reasonable fallback and no unexpected crashes or undocumented exceptions) for all backends and platforms which are reported by it, but I don't think that we should expand testing to all the available platforms and backends right now and just for this extension test.

@maarquitos14
Copy link
Contributor

I know https://github.com/gmlueck/llvm/blob/1f1712ed025c76a30ca251c97175685a59bf58bb/sycl/doc/extensions/proposed/sycl_ext_oneapi_composite_device.asciidoc#impact-to-the-oneapi_device_selector section just impacts Intel's implementation, but I wonder if similar mechanisms might exist for other implementations, and if there's a generic way to test this for any given SYCL implementation. To be specific: composite devices should only be returned if all their components are in the list of root devices returned from device::get_devices(), so if there's a way to only select some of the components --like Intel's implementation has with ONEAPI_DEVICE_SELECTOR--, then CTS should maybe test it.

@AlexeySachkov
Copy link
Contributor Author

I know https://github.com/gmlueck/llvm/blob/1f1712ed025c76a30ca251c97175685a59bf58bb/sycl/doc/extensions/proposed/sycl_ext_oneapi_composite_device.asciidoc#impact-to-the-oneapi_device_selector section just impacts Intel's implementation, but I wonder if similar mechanisms might exist for other implementations, and if there's a generic way to test this for any given SYCL implementation. To be specific: composite devices should only be returned if all their components are in the list of root devices returned from device::get_devices(), so if there's a way to only select some of the components --like Intel's implementation has with ONEAPI_DEVICE_SELECTOR--, then CTS should maybe test it.

ONEAPI_DEVICE_SELECTOR isn't really a part of the language, so I would say that interaction of the extension with it should be tested by the implementation, and not by CTS

@bader bader requested a review from keryell February 28, 2024 21:51
Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Interesting.

@bader bader merged commit fd512b7 into KhronosGroup:SYCL-2020 Feb 29, 2024
8 checks passed
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.

5 participants