[SYCL][E2E] Add a test to track UNSUPPORTED tests #15849

KornevNikita · 2024-10-24T12:26:34Z

Similar to XFAIL tests (no-xfail-without-tracker.cpp) adding a requirement to follow UNSUPPORTED by UNSUPPORTED-TRACKER.

Adding a test to track the amount and the list of improperly UNSUPPORTED tests.

Also update some tests to check the script.

Similar to XFAIL tests (no-xfail-without-tracker.cpp) adding a requirement to follow UNSUPPORTED by UNSUPPORTED-TRACKER. Adding a test to track the amount and the list of improperly UNSUPPORTED tests. Also update some tests to check the script.

KornevNikita · 2024-10-24T12:28:44Z

@steffenlarsen @AlexeySachkov I know you suggested "UNSUPPORTED-REASON", but if you don't mind I'd prefer "UNSUPPORTED-TRACKER" to unify this with XFAIL, so we don't have to remember what and where we should use. Or any other unified mark.

sycl/test-e2e/Basic/kernel_max_wg_size.cpp

KornevNikita · 2024-10-28T09:45:14Z

NOTE: need to re-run pre-commit before merging.

steffenlarsen

Let's get it running!

sycl/test-e2e/README.md

AlexeySachkov

I still prefer -REASON, but that's not a strong enough objection to block the PR.

