[SYCL] Removed deprecated environment variables #12007

eunakim0103 · 2023-11-26T07:27:45Z

Removed deprecated environment variables in below including in the .md files, corresponding source code as well as test codes:

SYCL_DEVICE_TYPE
SYCL_DEVICE_FILTER
SYCL_BE

SYCL_DEVICE_FILTER has been replaced with ONEAPI_DEVICE_SELECTOR. Removed the below two items from available backend lists and a device type which are deprecated in SYCL_DEVICE_FILTER.

backend: esimd_emulator (deprecated), **host (deprecated) was kept
device type: host (deprecated) but kept

Modified/Replaced test files are as below:

llvm/sycl/test/invoke_simd.cpp
llvm/sycl/test-e2e/DeprecatedFeatures/deprecated_removed_sycl_device_filter.cpp

Added “__INTEL_PREVIEW_BREAKING_CHANGES macro” macro for breaking changes

AlexeySachkov · 2023-11-26T10:21:29Z

sycl/test/invoke_simd/invoke_simd.cpp

@@ -1,350 +0,0 @@
-// RUN: %clangxx -fsycl -fsyntax-only -fno-sycl-device-code-split-esimd -Xclang -fsycl-allow-func-ptr %s


Removal of this test doesn't seem right: it has nothing to do with environment variable you are removing, the test doesn't even launch the binary it builds.

While removing SYCL_DEVICE_FILTER, I found esimd_emulator has been deprecated from backend list in SYCL_DEVICE_FILTER (and does not exist in ONEAPI_DEVICE_SELECTOR). I removed esimd_emulator part and test as well. Let me double check the spec first.

Please do not remove this test.

Thanks all! Evgeniy confirmed that this test is not for emulator-specific and we should keep it. I will fix it.

steffenlarsen · 2023-11-27T08:31:39Z

sycl/doc/MultiTileCardWithLevelZero.md

 ```
-$ SYCL_DEVICE_FILTER=level_zero sycl-ls
+$ ONEAPI_DEVICE_SELECTOR=level_zero sycl-ls


This is not a valid use of ONEAPI_DEVICE_SELECTOR. I believe it should be

Suggested change

$ ONEAPI_DEVICE_SELECTOR=level_zero sycl-ls

$ ONEAPI_DEVICE_SELECTOR=level_zero:* sycl-ls

Same applies to the other example below.

Also, we should use the official ext_oneapi_level_zero spelling.

According to this documentation, the official spelling is "level_zero".

sycl-ls uses ext_oneapi_* for level_zero/hip/cuda in its output. I would expect that the document is slightly obsolete and needs to be fixed.

We've used the spelling "level_zero" in every presentation that I can think of. @cperkinsintel, I think you implemented ONEAPI_DEVICE_SELECTOR. Does it expect "level_zero" or "ext_oneapi_level_zero" as the name of the backend?

I think the confusion came from the fact that ONEAPI_DEVICE_SELECTOR has

<backend> ::= { * | level_zero | opencl | cuda | hip | native_cpu } // case insensitive

while
https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/supported/sycl_ext_oneapi_backend_level_zero.md has

enum class backend { // ... ext_oneapi_level_zero, // ... };

SYCL2020 6.3.7. Adding a backend says this:

The extension should add a new value to the sycl::backend enumeration type using a naming scheme like ext__. For example, if the Acme vendor adds a backend named "foo", it would add an enumerated value named sycl::backend::ext_acme_foo.

As such, I have two questions:

What would happen if we'll have two vendors providing backends with the same name?

Why doesn't native_cpu follow the naming convention required by the specification (see

llvm/sycl/include/sycl/backend_types.hpp

Lines 18 to 28 in 7ed894a

enum class backend : char {

host __SYCL2020_DEPRECATED("'host' backend is no longer supported") = 0,

opencl = 1,

ext_oneapi_level_zero = 2,

ext_oneapi_cuda = 3,

all = 4,

ext_intel_esimd_emulator __SYCL_DEPRECATED(

"esimd emulator is no longer supported") = 5,

ext_oneapi_hip = 6,

ext_native_cpu = 7,

};

)?

What would happen if we'll have two vendors providing backends with the same name

Is your question about the enum class backend enumeration? Two vendors would have different "vendor strings" (e.g. ours is "oneapi"), so the enumerators would not collide.

Why doesn't native_cpu follow the naming convention required by the specification

This is a bug. @PietroGhg this should be named like ext_oneapi_native_cpu instead. Could you rename this? I'm not sure if you have existing users that might be depending on this symbol. If so, you can deprecate the old name and add a new one that is an alias like ext_oneapi_native_cpu = ext_native_cpu.

Hi @gmlueck, sorry about this, we'll create a PR to fix it soon. We should be able to just rename it, thank you

Thanks @PietroGhg!

I checked and the ONEAPI_DEVICE_SELECTOR environment variable accepts the string "level_zero" as the name of the backend. This is also what we show in the documentation.

I would like to voice a strong support for keeping the current behavior of the environment variable: accepting level_zero/cuda/hip without any prefixes. ONEAPI_* environment variables are already an implementation detail. Adding ext_oneapi_ in them won't add any clarity but will irritate users who has to type it.

That does not concern enum values in the code (as noted, the standard is clear how they should look like); if it is seen beneficial to allow setting ONEAPI_DEVICE_SELECTOR=ext_oneapi_level_zero:gpu, then I think that should be in addition to, not instead of, the current ONEAPI_DEVICE_SELECTOR=level_zero:gpu spelling.

steffenlarsen · 2023-11-27T08:40:38Z

sycl/source/detail/config.cpp

 getSyclDeviceTypeMap() {
-  static const std::array<std::pair<std::string, info::device_type>, 6>
-      SyclDeviceTypeMap = {{{"host", info::device_type::host},


The sycl_ext_oneapi_filter_selector still accepts "host" as device type and backend, so we can't remove it from here just yet.

can we just drop that from both? My understanding that even if the "host" is part of the extension, it cannot be selected because the support for the device itself has been dropped.

Problem is that the extension is supported, so we normally can't just change it out of the blue. Removing it might mean that filter_selector would start failing at construction with "host" input rather than on device selection.

@gmlueck | @jbrodman - Thoughts on this? We should probably write a new variant of this extension using SYCL 2020 device selector structure.

I also think it would be good to deprecate (and eventually remove) sycl_ext_oneapi_filter_selector, mostly because the input "filter" language doesn't match the language used by ONEAPI_DEVICE_SELECTOR. I think the structure of the selector is OK, though. Doesn't it already match the SYCL 2020 selector structure? As a comparison, see how the aspect_selector is structured.

Assuming we replace this with some new extension that uses the ONEAPI_DEVICE_SELECTOR language, we'd need some new name for the API. Suggestions are welcome.

Another option is to just deprecate / remove sycl_ext_oneapi_filter_selector without adding a replacement. Do we think this is an important extension that customers use?

I took back the host in both device type and backend. The "deprecate/remove sycl_ext_oneapi_filter_selector" job can be done separately from this environment variable removal.

rolandschulz · 2023-11-27T19:29:02Z

Can we please add a diagnostic if the removed environment variables are used?

eunakim0103 · 2023-11-27T19:33:23Z

Can we please add a diagnostic if the removed environment variables are used?

Good idea. Will do.

aelovikov-intel · 2023-11-28T21:52:56Z

devops/actions/khronos_cts_test/action.yml

@@ -14,9 +14,6 @@ inputs:
    default: 'llvm_sycl.tar.zst'
  decompress_command:
    default: zstd
-  sycl_device_filter:


That removal shouldn't be done - it needs to be updated to the new filter instead. That said, I don't think our CI uses this and we should probably just drop the entire file/directory (possibly in a separate PR).

There is also some (currently unused) CTS support in https://github.com/intel/llvm/blob/sycl/.github/workflows/sycl_linux_run_tests.yml

That's a point. We should probably open another ticket

…urce code

steffenlarsen · 2023-11-30T12:41:27Z

sycl/source/esimd_emulator_device_interface.cpp

    std::cerr << "ESIMD EMU plugin error or not loaded - try setting "
                 "SYCL_DEVICE_FILTER=esimd_emulator:gpu environment variable"
              << std::endl;
+#else
+    std::cerr << "ESIMD EMU plugin error or not loaded" << std::endl;
+#endif


I would say we just remove the old print. I don't think we should suggest a deprecated env var even if it is still there.

steffenlarsen · 2023-11-30T12:43:26Z

sycl/test-e2e/Plugin/sycl-ls-gpu-default-any.cpp

+// RUN: env --unset=SYCL_DEVICE_FILTER --unset=ONEAPI_DEVICE_SELECTOR sycl-ls --verbose >%t.default.out
+// RUN: %if preview-breaking-changes-supported %{ env --unset=ONEAPI_DEVICE_SELECTOR sycl-ls --verbose >%t.default.out %}


This new test case does not make sense to me. First of all, we simply overwrite the previous output file before we've even checked it. Second, the command that's run doesn't do anything special. I doubt we've built sycl-ls with the -fpreview-breaking-changes flag, so running it an extra time if that flag is supported adds nothing new to the test.

steffenlarsen · 2023-11-30T12:43:38Z

sycl/test-e2e/Plugin/sycl-ls-gpu-default-level-zero.cpp

 // RUN: env --unset=SYCL_DEVICE_FILTER --unset=ONEAPI_DEVICE_SELECTOR sycl-ls --verbose >%t.default.out
+// RUN: %if preview-breaking-changes-supported %{ env --unset=ONEAPI_DEVICE_SELECTOR sycl-ls --verbose >%t.default.out %}


Same as my other comment.

steffenlarsen · 2023-11-30T12:49:11Z

sycl/test/invoke_simd/invoke_simd.cpp

+    return device.is_gpu() ? 1000 : -1;
+  }
+};
+#endif //__INTEL_PREVIEW_BREAKING_CHANGES


ONEAPI_DEVICE_SELECTOR is available with or without breaking changes. Is there a reason we wouldn't just use it here instead of the deprecated one? In fact, we don't built this test with -fpreview-breaking-changes so the new branch is dead code.

In reality I don't even think it matters much. This test is compile-only so this selector really has no effect on whether the test passes or fails.

steffenlarsen · 2023-11-30T12:52:00Z

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

@@ -126,6 +127,7 @@ int main(int argc, char **argv) {
        << std::endl
        << std::endl;
  }
+#endif


I don't think we build sycl-ls with -fpreview-breaking-changes ever, so there is really no point in masking this out now, other than maybe to remind us to remove it in the future.

steffenlarsen · 2023-11-30T12:52:19Z

sycl/test/basic_tests/device-selectors-exception.cpp

@@ -1,5 +1,6 @@
 // RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple %s -o %t.out
 // RUN: env SYCL_DEVICE_FILTER="" %t.out
+// RUN: %if preview-breaking-changes-supported %{ env ONEAPI_DEVICE_SELECTOR="" %t.out %}


This doesn't do anything different than line 2, except being dependent on the preview flag being available. Seems like it should be:

Suggested change

// RUN: %if preview-breaking-changes-supported %{ env ONEAPI_DEVICE_SELECTOR="" %t.out %}

// RUN: %if preview-breaking-changes-supported %{ %clangxx -fsycl -fsycl-targets=%sycl_triple -fpreview-breaking-changes %s -o %t_prev.out %}

// RUN: %if preview-breaking-changes-supported %{ env ONEAPI_DEVICE_SELECTOR="" %t_prev.out %}

steffenlarsen · 2023-11-30T12:59:08Z

sycl/test-e2e/DeprecatedFeatures/deprecated_removed_sycl_device_filter.cpp

@@ -2,12 +2,13 @@
 // RUN: env SYCL_DEVICE_FILTER='*' %{run-unfiltered-devices} %t.out &> %t.log
 // RUN: FileCheck %s < %t.log
 //
-// CHECK:      WARNING: The enviroment variable SYCL_DEVICE_FILTER is deprecated.
+// CHECK:      WARNING: The enviroment variable SYCL_DEVICE_FILTER is removed.


Where does this warning come from? Originally it came from sycl/source/detail/config.hpp in the part that is now masked out by the preview macro, but I would expect it to still report "deprecated" in this test in its current form. I have no idea what it would say if you set -fpreview-breaking-changes in line 2, but I suspect it won't be this.

steffenlarsen · 2023-11-30T12:59:34Z

sycl/test-e2e/DeprecatedFeatures/deprecated_removed_sycl_device_filter.cpp

@@ -16,14 +17,14 @@
 //===-----------------------------------------------------------------------------------------===//

 // This test is to check if a warning message is displayed when using the
-// enviroment variable SYCL_DEVICE_FILTER
-// TODO: Remove test when SYCL_DEVICE_FILTER is removed
+// enviroment variable SYCL_DEVICE_FILTERll


Suggested change

// enviroment variable SYCL_DEVICE_FILTERll

// enviroment variable SYCL_DEVICE_FILTER.

Also see: intel#12007 (comment) intel#12048

Also see: #12007 (comment) #12048

github-actions · 2024-09-13T01:57:17Z

This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days.

github-actions · 2024-10-17T01:59:22Z

This pull request was closed because it has been stalled for 30 days with no activity.

removed deprecated environmental variables

2f02767

eunakim0103 requested review from a team as code owners November 26, 2023 07:27

eunakim0103 requested a review from steffenlarsen November 26, 2023 07:27

eunakim0103 marked this pull request as draft November 26, 2023 07:27

AlexeySachkov reviewed Nov 26, 2023

View reviewed changes

steffenlarsen reviewed Nov 27, 2023

View reviewed changes

eunakim0103 marked this pull request as ready for review November 28, 2023 16:57

aelovikov-intel reviewed Nov 28, 2023

View reviewed changes

eunakim0103 marked this pull request as draft November 30, 2023 01:35

eunakim0103 added 4 commits November 29, 2023 19:52

fixed the discussion in comments and added intel preview breaking macro

33b7378

added device selector query to the invoke_simd test and rearranged so…

6e99c4e

…urce code

merged with latest changes

d202d5b

fix the clang format error

14ef96e

eunakim0103 temporarily deployed to WindowsCILock November 30, 2023 05:30 — with GitHub Actions Inactive

eunakim0103 had a problem deploying to WindowsCILock November 30, 2023 05:58 — with GitHub Actions Failure

steffenlarsen requested changes Nov 30, 2023

View reviewed changes

AlexeySachkov mentioned this pull request Dec 1, 2023

ONEAPI_DEVICE_SELECTOR and sycl-ls use different terminology #12048

Closed

aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Dec 15, 2023

[SYCL][E2E][LIT] Normalize devices not to include "ext_oneapi_"

f1b1cc0

Also see: intel#12007 (comment) intel#12048

aelovikov-intel mentioned this pull request Dec 15, 2023

[SYCL][E2E][LIT] Normalize devices not to include "ext_oneapi_" #12185

Merged

aelovikov-intel added a commit that referenced this pull request Dec 19, 2023

[SYCL][E2E][LIT] Normalize devices not to include "ext_oneapi_" (#12185)

3b88e69

Also see: #12007 (comment) #12048

uditagarwal97 self-assigned this Jan 10, 2024

uditagarwal97 mentioned this pull request Jan 12, 2024

[SYCL] Remove SYCL_DEVICE_FILTER support #12384

Merged

github-actions bot added the Stale label Sep 13, 2024

github-actions bot closed this Oct 17, 2024

		@@ -1,350 +0,0 @@
		// RUN: %clangxx -fsycl -fsyntax-only -fno-sycl-device-code-split-esimd -Xclang -fsycl-allow-func-ptr %s

	$ ONEAPI_DEVICE_SELECTOR=level_zero sycl-ls
	$ ONEAPI_DEVICE_SELECTOR=level_zero:* sycl-ls

	enum class backend : char {
	host __SYCL2020_DEPRECATED("'host' backend is no longer supported") = 0,
	opencl = 1,
	ext_oneapi_level_zero = 2,
	ext_oneapi_cuda = 3,
	all = 4,
	ext_intel_esimd_emulator __SYCL_DEPRECATED(
	"esimd emulator is no longer supported") = 5,
	ext_oneapi_hip = 6,
	ext_native_cpu = 7,
	};

		// RUN: env --unset=SYCL_DEVICE_FILTER --unset=ONEAPI_DEVICE_SELECTOR sycl-ls --verbose >%t.default.out
		// RUN: %if preview-breaking-changes-supported %{ env --unset=ONEAPI_DEVICE_SELECTOR sycl-ls --verbose >%t.default.out %}

	// enviroment variable SYCL_DEVICE_FILTERll
	// enviroment variable SYCL_DEVICE_FILTER.

[SYCL] Removed deprecated environment variables #12007

[SYCL] Removed deprecated environment variables #12007

Uh oh!

Conversation

eunakim0103 commented Nov 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

al42and Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rolandschulz commented Nov 27, 2023

Uh oh!

eunakim0103 commented Nov 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 13, 2024

Uh oh!

github-actions bot commented Oct 17, 2024

Uh oh!

Uh oh!

eunakim0103 commented Nov 26, 2023 •

edited

Loading

aelovikov-intel Nov 27, 2023 •

edited

Loading

al42and Dec 15, 2023 •

edited

Loading