AcademySoftwareFoundation / OpenShadingLanguage

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

Expose API to add __optix_enabled__ prefix to shadeops that could call OptiX functions #1678

Closed DeclanRussell closed 11 months ago

DeclanRussell commented 1 year ago

Description

I'm creating this PR to start a discussion on how to best solve an issue that we hit while using OptiX 7 + OSL. With the OptiX 7 changes, the way we deal with renderer specific shadops is a little different from the OptiX 6 days. Essentially, renderer specific shadeops are now just marked extern, and then when we do the OptiX JIT compile, we link the definitions in from some other OptiX compiled module. The problem comes when these shadeop definitions call optix* device functions (e.g. optixTrace, optixDirectCall, etc...). If you didn't already know, if you have an optix* call in some function, the OptiX JIT compiler will inline the whole function call stack and remove all the definitions. This is a problem with OSL, because we need these definitions to be linked into our OSL program, not removed. A problematic example would be implementing oslTrace. Presumably, to implement this, you would need the osl_trace shadeop to call optixTrace. Currently, if you do this, the definition will be inlined and discarded by the OptiX JIT compiler and your OSL program that calls trace will just be left with a bunch of unresolved extern function calls.

The solution that we came up with, and is demonstrated by this PR, is to use the new OptiX 7.7 __optix_enabled__ feature. Essentially, if you add the __optix_enabled__ prefix to your function name, it will tell the OptiX JIT compiler that although this function contains optix* calls, we do not want it to be inlined. The modifications in the PR exposes an API (register_optix_enabled_functions) for the renderer to specify to the runtime which shadops will be calling optix* functions and will need this prefix added. There is then logic that will add this prefix to the function call. Unfortunately, __optix_enabled__ functions can only be called by other __optix_enabled__ functions. Therefore, there are some places, such as some matrix shadeops, that I have had to hardcode with the __optix_enabled__ prefix. The downside to this, is that if a renderer is not calling optix* functions here, they may have some worse performance due to less inlining.

I am interested to know what you guys think of this. Its a fairly specific problem, but one I'm sure other renderers will hit if creating a full OSL implementation in an OptiX renderer.

Tests

Checklist:

sfriedmapixar commented 1 year ago

This looks fairly intrusive for such a specific workaround. Maybe you've already talked to them, but if not could some of the NVidia folks chime in on whether there's a less intrusive version? Perhaps providing a list of symbols to preserve to the Optix JIT runtime instead of string-matching on the symbols themselves?

AlexMWells commented 1 year ago

The more I look at this, I feel that a single llvm optimization pass could be used to just iterate over functions in module and change their name. Maybe, if memory serves correct changing and existing function name might be more difficult than it sounds but is doable. The point is perhaps we can not modify any of the code gen at all and just handle "decorating" (renaming) these optix callable functions directly in the generated module based on a mapping handed to llvm_util. Would certainly avoid all these top level changes.

AlexMWells commented 1 year ago

Another thought is not to change the names of osl_get_matrix at all and instead provide a set of OPTIX enabled shim functions that turn around and call the inlined regular osl_get_matrix calls.

DeclanRussell commented 1 year ago

The more I look at this, I feel that a single llvm optimization pass could be used to just iterate over functions in module and change their name. Maybe, if memory serves correct changing and existing function name might be more difficult than it sounds but is doable. The point is perhaps we can not modify any of the code gen at all and just handle "decorating" (renaming) these optix callable functions directly in the generated module based on a mapping handed to llvm_util. Would certainly avoid all these top level changes.

This sounds ideal, as these top level changes are certainly pretty nasty. I'm not sure I really have the expertise to implement such a thing. I could certainly take a look though if someone could help point me in the right direction.

Its worth noting that Tim's new PR https://github.com/AcademySoftwareFoundation/OpenShadingLanguage/pull/1680 may also solve the the same issue, as it replaces the linking of shadops via OptiX JIT with instead compiling the shadops to LLVM IR and then giving that directly to the runtime to compile in directly.

aconty commented 1 year ago

Yes, I would also vote for Tim's patch instead of the prefixes.

tgrant-nv commented 1 year ago

Declan, have you had a chance to consider whether or not https://github.com/AcademySoftwareFoundation/OpenShadingLanguage/pull/1680 covers the functionality you need? We'd definitely like to close any functional gaps in that PR.

If it's helpful to add the __optix_enabled__ prefix to certain function names, that could be done before the do_optmize() call for each ShaderGroup. Alex's suggestion of using shim functions also sounds promising.

lgritz commented 11 months ago

This is abandoned, right? Can we close?

DeclanRussell commented 11 months ago

This is abandoned, right? Can we close?

Apologies, I haven't had time to try Tim's changes from https://github.com/AcademySoftwareFoundation/OpenShadingLanguage/pull/1680. I think we can close this, as it sounds like Tim's work solves the problem. I'll be sure to circle back around once I get to try them and notify if there are any issues.