chore(gpu): pass over all cuda bind #1514
Conversation
agnesLeroy
force-pushed
the
al/cuda_rust_binding
branch
from
f46be60 to
48cd4a7
Compare
agnesLeroy
force-pushed
the
al/cuda_rust_binding
branch
from
48cd4a7 to
5a7e8ab
Compare
| @@ -3,104 +3,58 @@ use std::ffi::c_void; | |||
| #[link(name = "tfhe_cuda_backend", kind = "static")] | |||
| extern "C" { | |||
There was a problem hiding this comment.
"stupid" question are we sure the C ABI is respected when the CUDA backend is compiled ? in the sense that C++ does not exactly have the C ABI and I don't know how both relate 🤔
There was a problem hiding this comment.
(related to the potential launch failure stuff)
There was a problem hiding this comment.
Hmm I'm not totally sure no
There was a problem hiding this comment.
There was a problem hiding this comment.
this way you make sure in the C++ code base that you expose the C function as extern "C"
There was a problem hiding this comment.
We should be alright, as all the entry points in the binding are actually "C compatible". Nothing C++-like is passed through the interface, only raw pointer types.
There was a problem hiding this comment.
while I agree, I would not be against the "ceinture et bretelles" approach here :)
There was a problem hiding this comment.
And we define all functions that go through the interface with "extern C" in the header files.
There was a problem hiding this comment.
so then we are good on the ABI declaration, nothing else to do on the C++ side, just need to make sure definitions agree, do you want someone to double check function by function ?
There was a problem hiding this comment.
I think this would be very troublesome and not so efficient: I already went through it thoroughly yesterday. But really at some point we should spend some time to figure out how to generate this API automatically, at least part of it.
| @@ -8,7 +8,7 @@ extern std::mutex m; | |||
| extern bool p2p_enabled; | |||
|
|
|||
| extern "C" { | |||
| int cuda_setup_multi_gpu(); | |||
| int32_t cuda_setup_multi_gpu(); | |||
There was a problem hiding this comment.
For curiosity, why do we want to have int32_t instead of int here?
There was a problem hiding this comment.
Just to be sure with the types on any architecture, I prefer to always specify the size of the integer (as we do on the Rust side as well).
closes:
PR content/description
I rewrote all the binding to be sure it's consistent with the c++ headers.
Check-list: