Skip to content

Conversation

@uditagarwal97
Copy link
Contributor

Call to Platform.get_devices() increments the device count for the adapter (in Adapter->setLastDeviceId()) and if a platform is banned (like OpenCL for AMD), it can cause incorrect device numbering, when used with ONEAPI_DEVICE_SELECTOR.

This PR skips call to Platform.get_devices() if a platform is banned.

@uditagarwal97 uditagarwal97 self-assigned this Jun 10, 2025
@uditagarwal97 uditagarwal97 requested a review from a team as a code owner June 10, 2025 02:18
@uditagarwal97 uditagarwal97 requested review from aelovikov-intel and Copilot and removed request for aelovikov-intel June 10, 2025 02:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR prevents banned platforms from incrementing device counts by skipping the Platform.get_devices() call, addressing incorrect device numbering when using ONEAPI_DEVICE_SELECTOR.

  • Adjusted the end-to-end device numbering test to remove outdated unsupported markers.
  • Wrapped the device query in platform_impl::getAdapterPlatforms with a banned‐platform guard.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
sycl/test-e2e/Regression/device_num.cpp Removed UNSUPPORTED markers for HIP and issue tracking comments.
sycl/source/detail/platform_impl.cpp Initialized HasAnyDevices to false and only called get_devices() when the platform isn’t banned.
Comments suppressed due to low confidence (2)

sycl/test-e2e/Regression/device_num.cpp:1

  • Removing the UNSUPPORTED: any-device-is-hip marker may cause this test to run on HIP platforms where device numbering behavior differs. Consider verifying test behavior on HIP or re-adding an appropriate unsupported directive.
-// UNSUPPORTED: any-device-is-hip

sycl/source/detail/platform_impl.cpp:121

  • [nitpick] The comment refers to incrementing the count for the platform, but the side effect occurs on the adapter (Adapter->setLastDeviceId()). Consider updating for accuracy.
// Platform.get_devices() increments the device count for the platform

Comment on lines +124 to +125
if (!IsBanned)
HasAnyDevices = !Platform.get_devices(info::device_type::all).empty();
Copy link

Copilot AI Jun 10, 2025

Choose a reason for hiding this comment

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

[nitpick] Add braces around this if block to improve readability and prevent mistakes if additional statements are added later.

Suggested change
if (!IsBanned)
HasAnyDevices = !Platform.get_devices(info::device_type::all).empty();
if (!IsBanned) {
HasAnyDevices = !Platform.get_devices(info::device_type::all).empty();
}

Copilot uses AI. Check for mistakes.
@uditagarwal97 uditagarwal97 merged commit f7583ca into sycl Jun 10, 2025
24 checks passed
@uditagarwal97 uditagarwal97 deleted the private/udit/hip_banned branch June 10, 2025 16:29
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.

ONEAPI_DEVICE_SELECTOR=*:0 doesn't work on systems with HIP + OpenCL devices

3 participants