AcademySoftwareFoundation / OpenShadingLanguage

Advanced shading language for production GI renderers
BSD 3-Clause "New" or "Revised" License
2.07k stars 350 forks source link

feat(gpu)!: GPU/OptiX support of ReParameter #1686

Closed lgritz closed 1 year ago

lgritz commented 1 year ago

BREAKING CHANGE: to RendererServices ABI (including for CPU) and to the renderer-side setup when using OptiX.

This overhauls the implementation of how interactively-editable parameters work, where they live in memory, and get it all working on GPU/OptiX so that renderers can support interactive adjustment of those params without recompiling shaders.

The basic gist is as follows:

A number of other things you will see that's worth calling out:

I added a device_ptr utility template that is just a wrapper around a device side pointer that makes it hard to accidentally dereference it on the host side.

Since I was changing RendererServices anyway, I also remove unused register_global, fetch_global, global_map which were unused. They were leftovers from the way we handled strings in OptiX 6.x.

Encapsulate cuda global symbol name mangling into BackendLLVM::global_unique_symname(). I did this early on, turns out it wasn't necessary, but I still like that encapsulation, so I'm keeping it.

I bumped the 3rd set of digits in the version to reflect that the changes in RendererServices break ABI. This is only in main, it obviously cannot be backported to a release branch.

All tests pass for scalar and batch and optix.

I added a new simple reparam test, and renamed the old reparem to reparam-array. Oddly, the reparam-array test doesn't work properly on optix (it had never been tried before), but it also failed in optix at main -- so it's not related to this patch! Putting that on my list of other oddities to investigate later. It may just be a quirk of testshade, I'm not really sure yet.

Added to BackendLLVM (and batched) a llvm_ptr_type(TypeSpec) method that returns the LLVM type of a pointer to the specified type.

sfriedmapixar commented 1 year ago

An important case that this particular pull request doesn't cover is when the requirements of running on the GPU need constant propagation to happen, but the want to keep something available to ReParameter edits would prevent it. In the simplest case, you can just avoid using ReParameter on eg. string args or something like that. However it becomes problematic in slightly more complicated cases.

For example, let's say there's an "int mode" parameter to the shader that can be set to 1 or 2. In mode 1, it does various things, including appending "foo" to a texture name. In mode 2 it does other things, including appending "bar" to a texture name. When constant propagation is allowed to do it's thing, it's very likely the mode selection will be propagated through and that string manipulation will be done at compile time, leaving a single constant string with no runtime manipulation.

However, if you keep "mode" available for editing, suddenly we need to do string manipulation in the shader. For the GPU this is a no-go, so you'll have to detect that is in use and fail the GPU compile, at which point your renderer can either not use the GPU path or just fail to render with that shader completely.

We have a pass implemented that can prevent that from happening, and in that case we just have to recompile when those parameters are edited. We're in the middle of catching our local dev up to the head of OSL development, so aren't quite in a position to upstream that yet, but wanted to point out the pitfall here.

lgritz commented 1 year ago

@sfriedmapixar and I discussed these separately as well.

Yes, this is an interesting case that I admit I had not considered when I was preparing this patch. I was thinking of the direct manipulation of parameters and didn't really recognize the issue that a parameter marked "interactive" could prevent a shader from correctly building for the GPU because it used the kind of construct that is fine in shader source code but only will work on GPU if it can be resolved to be a constant by the time we get done with the runtime optimization.

I suggest that if this patch is acceptable in other ways, that we go ahead and merge so that we can start using it and gain more experience with any limitations, knowing that shaders might be constructed in ways that certain parameters being marked interactive could make the shader group fail to build for GPU. We'll just watch out for those, and come back to the problem later with a more robust and automatic fix -- and if we are lucky, Stephen will have the opportunity to upstream the approach he already has.