Skip to content

[SYCL][Doc] Cluster Group Extension Document #13594

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
Merged
merged 43 commits into from
Aug 1, 2024
Merged

[SYCL][Doc] Cluster Group Extension Document #13594

merged 43 commits into from
Aug 1, 2024

Conversation

AD2605
Copy link
Contributor

@AD2605 AD2605 commented Apr 30, 2024

Initial public working draft for thread block cluster support in SYCL, intended to get feedback.

Contains the proposal for -

  1. Launching a kernel with cluster group
  2. Accessing the various ids associated with the cluster_group from the kernel
  3. Cluster level barrier
  4. Accessing another workgroup's local memory

@AD2605 AD2605 requested a review from a team as a code owner April 30, 2024 06:06
@AD2605 AD2605 marked this pull request as draft April 30, 2024 06:07
Ruyk
Ruyk suggested changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@Ruyk Ruyk 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 the proposal, please also add an example (or two) of how it would be used

sycl/doc/extensions/proposed/sycl_ext_oneapi_work_group_cluster.asciidoc Outdated

|`size_t get_local_linear_range(void)`
|Returns the total number of `workItems` within the cluster
|===
Copy link
Contributor

Choose a reason for hiding this comment

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

all the above methods should be const right?

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 for highlighting this, fixed

@jbrodman
Copy link
Contributor

Note: This direction is not approved. Do not merge this as the APIs and abstractions are not final.

AerialMantis
AerialMantis reviewed Apr 30, 2024
View reviewed changes
Copy link
Contributor

@AerialMantis AerialMantis 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 putting it together, overall this looks really good, I've made a couple of design suggestions.

I would also suggest adding a synopsis which shows the whole cluster_group class, including namespaces, this helps to see everything together, and it would be good to have an example of how this would be used, this helps people understand how everything fits together.

@JackAKirk JackAKirk mentioned this pull request Apr 30, 2024
Merged
@AD2605 AD2605 requested review from AerialMantis and Ruyk May 9, 2024 11:05
@AD2605
Copy link
Contributor Author

AD2605 commented May 9, 2024

Hi all, thanks for the initial review, I have addressed the comments (except one regarding copyright notice, I am yet to add that) and this now ready for the re-review

CC: @jbrodman

gmlueck
gmlueck reviewed May 21, 2024
View reviewed changes
@AD2605 AD2605 mentioned this pull request Jun 10, 2024
Merged
@AD2605 AD2605 marked this pull request as ready for review June 20, 2024 14:45
EwanC
EwanC reviewed Jun 26, 2024
View reviewed changes
sycl/doc/extensions/proposed/sycl_ext_oneapi_cuda_cluster_group.asciidoc Outdated

|`cluster_group<Dimensions> nd_item::ext_oneapi_cuda_get_cluster_group()`
|Returns the constituent `cluster_group` in the kernel, representing this
`cluster_group`'s overall position in the `nd_range`
Copy link
Contributor

Choose a reason for hiding this comment

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

This table renders slightly strangely
image

Might want this as the column headers in bold, also the monospace isn't quite right.

|Method
|Description

This should also be added to a couple other tables in this doc.

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 should be better now, and removed the accidental "boldness" of the phrase.

Added "method, description" in other places too

AerialMantis
AerialMantis reviewed Jun 26, 2024
View reviewed changes
gmlueck
gmlueck reviewed Jul 25, 2024
View reviewed changes
gmlueck
gmlueck reviewed Jul 26, 2024
View reviewed changes
AerialMantis
AerialMantis reviewed Jul 26, 2024
View reviewed changes
AerialMantis
AerialMantis reviewed Jul 26, 2024
View reviewed changes
AerialMantis
AerialMantis approved these changes Jul 30, 2024
View reviewed changes
Copy link
Contributor

@AerialMantis AerialMantis left a comment

Choose a reason for hiding this comment

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

Looks good!

@AD2605
Copy link
Contributor Author

AD2605 commented Jul 30, 2024

@intel/llvm-gatekeepers This is ready to Merge. Thanks

Ruyk
Ruyk suggested changes Aug 1, 2024
View reviewed changes
Copy link
Contributor

@Ruyk Ruyk left a comment

Choose a reason for hiding this comment

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

Minor comment about (C)

@Ruyk
Copy link
Contributor

Ruyk commented Aug 1, 2024

LGTM

@sommerlukas sommerlukas changed the title [SYCL] Cluster Group Extension Document [SYCL][Doc] Cluster Group Extension Document Aug 1, 2024
@sommerlukas sommerlukas merged commit 20bcfea into intel:sycl Aug 1, 2024
2 checks passed
AlexeySachkov pushed a commit to AlexeySachkov/llvm that referenced this pull request Nov 26, 2024
@AD2605 @gmlueck
@AerialMantis @Pennycook @Ruyk @AlexeySachkov
[SYCL][Doc] Cluster Group Extension Document (intel#13594)
a84c06c 
Initial public working draft for thread block cluster support in SYCL,
intended to get feedback.

Contains the proposal for - 
1. Launching a kernel with cluster group
2. Accessing the various `ids` associated with the cluster_group from
the kernel
3. Cluster level barrier
4. Accessing another workgroup's local memory

---------

Co-authored-by: Greg Lueck <[email protected]>
Co-authored-by: Gordon Brown <[email protected]>
Co-authored-by: John Pennycook <[email protected]>
Co-authored-by: Ruyman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pennycook Pennycook Pennycook left review comments

@EwanC EwanC EwanC left review comments

@gmlueck gmlueck gmlueck approved these changes

@AerialMantis AerialMantis AerialMantis approved these changes

@JackAKirk JackAKirk JackAKirk left review comments

@Ruyk Ruyk Ruyk requested changes

@rolandschulz rolandschulz rolandschulz approved these changes

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

Successfully merging this pull request may close these issues.
10 participants
@AD2605 @jbrodman @gmlueck @Ruyk @rolandschulz @Pennycook @EwanC @AerialMantis @JackAKirk @sommerlukas