Skip to content

[HIP] Add cuda_wrapper tests from Externals/CUDA using hipify-perl #243

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
merged 1 commit into from
Jun 6, 2025

Conversation

jmmartinez
Copy link
Contributor

This PR tries to adapt in a pretty rough way some tests from Externals/CUDA that test interactions with cuda_wrapper headers.

This catch the bug fixed by llvm/llvm-project#139697

@jmmartinez jmmartinez requested a review from yxsamliu May 13, 2025 09:50
@jmmartinez jmmartinez self-assigned this May 13, 2025
@jmmartinez
Copy link
Contributor Author

jmmartinez commented May 13, 2025

I'm not happy with this. Ideally, we could put CUDA and HIP common tests together. This is a first step to get some initial feedback (and see what the validation has to say about it).

@jmmartinez jmmartinez requested a review from jplehr May 13, 2025 09:52
@jmmartinez jmmartinez force-pushed the cuda_wrappers/hip branch from 1849bc4 to 4c88c3e Compare May 13, 2025 09:58
@jmmartinez jmmartinez force-pushed the cuda_wrappers/hip branch from 4c88c3e to 2f699f4 Compare May 16, 2025 07:53
@jmmartinez jmmartinez requested review from emankov and kzhuravl May 16, 2025 07:56
@jmmartinez jmmartinez changed the title [HIP] rough way of adding cuda_wrapper tests from Externals/CUDA [HIP] Add cuda_wrapper tests from Externals/CUDA using hipify-perl May 16, 2025
@jmmartinez jmmartinez force-pushed the cuda_wrappers/hip branch from 2f699f4 to 4abe1ec Compare May 16, 2025 12:31
@jmmartinez
Copy link
Contributor Author

@jplehr do you have some advice about how I can ensure that this change doesn't break any of our buildbots ?

I've run the test locally and it works. I still have to ensure that hipify-perl is installed on our test images.

@jplehr
Copy link
Contributor

jplehr commented May 19, 2025

I believe we currently do not install this as part of the CI images, so we need to update the container images that we run. Let me check and get back to you.

@jmmartinez jmmartinez force-pushed the cuda_wrappers/hip branch from 4abe1ec to 57ae476 Compare May 22, 2025 08:42
@jmmartinez
Copy link
Contributor Author

@jplehr I double checked and this PR is already not failing if hipify-perl is not found.

It shows:

CMake Warning at External/HIP/CMakeLists.txt:117 (message):
  hipify-perl not found for ROCm installation in /Externals/hip/rocm-6.3.0.
Call Stack (most recent call first):
  External/HIP/CMakeLists.txt:141 (create_local_hip_tests)                                                                                                                                                         External/HIP/CMakeLists.txt:196 (create_hip_test)
  External/HIP/CMakeLists.txt:267 (create_hip_tests)

If there are no extra remarks we could:

  1. land this (it should not break the production bots since it tolerates hipify not being installed)
  2. check what staging bots are telling us and if everything is ok
  3. move the staging bots into production

Maybe on Tuesday, cause I don't want to break everything before a 3 day weekend.

What do you all think ?

@jplehr
Copy link
Contributor

jplehr commented Jun 6, 2025

Our bots should pick up changes to llvm-test-suite whenever we run a build of LLVM.
So, I don't mind landing this now (or on Tuesday) and see what it does to our bots. We can revert if it makes the bot unhappy. Worst case: The revert is not picked up and we need to move one bot back to staging, which isn't the end of the world either.

Whatever works better for you.

Copy link
Contributor

@jplehr jplehr left a comment

Choose a reason for hiding this comment

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

LGTM

@jmmartinez
Copy link
Contributor Author

jmmartinez commented Jun 6, 2025

I'll give it a go today then. I'll watch the bots:

staging bot -> It worked ! https://lab.llvm.org/staging/#/builders/93/builds/6480
production bot

@jmmartinez jmmartinez merged commit 15f882d into llvm:main Jun 6, 2025
1 check passed
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.

2 participants