[SYCL] Don't select devices with no available images #6203

npmiller · 2022-05-26T16:52:24Z

This patch solves:

[SYCL] Default selector should filter devices based on available device images #2004

And is related to:

[SYCL][CUDA] Default selector behaviour #1665

In some cases the current selector may select a device for which we don't have an AOT or SPIR-V binary for, this patch ensures that such devices get skipped.

cperkinsintel · 2022-05-26T19:46:05Z

sycl/source/detail/program_manager/program_manager.cpp

@@ -1450,6 +1450,26 @@ kernel_id ProgramManager::getSYCLKernelID(const std::string &KernelName) {
  return KernelID->second;
 }

+bool ProgramManager::hasCompatibleImage(const device &Dev) {


maybe instead of running two loops and creating a temporary set, would it be possible to use std::find or std::any_of on the DeviceImages collection ? That might be easier to read and quicker.

Updated with a slightly easier to use map and use of std::any_of

npmiller · 2022-05-27T13:51:20Z

So after having another look at this I think it's actually incorrect to reject a device if it doesn't have an image, first off as the lit tests show it breaks sycl-ls since that uses device selectors but obviously doesn't have any device images.

I'll re-work this patch to take availability of device image into account in the device scores instead of just rejecting the device.

npmiller · 2022-05-27T16:48:00Z

So this turned into a bit of a larger refactoring of the device selector, here's the main changes:

Removed the initial score of 1000 when SYCL_DEVICE_FILTER is set. This is unnecessary as SYCL_DEVICE_FILTER filters the list of devices itself, so we can just use regular scoring on the already filtered list of devices
Replaced isDeviceOfPreferredSyclBe with a new getDevicePreference, which returns a "preference" score to add to the device score, this preference score includes preference of the level_zero backend, as well as a strong preference for devices that have images in the binary.
Remove use of isDeviceOfPreferredSyclBe as tie-breaker in select_device, this was redundant because it was already used to boost the score of preferred devices, so it should be included in the score.

npmiller · 2022-05-30T08:16:16Z

I misunderstood the purpose of the tie-breaker, it's not helpful for the standard selectors but it is for custom selectors, so I re-introduced the tie-breaking logic, but this time using the device preference score to tie-break. So a device with an image would be preferred over a level zero device with no image for example.

sycl/source/device_selector.cpp

Co-authored-by: Joe Todd <[email protected]>

cperkinsintel

LGTM.

sycl/source/device_selector.cpp

steffenlarsen · 2022-06-01T14:01:26Z

sycl/source/device_selector.cpp

-  // device::get_devices returns filtered list of devices.
-  // Keep 1000 for default score when filters were applied.
-  if (FilterList)
-    Score = 1000;


Why is this removed? I am not completely sure, but it seems like this tries to ensure that no device is rejected if the user applied a filter. Maybe we could simply change it to a += 1 to ensure this?

From the history I got the feeling that it was more of a leftover from a time where the default selector was actually using the environment variable to do some filtering, and it felt a bit pointless since if there is a filter all the devices here would start at 1000 score, last PR changing this: #5349

You make a good point about a filtered device potentially being rejected, although I don't think it can happen at the moment since all defined device types are covered and add score, but what would you think about just starting the score for the default selector at 0 rather than REJECT_DEVICE_SCORE, instead of restoring this? I don't think it really makes sense for the default selector to reject any devices.

Updated the patch as I suggested, let me know what you think

bso-intel

default_selector can throw an exception if no device is available to satisfy the SYCL_DEVICE_FILTER.
https://github.com/intel/llvm/blob/sycl/sycl/doc/EnvironmentVariables.md

npmiller · 2022-06-06T08:28:32Z

Yes and that still happens, SYCL_DEVICE_FILTER affects the list of devices, whereas the device selectors only score and rank the available devices, with the possibility of rejecting them.

If there is no device that satisfies SYCL_DEVICE_FILTER, the list of devices will be empty to begin with, so the default_selector won't have anything to score, and it will correctly throw a device not found exception.

steffenlarsen

Changes LGTM. @bso-intel approval required.

bso-intel

It makes sense. If SYCL_DEVICE_FILTER is set, no device that does not satisfy the filter can be created.
So, no additional score for filter is needed.
LGTM.

* Fix links to configure.py and compile.py * Linking to the file in tree caused the link in the docs to just download the python script. It makes more sense to link to the github web UI for these as they should be used in a checkout anyway. * Remove CUDA requiring device selector * Since intel#6203 the device selector should handle this case well. * Update HIP backend limitations * HIP is no longer in beta * Windows isn't supported but intel#17702 made the build work with it so it might work for some users. * Global offset has been supported since intel#5855 * Add common limitations * Update HIP for Nvidia section * This might work but is not supported * Update HIP section * Recommended HIP version + testing platforms * HIP is no longer in beta * Add note on target aliases for CUDA and HIP

* Fix links to configure.py and compile.py * Linking to the file in tree caused the link in the docs to just download the python script. It makes more sense to link to the github web UI for these as they should be used in a checkout anyway. * Remove CUDA requiring device selector * Since #6203 the device selector should handle this case well. * Update HIP backend limitations * HIP is no longer in beta * Windows isn't supported but #17702 made the build work with it so it might work for some users. * Global offset has been supported since #5855 * Add common limitations * Update HIP for Nvidia section * This might work but is not supported * Update HIP section * Recommended HIP version + testing platforms * HIP is no longer in beta * Add note on target aliases for CUDA and HIP

[SYCL] Don't select devices with no available images

326f40a

npmiller requested a review from a team as a code owner May 26, 2022 16:52

npmiller requested a review from cperkinsintel May 26, 2022 16:52

cperkinsintel reviewed May 26, 2022

View reviewed changes

cperkinsintel requested a review from steffenlarsen May 26, 2022 19:46

npmiller added 2 commits May 27, 2022 17:30

[SYCL] Refactor device selector

86483b9

[SYCL] Improve hasCompatibleImage

01d278e

npmiller added 2 commits May 27, 2022 17:50

[SYCL] Remove unused program_manager

17305b1

[SYCL] Use device preference for tie breaking

f19c374

npmiller mentioned this pull request May 30, 2022

[SYCL][CUDA] Default selector behaviour #1665

Closed

joeatodd reviewed May 30, 2022

View reviewed changes

sycl/source/device_selector.cpp Outdated Show resolved Hide resolved

[SYCL] Reverse pref order for readability

e9257cf

Co-authored-by: Joe Todd <[email protected]>

cperkinsintel previously approved these changes May 31, 2022

View reviewed changes

steffenlarsen reviewed Jun 1, 2022

View reviewed changes

npmiller added 2 commits June 1, 2022 17:41

[SYCL] Boost score of device type filter

5d91dec

[SYCL] Don't reject devices from default selector

a36d209

npmiller dismissed cperkinsintel’s stale review via a36d209 June 1, 2022 16:45

cperkinsintel requested a review from bso-intel June 1, 2022 20:46

bso-intel requested changes Jun 2, 2022

View reviewed changes

steffenlarsen approved these changes Jun 6, 2022

View reviewed changes

bso-intel approved these changes Jun 6, 2022

View reviewed changes

pvchupin merged commit 0e67db8 into intel:sycl Jun 7, 2022

npmiller mentioned this pull request Apr 9, 2025

[SYCL][DOC] CUDA and HIP GetStartedGuide updates #17928

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL] Don't select devices with no available images #6203

