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

uditagarwal97 · 2024-02-14T21:12:11Z

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.

aelovikov-intel · 2024-02-15T18:00:28Z

sycl/tools/sycl-ls/sycl-ls.cpp

+  } else {
+    // Parse CLI options.
+    for (int i = 1; i < argc; i++) {
+      if (std::string(argv[i]) == "--verbose")


I'd do

using namespace std::literals; if (argv[i] == "--verbose"sv)

instead.

aelovikov-intel · 2024-02-15T18:27:35Z

sycl/test/tools/sycl-ls.test

@@ -1,3 +1,18 @@
-- Check that sycl-ls exits with 0 exit code. 
+-- Check that sycl-ls exits with 0 exit code.


That's slightly inaccurate - you're verifying the output as well.

aelovikov-intel · 2024-02-15T18:28:08Z

sycl/tools/sycl-ls/sycl-ls.cpp

+// Restore filter related environment variables that we unset earlier.
+static void restoreFilterEnvVars() {
+  for (auto it : FilterEnvVars) {
+#ifdef _WIN32
+    _putenv_s(it.first.c_str(), it.second.c_str());
+#else
+    setenv(it.first.c_str(), it.second.c_str(), 1);
+#endif
+  }
+}


I don't think we need to do that.

IIUC, setenv is not supported by MSVC and that's why I used _putenv_s for windows. Some other tests also use a similar approach: https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/ext/intel/fpga_device_selector.hpp#L38

What I meant is we don't need to restore at all. sycl-ls process is going to die anywhere and its environment won't affect anyone.

aelovikov-intel · 2024-02-15T18:28:41Z

sycl/tools/sycl-ls/sycl-ls.cpp

+  if (argc == 1) {
+    verbose = false;
+    DiscardFilters = false;
+  } else {


Do we use any command line options parsing libraries anywhere in sycl?

We do use LLVM's CLI support (https://llvm.org/docs/CommandLine.html) at a couple of places but I'm not sure if we should use that here because we just have two CLI options for sycl-ls and both of them are just boolean flags.

cperkinsintel · 2024-02-16T19:10:02Z

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.

uditagarwal97 · 2024-02-20T00:43:08Z

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 · 2024-02-20T22:43:30Z

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

gmlueck · 2024-02-20T23:16:05Z

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".

uditagarwal97 · 2024-02-22T18:33:58Z

@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

cperkinsintel

lets go to mars!

uditagarwal97 · 2024-02-22T23:18:41Z

@intel/llvm-gatekeepers the PR is ready!

steffenlarsen · 2024-02-23T08:05:01Z

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 #12797 and #12798

Add 'discard-filter' CLI option

bc39b2b

uditagarwal97 self-assigned this Feb 14, 2024

uditagarwal97 had a problem deploying to WindowsCILock February 14, 2024 21:12 — with GitHub Actions Failure

uditagarwal97 added 2 commits February 14, 2024 21:18

Fix for windows.

26a9ee8

clang format

523f21c

uditagarwal97 temporarily deployed to WindowsCILock February 15, 2024 05:22 — with GitHub Actions Inactive

uditagarwal97 temporarily deployed to WindowsCILock February 15, 2024 05:41 — with GitHub Actions Inactive

Add test for discard filters op

c5e0944

uditagarwal97 temporarily deployed to WindowsCILock February 15, 2024 17:34 — with GitHub Actions Inactive

uditagarwal97 changed the title ~~[SYCL][DRAFT] Add '--discard-filter' CLI option to sycl-ls and improve warning messages~~ [SYCL] Add '--discard-filter' CLI option to sycl-ls and improve warning messages Feb 15, 2024

uditagarwal97 marked this pull request as ready for review February 15, 2024 17:55

uditagarwal97 requested a review from a team as a code owner February 15, 2024 17:55

uditagarwal97 requested review from maarquitos14, cperkinsintel and aelovikov-intel February 15, 2024 17:55

uditagarwal97 temporarily deployed to WindowsCILock February 15, 2024 18:13 — with GitHub Actions Inactive

aelovikov-intel reviewed Feb 15, 2024

View reviewed changes

No need to restore env vars

095c737

uditagarwal97 temporarily deployed to WindowsCILock February 15, 2024 20:24 — with GitHub Actions Inactive

uditagarwal97 temporarily deployed to WindowsCILock February 15, 2024 21:41 — with GitHub Actions Inactive

cperkinsintel approved these changes Feb 21, 2024

View reviewed changes

Change option name

e18ceb6

uditagarwal97 had a problem deploying to WindowsCILock February 21, 2024 22:17 — with GitHub Actions Error

Typo

97b0329

uditagarwal97 temporarily deployed to WindowsCILock February 21, 2024 22:20 — with GitHub Actions Inactive

uditagarwal97 changed the title ~~[SYCL] Add '--discard-filter' CLI option to sycl-ls and improve warning messages~~ [SYCL] Add '--ignore-device-selectors' CLI option to sycl-ls and improve warning messages Feb 21, 2024

uditagarwal97 had a problem deploying to WindowsCILock February 21, 2024 22:42 — with GitHub Actions Failure

uditagarwal97 requested a review from cperkinsintel February 22, 2024 18:34

cperkinsintel approved these changes Feb 22, 2024

View reviewed changes

uditagarwal97 temporarily deployed to WindowsCILock February 23, 2024 06:42 — with GitHub Actions Inactive

uditagarwal97 had a problem deploying to WindowsCILock February 23, 2024 07:01 — with GitHub Actions Failure

steffenlarsen merged commit 6e3aa21 into intel:sycl Feb 23, 2024
10 of 11 checks passed

uditagarwal97 deleted the add_sycl_ls branch March 21, 2024 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

uditagarwal97 commented Feb 14, 2024 •

edited

Loading

aelovikov-intel Feb 15, 2024

aelovikov-intel Feb 15, 2024

aelovikov-intel Feb 15, 2024

uditagarwal97 Feb 15, 2024

aelovikov-intel Feb 15, 2024

aelovikov-intel Feb 15, 2024

uditagarwal97 Feb 15, 2024

cperkinsintel commented Feb 16, 2024

uditagarwal97 commented Feb 20, 2024

uditagarwal97 commented Feb 20, 2024

gmlueck commented Feb 20, 2024

uditagarwal97 commented Feb 22, 2024

cperkinsintel left a comment

uditagarwal97 commented Feb 22, 2024

steffenlarsen commented Feb 23, 2024

		@@ -1,3 +1,18 @@
		-- Check that sycl-ls exits with 0 exit code.
		-- Check that sycl-ls exits with 0 exit code.

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

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

Conversation

uditagarwal97 commented Feb 14, 2024 • edited Loading

aelovikov-intel Feb 15, 2024

Choose a reason for hiding this comment

aelovikov-intel Feb 15, 2024

Choose a reason for hiding this comment

aelovikov-intel Feb 15, 2024

Choose a reason for hiding this comment

uditagarwal97 Feb 15, 2024

Choose a reason for hiding this comment

aelovikov-intel Feb 15, 2024

Choose a reason for hiding this comment

aelovikov-intel Feb 15, 2024

Choose a reason for hiding this comment

uditagarwal97 Feb 15, 2024

Choose a reason for hiding this comment

cperkinsintel commented Feb 16, 2024

uditagarwal97 commented Feb 20, 2024

uditagarwal97 commented Feb 20, 2024

gmlueck commented Feb 20, 2024

uditagarwal97 commented Feb 22, 2024

cperkinsintel left a comment

Choose a reason for hiding this comment

uditagarwal97 commented Feb 22, 2024

steffenlarsen commented Feb 23, 2024

uditagarwal97 commented Feb 14, 2024 •

edited

Loading