[Cycles/Cuda/Windows] Enable on demand kernel compilation on windows.
AcceptedPublic

Authored by LazyDodo (LazyDodo) on May 22 2016, 2:35 AM.

Details

Summary

This patch enables on demand compilation of kernels on windows given the following requirements are met

  1. A Supported version of nvcc is found on the system (same as linux)
  2. A supported version of Visual studio (Currently only 2013, but when cuda 8 comes out we can possibly support 2015 as well) is found.

Tested with standard kernel and Adaptive compilation.

Diff Detail

LazyDodo (LazyDodo) retitled this revision from to [Cycles/Cuda/Windows] Enable on demand kernel compilation on windows..May 22 2016, 2:35 AM
LazyDodo (LazyDodo) updated this object.
LazyDodo (LazyDodo) set the repository for this revision to rB Blender.
LazyDodo (LazyDodo) added a project: Cycles.
Thomas Dinges (dingto) requested changes to this revision.May 22 2016, 11:01 AM

In principal, it is fine to enable this functionality on Windows, but this platform specific code should not be in device_cuda.cpp. Doesn't CUEW already have some paths to NVCC saved?

This revision now requires changes to proceed.May 22 2016, 11:01 AM
Sergey Sharybin (sergey) requested changes to this revision.May 23 2016, 1:24 PM

There is an explicit reason why we didn't support runtime CUDA compilation on Windows even for the development builds: it's not that obviosu to setup a proper environment for building cuda kernels on Windows -- you need toolkit, you need specific version of MSVC and such. All this will just lead to an increasing frustration of people who'll try to use this feature in a buildbot and official builds.

What might work better is using NVRCT API. Unfortunately, it's only part of CUDA Toolkit currently, but that might change in the future (hopefully), and for now it'll be at least less tricky setup to have kernels compiled.

This revision now requires changes to proceed.May 23 2016, 1:24 PM

There is an explicit reason why we didn't support runtime CUDA compilation on Windows even for the development builds:

I assumed the feature was written on linux and nobody had taken the time to do the work on the windows side

it's not that obviosu to setup a proper environment for building cuda kernels on Windows -- you need toolkit, you need specific version of MSVC and such. All this will just lead to an increasing frustration of people who'll try to use this feature in a buildbot and official builds.

It's not that different on linux, you need the toolkit and a supported version of gcc (the people with just gcc 6.0 on their boxes aren't having that good of a time either) at-least i gave specific versions of what is needed.

What might work better is using NVRCT API.

Funny you bring that up, it's the route i originally took to fix this issue (given it only requires the toolkit to be installed as opposed to toolkit+supported compiler)
besides the icky redist rules for the runtime it has the following issues:

-NVRTC is nice and all for a small piece of code, but it lacks the c preprocessor to handle #includes, writing a scanner to look for includes and do it ourselves no that big of a deal, but still annoying..
-The real problem is we include a bunch of headers from both the cuda toolkit (cuda.h for instance) and the compiler chain on the system (float.h is a sample here) and i'm fairly sure we'll never get permission to redist those.

Unfortunately, it's only part of CUDA Toolkit currently, but that might change in the future (hopefully), and for now it'll be at least less tricky setup to have kernels compiled.

If our current strategy in resolving the lack of windows support for this feature is 'hoping nvidia might do something nice' I'd rather just get this patch committed and outline very specificly what the requirements are to make it work. I'm not big on hope

NVRTC is nice and all for a small piece of code, but it lacks the c preprocessor to handle #includes, writing a scanner to look for includes and do it ourselves no that big of a deal, but still annoying..

We already have this. See OpenCL kernel compilation.

If our current strategy in resolving the lack of windows support for this feature.

This isn't a feature we're looking forward enabling for users.

If i were to go the NVRTC route (looked further into it, can probably make it work for us), bringing the requirement down to just needing the cuda toolkit, will you consider it? or am I wasting my time here?

As a development tool that could be handy, but it wouldn't be something which we'll advertice to artists to use.

It's important to remember, that one of main points of Blender is ready to be used right away. It's quite unacceptable to advertise something to artists which involves (a) extra thingsthey have to do to their setup to sue particular feature (b) wait for the kernels to be compiled.

I think it would be reasonable to have this as a developer feature on Windows (I'm doing CUDA development mostly on Windows at the moment and would probably use this). I would also ensure it's clear this is a developer feature on Linux too. The choice of NVRTC or MSVC for me mostly depends on code complexity, whatever is simpler.

I think it's clear that this is a developer tool. It needs to be enabled via environment variable or Blenders Debug Menu (value 256). Don't mind to add this for Windows as well, but I would not add so much code for it. We also dont check for a supported gcc on Linux. Hardcode some common paths (default for MSVC on Windows), assume it is installed among the CUDA Toolkit, and then it works or doesnt.

Since it's a dev feature, how about this then? if the environment is setup correctly it'll work, but we don't go out of our way to set it up for them (the same way we don't really check if gcc 5 is installed on linux) It'll either work or it won't

Minor feedback.

Anyone to commit?

intern/cycles/device/device_cuda.cpp
344

Should be #ifdef _WIN32 instead.

Updated diff to latest master.

While testing i noticed that if you don't have the cubins and nvcc/environment is not setup correctly instead of the 'kernel sm_xx not found yadayadaya' error you now get a

`CUDA nvcc compiler not found. Install CUDA toolkit in default location. or nvcc fatal : Cannot find compiler 'cl.exe' in PATH` error message, which i'm not super stoked about....

Updated with previous feedback.

Looks good to me.

It would be good to improve the console messages as well.

Is there anything holding up a commit? I have a few extensions to this patch to make it work on my system. I see that here the CUDA installer set an environment variable where we can grab the Toolkit location from on Windows. Since it defaults to "C:\Program Files\etc" it also needs slight changes to have quotation marks in the right places to deal with the spaces in the path.

I don't know. Committing this patch or an improved version using the environment variable is fine with me.

  1. @Sergey Sharybin (sergey) still has a red dot on this review, didn't want to commit before that was resolved.
  2. I'm not too fond of the current error message, given it will be shown when people start plugging in their brand new nvidia cards (gtx 2080 of whatever the next gen will be called) that we don't quite support.
  3. I'm getting increasingly frustrated with nvidia's slow release schedule, msvc 2017 is supported with cuda 9, update 1 support is in beta, and we're on update 5 now. I started an nvrtc based cli compiler to make cubins with no interaction with the host compiler but haven't gotten anything workable there yet but once that was working i was planning on abandoning this patch and using that compiler instead on a development system.

Since CUEW wraps NVRTC, it could also be an option to use NVRTC directly instead of trying to find and launch nvcc from the command line. That way, this feature would be independent of a 3rd party compiler tool chain and would have less platform-dependent code.

@LazyDodo (LazyDodo), removed the red dot.

Using NVRTC would indeed be interesting, but not fussed if that's being checked on as a separate change.

This revision is now accepted and ready to land.Nov 8 2017, 12:05 PM