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

Merged

pvchupin merged 19 commits intointel:syclfrom

elizabethandrews:NoDecompPtr

Sep 27, 2022

Contributor

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:

Array of pointers or array of types with pointers are still decomposed to it's elements.
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 added 3 commits

July 29, 2022 09:11


          Mark structs which contain pointers. Modify DeclCreator

5637abb

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>


          Remove DeclCreator changes

235a2b8

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>


          Broken first attempt. Missing BodyCreator

88905fe

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews temporarily deployed to aws

September 8, 2022 03:54

Inactive

Fznamznon changed the title ~~{WIP] Redesign pointer handling for openCL kernel generation~~ [WIP] Redesign pointer handling for openCL kernel generation

elizabethandrews added 3 commits

September 10, 2022 17:10


          Refactor code and fix base class handling

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>


          Add BodyCreator Statements (incomplete)

f12a2d1

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>


          Remove stale comments

97f0595

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews temporarily deployed to aws

September 11, 2022 23:59

Inactive

Fznamznon reviewed

View reviewed changes

Contributor

Fznamznon left a comment •

edited

Loading

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


          Fixed some bugs and added comments

78eabb1

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews temporarily deployed to aws

September 12, 2022 23:59

Inactive

elizabethandrews added 2 commits

September 14, 2022 06:08


          Remove code handling arrays. Will be done in followup PR

4344fcb

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>


          Remove code for array and fix base memcpy

a2dc51d

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews temporarily deployed to aws

September 15, 2022 00:00

Inactive

Contributor Author

elizabethandrews commented Sep 15, 2022

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 commented

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved

elizabethandrews requested review from erichkeane and premanandrao

September 15, 2022 00:15

elizabethandrews added 6 commits

September 15, 2022 19:20


          Exclude Lambdas and fix some tests

bcf0ab3

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>


          Merge remote-tracking branch 'intel_llvm/sycl' into NoDecompPtr

1686ecd


          Fix dangling else

dc09e07

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>


          Fix tests

6e612b3

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>


          Do not generate new type for non-trivial types

dd907e4

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>


          Handle opt-report generation correctly and add a few tests

324cc47

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews changed the title ~~[WIP] Redesign pointer handling for openCL kernel generation~~ Redesign pointer handling for openCL kernel generation

elizabethandrews marked this pull request as ready for review

September 20, 2022 00:28

elizabethandrews requested a review from a team as a code owner

September 20, 2022 00:28

Contributor Author

elizabethandrews commented Sep 20, 2022

PR is ready for review.

Fznamznon reviewed

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


          Implement review comments

f03b49e

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

smanna12 reviewed

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

smanna12 reviewed

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

smanna12 reviewed

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

smanna12 reviewed

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

smanna12 reviewed

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved

Fznamznon reviewed

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved


          Implement review comments

449deec

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews marked this pull request as draft

September 21, 2022 22:49

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.


          Fix crash on array of type containing pointers. Implement review comm…

babd36c

…ents

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews commented

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

elizabethandrews commented

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved


          Fix test. Minor NFC. Add test

e8cea2f

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews marked this pull request as ready for review

September 22, 2022 22:34

Fznamznon mentioned this pull request

[SYCL] Add a test checking wrapped USM pointers intel/llvm-test-suite#1288

Merged

Fznamznon approved these changes

View reviewed changes

smanna12 reviewed

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved

smanna12 approved these changes

View reviewed changes

Contributor

smanna12 left a comment •

edited

Loading

Thank you for the detailed review @Fznamznon.

bader changed the title ~~Redesign pointer handling for openCL kernel generation~~ [SYCL] Redesign pointer handling for openCL kernel generation

bader changed the title ~~[SYCL] Redesign pointer handling for openCL kernel generation~~ [SYCL] Redesign pointer handling for OpenCL kernel generation

Contributor Author

elizabethandrews commented Sep 27, 2022

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

pvchupin merged commit 3916d3b into intel:sycl

Contributor

pvchupin commented Sep 27, 2022

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

Contributor Author

elizabethandrews commented Sep 28, 2022

@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

Fznamznon Fznamznon approved these changes

premanandrao Awaiting requested review from premanandrao

erichkeane Awaiting requested review from erichkeane

+1 more reviewer

smanna12 smanna12 approved these changes

Labels

None yet