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.

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

dlopen libcuda.so #70

Merged
merged 7 commits into from
May 13, 2022
Merged

dlopen libcuda.so #70

merged 7 commits into from
May 13, 2022

Conversation

madsbk
Copy link
Member

@madsbk madsbk commented May 6, 2022

Like we do with cuFile, dlopen the CUDA driver library

@shwina

@madsbk madsbk added improvement Improves an existing functionality non-breaking Introduces a non-breaking change c++ Affects the C++ API of KvikIO labels May 6, 2022
@madsbk madsbk marked this pull request as ready for review May 6, 2022 11:55
@madsbk madsbk marked this pull request as draft May 6, 2022 14:09
@madsbk madsbk marked this pull request as ready for review May 6, 2022 15:41
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

Overall looks great!

We also need to drop the following line from the CMakeLists.txt since we don't need to link to libcuda.so anymore.

target_link_libraries(kvikio INTERFACE CUDA::cuda_driver)

cpp/include/kvikio/shim/cuda.hpp Outdated Show resolved Hide resolved
@madsbk
Copy link
Member Author

madsbk commented May 10, 2022

We also need to drop the following line from the CMakeLists.txt since we don't need to link to libcuda.so anymore.

@robertmaynard since we still need cuda.h, do you know if we can do something similar to:

target_link_libraries(kvikio INTERFACE cufile::cuFile_interface)

Or do we just assume that cuda.h in the include path?

@robertmaynard
Copy link
Contributor

@robertmaynard since we still need cuda.h, do you know if we can do something similar to:

target_link_libraries(kvikio INTERFACE cufile::cuFile_interface)

Yes, if you link to the CUDA::toolkit target you will get the required include directories with no actual linking to a cuda library

@madsbk
Copy link
Member Author

madsbk commented May 10, 2022

Thanks for the review @robertmaynard, we now return references and linking like:

target_link_libraries(kvikio INTERFACE CUDA::toolkit)

@jakirkham
Copy link
Member

Should we add some kind of test to ensure we are not linking to libcuda.so?

Is there anything we need to do on the Python side? Or for nvCOMP bindings?

@madsbk
Copy link
Member Author

madsbk commented May 11, 2022

Should we add some kind of test to ensure we are not linking to libcuda.so?

I checked this previously f837214 but now that we don't link to libcuda.so, we get a compile error if using the CUDA API directly:

Is there anything we need to do on the Python side? Or for nvCOMP bindings?

I don't think so, nvCOMP is using the Runtime CUDA API.

@robertmaynard
Copy link
Contributor

Why did we drop the following check, as it also confirmed that we didn't bring in a hard-dependency transitively?

# Check that `libcuda.so` is NOT being linked
LDD_BASIC_IO=`ldd ${WORKSPACE}/libkvikio-debug-build/examples/basic_io`
if [[ "$LDD_BASIC_IO" == *"libcuda.so"* ]]; then
  echo "[ERROR] examples/basic_io shouln't link to libcuda.so: ${LDD_BASIC_IO}"
  return 1
fi

@madsbk
Copy link
Member Author

madsbk commented May 11, 2022

Why did we drop the following check, as it also confirmed that we didn't bring in a hard-dependency transitively?

Good point, adding the check again :)

@jakirkham
Copy link
Member

@robertmaynard what do you think about the changes above? 🙂

@robertmaynard
Copy link
Contributor

@robertmaynard what do you think about the changes above? slightly_smiling_face

Approved

@jakirkham
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit b6ec258 into rapidsai:branch-22.06 May 13, 2022
@madsbk
Copy link
Member Author

madsbk commented May 16, 2022

Thanks @robertmaynard and @jakirkham !

@madsbk madsbk deleted the shim_cuda_so branch May 30, 2022 08:54
@madsbk madsbk mentioned this pull request Aug 1, 2022
vuule pushed a commit to vuule/kvikio that referenced this pull request Nov 8, 2023
Record both total time recorded in function calls and the wall time in profiler output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Affects the C++ API of KvikIO improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants