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

Support for arrays as kernel parameters. #1841

Merged
merged 21 commits into from
Jul 2, 2020
Merged

Support for arrays as kernel parameters. #1841

merged 21 commits into from
Jul 2, 2020

Conversation

rdeodhar
Copy link
Contributor

@rdeodhar rdeodhar commented Jun 9, 2020 •
edited
Loading

[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 rajiv.deodhar@intel.com

@rdeodhar
Support for arrays as kernel parameters.
7974c35 
Signed-off-by: rdeodhar <rajiv.deodhar@intel.com>
@rdeodhar rdeodhar requested a review from rbegam June 9, 2020 00:06
Fznamznon
Fznamznon reviewed Jun 9, 2020
View reviewed changes
Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

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

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

@Fznamznon
Copy link
Contributor

Fznamznon commented Jun 10, 2020 •
edited
Loading

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?

@Fznamznon Fznamznon linked an issue Jun 10, 2020 that may be closed by this pull request
ICE when array used in Functor #749
Closed
pvchupin
pvchupin reviewed Jun 10, 2020
View reviewed changes
Copy link
Contributor

@pvchupin pvchupin 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 ping. Few minor comments from me. @kbobrovs will review carefully.

elizabethandrews
elizabethandrews reviewed Jun 10, 2020
View reviewed changes
Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

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

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.

@elizabethandrews
Copy link
Contributor

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?

kbobrovs
kbobrovs suggested changes Jun 11, 2020
View reviewed changes
@kbobrovs
Copy link
Contributor

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

rbegam
rbegam reviewed Jun 11, 2020
View reviewed changes
Copy link
Contributor

@rbegam rbegam left a comment

Choose a reason for hiding this comment

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

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

@rbegam rbegam requested a review from againull June 11, 2020 22:18
@Ruyk Ruyk mentioned this pull request Jun 15, 2020
Closed
@rdeodhar 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
erichkeane reviewed Jun 29, 2020
View reviewed changes
Copy link
Contributor

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

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.

@rdeodhar 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
kbobrovs suggested changes Jun 30, 2020
View reviewed changes
Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

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

The doc looks good. Few comments on tests

rbegam
rbegam previously approved these changes Jun 30, 2020
View reviewed changes
@rbegam
Copy link
Contributor

rbegam commented Jun 30, 2020

Please resolve the latest conversations @rdeodhar

rbegam
rbegam previously approved these changes Jun 30, 2020
View reviewed changes
@elizabethandrews elizabethandrews mentioned this pull request Jun 30, 2020
Closed
@rdeodhar @kbobrovs
Update clang/test/CodeGenSYCL/kernel-param-member-acc-array-ih.cpp
5660269 
Co-authored-by: kbobrovs <Konstantin.S.Bobrovsky@intel.com>
@rdeodhar rdeodhar dismissed stale reviews from elizabethandrews and rbegam via 5660269 June 30, 2020 20:38
kbobrovs
kbobrovs previously approved these changes Jun 30, 2020
View reviewed changes
Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

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

LGTM

@elizabethandrews
Copy link
Contributor

ClangFormat fail is a trailing white space in comment.

@bader bader merged commit 00e7308 into intel:sycl Jul 2, 2020
@Fznamznon Fznamznon mentioned this pull request Jul 2, 2020
Merged
@rdeodhar rdeodhar deleted the akp2 branch January 15, 2021 17:32
@bader bader mentioned this pull request Feb 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erichkeane erichkeane erichkeane left review comments

@pvchupin pvchupin pvchupin left review comments

@Fznamznon Fznamznon Fznamznon approved these changes

@kbobrovs kbobrovs kbobrovs approved these changes

@elizabethandrews elizabethandrews elizabethandrews approved these changes

@rbegam rbegam rbegam approved these changes

@premanandrao premanandrao Awaiting requested review from premanandrao

@againull againull Awaiting requested review from againull

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

Successfully merging this pull request may close these issues.

ICE when array used in Functor
8 participants
@rdeodhar @Fznamznon @elizabethandrews @kbobrovs @rbegam @erichkeane @pvchupin @bader