make hash functions constexpr friendly so we can compute string hashe… #2843
Conversation
…s at compile-time for CUDA/OptiX shaders in OSL.
There was a problem hiding this comment.
This is great. I have just a few suggestions, which are optional for you versus merging this and I can always patch it up as part of subsequent work that I'll do...
-
As discussed in chat, I'm curious to know if there's a reason why on the Cuda side you needed
__forceinline__versus regularinline, and if there is not a good reason, let's prefer the latter, as it gives the compiler a little more leeway in deciding when to fully inline. (Truly forcing inline should be used sparingly because there are times it can reduce perf.) -
I somewhat prefer OIIO::farmhash::inlined::foo() over OIIO::inlined_hashes::farmhash::foo? It feels like it should be more things inside the farmhash namespace, if you know what I mean.
-
I'm wondering if we should move farmhash.h into the include/OpenImageIO/detail/ subdirectory, because we don't really intend users to directly include it or call the functions therein. It has to be installed, but it's not really part of the direct public API, and we're leaning toward denoting that by the use of the detail subdir or namespace. I think I just want your opinion on this, you need not make that move right now; I can do it subsequently because it also will require a change to some of the CMake files to get it installed in the right way.
|
|
That all sounds good to me. |
- making a "nested" 'inlined' namespace - removing __forceinline__ for CUDA device code
|
just pushed an update PR addressing the comments |
AcademySoftwareFoundation#2843) make hash functions constexpr friendly so we can compute string hashes at compile-time for CUDA/OptiX shaders in OSL.
This makes the hash functions
constexprfriendly. If you're compiling for a CUDA device, then they will also be declared with__device__as well.Description
I have pretty much excised all the hashing code from
farmhash.cppand have moved it to a new file,farmhash.h. As such, I suppose we should change the copyright notice infarmhash.cppsince it no longer contains any google code.Tests
I have a local OSL branch that uses ustring hashes in lieu of
const char *for CUDA/OptiX code and it successfully grabs the the constexpr ustring hash at compile time when building the CUDA code. Also, I don't see any changes in the testsuite in this branch compared to 'master' on my machine.Checklist:
have previously submitted a Contributor License Agreement
(individual, and if there is any way my
employers might think my programming belongs to them, then also
corporate).
(adding new test cases if necessary).