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] Check if kernel_bundle contains compatible kernels #7691

Merged
merged 11 commits into from
Dec 9, 2022
Merged

[SYCL] Check if kernel_bundle contains compatible kernels #7691

merged 11 commits into from
Dec 9, 2022

Conversation

KornevNikita
Copy link
Contributor

According to the SYCL2020 get_kernel_bundle() and
has_kernel_bundle() should check that kernels are compatible
with device.

@KornevNikita
[SYCL] Add a unittest for is_compatible
42d20a5
@KornevNikita
[SYCL] Check if kernel_bundle contain compatible kernels
fa1c6ff 
According to the SYCL2020 get_kernel_bundle() and
has_kernel_bundle() should check that kernels are compatible
with device.
@KornevNikita
Merge branch 'sycl' of https://github.com/intel/llvm into check-is-co…
bde2894 
…mpatible
@KornevNikita KornevNikita requested a review from a team as a code owner December 7, 2022 18:43
@aelovikov-intel aelovikov-intel changed the title [SYCL] Check if kernel_bundle contain compatible kernels [SYCL] Check if kernel_bundle contains compatible kernels Dec 7, 2022
aelovikov-intel
aelovikov-intel reviewed Dec 7, 2022
View reviewed changes
Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

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

I only have some minor stylistic concerns, but I haven't had much experience with the kernel_bundles. You might want to find an additional reviewer more familiar with the area.

sycl/source/detail/program_manager/program_manager.cpp Outdated Show resolved Hide resolved
sycl/source/detail/program_manager/program_manager.cpp Outdated Show resolved Hide resolved
sycl/source/detail/program_manager/program_manager.cpp Outdated Show resolved Hide resolved
sycl/source/detail/program_manager/program_manager.cpp Outdated Show resolved Hide resolved
sycl/source/detail/program_manager/program_manager.cpp Outdated Show resolved Hide resolved
sycl/unittests/SYCL2020/KernelBundle.cpp Outdated Show resolved Hide resolved
sycl/unittests/SYCL2020/KernelBundle.cpp Outdated Show resolved Hide resolved
sycl/unittests/SYCL2020/KernelBundle.cpp Outdated Show resolved Hide resolved
sycl/unittests/SYCL2020/KernelBundle.cpp Outdated Show resolved Hide resolved
sycl/unittests/SYCL2020/KernelBundle.cpp Outdated Show resolved Hide resolved
@KornevNikita
Copy link
Contributor Author

@aelovikov-intel I hope I haven't miss any of your comments, could you please take one more look?

@KornevNikita KornevNikita mentioned this pull request Dec 8, 2022
Merged
aelovikov-intel
aelovikov-intel approved these changes Dec 8, 2022
View reviewed changes
Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

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

LGTM style-wise (with just a few minor comments) and function-wise based on my limited knowledge.

sycl/source/detail/program_manager/program_manager.cpp Outdated Show resolved Hide resolved
sycl/source/detail/program_manager/program_manager.cpp Outdated Show resolved Hide resolved
sycl/source/detail/program_manager/program_manager.cpp Outdated
for (RTDeviceBinaryImage *Img : BinImages) {
const RTDeviceBinaryImage::PropertyRange &PropRange =
Img->getDeviceRequirements();
for (RTDeviceBinaryImage::PropertyRange::ConstIterator It : PropRange) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll reply near the new code.

sycl/source/detail/program_manager/program_manager.cpp Outdated Show resolved Hide resolved
sycl/source/kernel_bundle.cpp Outdated Show resolved Hide resolved
sycl/source/detail/program_manager/program_manager.cpp Outdated
Comment on lines 2143 to 2144
while (!Aspects.empty()) {
aspect Aspect = Aspects.consume<aspect>();
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be widespread enough pattern to justify its own utility. Should go to a separate PR though.

@KornevNikita KornevNikita requested a review from a team December 8, 2022 18:10
@KornevNikita
Copy link
Contributor Author

@aelovikov-intel thanks for the comments! @intel/sycl-language-enabling-triage could you take a look please?

@KornevNikita
Copy link
Contributor Author

I believe Plugin/level_zero_batch_event_status.cpp failure is unrelated

@KornevNikita
Copy link
Contributor Author

@intel/llvm-gatekeepers

@bader bader merged commit 91b1515 into intel:sycl Dec 9, 2022
@KornevNikita KornevNikita deleted the check-is-compatible branch December 9, 2022 20:51
dm-vodopyanov pushed a commit that referenced this pull request Dec 12, 2022
@KornevNikita
[SYCL] Move is IsCompatible.cpp to SYCL2020 (#7702)
44c6437 
Should be merged after: #7691
Revert of: #7683
To address: #7685
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aelovikov-intel aelovikov-intel aelovikov-intel approved these changes

@romanovvlad romanovvlad romanovvlad approved these changes

@steffenlarsen steffenlarsen steffenlarsen approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@KornevNikita @romanovvlad @aelovikov-intel @steffenlarsen @bader