I think it is good to be merged. @KornevNikita, could you please do a local merge + test and if everything is fine, let me know and I will press the merge button (we don't really need to re-run the whole CI for this PR).

AlexeySachkov · 2024-10-28T13:00:32Z

sycl/test-e2e/Basic/kernel_max_wg_size.cpp

@@ -5,6 +5,9 @@
 // https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/experimental/sycl_ext_intel_grf_size.asciidoc
 // REQUIRES: arch-intel_gpu_pvc || gpu-intel-dg2
 // UNSUPPORTED: cuda || hip
+// UNSUPPORTED-INTENDED: This extension is currently implemented in DPC++ only


Unrelated to the main change in the PR, but tagging @sarnex here. I'm not sure if having an optional property unguarded by an aspect is a good interface for end users. Even if we don't introduce an aspect, can we update the spec to say that submitting a kernel with grf property to a device and/or using a backend which doesn't support that results in an exception? This way we would be able to rewrite the test to avoid use of `UNSUPPORTED.

Moreover, we have REQUIRES which points to specific GPUs, do we even need UNSUPPORTED in that case?

AlexeySachkov · 2024-10-28T13:05:11Z

sycl/test/e2e_test_requirements/no-unsupported-without-info.cpp

+// RUN: grep -rI "UNSUPPORTED:" %S/../../test-e2e \
+// RUN: -A 1 --include=*.c --include=*.cpp --no-group-separator | \
+// RUN: grep -v "UNSUPPORTED:" | \
+// RUN: grep -Pv "UNSUPPORTED-TRACKER:\s+(?:https://github.com/[\w\d-]+/[\w\d-]+/issues/[\d]+)|(?:[\w]+-[\d]+)|(?:UNSUPPORTED-INTENDED:\s*.+)" > %t


Suggested change

// RUN: grep -Pv "UNSUPPORTED-TRACKER:\s+(?:https://github.com/[\w\d-]+/[\w\d-]+/issues/[\d]+)|(?:[\w]+-[\d]+)|(?:UNSUPPORTED-INTENDED:\s*.+)" > %t

// RUN: grep -Pv "UNSUPPORTED-TRACKER:\s+(?:https://github.com/[\w\d-]+/[\w\d-]+/issues/[\d]+)|(?:[\w]+-[\d]+)|(?:UNSUPPORTED-INTENDED:\s\w.*)" > %t

to require nice formatting (i.e. exactly one spacing symbol after -INDENDED:

to require at least single alphanumeric symbol as an explanation. Not that it would prevent all possible malformed explanations, but that's at least something.

But that is mostly a minor comment, I believe that formatting and reasoning will be up to reviewers anyways.

AlexeySachkov · 2024-10-28T13:06:27Z

sycl/test/e2e_test_requirements/no-unsupported-without-info.cpp

+// - ...and check if the list of improperly UNSUPPORTED tests needs to be updated.
+//
+// RUN: grep -rI "UNSUPPORTED:" %S/../../test-e2e \
+// RUN: -A 1 --include=*.c --include=*.cpp --no-group-separator | \


.c include is a certain legacy from XFAIL test: I'm not entirely sure if we ever should have any .c tests. SYCL is C++17

That's minor, we can address it in a follow-up PR for both XFAIL and UNSUPPORTED

Looks like there are no .c tests in test-e2e at all.

Looks like there are no .c tests in test-e2e at all.

Yeah, there shouldn't be. I probably followed suffixes which we have listed in lit.cfg.py. In any case I don't think that it is possible to use -fsycl in C mode, or use SYCL headers in .c mode, so I think that we can do a separate cleanup everywhere to explicitly say that .c is not considered as a test for us and drop it from XFAIL/UNSUPPORTED checks

AlexeySachkov · 2024-10-28T13:24:54Z

The update to the PR is very small (one more test was recorded as improperly unsupported), @KornevNikita verified it locally and I don't think that it is reasonable to wait for the whole pre-commit and risk the new test being broken again by some other PRs going in. Therefore, I plan to cancel the remaining of the pre-commit and proceed with the merge.

@KornevNikita, please monitor sycl branch state after the merge (i.e. post-commit) to quickly send any follow-up PRs if we happen to have conflicts with other PRs merged whilst I was writing this comment

AlexeySachkov · 2024-10-28T13:27:13Z

Also tagging @intel/llvm-gatekeepers here for awareness: any PR which deals with UNSUPPORTED directives and had passed pre-commit before this change was merged have to be re-tested, or otherwise we will see failures in both post and pre-commits.

Those failures will be easy to address, but still pinging you explicitly for awareness.

sarnex · 2024-10-28T14:24:30Z

sycl/test/e2e_test_requirements/no-unsupported-without-info.cpp

+// - in the result, search for "UNSUPPORTED" again, but invert the result - this
+//   allows us to get the line *after* UNSUPPORTED
+// - in those lines, check that UNSUPPORTED-TRACKER or UNSUPPORTED-INTENDED is
+//   present and correct. Once again, invert the search to get all "bad" lines


i didnt actually review the code but i can say it looks correct to run in check-sycl, thanks!

ayylol · 2024-10-29T16:37:03Z

Hey, just wanted to ask if there are plans to deal with other ways that tests are currently marked as unsupported not directly through the unsupported keyword. The two cases I am aware of would be, requiring a feature that doesn't exist (in the cases i've seen TEMPORARY_DISABLED), or misspelling RUN: (as RUNx: in the examples i've seen) so that certain lines are not picked up.

Some examples:

Basic/fpga_tests/fpga_io_pipes.cpp This test compiles but never runs because of RUNx:
SubGroup/generic_reduce.cpp This test uses REQUIRES: TEMPORARY_DISABLED

sarnex · 2024-10-29T16:39:29Z

IMO we should fix all of those to use UNSUPPORTED and if it's possible add a test to verify anything unsupported is using UNSUPPORTED

KornevNikita · 2024-10-29T16:58:55Z

Hey, just wanted to ask if there are plans to deal with other ways that tests are currently marked as unsupported not directly through the unsupported keyword. The two cases I am aware of would be, requiring a feature that doesn't exist (in the cases i've seen TEMPORARY_DISABLED), or misspelling RUN: (as RUNx: in the examples i've seen) so that certain lines are not picked up.

Some examples:

Basic/fpga_tests/fpga_io_pipes.cpp This test compiles but never runs because of RUNx:

SubGroup/generic_reduce.cpp This test uses REQUIRES: TEMPORARY_DISABLED

RIght, we plan to update TEMPORARY_DISABLED tests to use UNSUPPORTED. But currently UNSUPPORTED: * results in failed test, so need to update this behavior first.

bader · 2024-10-29T17:16:37Z

Basic/fpga_tests/fpga_io_pipes.cpp This test compiles but never runs because of RUNx:

I suggest removing the RUNx: command in this specific test.

KornevNikita requested review from AlexeySachkov and steffenlarsen October 24, 2024 12:26

KornevNikita requested a review from a team as a code owner October 24, 2024 12:26

KornevNikita temporarily deployed to WindowsCILock October 24, 2024 12:26 — with GitHub Actions Inactive

steffenlarsen reviewed Oct 24, 2024

View reviewed changes

sycl/test-e2e/Basic/kernel_max_wg_size.cpp Outdated Show resolved Hide resolved

KornevNikita temporarily deployed to WindowsCILock October 24, 2024 12:50 — with GitHub Actions Inactive

KornevNikita added 2 commits October 24, 2024 06:53

format

850ca83

apply suggestion

7974dbf

KornevNikita temporarily deployed to WindowsCILock October 24, 2024 14:43 — with GitHub Actions Inactive

KornevNikita temporarily deployed to WindowsCILock October 24, 2024 16:12 — with GitHub Actions Inactive

KornevNikita added 3 commits October 25, 2024 00:59

do not use pipe

bc45fad

Merge remote-tracking branch 'origin/sycl' into no-unsupported

85ca5f2

move test to another dir

55808c3

KornevNikita temporarily deployed to WindowsCILock October 25, 2024 14:26 — with GitHub Actions Inactive

KornevNikita had a problem deploying to WindowsCILock October 25, 2024 15:13 — with GitHub Actions Error

fix path

21bdd4e

KornevNikita temporarily deployed to WindowsCILock October 25, 2024 15:57 — with GitHub Actions Inactive

KornevNikita temporarily deployed to WindowsCILock October 25, 2024 17:17 — with GitHub Actions Inactive

KornevNikita added 2 commits October 25, 2024 23:17

Merge remote-tracking branch 'origin/sycl' into no-unsupported

1b562eb

update list

886f9a2

KornevNikita temporarily deployed to WindowsCILock October 26, 2024 06:43 — with GitHub Actions Inactive

KornevNikita temporarily deployed to WindowsCILock October 26, 2024 07:08 — with GitHub Actions Inactive

steffenlarsen approved these changes Oct 28, 2024

View reviewed changes

Merge remote-tracking branch 'origin/sycl' into no-unsupported

799fa9f

dm-vodopyanov reviewed Oct 28, 2024

View reviewed changes

sycl/test-e2e/README.md Show resolved Hide resolved

AlexeySachkov approved these changes Oct 28, 2024

View reviewed changes

another one number update

376ac28

KornevNikita had a problem deploying to WindowsCILock October 28, 2024 13:17 — with GitHub Actions Error

AlexeySachkov merged commit e427e2f into intel:sycl Oct 28, 2024
3 of 10 checks passed

sarnex reviewed Oct 28, 2024

View reviewed changes

	// RUN: grep -Pv "UNSUPPORTED-TRACKER:\s+(?:https://github.com/[\w\d-]+/[\w\d-]+/issues/[\d]+)\|(?:[\w]+-[\d]+)\|(?:UNSUPPORTED-INTENDED:\s*.+)" > %t
	// RUN: grep -Pv "UNSUPPORTED-TRACKER:\s+(?:https://github.com/[\w\d-]+/[\w\d-]+/issues/[\d]+)\|(?:[\w]+-[\d]+)\|(?:UNSUPPORTED-INTENDED:\s\w.*)" > %t

[SYCL][E2E] Add a test to track UNSUPPORTED tests #15849

[SYCL][E2E] Add a test to track UNSUPPORTED tests #15849

Uh oh!

Conversation

KornevNikita commented Oct 24, 2024

Uh oh!

KornevNikita commented Oct 24, 2024

Uh oh!

Uh oh!

KornevNikita commented Oct 28, 2024

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexeySachkov left a comment

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

KornevNikita Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov commented Oct 28, 2024

Uh oh!

Uh oh!

AlexeySachkov commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sarnex Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

ayylol commented Oct 29, 2024

Uh oh!

sarnex commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KornevNikita commented Oct 29, 2024

Uh oh!

bader commented Oct 29, 2024

Uh oh!

Uh oh!

AlexeySachkov commented Oct 28, 2024 •

edited

Loading

sarnex commented Oct 29, 2024 •

edited

Loading