Skip to content

[SYCL][E2E] Update some incompatible requirements #18007

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 8, 2025

Conversation

KornevNikita
Copy link
Contributor

@KornevNikita KornevNikita commented Apr 14, 2025

These test are never launched due to the REQUIRES set.

There test are neved launched due to the REQUIRES set.
Update these sets and remove accelerator-related parts if needed,
otherwise some test may fail due to lack of device.
@KornevNikita KornevNikita marked this pull request as ready for review April 15, 2025 14:56
@KornevNikita KornevNikita requested review from a team as code owners April 15, 2025 14:56
@KornevNikita KornevNikita requested a review from slawekptak April 15, 2025 14:56
Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit.

@KornevNikita KornevNikita requested a review from a team as a code owner May 6, 2025 11:41
@@ -1,4 +1,4 @@
// REQUIRES: linux, cpu && (gpu && level_zero)
// REQUIRES: linux, cpu || (gpu && level_zero)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to other tests in this dir - I believe it's just a typo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is not a typo, but maybe I used wrong pattern.
I want this test can be tested on machines which have both CPU and GPU, as you can see

sycl::queue gpu_queue(sycl::gpu_selector_v);
sycl::queue cpu_queue(sycl::cpu_selector_v);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I got it, I'll update the REQUIRES string

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated this string - 7163eb7
Anyways, looks like this test wasn't launched in pre-commit on gen12 - it's in the list of unsupported tests.
@intel/dpcpp-devops-reviewers could you help me to figure out if it's true?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, definitely looks like it didn't run. i dont think cpu and gpu will ever both be true, i think we need any-device-is-cpu && any-device-is-gpu

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sarnex take a look at this test - https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/AddressSanitizer/aot/cpu.cpp
the REQUIRES there is even simpler and it also uses run-aux, but it's also in the list of unsupported tests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok yeah good find, let me actually debug it, thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol okay, then I think we can leave the test as is. it should work when the issue with gen12 is fixed.
@intel/dpcpp-sanitizers-review take a look please.

Copy link
Contributor

@AllanZyne AllanZyne May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for correcting the require pattern.
Yep, we haven't fixed the GEN12 issue yet, because it's low priority.
We will take a look at GEN12 in the future.

@KornevNikita
Copy link
Contributor Author

I've deleted selector-tests-related changes, as these tests require refactoring / converting to unit tests. @maarquitos14 could you please take one more look just in case.

@KornevNikita
Copy link
Contributor Author

Unrelated failure

@KornevNikita
Copy link
Contributor Author

failures:
SpecConstants/2020/nested-non-packed-struct.cpp - #17650
ThreadSanitizer/check_device_global.cpp - #18349
gen12 timed out, but it passed with the previous commit (the last one is just a merge from sycl)

@intel/llvm-gatekeepers could you please merge?

@sarnex sarnex merged commit aed8b0b into intel:sycl May 8, 2025
17 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants