Skip to content

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