Skip to content

[SYCL] Don't return empty platforms #7923

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

Merged
merged 3 commits into from
Jan 12, 2023

Conversation

cperkinsintel
Copy link
Contributor

collecting empty platforms leads to confusion for things like platform.has(aspect)

@smaslov-intel
Copy link
Contributor

Does spec mandate (or even allow) that platforms with no devices be filtered?
A test that I am adding for UR wants the empty platform be reported: intel/llvm-test-suite#1450

@cperkinsintel cperkinsintel temporarily deployed to aws January 5, 2023 03:07 — with GitHub Actions Inactive
@cperkinsintel cperkinsintel temporarily deployed to aws January 5, 2023 03:38 — with GitHub Actions Inactive
@cperkinsintel cperkinsintel marked this pull request as ready for review January 5, 2023 17:22
@cperkinsintel cperkinsintel requested a review from a team as a code owner January 5, 2023 17:22
@cperkinsintel cperkinsintel temporarily deployed to aws January 5, 2023 18:12 — with GitHub Actions Inactive
@cperkinsintel
Copy link
Contributor Author

/verify with intel/llvm-test-suite#1491

@againull
Copy link
Contributor

againull commented Jan 5, 2023

Probably we should report that empty platform doesn't have any device aspects, because it doesn't have any devices.
Now we report that empty platform has any aspect which seems like a mistake. @smaslov-intel , @cperkinsintel what do you think?
In this case we still can expose empty platforms through get_platforms().

@smaslov-intel
Copy link
Contributor

Probably we should report that empty platform doesn't have any device aspects, because it doesn't have any devices. Now we report that empty platform has any aspect which seems like a mistake. @smaslov-intel , @cperkinsintel what do you think? In this case we still can expose empty platforms through get_platforms().

With this change empty platforms are not reported, so there is no way to query aspects from such.

@bader bader changed the title [SYCL] empty platforms shouldn't be gathered [SYCL] Don't return empty platforms Jan 5, 2023
@cperkinsintel
Copy link
Contributor Author

cperkinsintel commented Jan 5, 2023

Matching update to test suite here: intel/llvm-test-suite#1493

@cperkinsintel cperkinsintel temporarily deployed to aws January 5, 2023 23:22 — with GitHub Actions Inactive
@cperkinsintel cperkinsintel temporarily deployed to aws January 6, 2023 00:40 — with GitHub Actions Inactive
@bader
Copy link
Contributor

bader commented Jan 6, 2023

Matching update to test suite here: intel/llvm-test-suite#1493

Please, test them together.

@cperkinsintel cperkinsintel temporarily deployed to aws January 9, 2023 18:26 — with GitHub Actions Inactive
@cperkinsintel cperkinsintel temporarily deployed to aws January 9, 2023 22:06 — with GitHub Actions Inactive
@cperkinsintel
Copy link
Contributor Author

/verify with intel/llvm-test-suite#1493

@cperkinsintel cperkinsintel temporarily deployed to aws January 12, 2023 08:57 — with GitHub Actions Inactive
@againull againull merged commit 5e8ae21 into intel:sycl Jan 12, 2023
@cperkinsintel cperkinsintel temporarily deployed to aws January 13, 2023 12:24 — with GitHub Actions Inactive
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