llvm icon indicating copy to clipboard operation
llvm copied to clipboard

[SYCL] Add '--ignore-device-selectors' CLI option to sycl-ls and improve warning messages

Open uditagarwal97 opened this issue 1 year ago • 4 comments
trafficstars

This PR adds a '--ignore-device-selectors' CLI option to sycl-ls that prints all platforms available in the user's system, irrespective of the DPCPP filter environment variables like ONEAPI_DEVICE_SELECTOR.

uditagarwal97 avatar Feb 14 '24 21:02 uditagarwal97

Do we really need this option? Why not just modify sycl-ls so that's what it always does? ( ie. always shows all the devices, regardless of the filter).

If there are real use cases for using ONEAPI_DEVICE_SELECTOR in conjunction with sycl-ls, then maybe this could be reversed to have a --allow-filter flag.

cperkinsintel avatar Feb 16 '24 19:02 cperkinsintel

Do we really need this option? Why not just modify sycl-ls so that's what it always does? ( ie. always shows all the devices, regardless of the filter).

If there are real use cases for using ONEAPI_DEVICE_SELECTOR in conjunction with sycl-ls, then maybe this could be reversed to have a --allow-filter flag.

I mostly agree with that. My only concern with changing the default behavior of sycl-ls is that it might be ABI-breaking (is it?), if we have any customer application using ONEAPI_DEVICE_SELECTOR or SYCL_DEVICE_ALLOWLIST along with sycl-ls.

uditagarwal97 avatar Feb 20 '24 00:02 uditagarwal97

Tagging @jbrodman @gmlueck to get additional feedback regarding changing the default behavior of sycl-ls.

uditagarwal97 avatar Feb 20 '24 22:02 uditagarwal97

I mostly agree with that. My only concern with changing the default behavior of sycl-ls is that it might be ABI-breaking (is it?), if we have any customer application using ONEAPI_DEVICE_SELECTOR or SYCL_DEVICE_ALLOWLIST along with sycl-ls.

It would probably be safer to wait until a major release before changing the default behavior.

I think our plan is to move the handling of ONEAPI_DEVICE_SELECTOR into the unified runtime at some point, and I think the unified runtime will provide two APIs: one that retrieves the filtered device list and another that retrieves the unfiltered list. When that happens, sycl-ls can use the new API to get the unfiltered list.

Be careful with documenting this option. I presume sycl-ls will always respect Level Zero environment variables like ZE_AFFINITY_MASK, and some people might consider this a "filter".

gmlueck avatar Feb 20 '24 23:02 gmlueck

@cperkinsintel The only thing changed since your last review is re-naming of "--discard-filters" to "--ignore-device-selectors".

The failure in pre-commit on Windows is unrelated: it looks like a driver issue on our runners. I've seen similar failures on other PRs as well: https://github.com/intel/llvm/actions/runs/7995620747/job/21837754530

uditagarwal97 avatar Feb 22 '24 18:02 uditagarwal97

@intel/llvm-gatekeepers the PR is ready!

uditagarwal97 avatar Feb 22 '24 23:02 uditagarwal97

Windows Gen12:

Failed Tests (8):
  SYCL :: Assert/assert_in_kernels_win.cpp
  SYCL :: Assert/assert_in_multiple_tus_one_ndebug_win.cpp
  SYCL :: Assert/assert_in_multiple_tus_win.cpp
  SYCL :: Assert/assert_in_one_kernel_win.cpp
  SYCL :: Assert/assert_in_simultaneous_kernels_win.cpp
  SYCL :: Assert/assert_in_simultaneously_multiple_tus.cpp
  SYCL :: Assert/assert_in_simultaneously_multiple_tus_one_ndebug.cpp
  SYCL :: Plugin/sycl-ls-unified-runtime.cpp

Reported in https://github.com/intel/llvm/issues/12797 and https://github.com/intel/llvm/issues/12798

steffenlarsen avatar Feb 23 '24 08:02 steffenlarsen