[SYCL] Don't select devices with no available images #6203

Uh oh!

npmiller commented May 26, 2022

Uh oh!

cperkinsintel May 26, 2022

Uh oh!

npmiller May 27, 2022

Uh oh!

npmiller commented May 27, 2022

Uh oh!

npmiller commented May 27, 2022 •

edited

Loading

Uh oh!

npmiller commented May 30, 2022

Uh oh!

Uh oh!

cperkinsintel left a comment

Uh oh!

Uh oh!

steffenlarsen Jun 1, 2022

Uh oh!

npmiller Jun 1, 2022

Uh oh!

npmiller Jun 1, 2022

Uh oh!

bso-intel left a comment

Uh oh!

npmiller commented Jun 6, 2022

Uh oh!

steffenlarsen left a comment

Uh oh!

bso-intel left a comment

Uh oh!

Uh oh!

[SYCL] Don't select devices with no available images #6203

[SYCL] Don't select devices with no available images #6203

Uh oh!

Conversation

npmiller commented May 26, 2022

Uh oh!

cperkinsintel May 26, 2022

Choose a reason for hiding this comment

Uh oh!

npmiller May 27, 2022

Choose a reason for hiding this comment

Uh oh!

npmiller commented May 27, 2022

Uh oh!

npmiller commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

npmiller commented May 30, 2022

Uh oh!

Uh oh!

cperkinsintel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steffenlarsen Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

npmiller Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

npmiller Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

bso-intel left a comment

Choose a reason for hiding this comment

Uh oh!

npmiller commented Jun 6, 2022

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

bso-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

npmiller commented May 27, 2022 •

edited

Loading