Skip to content

[SYCL][PI] Restore CUDA and HIP plugin ABI #10518

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 5 commits into from
Jul 27, 2023
Merged

[SYCL][PI] Restore CUDA and HIP plugin ABI #10518

merged 5 commits into from
Jul 27, 2023

Conversation

@kbenzie
Copy link
Contributor

@kbenzie kbenzie commented Jul 21, 2023 •
edited
Loading

During the port to UR the CUDA and HIP PI plugin ABI's were unintentionally changed. There does not appear to be symbol checks for these plugins, unlike the Level Zero symbol check and OpenCL symbol check. As such, the ABI change went unnoticed until #10490 was opened using the same approach for the OpenCL port, which failed the OpenCL symbol check.

This PR restores the expected ABI for the CUDA and HIP plugins and introduces new CUDA and HIP symbol check tests.

kbenzie added 2 commits July 21, 2023 11:00
@kbenzie
[SYCL][PI][CUDA] Restore CUDA plugin ABI
fd99be7 
During the port to UR the CUDA PI plugin ABI was unintentionally
changed. This patch restores the expected ABI for the CUDA plugin.
@kbenzie
[SYCL][PI][HIP] Restore HIP plugin ABI
8e0bebc 
During the port to UR the HIP PI plugin ABI was unintentionally changed.
This patch restores the expected ABI for the HIP plugin.
@kbenzie kbenzie requested review from a team as code owners July 21, 2023 11:33
@kbenzie kbenzie requested a review from steffenlarsen July 21, 2023 11:33
steffenlarsen
steffenlarsen approved these changes Jul 21, 2023
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Good catch! Would we be able to add symbol checks for HIP and CUDA or would it make no sense at this point?

@kbenzie
Copy link
Contributor Author

kbenzie commented Jul 21, 2023 •
edited
Loading

Good catch! Would we be able to add symbol checks for HIP and CUDA or would it make no sense at this point?

I tried this out locally by copy pasting the Level Zero symbol check (changing the library path), using REQUIRES: cuda runs and passes but REQUIRES: hip is marked unsupported so I guess that's not an existing lit feature. Also copy pasting is not what the ABI Policy suggests.

@steffenlarsen
Copy link
Contributor

steffenlarsen commented Jul 21, 2023

Good catch! Would we be able to add symbol checks for HIP and CUDA or would it make no sense at this point?

I tried this out locally by copy pasting the Level Zero symbol check, using REQUIRES: cuda runs and passes but REQUIRES: hip is marked unsupported so I guess that's not an existing lit feature. Also copy pasting is not what the ABI Policy suggests.

You should be able to generate the current symbols by using the same abi_check.py script and replacing --mode check_symbols --reference with --mode dump_symbols --output

@kbenzie kbenzie temporarily deployed to aws July 21, 2023 11:53 — with GitHub Actions Inactive
@kbenzie
Copy link
Contributor Author

kbenzie commented Jul 21, 2023

You should be able to generate the current symbols by using the same abi_check.py script and replacing --mode check_symbols --reference with --mode dump_symbols --output

I think the script might need to be slightly modified as it doesn't output a REQUIRES: clause and I don't believe plugins and built by default so would probably result in fail test when they are not present.

@steffenlarsen
Copy link
Contributor

steffenlarsen commented Jul 21, 2023

You should be able to generate the current symbols by using the same abi_check.py script and replacing --mode check_symbols --reference with --mode dump_symbols --output

I think the script might need to be slightly modified as it doesn't output a REQUIRES: clause and I don't believe plugins and built by default so would probably result in fail test when they are not present.

Good point. Maybe it can be used to generate the initial version, but then we will have to maintain it without generating it through the script afterwards, like we do for L0 and OpenCL. I am also okay with having a new option for the checker, but I don't know if it makes sense for PI symbols that I assume we will remove eventually anyway.

@kbenzie kbenzie temporarily deployed to aws July 21, 2023 12:31 — with GitHub Actions Inactive
@kbenzie
Copy link
Contributor Author

kbenzie commented Jul 21, 2023

Good point. Maybe it can be used to generate the initial version, but then we will have to maintain it without generating it through the script afterwards, like we do for L0 and OpenCL. I am also okay with having a new option for the checker, but I don't know if it makes sense for PI symbols that I assume we will remove eventually anyway.

I've gone with using the script to generate the checks then manually adding REQUIRES: cuda_be and REQUIRES: hip_be where appropriate.

@kbenzie
Copy link
Contributor Author

kbenzie commented Jul 21, 2023

I don't know what's don't wrong with this check...

@steffenlarsen
Copy link
Contributor

steffenlarsen commented Jul 21, 2023

I don't know what's don't wrong with this check...

Looks like the Github unicorn strikes again! It was most likely just a github hiccup.

