-
Notifications
You must be signed in to change notification settings - Fork 13
Upgrade cudarc to 0.16.3 #3
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | |||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1565,22 +1565,14 @@ from_t!(F8E4M3); | ||||||||||
| from_t!(F8E5M2); | |||||||||||
|
|
|||||||||||
| #[cfg(feature = "cuda")] | |||||||||||
| unsafe impl cudarc::driver::DeviceRepr for F8E4M3 { | |||||||||||
| fn as_kernel_param(&self) -> *mut std::ffi::c_void { | |||||||||||
| use std::ptr::addr_of; | |||||||||||
| addr_of!(self.0) as *const u8 as *mut _ | |||||||||||
| } | |||||||||||
| } | |||||||||||
| unsafe impl cudarc::driver::DeviceRepr for F8E4M3 {} | |||||||||||
|
|
|||||||||||
| #[cfg(feature = "cuda")] | |||||||||||
| unsafe impl cudarc::driver::ValidAsZeroBits for F8E4M3 {} | |||||||||||
|
|
|||||||||||
| #[cfg(feature = "cuda")] | |||||||||||
| unsafe impl cudarc::driver::safe::DeviceRepr for F8E5M2 { | |||||||||||
| fn as_kernel_param(&self) -> *mut std::ffi::c_void { | |||||||||||
| use std::ptr::addr_of; | |||||||||||
| addr_of!(self.0) as *const u8 as *mut _ | |||||||||||
| } | |||||||||||
| } | |||||||||||
| unsafe impl cudarc::driver::safe::DeviceRepr for F8E5M2 {} | |||||||||||
|
|
|||||||||||
|
Comment on lines
+1568
to
+1575
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainInconsistent use of F8E4M3 → implements Unless there is a documented reason that one type must be “unsafe-param” and the other the “safe” variant, this asymmetry is surprising and makes the maintenance story harder (especially after the cudarc bump).
#[cfg(feature = "cuda")]
unsafe impl cudarc::driver::DeviceRepr for F8E4M3 {}
+#[cfg(feature = "cuda")]
+unsafe impl cudarc::driver::safe::DeviceRepr for F8E4M3 {}
Please double-check the cudarc 0.16 trait docs and align the two impls accordingly. 🏁 Script executed: #!/bin/bash
# Inspect implementations of DeviceRepr and safe::DeviceRepr in src/lib.rs around F8E4M3 and F8E5M2
rg -n "F8E4M3" src/lib.rs
rg -n "F8E5M2" src/lib.rs
sed -n '1550,1600p' src/lib.rs
rg -n "DeviceRepr" src/lib.rsLength of output: 12573 🌐 Web query: 💡 Result: Definitions and Requirements of
|
|||||||||||
| Trait Location | Type of Trait | Key Requirements/Guarantees |
|---|---|---|
| cudarc::driver::DeviceRepr | Unsafe | Only for types compatible with CUDA device memory, must be Copy, Sized, and have a C-compatible layout; unsafe to implement incorrectly. |
| cudarc::driver::safe::DeviceRepr | Safe API | Same requirements as above; used in higher-level abstractions for safer device memory access. |
Practical Usage
These traits are primarily used to constrain the types that can be passed to or received from device buffers. They protect against common errors, such as attempting to transfer complex Rust types (structs with references, heap allocations, or non-C-compatible layouts) directly to the GPU, which would result in undefined or erroneous behavior.
"Both of these traits can generically accept memory allocated by the driver in the form of:
driver::CudaSlice<T>,driver::CudaView<T>, anddriver::CudaViewMut<T>..."[3]
Conclusion
DeviceRepris a marker trait for types that are plain-old-data and have a memory layout directly compatible with CUDA device memory.- Only safe, simple types (like primitives and
#[repr(C)]structs containing them) should implement these traits. - The trait is declared as unsafe in the core API and is used as a bound in cudarc's safe, ergonomic abstractions for device memory buffers[3].
If you require the exact trait definition, it can be reviewed in the cudarc source code on docs.rs[1] or the project's GitHub repository, but the above summarizes its definition and requirements accurately.
Citations:
- 1: https://docs.rs/crate/cudarc/latest/source/src/lib.rs
- 2: https://docs.rs/crate/cudarc/latest/source/src/types.rs
- 3: https://docs.rs/cudarc
- 4: https://github.com/coreylowman/cudarc/blob/main/README.md
- 5: https://docs.nvidia.com/cuda/cuda-driver-api/group__CUDA__TYPES.html
- 6: https://github.com/coreylowman/cudarc
- 7: https://rust-gpu.github.io/blog/transition-announcement
- 8: https://docs.rs/cudarc/latest/x86_64-pc-windows-msvc/cudarc/driver/safe/trait.DeviceSlice.html
- 9: https://crates.io/crates/cudarc/0.6.1
- 10: https://crates.io/crates/cudarc/0.7.0
Align DeviceRepr impls for F8E4M3 under both CUDA features
In cudarc 0.16 the safe::DeviceRepr trait has identical requirements to the root DeviceRepr but is the preferred, higher-level API. Currently:
-
Under
feature = "cuda",
• F8E4M3 implementscudarc::driver::DeviceRepr
• F8E5M2 implementscudarc::driver::safe::DeviceRepr -
Under
feature = "mistralrs_cudarc_fork",
• F8E4M3 implementsmistralrs_cudarc_fork::driver::DeviceRepr
• F8E5M2 implementsmistralrs_cudarc_fork::driver::safe::DeviceRepr
Please make them symmetric by adding the missing safe impls for F8E4M3:
• In src/lib.rs under #[cfg(feature = "cuda")], after
unsafe impl cudarc::driver::DeviceRepr for F8E4M3 {}add
#[cfg(feature = "cuda")]
+unsafe impl cudarc::driver::safe::DeviceRepr for F8E4M3 {}• In the same file under #[cfg(feature = "mistralrs_cudarc_fork")], after
unsafe impl mistralrs_cudarc_fork::driver::DeviceRepr for F8E4M3 {
fn as_kernel_param(&self) -> *mut std::ffi::c_void { … }
}add
#[cfg(feature = "mistralrs_cudarc_fork")]
+unsafe impl mistralrs_cudarc_fork::driver::safe::DeviceRepr for F8E4M3 {
+ fn as_kernel_param(&self) -> *mut std::ffi::c_void {
+ use std::ptr::addr_of;
+ addr_of!(self.0) as *const u8 as *mut _
+ }
+}Then verify:
cargo check --features="cuda mistralrs_cudarc_fork"to ensure you haven’t introduced any missing trait errors.
🤖 Prompt for AI Agents
In src/lib.rs around lines 1568 to 1575, the implementations of DeviceRepr
traits for F8E4M3 and F8E5M2 are inconsistent: F8E4M3 implements
cudarc::driver::DeviceRepr while F8E5M2 implements
cudarc::driver::safe::DeviceRepr under the "cuda" feature. To fix this, add an
unsafe impl of cudarc::driver::safe::DeviceRepr for F8E4M3 under the
#[cfg(feature = "cuda")] block after the existing DeviceRepr impl. Similarly,
under the #[cfg(feature = "mistralrs_cudarc_fork")] feature, add the
safe::DeviceRepr impl for F8E4M3 after its DeviceRepr impl. Finally, run cargo
check with both features enabled to confirm no trait errors remain.
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.
Unsure why cudarc removed
as_kernel_param, could not find any documentation about that change. It was part of a much larger code reorganization.