[SYCL] Remove SYCL_DEVICE_FILTER support #12384

uditagarwal97 · 2024-01-12T16:16:45Z

Overview
This PR removes support for SYCL_DEVICE_FILTER, and refactors Basic/diagnostics/device-check.cpp to use ONEAPI_DEVICE_FILTER instead of SYCL_DEVICE_TYPE, as the latter is depreciated.

sarnex

ESIMD changes (sycl/test/invoke_simd/invoke_simd.cpp only) LGTM

uditagarwal97 · 2024-01-12T21:29:30Z

@steffenlarsen I've addressed your reviews from #12007 in this PR. Since Euna is inactive, I'll be taking forward this effort (that's why created a separate PR).

sycl/source/detail/config.cpp

aelovikov-intel · 2024-01-12T21:36:31Z

sycl/source/detail/filter_selector_impl.cpp

@@ -129,6 +131,7 @@ int filter_selector_impl::operator()(const device &Dev) const {
      else
        DeviceTypeOK = (DT == Filter.DeviceType);
    }
+#ifndef __INTEL_PREVIEW_BREAKING_CHANGES


Can you clarify why this code relates only to one path and not the other? It's not immediately clear from the variable names below...

I've fixed this. Apparently, removing this branch was changing the functionality of filter selector. So, instead I've now added a variable "MatchesSeen" in ods_target to keep track of the devices that matches this filter.

aelovikov-intel · 2024-01-12T21:38:22Z

sycl/source/detail/config.cpp

+// TODO: host device type will be removed once sycl_ext_oneapi_filter_selector
+// is removed.


Do you know if we should remove it under the same task (even though in a separate PR)?

AFAIK, from #12007 (comment), we should perhaps first depreciate sycl_ext_oneapi_filter_selector and then, eventually remove or replace it. I think it would be better to do that in a separate task as it would require more discussion and it is loosely related to the current task of removing depreciated variables.

aelovikov-intel · 2024-01-12T21:41:21Z

sycl/test-e2e/DeprecatedFeatures/deprecated_sycl_device_filter.cpp

-// RUN: %{build} -o %t.out
-// RUN: env SYCL_DEVICE_FILTER='*' %{run-unfiltered-devices} %t.out &> %t.log
-// RUN: FileCheck %s < %t.log
+// RUN: %if !preview-breaking-changes-supported %{ %{build} -o %t.out %}


I don't think we need any changes in the tests using old filter - it's the default still.

gmlueck · 2024-01-12T21:47:42Z

sycl/source/detail/filter_selector_impl.cpp

@@ -129,6 +131,7 @@ int filter_selector_impl::operator()(const device &Dev) const {
      else
        DeviceTypeOK = (DT == Filter.DeviceType);
    }
+#ifndef __INTEL_PREVIEW_BREAKING_CHANGES


Are these #ifdef lines changing the behavior of the filter_selector? I'm not sure we want to do that, and I didn't see any changes to the "sycl_ext_oneapi_filter_selector" extension specification.

Thanks, I've fixed this. It was indeed changing the behavior of filter selector.

kbenzie

UR LGTM

AlexeySachkov

doc changes LGTM

AlexeySachkov · 2024-01-16T16:06:38Z

sycl/source/detail/config.cpp

@@ -175,8 +176,21 @@ getSyclDeviceTypeMap() {
  return SyclDeviceTypeMap;
 }

-// Array is used by SYCL_DEVICE_FILTER and SYCL_DEVICE_ALLOWLIST and
-// ONEAPI_DEVICE_SELECTOR
+#ifdef __INTEL_PREVIEW_BREAKING_CHANGES


Can we just #ifdef a single line with esimd_emulator and use auto return type to unify those two versions?

sycl/source/detail/pi.cpp

gmlueck

Doc and spec changes LGTM.

uditagarwal97 · 2024-01-17T19:17:07Z

A couple of minor comments.

I suggest updating PR title and description: "[SYCL] Remove depreciated environment variables" -> "[SYCL] Remove SYCL_DEVICE_FILTER support".

90+% of this patch related to just one environment variable.

Same as https://github.com/intel/llvm/pull/12007with additional changes.

Overview

NOTE: PR description will be a git log message when merged and I don't think we need this this information there. I would move this information from PR description to PR comments.

This PR removes depreciated environment variables, namely SYCL_BE, SYCL_DEVICE_FILTER, and SYCL_DEVICE_TYPE, along with depreciated esimd_emulator backend.

I don't see any instance of SYCL_BE in this PR.

SYCL_DEVICE_TYPE is used only in one test which just checks runtime emits diagnostics when this variable is set.

esimd_emulator is not an environment variable, so I would move related changes to a separate PR.

Thanks, Alexey. I've made most of the suggested changes. Regarding moving esimd_emulator related changes to a separate PR, my concern is that esimd_emulator is supported only with SYCL_DEVICE_FILTER, not with ONEAPI_DEVICE_SELECTOR. So, the stuff I am migrating from SYCL_DEVICE_FILTER to ONEAPI_DEVICE_SELECTOR (like filter_selector) might fail if we don't remove the esimd_emulator support right away. The converse of this, i.e. removing esimd_emulator support before merging this PR, would also be incorrect as it is still valid for SYCL_DEVICE_FILTER.

bader

Thanks!

uditagarwal97 · 2024-01-18T18:51:29Z

Converted this PR to draft to prevent accidental merges. I'm trying to separate out esimd_emulator related changes from this PR to #12428. Once it gets merged, I'll refactor and open this PR again.

uditagarwal97 · 2024-01-24T23:12:12Z

@aelovikov-intel @bso-intel could you review this PR again? The only change made since the last review is the separation of ESIMD related changes in #12428

aelovikov-intel · 2024-01-26T19:24:37Z

Post commit failures on Arc GPU:

Failed Tests (2):
  SYCL :: Basic/get_backend.cpp
  SYCL :: OneapiDeviceSelector/level_zero_top.cpp

The first is kind of known, see #12463 (comment).
The second is new to me, but looks passing in the previous/next jobs, so possibly unrelated.

uditagarwal97 added 2 commits January 12, 2024 08:06

Remove depreciated variables.

5fc7e05

Fix formatting reported by clang format

107a593

uditagarwal97 self-assigned this Jan 12, 2024

uditagarwal97 added 2 commits January 12, 2024 08:33

Merge remote-tracking branch 'origin/sycl' into remove_depr_vars

1196583

Resolve merge conflicts.

33fb390

uditagarwal97 temporarily deployed to WindowsCILock January 12, 2024 17:19 — with GitHub Actions Inactive

uditagarwal97 temporarily deployed to WindowsCILock January 12, 2024 18:46 — with GitHub Actions Inactive

uditagarwal97 added 2 commits January 12, 2024 13:22

Fix device-check diagnostic test case.

c543e28

Fix formatting.

d6f497c

uditagarwal97 temporarily deployed to WindowsCILock January 12, 2024 21:25 — with GitHub Actions Inactive

uditagarwal97 changed the title ~~[DRAFT][SYCL] Remove depreciated environment variables~~ [SYCL] Remove depreciated environment variables Jan 12, 2024

uditagarwal97 marked this pull request as ready for review January 12, 2024 21:25

uditagarwal97 requested review from a team as code owners January 12, 2024 21:25

uditagarwal97 requested review from bso-intel, aelovikov-intel and steffenlarsen January 12, 2024 21:25

sarnex approved these changes Jan 12, 2024

View reviewed changes

aelovikov-intel reviewed Jan 12, 2024

View reviewed changes

gmlueck reviewed Jan 12, 2024

View reviewed changes

uditagarwal97 temporarily deployed to WindowsCILock January 12, 2024 22:32 — with GitHub Actions Inactive

kbenzie approved these changes Jan 16, 2024

View reviewed changes

AlexeySachkov approved these changes Jan 16, 2024

View reviewed changes

Address reviews. Minor formatting.

b3bef9f

uditagarwal97 had a problem deploying to WindowsCILock January 17, 2024 18:06 — with GitHub Actions Failure

uditagarwal97 requested a review from aelovikov-intel January 17, 2024 18:10

Minor fix

7f8638f

uditagarwal97 changed the title ~~[SYCL] Remove depreciated environment variables~~ [SYCL] Remove SYCL_DEVICE_FILTER support Jan 17, 2024

uditagarwal97 had a problem deploying to WindowsCILock January 17, 2024 18:49 — with GitHub Actions Error

gmlueck approved these changes Jan 17, 2024

View reviewed changes

Fix typo.

db81430

uditagarwal97 had a problem deploying to WindowsCILock January 17, 2024 19:35 — with GitHub Actions Error

bader approved these changes Jan 17, 2024

View reviewed changes

Fix typo

71f3a31

uditagarwal97 temporarily deployed to WindowsCILock January 17, 2024 20:13 — with GitHub Actions Inactive

uditagarwal97 temporarily deployed to WindowsCILock January 17, 2024 20:38 — with GitHub Actions Inactive

uditagarwal97 mentioned this pull request Jan 17, 2024

[SYCL] Remove esimd_emulator from filters #12428

Merged

Reset MatchesSeen.

6a19535

uditagarwal97 temporarily deployed to WindowsCILock January 18, 2024 00:34 — with GitHub Actions Inactive

uditagarwal97 temporarily deployed to WindowsCILock January 18, 2024 00:56 — with GitHub Actions Inactive

bso-intel approved these changes Jan 18, 2024

View reviewed changes

uditagarwal97 marked this pull request as draft January 18, 2024 18:49

uditagarwal97 added 3 commits January 24, 2024 14:59

Merge branch 'sycl' into remove_depr_vars

f2b5792

Remove esimd stuff

edf4094

Fix formatting

df2226f

uditagarwal97 marked this pull request as ready for review January 24, 2024 23:06

uditagarwal97 requested a review from bso-intel January 24, 2024 23:08

uditagarwal97 temporarily deployed to WindowsCILock January 24, 2024 23:49 — with GitHub Actions Inactive

uditagarwal97 temporarily deployed to WindowsCILock January 25, 2024 00:17 — with GitHub Actions Inactive

bso-intel approved these changes Jan 25, 2024

View reviewed changes

aelovikov-intel merged commit 8d0fa98 into intel:sycl Jan 26, 2024

uditagarwal97 deleted the remove_depr_vars branch March 21, 2024 19:59

		// TODO: host device type will be removed once sycl_ext_oneapi_filter_selector
		// is removed.

[SYCL] Remove SYCL_DEVICE_FILTER support #12384

[SYCL] Remove SYCL_DEVICE_FILTER support #12384

Uh oh!

Conversation

uditagarwal97 commented Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sarnex left a comment

Choose a reason for hiding this comment

Uh oh!

uditagarwal97 commented Jan 12, 2024

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!

kbenzie left a comment

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmlueck left a comment

Choose a reason for hiding this comment

Uh oh!

uditagarwal97 commented Jan 17, 2024

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

uditagarwal97 commented Jan 18, 2024

Uh oh!

uditagarwal97 commented Jan 24, 2024

Uh oh!

aelovikov-intel commented Jan 26, 2024

Uh oh!

Uh oh!

uditagarwal97 commented Jan 12, 2024 •

edited

Loading