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] change sycl version definition to 202012 #15890

Open
wants to merge 5 commits into
base: sycl
Choose a base branch
from

Conversation

dklochkov-emb
Copy link
Contributor

Sycl language version macro is changed from 202001 to 202012L

@@ -141,7 +141,7 @@ using cl_double = double;
} // namespace opencl

// Vector aliases are different between SYCL 1.2.1 and SYCL 2020
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not support 1.2.1 mode anymore. I think that we could use the opportunity and just drop them while we are here

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets do it in a separate pull request

@@ -57,7 +57,7 @@
#endif // __SYCL_DEPRECATED

#ifndef __SYCL2020_DEPRECATED
#if SYCL_LANGUAGE_VERSION >= 202001 && \
#if SYCL_LANGUAGE_VERSION >= 202012L && \
Copy link
Contributor

@AlexeySachkov AlexeySachkov Oct 28, 2024

Choose a reason for hiding this comment

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

Considering that there will be a single SYCL_LANGUAGE_VERSION per SYCL specification, I'm not sure if >= is a correct comparison going forward.

If something is only deprecated in SYCL 2020, then we should check for equality. SYCL-Next is not that far away, so I suggest that we prepare for that and use == right away. >= had more sense before the recent spec change which prompted this PR, because there could be several SYCL_LANGUAGE_VERSION values matching SYCL 2020, but it is not the case anymore.

Before any changes are made, I would like to hear feedback from @intel/llvm-reviewers-runtime as well

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@@ -68,7 +68,7 @@
// 3300+ lines of code

// SYCL_LANGUAGE_VERSION is 4 digit year followed by 2 digit revision
#if !SYCL_LANGUAGE_VERSION || SYCL_LANGUAGE_VERSION < 202001
#if !SYCL_LANGUAGE_VERSION || SYCL_LANGUAGE_VERSION < 202012L
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comment here, if we had any code paths which are specific to SYCL 1.2.1, we should probably use this opportunity to completely drop them, because we do not support SYCL 1.2.1 anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets do it in a separate pull request

@bader
Copy link
Contributor

bader commented Oct 28, 2024

SYCL_LANGUAGE_VERSION support has been upstreamed already, so we should land this patch to https://github.com/llvm/llvm-project/.

@elizabethandrews
Copy link
Contributor

SYCL_LANGUAGE_VERSION support has been upstreamed already, so we should land this patch to https://github.com/llvm/llvm-project/.

Upstream code seems to be quite updated. It has code to define CL_SYCL_LANGUAGE_VERSION, etc. It would be nice to clean that up. The code in syclos and upstream aren't an exact match though. We don't have getSYCLVersion and I don't think we can add it there since we have different code actually calling this yet.

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.

5 participants