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] Optional kernel features: implement split based on reqd-sub-group-size (take 2) #9928

Merged
merged 20 commits into from
Jun 27, 2023
Merged

[SYCL] Optional kernel features: implement split based on reqd-sub-group-size (take 2) #9928

merged 20 commits into from
Jun 27, 2023

Conversation

jzc
Copy link
Contributor

@jzc jzc commented Jun 15, 2023

Based off #8167

@jzc jzc force-pushed the split-reqd-sub-group-size branch from 2ae117f to 5d1fd71 Compare June 15, 2023 22:28
@jzc jzc force-pushed the split-reqd-sub-group-size branch from 5d1fd71 to 710fd21 Compare June 15, 2023 22:43
@jzc jzc temporarily deployed to aws June 15, 2023 23:11 — with GitHub Actions Inactive
@jzc jzc temporarily deployed to aws June 15, 2023 23:50 — with GitHub Actions Inactive
@jzc jzc temporarily deployed to aws June 20, 2023 22:28 — with GitHub Actions Inactive
@jzc jzc temporarily deployed to aws June 20, 2023 23:09 — with GitHub Actions Inactive
@jzc jzc temporarily deployed to aws June 22, 2023 01:43 — with GitHub Actions Inactive
@jzc jzc temporarily deployed to aws June 22, 2023 02:24 — with GitHub Actions Inactive
AlexeySachkov
AlexeySachkov reviewed Jun 22, 2023
View reviewed changes
llvm/tools/sycl-post-link/SYCLDeviceRequirements.cpp Outdated Show resolved Hide resolved
llvm/test/tools/sycl-post-link/device-requirements/reqd-sub-group-size.ll Outdated Show resolved Hide resolved
sycl/source/detail/program_manager/program_manager.cpp Outdated
Comment on lines 630 to 633
// ReqdSubGroupSize != 1 is a WA for ESIMD, no backend
// currently includes 1 as a valid sub-group size.
// TODO: remove this WA when backends support this.
if (ReqdSubGroupSize != 1 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that in one of previous commits you also had a check for whether the device image is a ESIMD device image - should we return it back? Just completely ignoring 1 here technically violates the spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, we should - I removed it because I misunderstood why the workaround wasn't completely working in the first place, so I lightened the restriction to only check the reqd-sub-group-size for 1. Actually, I think checking if the image is an ESIMD image suffices.

@jzc jzc temporarily deployed to aws June 22, 2023 17:49 — with GitHub Actions Inactive
@jzc jzc temporarily deployed to aws June 22, 2023 18:27 — with GitHub Actions Inactive
@jzc jzc temporarily deployed to aws June 23, 2023 01:42 — with GitHub Actions Inactive
@jzc jzc temporarily deployed to aws June 23, 2023 02:20 — with GitHub Actions Inactive
jzc and others added 3 commits June 23, 2023 19:28
@jzc
Fix WA again
381a518
@jzc
M -> MD
91c0718
@jzc @AlexeySachkov
Apply suggestions from code review
3782e9b 
Co-authored-by: Alexey Sachkov <alexey.sachkov@intel.com>
@jzc jzc marked this pull request as ready for review June 23, 2023 19:41
@jzc jzc requested review from a team as code owners June 23, 2023 19:41
@jzc jzc requested a review from dm-vodopyanov June 23, 2023 19:41
@jzc jzc temporarily deployed to aws June 23, 2023 20:15 — with GitHub Actions Inactive
@jzc jzc temporarily deployed to aws June 23, 2023 20:53 — with GitHub Actions Inactive
AlexeySachkov
AlexeySachkov approved these changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

Overall looks good to me.
Tagging @intel/sycl-language-enabling-triage for awareness.

llvm/test/tools/sycl-post-link/sycl-esimd/lower-with-no-esimd-entry.ll Show resolved Hide resolved
llvm/tools/sycl-post-link/SYCLDeviceRequirements.cpp Outdated Show resolved Hide resolved
llvm/tools/sycl-post-link/SYCLDeviceRequirements.h Outdated Show resolved Hide resolved
dm-vodopyanov
dm-vodopyanov approved these changes Jun 26, 2023
View reviewed changes
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.

sycl/ part LGTM

@jzc jzc temporarily deployed to aws June 26, 2023 16:18 — with GitHub Actions Inactive
@jzc jzc temporarily deployed to aws June 26, 2023 17:18 — with GitHub Actions Inactive
@AlexeySachkov AlexeySachkov temporarily deployed to aws June 27, 2023 08:51 — with GitHub Actions Inactive
@AlexeySachkov AlexeySachkov temporarily deployed to aws June 27, 2023 10:09 — with GitHub Actions Inactive
@AlexeySachkov AlexeySachkov merged commit 8417687 into intel:sycl Jun 27, 2023
@aelovikov-intel
Copy link
Contributor

It seems post-commit started failing after this with

Unexpectedly Passed Tests (2):
  SYCL :: InvokeSimd/Feature/ImplicitSubgroup/SPMD_invoke_ESIMD_external.cpp
  SYCL :: InvokeSimd/Feature/SPMD_invoke_ESIMD_external.cpp

@AlexeySachkov AlexeySachkov mentioned this pull request Jun 29, 2023
Closed
@fwyzard fwyzard mentioned this pull request Jul 6, 2023
Open
Chenyang-L pushed a commit that referenced this pull request Jul 11, 2023
@jzc @AlexeySachkov @Chenyang-L
[SYCL] Optional kernel features: implement split based on reqd-sub-gr…
0fd711c 
…oup-size (#9928)

Based off #8167

---------

Co-authored-by: Alexey Sachkov <alexey.sachkov@intel.com>
@Chenyang-L Chenyang-L mentioned this pull request Jul 11, 2023
Closed
@al42and al42and mentioned this pull request Aug 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexeySachkov AlexeySachkov AlexeySachkov approved these changes

@dm-vodopyanov dm-vodopyanov dm-vodopyanov 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.
4 participants
@jzc @aelovikov-intel @dm-vodopyanov @AlexeySachkov