Skip to content

[SYCL] Provide SYCL 2020 function objects #3868

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
merged 15 commits into from
Jun 8, 2021
Merged

Conversation

maximdimakov
Copy link
Contributor

@maximdimakov maximdimakov commented Jun 2, 2021

Provided sycl function objects accordingly to sycl 2020 specification
The test for function objects in test_suite - intel/llvm-test-suite#310
Signed-off-by: mdimakov maxim.dimakov@intel.com

Signed-off-by: mdimakov <maxim.dimakov@intel.com>
@maximdimakov maximdimakov requested a review from a team as a code owner June 2, 2021 13:56
@Pennycook
Copy link
Contributor

Pennycook commented Jun 3, 2021

I don't think we should be importing extensions into the sycl:: namespace via aliases. There are several (subtle) ways in which a SYCL 2020 feature may differ from its ONEAPI equivalent, and we need to be very careful.

Also, I think the specific algorithm you're adding here was already added as part of #3786.

@maximdimakov
Copy link
Contributor Author

sycl::reduce_over_group was merged yesterday. The only sycl::plus is remained

@maximdimakov maximdimakov changed the title [SYCL] Provided alias to ONEAPI::reduce and sycl::plus [SYCL] Provided sycl::plus Jun 3, 2021
@sergey-semenov
Copy link
Contributor

There are several other function objects that are in SYCL 2020 but are currently provided only in ONEAPI namespace just like plus: multiplies, bit_and, etc. I think it makes sense to handle them all in a single patch since the changes are quite minor, but I don't mind if those are going to be added separately.

Minor comment: I think it makes more sense to provide sycl::plus in another file in the sycl directory rather than sycl/ONEAPI/.

@maximdimakov
Copy link
Contributor Author

@sergey-semenov I added functional objects accordingly to 4.17.2 point of spec. Is stl.hpp suitable for these function objects?

@maximdimakov maximdimakov changed the title [SYCL] Provided sycl::plus [SYCL] Provided sycl 2020 function objects Jun 3, 2021
Copy link
Contributor

@sergey-semenov sergey-semenov left a comment

Choose a reason for hiding this comment

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

Is stl.hpp suitable for these function objects?

I think creating sycl/functional.hpp for these would be more appropriate.

@maximdimakov
Copy link
Contributor Author

The test in test_suite - intel/llvm-test-suite#310

Copy link
Contributor

@sergey-semenov sergey-semenov left a comment

Choose a reason for hiding this comment

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

LGTM overall

sergey-semenov
sergey-semenov previously approved these changes Jun 7, 2021
@bader bader changed the title [SYCL] Provided sycl 2020 function objects [SYCL] Provide SYCL 2020 function objects Jun 7, 2021
@bader bader requested review from sergey-semenov and Pennycook June 7, 2021 11:10
sergey-semenov
sergey-semenov previously approved these changes Jun 7, 2021
@bader bader merged commit 24a2ad8 into intel:sycl Jun 8, 2021
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