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] Redesign pointer handling for OpenCL kernel generation #6728

Merged
merged 19 commits into from
Sep 27, 2022
Merged

[SYCL] Redesign pointer handling for OpenCL kernel generation #6728

merged 19 commits into from
Sep 27, 2022

Conversation

elizabethandrews
Copy link
Contributor

@elizabethandrews elizabethandrews commented Sep 8, 2022 •
edited by bader
Loading

Requirement - Do not decompose types with pointers when generating OpenCL kernel arguments.

This PR adds logic to stop decomposing trivial types containing pointers. For every SYCL kernel argument which is a record type containing a pointer (or has a field or a base class with a pointer), we generate a new record type with all pointers in __global address space. This compiler generated type is the openCL kernel argument. In the kernel body, we initialize the local clone via memcpy.

Limitations:

  1. Array of pointers or array of types with pointers are still decomposed to it's elements.
  2. Due to current implementation restrictions, types which are not default constructible, continue to trigger decomposition if they contain pointers.

Both limitations above will hopefully be fixed in follow-up PRs.

@elizabethandrews
Mark structs which contain pointers. Modify DeclCreator
5637abb 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Remove DeclCreator changes
235a2b8 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Broken first attempt. Missing BodyCreator
88905fe 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews elizabethandrews temporarily deployed to aws September 8, 2022 03:54 Inactive
@Fznamznon Fznamznon changed the title {WIP] Redesign pointer handling for openCL kernel generation [WIP] Redesign pointer handling for openCL kernel generation Sep 8, 2022
@elizabethandrews
Refactor code and fix base class handling
5983411 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Add BodyCreator Statements (incomplete)
f12a2d1 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Remove stale comments
97f0595 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews elizabethandrews temporarily deployed to aws September 11, 2022 23:59 Inactive
Fznamznon
Fznamznon reviewed Sep 12, 2022
View reviewed changes
Copy link
Contributor

@Fznamznon Fznamznon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Although I've spent noticeable amount of time to understand the code, I'm not opposed to the approach itself. We just need to make sure that everything works fine, I see that some LIT tests are failing and llvm-test-suite tests didn't run.

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
@elizabethandrews
Fixed some bugs and added comments
78eabb1 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews elizabethandrews temporarily deployed to aws September 12, 2022 23:59 Inactive
@elizabethandrews
Remove code handling arrays. Will be done in followup PR
4344fcb 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Remove code for array and fix base memcpy
a2dc51d 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews elizabethandrews temporarily deployed to aws September 15, 2022 00:00 Inactive
@elizabethandrews
Copy link
Contributor Author

There's still some Clang tests to fix. I fix one test to illustrate what this PR does. Before fixing the others, I figured I should ensure memcpy is actually working as expected via a runtime test. I'll take a look at that tomorrow

@elizabethandrews
Exclude Lambdas and fix some tests
bcf0ab3 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Merge remote-tracking branch 'intel_llvm/sycl' into NoDecompPtr
1686ecd
@elizabethandrews
Fix dangling else
dc09e07 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Fix tests
6e612b3 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Do not generate new type for non-trivial types
dd907e4 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Handle opt-report generation correctly and add a few tests
324cc47 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews elizabethandrews changed the title [WIP] Redesign pointer handling for openCL kernel generation Redesign pointer handling for openCL kernel generation Sep 20, 2022
@elizabethandrews elizabethandrews marked this pull request as ready for review September 20, 2022 00:28
@elizabethandrews elizabethandrews requested a review from a team as a code owner September 20, 2022 00:28
@elizabethandrews
Copy link
Contributor Author

PR is ready for review.

Fznamznon
Fznamznon reviewed Sep 20, 2022
View reviewed changes
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
clang/test/SemaSYCL/decomposition.cpp Show resolved Hide resolved
clang/test/SemaSYCL/decomposition.cpp Show resolved Hide resolved
@elizabethandrews
Implement review comments
f03b49e 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
Fznamznon
Fznamznon reviewed Sep 20, 2022
View reviewed changes
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved
@elizabethandrews
Implement review comments
449deec 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews elizabethandrews marked this pull request as draft September 21, 2022 22:49
@elizabethandrews
Copy link
Contributor Author

elizabethandrews commented Sep 21, 2022 •
edited
Loading

Reverted PR to Draft because arrays of type containing pointer is broken. The code to exclude such cases from this logic is incorrect. Looking into a possible solution now.

@elizabethandrews
Fix crash on array of type containing pointers. Implement review comm…
babd36c 
…ents

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Fix test. Minor NFC. Add test
e8cea2f 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews elizabethandrews marked this pull request as ready for review September 22, 2022 22:34
@Fznamznon Fznamznon mentioned this pull request Sep 23, 2022
Merged
smanna12
smanna12 approved these changes Sep 23, 2022
View reviewed changes
Copy link
Contributor

@smanna12 smanna12 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for the detailed review @Fznamznon.

@bader bader changed the title Redesign pointer handling for openCL kernel generation [SYCL] Redesign pointer handling for openCL kernel generation Sep 27, 2022
@bader bader changed the title [SYCL] Redesign pointer handling for openCL kernel generation [SYCL] Redesign pointer handling for OpenCL kernel generation Sep 27, 2022
@elizabethandrews
Copy link
Contributor Author

ping @intel/llvm-gatekeepers. This PR is ready for review and merge. The failing CUDA Test suite looks like an infrastructure issue.

@pvchupin pvchupin merged commit 3916d3b into intel:sycl Sep 27, 2022
@pvchupin
Copy link
Contributor

@elizabethandrews, can you follow up on post commit issues on windows? https://github.com/intel/llvm/actions/runs/3137630364/jobs/5096023697

@elizabethandrews
Copy link
Contributor Author

@elizabethandrews, can you follow up on post commit issues on windows? https://github.com/intel/llvm/actions/runs/3137630364/jobs/5096023697

Fixed here - #6895

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smanna12 smanna12 smanna12 approved these changes

@Fznamznon Fznamznon Fznamznon approved these changes

@premanandrao premanandrao Awaiting requested review from premanandrao

@erichkeane erichkeane Awaiting requested review from erichkeane

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
@elizabethandrews @pvchupin @Fznamznon @smanna12