-
Notifications
You must be signed in to change notification settings - Fork 5
Fix GPU handle pool singleton aliasing #525
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
Conversation
|
Are you sure the PR does what you describe in the title and description? |
edd7ffb to
cf8b52f
Compare
cf8b52f to
4d49336
Compare
|
@Arech8 I modified the content of commit and title, thanks |
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.
Should be fine now.
jaxlib/gpu/vendor.h
Outdated
| return hipblasCreate(reinterpret_cast<hipblasHandle_t*>(handle)); | ||
| } | ||
|
|
||
| inline hipblasStatus_t gpublasSetStream(gpublasHandle_t handle, hipStream_t stream) { |
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.
Why do you need this one? Does it work if you leave it as define?
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.
Nice idea, thanks, testing
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.
Sorry for being a nitpick here. Does #define gpublasSetStream hipblasSetStream work? I was expecting that to work.
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.
Currently, i am set like
#define gpublasCreate(handle) hipblasCreate(reinterpret_cast<hipblasHandle_t*>(handle))
#define gpublasSetStream(handle, stream) hipblasSetStream(reinterpret_cast<hipblasHandle_t>(handle), stream)
It works,
4d49336 to
2bc0153
Compare
…tion Problem: - BLAS and SOLVER handle pools were sharing the same static singleton instance - C++ template instantiation created only one pool when HandleType/StreamType were identical - This caused resource contamination between hipBLAS and hipSOLVER operations - Led to potential memory corruption and unexpected GPU library behavior
2bc0153 to
f9ef066
Compare
Addresses handle pool singleton sharing issue between different GPU operation types in ROCm/HIP backend.
Addresses handle pool singleton sharing issue between different GPU operation types in ROCm/HIP backend. (cherry picked from commit 4b5b368)
Addresses handle pool singleton sharing issue between different GPU
operation types in ROCm/HIP backend.
Changes made: