Skip to content
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

[SYCL] Deprecate current implementations of get_backend_info() #16700

Merged
merged 3 commits into from
Jan 23, 2025

Conversation

HPS-1
Copy link
Contributor

@HPS-1 HPS-1 commented Jan 21, 2025

The get_backend_info() functions implemented in #12906 are returning some info descriptors that are not backend-specific info descriptors but SYCL core info descriptors. This leads to problems as described in #16272 . Therefore, its current implementations are being deprecated, and removed under the __INTEL_PREVIEW_BREAKING_CHANGES flag.

@HPS-1 HPS-1 marked this pull request as ready for review January 21, 2025 02:58
@HPS-1 HPS-1 requested a review from a team as a code owner January 21, 2025 02:58
@HPS-1 HPS-1 requested a review from steffenlarsen January 21, 2025 02:58
@HPS-1
Copy link
Contributor Author

HPS-1 commented Jan 23, 2025

Requesting @intel/llvm-gatekeepers to merge this PR. Thanks!

@sarnex sarnex merged commit c6e45cf into intel:sycl Jan 23, 2025
17 checks passed
@aelovikov-intel
Copy link
Contributor

SYCL :: Basic/info.cpp seems to be failing in post-commit on Windows after this. @HPS-1 , can you take a look please?

@HPS-1
Copy link
Contributor Author

HPS-1 commented Jan 23, 2025

SYCL :: Basic/info.cpp seems to be failing in post-commit on Windows after this. @HPS-1 , can you take a look please?

@aelovikov-intel Sure, can you send me a link to the failed test run? Thanks!

@aelovikov-intel
Copy link
Contributor

SYCL :: Basic/info.cpp seems to be failing in post-commit on Windows after this. @HPS-1 , can you take a look please?

@aelovikov-intel Sure, can you send me a link to the failed test run? Thanks!

https://github.com/intel/llvm/actions/runs/12933016036/job/36074301399

@HPS-1
Copy link
Contributor Author

HPS-1 commented Jan 23, 2025

SYCL :: Basic/info.cpp seems to be failing in post-commit on Windows after this. @HPS-1 , can you take a look please?

@aelovikov-intel Sure, can you send me a link to the failed test run? Thanks!

https://github.com/intel/llvm/actions/runs/12933016036/job/36074301399

@aelovikov-intel I've inspected that issue and I believe it's irrelevant to my changes? I mean there're two points:

  1. I didn't change this test, also as far as I can tell it doesn't use any of the code pieces I'm deprecating/removing under the flag __INTEL_PREVIEW_BREAKING_CHANGES;
  2. If you look at the error it says: # | Assertion failed: backend == sycl::backend::opencl && "An exception is expected for non OpenCL backend", file D:\github\_work\llvm\llvm\llvm\sycl\test-e2e\Basic\info.cpp, line 321 (Link here: https://github.com/intel/llvm/actions/runs/12933016036/job/36074301399#step:14:627) And this line is actually added in commit 4e5a72e, not my commits. So I guess it's a problem about that PR [SYCL] Align execution_capability info with spec #16673 instead?

@aelovikov-intel
Copy link
Contributor

aelovikov-intel commented Jan 23, 2025

@HPS-1
Copy link
Contributor Author

HPS-1 commented Jan 23, 2025

This is weird, https://github.com/intel/llvm/actions/workflows/sycl-post-commit.yml clearly shows the pass->fail change between https://github.com/intel/llvm/actions/runs/12932360753 and https://github.com/intel/llvm/actions/runs/12933016036.

Tagging @KseniyaTikhomirova as well.

@aelovikov-intel I believe there is a delay between the start of the whole post-commit run and the start of the e2e-win run. So my assumption is that, if some code is pushed during this delay window, it may be actually used in the e2e-win run and cause such weird behavior?

@aelovikov-intel
Copy link
Contributor

aelovikov-intel commented Jan 23, 2025

AFAIK, we lock in exact commit hashes, unless that was silently dropped recently. Hmm, #16560 might have done just that. @KornevNikita , any ideas if that's true?

@aelovikov-intel
Copy link
Contributor

AFAIK, we lock in exact commit hashes, unless that was silently dropped recently. Hmm, #16560 might have done just that. @KornevNikita , any ideas if that's true?

I've uploaded #16757 to fix the race condition. @KornevNikita, I'm surprised why you needed this in the first place...

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