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.

Sign up for GitHub

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

Jump to bottom

[SYCL] Implement backend content extension #16633

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

[SYCL] Implement backend content extension #16633

wants to merge 55 commits into from

Conversation

lbushi25
Copy link
Contributor

@lbushi25 lbushi25 commented Jan 14, 2025 •
edited
Loading

lbushi25
lbushi25 commented Jan 17, 2025
View reviewed changes
sycl/test/abi/sycl_symbols_windows.dump Outdated
Copy link
Contributor Author

@lbushi25 lbushi25 Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Most of the changes here are unrelated to the symbols I added. I suppose sometime in the past someone has modified this file manually which caused some of the symbols to get out of order.

cperkinsintel
cperkinsintel reviewed Jan 24, 2025
View reviewed changes
sycl/doc/extensions/experimental/sycl_ext_oneapi_device_image_backend_content.asciidoc
@@ -101,7 +99,7 @@ class device_image {
backend ext_oneapi_get_backend() const noexcept;
std::vector<std::byte> ext_oneapi_get_backend_content() const;

std::span<std::byte> ext_oneapi_get_backend_content_view() const; // Requires C++20
std::span<const std::byte> ext_oneapi_get_backend_content_view() const; // Requires C++20
Copy link
Contributor

@cperkinsintel cperkinsintel Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I know the new extension spec says std::span and goes on to note that this requires C++20.

But it might be worth noting that we do have sycl::span which is available even if not using C++20.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Chris!
@gmlueck FYI

Copy link
Contributor

Choose a reason for hiding this comment

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

sycl::span and std::span are different types. Therefore, we would need two separate functions: one that returns std::span and another that returns sycl::span. I considered doing this, but it did not seem worth the effort and additional maintenance cost. If an application wants the more efficient API, I think it's reasonable to ask them to compile in C++20.

@gmlueck gmlueck mentioned this pull request Jan 27, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexeySachkov AlexeySachkov AlexeySachkov left review comments

@gmlueck gmlueck gmlueck approved these changes

@cperkinsintel cperkinsintel cperkinsintel left review comments

@Pennycook Pennycook Pennycook approved these changes

@sergey-semenov sergey-semenov Awaiting requested review from sergey-semenov sergey-semenov is a code owner automatically assigned from intel/llvm-reviewers-runtime

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lbushi25 @Pennycook @cperkinsintel @gmlueck @AlexeySachkov