Skip to content
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

Make ScalParams a proper generic struct. #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DiamondLovesYou
Copy link
Contributor

So downstream crates can also be generic.

@MichaelHirn
Copy link
Member

Great, PR. Looks very cool.

But I am a bit nervous about the unsafe { transmute_copy(&&scale.x) } thing. Have you tried to run the tests and operations locally with it? (This is not covered by Travis, because Travis doesn't have NVIDIA GPU support).

And could you elaborate a bit on why ScalParams need to be a proper generic struct and what downstream crates could then do, which is currently not possible?

@DiamondLovesYou
Copy link
Contributor Author

Sorry, accidentally pressed the comment button.

@DiamondLovesYou
Copy link
Contributor Author

But I am a bit nervous about the unsafe { transmute_copy(&&scale.x) } thing. Have you tried to run the tests and operations locally with it?

Yes, the tests were successful. W.r.t. transmute_copy: if the argument is the type &U and the return type is T, then transmute_copy casts its argument to &T and returns a copy of *&T.

And could you elaborate a bit on why ScalParams need to be a proper generic struct and what downstream crates could then do, which is currently not possible?

This does a couple of things: for one, the alpha and beta members were publically accessible pointers. Changing them to be the type specified by the generic type (and removing the private PhantomData member) allows downstream crates to be able to modify the values using f32 or f64 types and avoids requiring pointers for when they want to use values other than the default.

The second thing this does is allow us to call ScalParams's std::default::Default generically; otherwise Rust requires the exact implementation to be known ahead of time (ie mandating that either the f32 or f64 implementation to be called), which prevents any use of ScalParams when the type of T isn't known.

@alexandermorozov
Copy link

Doesn't unsafe { transmute_copy(&&scale.a) } give the same result as unsafe { transmute(&scale.a) }? Latter directly transforms pointer to another pointer type, former transforms pointer to pointer, then dereferences and copies it.

@alexandermorozov
Copy link

Here is a simple program:

extern crate libc;                                                                       

fn print(p: *const libc::c_void)                                                         
{                                                                                        
    println!("ptr: {:?}", p);                                                            
}                                                                                        

fn main() {                                                                              
    let x: f64 = 32434.0;                                                                
    print(unsafe {std::mem::transmute(x)}); // incorrect                                 
    print(unsafe {std::mem::transmute(&x)});                                             
    print(unsafe {std::mem::transmute_copy(&&x)});                                       
}

Output:

ptr: 0x40dfac8000000000
ptr: 0x7fff95d2af88
ptr: 0x7fff95d2af88

drahnr referenced this pull request in fff-rs/rust-cudnn Mar 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants