Support for arrays as kernel parameters. #1841

rdeodhar · 2020-06-09T00:06:01Z

[SYCL] This patch adds support for arrays as kernel parameters. Arrays within structs are not currently supported and will be supported when struct decomposition is implemented.
Signed-off-by: rdeodhar [email protected]

Signed-off-by: rdeodhar <[email protected]>

Fznamznon

Initial feedback for SemaSYCL, will take a look into tests and other parts later.

clang/lib/Sema/SemaSYCL.cpp

Fznamznon · 2020-06-10T18:30:23Z

Ah, I already saw the tests, they look ok.
We need to make sure that it will work correctly with Elizabeth's changes from #1833, so I'd expected some feedback from Elizabeth as well.
BTW, there is some doc, @kbobrovs , @pvchupin since you are code owners of docs, could you please review it as well?

pvchupin

Thanks for ping. Few minor comments from me. @kbobrovs will review carefully.

sycl/doc/Array_Kernel_Parameters.md

elizabethandrews

Sorry for the delay in review. The logic is a bit complicated and I was trying to understand this PR and #1764 properly.

I have not looked at tests or integration header generation yet but I had some questions in my first pass.

clang/lib/Sema/SemaSYCL.cpp

elizabethandrews · 2020-06-10T21:47:11Z

Also, I could not figure out how to leave a comment for VisitFields in this PR since this was added in #1764, but shouldn't the 'if statements' be 'if - else if' ? Otherwise won't accessors and samplers also enter VisitAccessorWrapper?

sycl/doc/Array_Kernel_Parameters.md

kbobrovs · 2020-06-11T04:59:27Z

@rdeodhar, thanks for the detailed doc! (especially int header and general kernel parameter generation)

sycl/test/array_param/array-kernel-param-run.cpp

rbegam

Except the minor query, the sycl section LGTM with all the corrections already suggested. For the clang part, @againull could you please review?

erichkeane

This generally seems to be done in a 'hacky' way in quite a few places. In general, I think the quality of this implementation is fairly mediocre.

That said, it is isolated in a few places all in SemaSYCL, and we want to get the dependent patch in ASAP, so I guess this is good enough for me for now.

clang/lib/Sema/SemaSYCL.cpp

kbobrovs

The doc looks good. Few comments on tests

clang/test/CodeGenSYCL/kernel-param-acc-array-ih.cpp

clang/test/CodeGenSYCL/kernel-param-member-acc-array-ih.cpp

clang/test/CodeGenSYCL/kernel-param-pod-array-ih.cpp

clang/test/SemaSYCL/array-kernel-param.cpp

sycl/doc/KernelParameterPassing.md

rbegam · 2020-06-30T19:24:34Z

Please resolve the latest conversations @rdeodhar

sycl/test/array_param/array-kernel-param-nested-run.cpp

clang/test/CodeGenSYCL/kernel-param-member-acc-array-ih.cpp

Co-authored-by: kbobrovs <[email protected]>

kbobrovs

LGTM

elizabethandrews · 2020-06-30T20:44:12Z

ClangFormat fail is a trailing white space in comment.

Support for arrays as kernel parameters.

7974c35

Signed-off-by: rdeodhar <[email protected]>

rdeodhar requested review from a team, Fznamznon, elizabethandrews, kbobrovs, premanandrao and pvchupin as code owners June 9, 2020 00:06

rdeodhar requested a review from rbegam June 9, 2020 00:06

Fznamznon reviewed Jun 9, 2020

View reviewed changes

rdeodhar added 3 commits June 9, 2020 12:37

Reusing some memberexpr building code.

4907194

Merge branch 'sycl' of https://github.com/otcshare/llvm into akp2

d54c0ca

Merge branch 'sycl' of https://github.com/otcshare/llvm into akp2

546c58d

Fznamznon reviewed Jun 10, 2020

View reviewed changes

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

Fznamznon linked an issue Jun 10, 2020 that may be closed by this pull request

ICE when array used in Functor #749

Closed

pvchupin reviewed Jun 10, 2020

View reviewed changes

sycl/doc/Array_Kernel_Parameters.md Outdated Show resolved Hide resolved

sycl/doc/Array_Kernel_Parameters.md Show resolved Hide resolved

sycl/doc/Array_Kernel_Parameters.md Outdated Show resolved Hide resolved

sycl/doc/Array_Kernel_Parameters.md Show resolved Hide resolved

elizabethandrews reviewed Jun 10, 2020

View reviewed changes

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

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

kbobrovs reviewed Jun 11, 2020

View reviewed changes

sycl/doc/Array_Kernel_Parameters.md Outdated Show resolved Hide resolved

kbobrovs suggested changes Jun 11, 2020

View reviewed changes

rbegam reviewed Jun 11, 2020

View reviewed changes

sycl/test/array_param/array-kernel-param-run.cpp Show resolved Hide resolved

rbegam reviewed Jun 11, 2020

View reviewed changes

rbegam requested a review from againull June 11, 2020 22:18

rdeodhar added 4 commits June 12, 2020 10:41

Updated support for arrays.

52ce3f2

Merge branch 'sycl' of https://github.com/otcshare/llvm into akp2

983b3d5

Formatting changes.

1bf0903

Formatting changes.

5d5121b

Ruyk mentioned this pull request Jun 15, 2020

[SYCL][CUDA] Incorrect work with constant memory #1819

Closed

Correction to a test.

f03edd9

rdeodhar changed the title ~~Support for arrays as kernel parameters.~~ Support for arrays as kernel parameters. Arrays within structs will be supported when struct decomposition is implemented. Jun 29, 2020

erichkeane reviewed Jun 29, 2020

View reviewed changes

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

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

rdeodhar changed the title ~~Support for arrays as kernel parameters. Arrays within structs will be supported when struct decomposition is implemented.~~ Support for arrays as kernel parameters. Jun 29, 2020

kbobrovs suggested changes Jun 30, 2020

View reviewed changes

rbegam previously approved these changes Jun 30, 2020

View reviewed changes

Changed how some lit tests are run.

9196a30

rdeodhar dismissed rbegam’s stale review via 9196a30 June 30, 2020 19:24

rbegam previously approved these changes Jun 30, 2020

View reviewed changes

elizabethandrews previously approved these changes Jun 30, 2020

View reviewed changes

kbobrovs reviewed Jun 30, 2020

View reviewed changes

sycl/test/array_param/array-kernel-param-nested-run.cpp Show resolved Hide resolved

elizabethandrews mentioned this pull request Jun 30, 2020

[SYCL] Confirm support for nested arrays after struct decomposition complete in PR#1877 #2018

Closed

kbobrovs suggested changes Jun 30, 2020

View reviewed changes

clang/test/CodeGenSYCL/kernel-param-member-acc-array-ih.cpp Show resolved Hide resolved

Update clang/test/CodeGenSYCL/kernel-param-member-acc-array-ih.cpp

5660269

Co-authored-by: kbobrovs <[email protected]>

rdeodhar dismissed stale reviews from elizabethandrews and rbegam via 5660269 June 30, 2020 20:38

kbobrovs previously approved these changes Jun 30, 2020

View reviewed changes

Fixed formatting.

81ace26

rdeodhar dismissed kbobrovs’s stale review via 81ace26 June 30, 2020 20:50

elizabethandrews approved these changes Jun 30, 2020

View reviewed changes

kbobrovs approved these changes Jun 30, 2020

View reviewed changes

rbegam approved these changes Jun 30, 2020

View reviewed changes

Fznamznon approved these changes Jul 2, 2020

View reviewed changes

bader merged commit 00e7308 into intel:sycl Jul 2, 2020

Fznamznon mentioned this pull request Jul 2, 2020

[SYCL] Decompose kernel parameters and add inheritance support #1877

Merged

rdeodhar deleted the akp2 branch January 15, 2021 17:32

bader mentioned this pull request Feb 4, 2021

[SYCL] Unsupported kernel parameter type #1476

Closed

Support for arrays as kernel parameters. #1841

Support for arrays as kernel parameters. #1841

Uh oh!

Conversation

rdeodhar commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fznamznon commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pvchupin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

elizabethandrews commented Jun 10, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kbobrovs commented Jun 11, 2020

Uh oh!

Uh oh!

rbegam left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kbobrovs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rbegam commented Jun 30, 2020

Uh oh!

Uh oh!

Uh oh!

kbobrovs left a comment

Choose a reason for hiding this comment

Uh oh!

elizabethandrews commented Jun 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

rdeodhar commented Jun 9, 2020 •

edited

Loading

Fznamznon commented Jun 10, 2020 •

edited

Loading