Skip to content

[SYCL] Implement SYCL 2020 vec aliases #7889

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

Conversation

AlexeySachkov
Copy link
Contributor

This commit added aliases for sycl::vec class as described in section 4.14.2.2. Aliases of SYCL 2020 specification, revision 6.

Aliases which use OpenCL type, such as cl_int2 are still preserved, because they are used internally, see #7888.

This commit added aliases for `sycl::vec` class as described in section
4.14.2.2. Aliases of SYCL 2020 specification, revision 6.

Aliases which use OpenCL type, such as `cl_int2` are still preserved,
because they are used internally, see intel#7888.
@AlexeySachkov AlexeySachkov requested a review from a team December 29, 2022 11:34
@AlexeySachkov AlexeySachkov requested a review from a team as a code owner December 29, 2022 11:34
@AlexeySachkov AlexeySachkov temporarily deployed to aws December 29, 2022 11:47 — with GitHub Actions Inactive
@AlexeySachkov AlexeySachkov temporarily deployed to aws December 29, 2022 14:15 — with GitHub Actions Inactive
@AlexeySachkov AlexeySachkov temporarily deployed to aws December 29, 2022 14:45 — with GitHub Actions Inactive
Copy link
Contributor

@dm-vodopyanov dm-vodopyanov left a comment

Choose a reason for hiding this comment

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

Impl LGTM

AlexeySachkov added a commit to AlexeySachkov/llvm-test-suite that referenced this pull request Dec 30, 2022
According to 4.14.2.2. Aliases of SYCL 2020 specification, revision 6,
`longlong{N}` aliases do not exists.

There is corresponding PR to remove them from the implementation:
intel/llvm#7889
@AlexeySachkov
Copy link
Contributor Author

/verify with intel/llvm-test-suite#1477

AlexeySachkov added a commit to intel/llvm-test-suite that referenced this pull request Dec 30, 2022
According to 4.14.2.2. Aliases of SYCL 2020 specification, revision 6,
`longlong{N}` aliases do not exists.

There is corresponding PR to remove them from the implementation:
intel/llvm#7889
@AlexeySachkov AlexeySachkov temporarily deployed to aws December 30, 2022 16:04 — with GitHub Actions Inactive
@bader bader temporarily deployed to aws December 30, 2022 23:58 — with GitHub Actions Inactive
@@ -73,6 +90,9 @@ using ulong = unsigned long;
using longlong = long long;
using ulonglong = unsigned long long;
using half = sycl::detail::half_impl::half;

// FIXME: SYCL 2020 spec says that cl_* aliases should reside in sycl::opencl
// namespace.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is blocking us from applying these FIXME comments right away?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently, we still use some of these aliases: #7888. I don't know exactly about cl_* scalar aliases, but we for sure use cl_* vector aliases.

I would prefer an incremental approach and get this merged first as a fix for vector aliases and then submit a separate patch for scalar aliases

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 took another look at this: vector cl_* aliases depend on scalar cl_* aliases and those in turn are still used (#7888)

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently, we still use some of these aliases: #7888. I don't know exactly about cl_* scalar aliases, but we for sure use cl_* vector aliases.

I would prefer an incremental approach and get this merged first as a fix for vector aliases and then submit a separate patch for scalar aliases

I also prefer incremental approach, which should be remove cl_* alias usages from DPC++ code base first and remove alias definitions after that. This way there will be no technical debt.

@bader bader temporarily deployed to aws December 31, 2022 00:28 — with GitHub Actions Inactive
@AlexeySachkov AlexeySachkov merged commit 1299e21 into intel:sycl Dec 31, 2022
AlexeySachkov added a commit to AlexeySachkov/llvm that referenced this pull request Jan 2, 2023
This is a follow-up from intel#7889. Adjusted LIT tests to acount
for the new definition of long{n} alias, which may not always be equal
to sycl::vec<long, n> now.
@AlexeySachkov
Copy link
Contributor Author

Post-commit failures are being addressed in #7896

@AlexeySachkov
Copy link
Contributor Author

I've just realized that some of the failures are SYCL-CTS and with the current spec wording, that is a bug in CTS and not in the implementation. Submitted KhronosGroup/SYCL-CTS#446. We should probably temporarily disable some CTS tests to have green post-commit results

AlexeySachkov added a commit that referenced this pull request Jan 3, 2023
This is a follow-up from #7889. Adjusted LIT tests to account
for the new definition of `long{n}` alias, which may not always be equal
to `sycl::vec<long, n>` now. Removed some dead code (type traits) from the
runtime.
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
…suite#1477)

According to 4.14.2.2. Aliases of SYCL 2020 specification, revision 6,
`longlong{N}` aliases do not exists.

There is corresponding PR to remove them from the implementation:
intel#7889
@AlexeySachkov AlexeySachkov deleted the private/asachkov/implement-sycl-2020-vec-aliases 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