Skip to content

[SYCL] Add non-standard vec aliases back to fix SYCL-CTS #7925

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

Conversation

AlexeySachkov
Copy link
Contributor

SYCL-CTS are still using some aliases which were removed from SYCL 2020 specification, see KhronosGroup/SYCL-CTS#446. Those tests are part of our post-commit and to make it green, this commit temporary adds those aliases back.

Note that it is not entierly clear if those aliases should have been removed in the first place, see KhronosGroup/SYCL-Docs#335.

SYCL-CTS are still using some aliases which were removed from SYCL 2020
specification, see KhronosGroup/SYCL-CTS#446. Those tests are part of
our post-commit and to make it green, this commit temporary adds those
aliases back.

Note that it is not entierly clear if those aliases should have been
removed in the first place, see KhronosGroup/SYCL-Docs#335.
@AlexeySachkov AlexeySachkov requested a review from a team as a code owner January 5, 2023 09:24
@AlexeySachkov AlexeySachkov temporarily deployed to aws January 5, 2023 09:49 — with GitHub Actions Inactive
@AlexeySachkov AlexeySachkov temporarily deployed to aws January 5, 2023 10:19 — with GitHub Actions Inactive
@bader
Copy link
Contributor

bader commented Jan 5, 2023

@cperkinsintel, @intel/llvm-reviewers-runtime, please, review ASAP. We need this patch to fix post-commit validation.

@aelovikov-intel
Copy link
Contributor

IMO, it would be better to revert KhronosGroup/SYCL-CTS#440 until the spec issue is resolved. Until that, the CTS is technically in contradiction with the spec and hence is wrong.

I am only basing that on the links provided in this PR's description and might be missing some bigger picture.

@bader
Copy link
Contributor

bader commented Jan 5, 2023

KhronosGroup/SYCL-CTS#440 aligns CTS with the spec, so there is no contradiction and reasons for reverting this PR. We need other patches removing data types intentionally removed from the spec and fixes in the spec and CTS to add unintentionally removed data types. All these changes should go before DPC++ changes to avoid post-commit CI failures.

As alternative option we can disable broken tests in our CI, but I don't think this is preferred way.

@aelovikov-intel
Copy link
Contributor

I don't know where I got that link, please ignore my comment.

Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

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

Discussed with Alexey offline. I agree that it's better to revert the change causing the regression in post-commit until the CTS and/or spec issue is addressed.

@bader bader merged commit 18f2cda into intel:sycl Jan 6, 2023
@AlexeySachkov AlexeySachkov deleted the private/asachkov/temporary-fix-for-cts branch March 29, 2023 12:25
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.

3 participants