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][DOC] Add sycl_ext_oneapi_user_defined_reductions specification #7202

Conversation

AlexeySachkov
Copy link
Contributor

No description provided.

Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

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

Thanks for writing this up. I have a few small suggestions.

One question I had that doesn't really fit anywhere is whether we want to tie this to reductions. It occurred to me while reviewing that we could extend the scan functions in exactly the same way. I don't think we have to add those functions now, but I'm wondering if we should call this something like user_defined_group_algorithms so we can add more algorithms later, or if we should introduce separate extensions like user_defined_scans.

@AlexeySachkov
Copy link
Contributor Author

One question I had that doesn't really fit anywhere is whether we want to tie this to reductions. It occurred to me while reviewing that we could extend the scan functions in exactly the same way. I don't think we have to add those functions now, but I'm wondering if we should call this something like user_defined_group_algorithms so we can add more algorithms later, or if we should introduce separate extensions like user_defined_scans.

I think that depends on whether or not we will be able to group all functions which could be extended like this under a single umbrella name and whether or not we want to allow implementations to support only some of them but not all.

Let's maybe get more opinions on the matter, tagging @gmlueck, @rolandschulz

@AlexeySachkov AlexeySachkov changed the title Add first draft of sycl_ext_intel_user_defined_reductions [SYCL][DOC] Add sycl_ext_oneapi_user_defined_reductions specification Oct 28, 2022
@dm-vodopyanov dm-vodopyanov marked this pull request as ready for review November 7, 2022 12:53
@dm-vodopyanov dm-vodopyanov requested a review from a team as a code owner November 7, 2022 12:53
@dm-vodopyanov
Copy link
Contributor

One question I had that doesn't really fit anywhere is whether we want to tie this to reductions. It occurred to me while reviewing that we could extend the scan functions in exactly the same way. I don't think we have to add those functions now, but I'm wondering if we should call this something like user_defined_group_algorithms so we can add more algorithms later, or if we should introduce separate extensions like user_defined_scans.

I think that depends on whether or not we will be able to group all functions which could be extended like this under a single umbrella name and whether or not we want to allow implementations to support only some of them but not all.

Let's maybe get more opinions on the matter, tagging @gmlueck, @rolandschulz

@gmlueck, @rolandschulz, could you please comment on this?

@gmlueck
Copy link
Contributor

gmlueck commented Nov 7, 2022

One question I had that doesn't really fit anywhere is whether we want to tie this to reductions. It occurred to me while reviewing that we could extend the scan functions in exactly the same way. I don't think we have to add those functions now, but I'm wondering if we should call this something like user_defined_group_algorithms so we can add more algorithms later, or if we should introduce separate extensions like user_defined_scans.

I think that depends on whether or not we will be able to group all functions which could be extended like this under a single umbrella name and whether or not we want to allow implementations to support only some of them but not all.

Let's maybe get more opinions on the matter, tagging @gmlueck, @rolandschulz

I think we want to have a single extension that covers both reduce and scan. I presume our goal is to get this adopted into SYCL-Next, and I don't think the Khronos WG will adopt user-define functions for reduce without also adopting them for scan. It would seem make the API very inconsistent to do so.

dm-vodopyanov added a commit to dm-vodopyanov/llvm that referenced this pull request Nov 17, 2022
dm-vodopyanov added a commit to dm-vodopyanov/llvm-test-suite that referenced this pull request Nov 17, 2022
dm-vodopyanov added a commit to dm-vodopyanov/llvm-test-suite that referenced this pull request Nov 17, 2022
@dm-vodopyanov
Copy link
Contributor

@gmlueck and @Pennycook - could you please review again?

@dm-vodopyanov dm-vodopyanov merged commit cd4fd8c into intel:sycl Dec 13, 2022
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
@AlexeySachkov AlexeySachkov deleted the private/asachkov/user-defined-reductions-spec branch May 22, 2024 09:51
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