Skip to content

[SYCL] Fix LastDeviceIds assignment for Platform w/o device #5695

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
Mar 10, 2022

Conversation

KseniyaTikhomirova
Copy link
Contributor

@KseniyaTikhomirova KseniyaTikhomirova commented Mar 1, 2022

When platform has no devices LastDeviceIds[] for the platform will stay 0 that will affect device enumeration for the next platforms.
Now we do adjustment if no devices for platform was obtained from backend.

Testing: issue is found/partly covered by E2E test in llvm-test_suite/SYCL/Regression/device_num.cpp
Signed-off-by: Tikhomirova, Kseniya [email protected]

@KseniyaTikhomirova KseniyaTikhomirova requested a review from a team as a code owner March 1, 2022 11:32
@KseniyaTikhomirova
Copy link
Contributor Author

/summary:run

1 similar comment
@KseniyaTikhomirova
Copy link
Contributor Author

/summary:run

@KseniyaTikhomirova
Copy link
Contributor Author

Hi @bso-intel could you please review?

bso-intel
bso-intel previously approved these changes Mar 3, 2022
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

alexbatashev
alexbatashev previously approved these changes Mar 4, 2022
@bader
Copy link
Contributor

bader commented Mar 5, 2022

 Failed Tests (4):
   SYCL :: Basic/accessor/accessor.cpp
   SYCL :: Basic/buffer/buffer_full_copy.cpp
   SYCL :: Basic/partition_supported.cpp
   SYCL :: Basic/queue/queue.cpp

Why these llvm-test-suite tests are failing?

When platform doesn't have device LastDeviceIds[] for the platform will stay 0 that will affect device enumeration for the next platforms.
Now we do adjustment of that if no devices for platform was obtained from backend.

Testing: E2E test in llvm-test_suite/SYCL/Regression/device_num.cpp when Mesa is installed
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
@KseniyaTikhomirova
Copy link
Contributor Author

KseniyaTikhomirova commented Mar 5, 2022

 Failed Tests (4):
   SYCL :: Basic/accessor/accessor.cpp
   SYCL :: Basic/buffer/buffer_full_copy.cpp
   SYCL :: Basic/partition_supported.cpp
   SYCL :: Basic/queue/queue.cpp

Why these llvm-test-suite tests are failing?

Hi @bader unfortunately, it is not clear right now, can not reproduce it locally. These tests were changed 2 days ago and have corresponding RT change, could be that not both changes were picked up for testing.

Did rebase of my commit to verify this assumption.

@bader
Copy link
Contributor

bader commented Mar 5, 2022

Okay. Please, use merge instead of rebase for future PRs.

@KseniyaTikhomirova
Copy link
Contributor Author

Hi @bso-intel , @alexbatashev, your reviews were dismissed after rebase, could you please take a look again?

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

@bader bader merged commit 0272ec2 into intel:sycl Mar 10, 2022
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