-
Notifications
You must be signed in to change notification settings - Fork 583
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
Dynamic cuda wrapper #1065
base: main
Are you sure you want to change the base?
Dynamic cuda wrapper #1065
Conversation
* fix cmake version: default cmake version of ubuntu20.04 is too low
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
if(NOT USE_CUDA_WRAPPER) | ||
target_link_libraries(bitsandbytes PUBLIC CUDA::cublas CUDA::cusparse) | ||
endif() | ||
if(NO_CUBLASLT) | ||
target_compile_definitions(bitsandbytes PUBLIC NO_CUBLASLT) | ||
else() | ||
target_link_libraries(bitsandbytes PUBLIC CUDA::cublasLt) | ||
if(NOT USE_CUDA_WRAPPER) | ||
target_link_libraries(bitsandbytes PUBLIC CUDA::cublasLt) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake has CMAKE_CUDA_RUNTIME_LIBRARY
to control this; maybe would be better here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://cmake.org/cmake/help/latest/variable/CMAKE_CUDA_RUNTIME_LIBRARY.html
docuement says CMAKE_CUDA_RUNTIME_LIBRARY
only control cudart
(Currently, with or without this PR, cudart_static
added by default)
I'm a little worried that the API or ABI could have changed between 11 and 12, causing very weird runtime errors if we just use symbols from v12 and call them like they were v11, or vice versa. 🤔 |
Hey @wkpark @akx @matthewdouglas, I'm hoping to merge this in the coming two weeks. One thing I feel we need to discuss is how we can test if this works as intended without introducing any issues: Do you have any thoughts on that? I'm willing to do the work, but if you already have a few concrete things in mind, that would definitely help speed this up. Thanks @wkpark on your initiative on this, really appreciated! Another thing needed would be to resolve the conflicts. |
I'm not sure how I feel about this one yet. I will think about it a little more but my initial thought is that it seems to be quite hacky feeling. Maybe this is a common thing in C++ world, but I'm not used to seeing it. Edit: This would definitely conflict with what I was thinking with #1126 too. |
I also get a hacky/too-complex feeling here, but I'm not sure what the better option would be. |
Thanks for your valuable inputs ❤️ Really appreciated. |
This is a proof-of-concept, quick and dirty, CUDA version independent, dymamic CUDA library wrapper fix.
cuda_setup/main.py
compatible with older libraries. (older libraries mean libs with cuda version tags)cublas*
,cusparse*
,cublasLt*
functions are wrapped usingdlsym()/GetProcessAddress()
cudart_static
lib.ubuntu-20.04 + cuda11.8 environment.
windows 10 + cuda11.8 environment (mingw64 terminal)