@kbenzie kbenzie temporarily deployed to aws July 21, 2023 13:30 — with GitHub Actions Inactive
@kbenzie kbenzie temporarily deployed to aws July 21, 2023 14:08 — with GitHub Actions Inactive
@kbenzie kbenzie force-pushed the benie/restore-cuda-hip-abi branch from 76ad779 to 408be20 Compare July 21, 2023 14:12
@kbenzie kbenzie temporarily deployed to aws July 21, 2023 14:27 — with GitHub Actions Inactive
@kbenzie kbenzie temporarily deployed to aws July 21, 2023 15:05 — with GitHub Actions Inactive
@kbenzie kbenzie mentioned this pull request Jul 26, 2023
Merged
jandres742
jandres742 approved these changes Jul 26, 2023
View reviewed changes
@kbenzie kbenzie temporarily deployed to aws July 26, 2023 15:42 — with GitHub Actions Inactive
@kbenzie kbenzie temporarily deployed to aws July 26, 2023 16:28 — with GitHub Actions Inactive
@kbenzie
Copy link
Contributor Author

kbenzie commented Jul 27, 2023

@intel/llvm-gatekeepers this is ready to merge now

@steffenlarsen steffenlarsen merged commit 2da85e1 into intel:sycl Jul 27, 2023
fabiomestre pushed a commit to fabiomestre/llvm that referenced this pull request Sep 26, 2023
@kbenzie
[SYCL][PI] Restore CUDA and HIP plugin ABI (intel#10518)
f75632f 
During the port to UR the CUDA and HIP PI plugin ABI's were
unintentionally changed. There does not appear to be symbol checks for
these plugins, unlike the [Level Zero symbol
check](https://github.com/intel/llvm/blob/sycl/sycl/test/abi/pi_level_zero_symbol_check.dump)
and [OpenCL symbol
check](https://github.com/intel/llvm/blob/sycl/sycl/test/abi/pi_opencl_symbol_check.dump).
As such, the ABI change went unnoticed until
intel#10490 was opened using the same
approach for the OpenCL port, which
[failed](https://github.com/intel/llvm/actions/runs/5610646255/job/15200624025?pr=10490)
the OpenCL symbol check.

This PR restores the expected ABI for the CUDA and HIP plugins and
introduces new CUDA and HIP symbol check tests.
veselypeta pushed a commit to veselypeta/llvm that referenced this pull request Sep 28, 2023
@kbenzie
[SYCL][PI] Restore CUDA and HIP plugin ABI (intel#10518)
d618fb7 
During the port to UR the CUDA and HIP PI plugin ABI's were
unintentionally changed. There does not appear to be symbol checks for
these plugins, unlike the [Level Zero symbol
check](https://github.com/intel/llvm/blob/sycl/sycl/test/abi/pi_level_zero_symbol_check.dump)
and [OpenCL symbol
check](https://github.com/intel/llvm/blob/sycl/sycl/test/abi/pi_opencl_symbol_check.dump).
As such, the ABI change went unnoticed until
intel#10490 was opened using the same
approach for the OpenCL port, which
[failed](https://github.com/intel/llvm/actions/runs/5610646255/job/15200624025?pr=10490)
the OpenCL symbol check.

This PR restores the expected ABI for the CUDA and HIP plugins and
introduces new CUDA and HIP symbol check tests.
mdtoguchi pushed a commit to mdtoguchi/llvm that referenced this pull request Oct 18, 2023
@kbenzie @mdtoguchi
[SYCL][PI] Restore CUDA and HIP plugin ABI (intel#10518)
abe8043 
During the port to UR the CUDA and HIP PI plugin ABI's were
unintentionally changed. There does not appear to be symbol checks for
these plugins, unlike the [Level Zero symbol
check](https://github.com/intel/llvm/blob/sycl/sycl/test/abi/pi_level_zero_symbol_check.dump)
and [OpenCL symbol
check](https://github.com/intel/llvm/blob/sycl/sycl/test/abi/pi_opencl_symbol_check.dump).
As such, the ABI change went unnoticed until
intel#10490 was opened using the same
approach for the OpenCL port, which
[failed](https://github.com/intel/llvm/actions/runs/5610646255/job/15200624025?pr=10490)
the OpenCL symbol check.

This PR restores the expected ABI for the CUDA and HIP plugins and
introduces new CUDA and HIP symbol check tests.
iclsrc pushed a commit that referenced this pull request Feb 9, 2024
@Endilll
[clang][Sema] Add checks for validity of default ctor's class (#78898)
c58c6aa 
Fixes #10518
Fixes #67914
Fixes #78388
Also addresses the second example in #49103

This patch is based on suggestion from @cor3ntin in
llvm/llvm-project#67914 (comment)
@kbenzie kbenzie deleted the benie/restore-cuda-hip-abi branch December 18, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@steffenlarsen steffenlarsen steffenlarsen approved these changes

+2 more reviewers

@jchlanda jchlanda jchlanda approved these changes

@jandres742 jandres742 jandres742 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.

4 participants

@kbenzie @steffenlarsen @jchlanda @jandres742