Skip to content

[SYCL] Add AMDGPU_kernel calling convention to detected kernels #14581

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.

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

Merged

Conversation

omarahmed1111
Copy link
Contributor

@omarahmed1111 omarahmed1111 commented Jul 16, 2024

free_function_kernels.cpp test had a bug where the kernels with demangled name for free functions will be deleted in the sycl-post-link step of compilation. And this happened as AMD kernels was not detected due to a missing condition. This was not detected before as the HIP device on CI doesn't have usm_shared_allocations aspect available so it was detected as unsupported but when I tried it locally with a device with usm_shared_allocations aspect available, the test was failing.

@omarahmed1111 omarahmed1111 requested a review from a team as a code owner July 16, 2024 09:46
@asudarsa
Copy link
Contributor

Change looks good. Will it be reasonable to add a unit test for this?

Thanks

@omarahmed1111 omarahmed1111 force-pushed the fix-free-function-kernels-test-on-hip branch 2 times, most recently from fcbfdec to bd49a51 Compare August 8, 2024 15:03
@omarahmed1111 omarahmed1111 force-pushed the fix-free-function-kernels-test-on-hip branch from bd49a51 to 7599fa9 Compare August 8, 2024 15:09
@omarahmed1111 omarahmed1111 force-pushed the fix-free-function-kernels-test-on-hip branch from 7599fa9 to df753f2 Compare August 8, 2024 16:22
@omarahmed1111 omarahmed1111 requested a review from a team as a code owner August 8, 2024 16:22
@omarahmed1111
Copy link
Contributor Author

Change looks good. Will it be reasonable to add a unit test for this?

Thanks

Sry for late, I updated that with a unittest

@omarahmed1111 omarahmed1111 force-pushed the fix-free-function-kernels-test-on-hip branch from df753f2 to 1995c50 Compare August 9, 2024 09:53
@omarahmed1111 omarahmed1111 force-pushed the fix-free-function-kernels-test-on-hip branch from 1995c50 to 58f300c Compare August 9, 2024 13:55
Copy link
Contributor

@uditagarwal97 uditagarwal97 left a comment

Choose a reason for hiding this comment

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

SYCL Changes LGTM!

@bader
Copy link
Contributor

bader commented Aug 10, 2024

@omarahmed1111, please, add [SYCL] tag to the PR title.
See https://github.com/intel/llvm/blob/sycl/sycl/doc/developer/ContributeToDPCPP.md for more guidelines.

@omarahmed1111 omarahmed1111 changed the title Add AMDGPU_kernel calling convention to detected kernels [SYCL] Add AMDGPU_kernel calling convention to detected kernels Aug 12, 2024
@omarahmed1111
Copy link
Contributor Author

@intel/llvm-gatekeepers Please merge when possible, Thanks!

@steffenlarsen steffenlarsen merged commit f33fc10 into intel:sycl Aug 12, 2024
12 checks passed
AlexeySachkov pushed a commit to AlexeySachkov/llvm that referenced this pull request Nov 26, 2024
…l#14581)

`free_function_kernels.cpp` test had a bug where the kernels with
demangled name for free functions will be deleted in the
`sycl-post-link` step of compilation. And this happened as AMD kernels
was not detected due to a missing condition. This was not detected
before as the HIP device on CI doesn't have `usm_shared_allocations`
aspect available so it was detected as unsupported but when I tried it
locally with a device with `usm_shared_allocations` aspect available,
the test was failing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants