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
Add CUDA support for MSVC #426
Conversation
Thanks! It looks like the main difference here is |
Oh cool, didn't know that. Does that mean we can remove all of those |
Perhaps yeah! I forget which target those were added for, so I'd just follow the various logic internally to see if it's necessary to keep it or not. |
Alright, there we go. I also tested |
Ah. That commit broke it |
There we go |
src/lib.rs
Outdated
@@ -1276,8 +1248,11 @@ impl Build { | |||
// the SDK, but for all released versions of the | |||
// Windows SDK it is required. | |||
if target.contains("arm") || target.contains("thumb") { | |||
cmd.args | |||
.push("/D_ARM_WINAPI_PARTITION_DESKTOP_SDK_AVAILABLE=1".into()); | |||
if self.cuda { |
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.
Are changes like this necessary? This PR feels a bit overreaching in terms of changing everything it finds?
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.
I wasn't too sure about this define, so I added it when compiling with nvcc as well. We can just simplify it to a -D
option instead and it will define it for both nvcc & non-nvcc versions?
src/lib.rs
Outdated
if self.cuda { | ||
cmd.args.push("-m64".into()); | ||
} | ||
cmd.push_cc_arg("-m64".into()); |
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.
I'm not really sure what's going on here, how come the flag is pushed twice?
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.
I wasn't too sure about if the nvcc compiler passed through the -m32 and -m64 options, so I added it for both. I think it does though, so I'll modify these lines to just add the options onto the command args directly, as both msvc and nvcc support these options.
Also, for context, the nvcc compiler only supports 32/64 bit cross-compilation, and it's phasing out 32 bit compilation altogether as well it seems. I'm not sure what to do about cross-compilation & CUDA btw -- should it be better supported/disallowed entirely?
Hm I was sort of hoping that everything that was |
Ah, ok - should I try and remove all of the |
I think that should work yeah, and I think it's probably better than sprinkling |
Ok, removed all Removed the edits that are mentioned in #426 (comment) as cross-compilation is not supported with nvcc, so it's probably better not to even pretend that we support it. Simplified the change in #426 (comment) |
Thanks @trolleyman! |
Tested building https://github.com/trolleyman/cuda-macros with this (the
cuda-macros-test
crate) and it builds & links correctly. Haven't had a chance to test that this runs yet, but will in the morning.I wasn't sure that this way is the most elegant, but this seemed like the way that did the least amount of changes. I am also not sure that this is the correct way of doing this, especially regarding cross-compiling, but it gets it up and running at least.
To test you can do a
cargo build
in the root of the repo linked above. The build stuff is a bit hacky, but essentially it generates the CUDA function below & calls it.