Add C++ support for raw CUDA pointer backed state-vector #16
Conversation
|
Hello. You may have forgotten to update the changelog!
|
mlxd
changed the title
mlxd
changed the title
mlxd
changed the title
mlxd
requested review from
AmintorDusko,
chaeyeunpark,
maliasadi and
trevor-vincent
| @@ -41,7 +41,12 @@ jobs: | |||
| with: | |||
| access_token: ${{ github.token }} | |||
|
|
|||
| - uses: actions/checkout@v2 | |||
| - name: Remove unused SDKs for extra storage during build | |||
| static_cast<void>(ptr); | ||
| } | ||
|
|
||
| /*StateVectorCudaRaw(const StateVectorCudaRaw &other, cudaStream_t stream = |
There was a problem hiding this comment.
Any reason to keep this here as a comment?
There was a problem hiding this comment.
Nope, you are completely right. This can go.
| } | ||
| } | ||
| /** | ||
| * @brief STL-fiendly variant of `applyOperation( |
There was a problem hiding this comment.
| * @brief STL-fiendly variant of `applyOperation( | |
| * @brief STL-friendly variant of `applyOperation( |
|
Hey @mlxd, I just have one comment, one question, and one suggestion. |
| const std::vector<Precision> &)>; | ||
| using FMap = std::unordered_map<std::string, ParFunc>; | ||
| const FMap par_gates_; | ||
| custatevecHandle_t handle; |
There was a problem hiding this comment.
why not
| custatevecHandle_t handle; | |
| custatevecHandle_t handle_; |
?
| StateVectorCudaRaw(void *gpu_data, size_t length, cudaStream_t stream = 0) | ||
| : StateVectorCudaRaw(Util::log2(length), stream) { | ||
| CFP_t *ptr = BaseType::getData(); | ||
| ptr = reinterpret_cast<CFP_t *>(gpu_data); |
There was a problem hiding this comment.
What does this line do? (as ptr doesn't seem to be used)
There was a problem hiding this comment.
I believe this is an error that I made on previous tests with casting rules --- completely skipped my brain, thanks for this.
| * @tparam Precision Floating-point precision type. | ||
| */ | ||
| template <class Precision> | ||
| class SVCudaTorch final : public StateVectorCudaRaw<Precision> { |
There was a problem hiding this comment.
I am curious what is the exact difference between this class and StateVectorCudaRaw. Can we just use StateVectorCudaRaw by providing a function converting torch::tensor to StateVectorCudaRaw?
There was a problem hiding this comment.
That is a good idea, but the issue is with the lifetime of the torch objects --- when capturing a tensor, we can guarantee it's existence, but doing so with a raw pointer (assuming we cannot hold it elsewhere) the underlying allocator may move the pool, or just copy it to a new block and throw this away. I think it was safer to explicitly ensure the reference counter on a tensor was ticked up by capturing it here, rather than just grabbing the raw pointer to its data.
There was a problem hiding this comment.
Aha I see. So we anyway need to keep torch::tensor instance for a lifetime.
| std::string cls_name = | ||
| "LightningGPU_" + SVType<PrecisionT>::getClassName() + "_C" + bitsize; | ||
|
|
||
| py::class_<SVType<PrecisionT>>(m, cls_name.c_str()) |
There was a problem hiding this comment.
I am also slightly worried about the code publication here (with bindings/Binding.cpp).
There was a problem hiding this comment.
Yea, this was a nightmare to integrate with the existing bindings. The amount of duplication is a pain here, but we definitely want to keep these modules separated from another for now. I think this requires a complete reworking of the build system to better support such optional packages as this.
There was a problem hiding this comment.
I agree that it is better to separate this module, but can we just make move this function to another header file and use them in both cases?
There was a problem hiding this comment.
That is probably the better option here, thanks for the suggestion.
Context: This PR allows an existing CUDA memory block to be accessed by LightningGPU through a new class
StateVectorCudaRaw.Description of the Change: New class allowing raw CUDA pointer access to cuQuantum routines. Associated tests extended for new class. Modified internal binding names and import structure. IN addition, following on from these supports, a new builder is added to allow Torch tensors to be mapped to the C++ class backend through the raw pointer class.
Benefits: Allows use of externally managed CUDA memory with LightningGPU
Possible Drawbacks: Additional maintenance overhead with additional class. Potential future restructure may be required.
Related GitHub Issues: