SciNim / nimcuda

Nim bindings for CUDA
Apache License 2.0
42 stars 9 forks source link

Windows compatibility #4

Closed WBosonic closed 5 years ago

WBosonic commented 5 years ago

This should fix the library names used on windows.

Note: I did not directly test this as I do not have any experience with c2nim, but changing libName in the generated *.nim files as in the headers did the trick.

andreaferretti commented 5 years ago

Thank you! You should change the name in the generated nim files as well, though, since they are committed. The c2nim headers are there just to be able to regenerate the thing, but then some manual adjustment is needed.

I have one question, though. Are you sure there are the correct DLL names? As in, the names used in a standard installation of CUDA straight from the offiicial NVIDIA site? Or do you have some non-standard installation?

A second question: these names seem specific to the 8.0 version of CUDA. What about people having a later version? Maybe it would make sense to add a way to pass the suffix as a parameter on the command line?

WBosonic commented 5 years ago

The names are now modified in the nim files aswell.

My CUDA installation should be standard, I don't remember changing anything.

As for the version in the dll names, nimcuda explicitely states that it works with CUDA v8.0 so I didn't think it would be a problem, a command line parameter can always be added of course.

I have two installations of CUDA: v8.0 and v7.5, both bin folders have been automatically added to my system path, which explains why the version number is used in naming the dlls. I have not used CUDA on a linux distro yet, so I don't know how such conflicts would be solved.

WBosonic commented 5 years ago

Tested on windows with nim-0.19.4, all the examples and the neo gpu benchmark work.

Something that I find strange is is that vector_types.h needs to be included, which is why I needed to patch the exampleConfig in the nimble file. Can't this be resolved with a passc pragma?

WBosonic commented 5 years ago

Also cudnn is not in the default CUDA installation on windows, so currently it is commented out.

I'm thinking maybe autodetect its availability or be able to --define:cudnn.

Making cudnn an explicit import is maybe better, currently it's imported automatically when nimcuda is imported. With dynlib that was no problem as that only loaded the library when a function was used but with passL it is needed at compiletime.

andreaferretti commented 5 years ago

Thank you! I agree that CUDNN should be a separate import. It should be enough to move the things related to CUDNN from here to a separate file, say nimcuddn.nim.

My only issue with this PR is that it requires manually editing a lot of files. Most work was automated using nimble headers - after that, only driver_types and cuda_occupancy required manual tweaking. This will make it harder to port to new versions of CUDA (we are already lagging).

I think the best thing would be to accept this PR, with the caveat that future versions may break Windows compatibility again (this version would be tagged anyway, so you could keep using it).

In the long run, we should make everything fullly automated with nimgen, maybe with some help from @genotrance :-)

genotrance commented 5 years ago

Considering these are C headers, I'm interested to see how nimterop will handle it. However, it seems to be a 2GB download. Is there a way to get to the headers without the full download?

andreaferretti commented 5 years ago

I could send them to you, but the fact is that if you do not have CUDA installed, you will not have a way to test the wrappers, and if you have CUDA installed, you already have the headers :-)

WBosonic commented 5 years ago

The first part can be done with c2nim as:

#@
when defined(windows):
  import os
  {.passL: "\"" & os.getEnv("CUDA_PATH") / "lib/x64" / "libname.lib" & "\"".}
  {.pragma: dyn.}
elif defined(macosx):
  const
    libName = "libname.dylib"
  {.pragma: dyn, dynlib: libName.}
else:
  const
    libName = "libname.so"
  {.pragma: dyn, dynlib: libName.}
@#

instead of

#if defined(windows)
#  stdcall
#  define libName "cudart.dll"
#elif defined(macosx)
#  define libName "libcudart.dylib"
#else
#  define libName "libcudart.so"
#endif

And then the only thing left to do is replace dynlib: libname with dyn which I did with my editor, but should not be difficult to automate.

andreaferretti commented 5 years ago

Nice, I am merging this for now and I will perform the change in the .c2nim files later