Skip to content

[SYCL RT] Add support for composite specialization constants #2797

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
Merged
merged 6 commits into from
Dec 8, 2020
Merged

[SYCL RT] Add support for composite specialization constants #2797

merged 6 commits into from
Dec 8, 2020

Conversation

AlexeySachkov
Copy link
Contributor

This PR implements runtime support for composite specialization constants. The test is disabled at the moment, because we need to merge #2779 first + enable SPIR-V support for composite spec constants

@AlexeySachkov AlexeySachkov requested review from a team and smaslov-intel as code owners November 19, 2020 14:12
@AlexeySachkov
Copy link
Contributor Author

@kbobrovs, @smaslov-intel, please review this PR

smaslov-intel
smaslov-intel reviewed Nov 30, 2020
View reviewed changes
smaslov-intel
smaslov-intel reviewed Nov 30, 2020
View reviewed changes
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

Are you going to fix the style check failure?

sycl/include/CL/sycl/detail/pi.hpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// encode scalar specialization constants, that form a composite one.
/// encode scalar specialization constants, that form the composite one.

What if it is a nested structural type? Can you maybe give an example here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The whole structure will be flattened. Sure, I will add an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and added an example in 0800c2bc8278c364eeee5951d0be180df228dcef

@AlexeySachkov
Copy link
Contributor Author

Are you going to fix the style check failure?

Sure, applied it in 0800c2bc8278c364eeee5951d0be180df228dcef. I was just waiting for some review comments to bundle clang-format change together with them to avoid wasting CI run

sergey-semenov
sergey-semenov previously approved these changes Nov 30, 2020
View reviewed changes
Copy link
Contributor

@sergey-semenov sergey-semenov left a comment

Choose a reason for hiding this comment

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

Minor typo aside, LGTM

smaslov-intel
smaslov-intel reviewed Nov 30, 2020
View reviewed changes
@AlexeySachkov
[SYCL] Add support for POD spec constants in RT
fa47324
@AlexeySachkov
Apply comments
c560a62 
Added an example of composite constant descriptor. Fixed a few typos.
Applied clang-format
@AlexeySachkov
Fix clang-format and apply comments
67836cc
@AlexeySachkov
Align size of spec_constant class between host and device
33d88b0 
Added a padding field to device representation of spec_constant class in
order to align its size with size of it on host. If sizes are not the
same it could lead to memory corruptions in SYCL RT when it tries to
access arguments of SYCL Kernel function.
@AlexeySachkov
Update LIT tests for composite spec constants
c8b42fd 
Moved them into on-device directory as they are E2E tests required
low-level runtimes and HW. Added one more test for composite spec
constants.
@AlexeySachkov AlexeySachkov force-pushed the private/asachkov/support-for-composite-spec-constants-in-runtime branch from a37e679 to c8b42fd Compare December 2, 2020 15:41
@AlexeySachkov
Copy link
Contributor Author

Enabled E2E tests as we have the rest of functionality in the repo already. Fixed an issue I found with different size of spec_constant class on host and device

Fznamznon
Fznamznon reviewed Dec 2, 2020
View reviewed changes
@AlexeySachkov AlexeySachkov changed the title [SYCL RT] Add support for POD specialization constants [SYCL RT] Add support for composite specialization constants Dec 3, 2020
@AlexeySachkov
Copy link
Contributor Author

@kbobrovs, please take a look as well

kbobrovs
kbobrovs approved these changes Dec 3, 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.

New fix in spec_constant.hpp LGTM

@AlexeySachkov
Copy link
Contributor Author

@sergey-semenov, @smaslov-intel, please take a look at the update version

@romanovvlad romanovvlad merged commit c62860f into intel:sycl Dec 8, 2020
@AlexeySachkov AlexeySachkov deleted the private/asachkov/support-for-composite-spec-constants-in-runtime branch February 25, 2021 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fznamznon Fznamznon Fznamznon left review comments

@sergey-semenov sergey-semenov Awaiting requested review from sergey-semenov

+2 more reviewers

@kbobrovs kbobrovs kbobrovs approved these changes

@smaslov-intel smaslov-intel smaslov-intel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@AlexeySachkov @Fznamznon @kbobrovs @sergey-semenov @smaslov-intel @romanovvlad