Ustringhash phase 3

[steenax86]

Description

Move OSL library functions and remaining execution time interface from ustring to ustringhash.

Intent is all code for cpu or device just use ustringhash only. This allows us to remove STRING_PARAM, ustringrep, and OSL_USTRINGREP_IS_HASH. All interfaces and library code should explicitly use ustringhash or ustring, or for library functions llvm can call ustringhash_pod or ustring_pod

Compile time strings used in the OSL Library are represented in strdecls.h and will be available as ustring variables in the Strings namespace as well as available as unstringhash constexpr variables in the Hashes namespace.

Compile time strings used by the Renderer should adopt a similiar strategy as exemplified by TestShade which keeps its strings in rs_strdecls.h and made available as ustring variables in the RS::Strings namespace as well as available as unstringhash constexpr variables in the RS::Hashes namespace.

Added helper utility functions to generate ustring from ustringhash_pods

Constant string arrays: Previously, array copys required copying data via address of symbol on host. To enable working on device, replaced memcpy with array allocation and loading of values. Constant arrays are now present as global arrays in final llvm module.

On the Batched side compatability/integration: Batched OSL Library functions continue to use ustrings (not ustringhashes to reduce scope/risk). Batched OSL library functions that call their scalar counterparts were adjusted to pass arguments as ustringhash_pods and store results as ustrings

STRING_PARAMS, device_string.h, and string_hash.h: Built-in strings on the CUDA side are now accessed via Hashes namespace, instead of STRING_PARAMS and are evaluated as ustringhashes. Similarly, DeviceString, StringParams, HDSTR are obsolete, and device_string.h is eliminated. Hashes.h, which only contains OIIO's strhash and pvtstrlen functions (both formerly in string_hash.h), replaces all inclusions of device_string.h. Namespace Hashes in hashes.h contains pre-compiled ustringhashes representing strings from strdecls.h.

Texture, dictionary, closure: Helper implementation functions are used by both scalar and batched versions. However, since scalar uses ustringhashes and the batched version uses ustrings added a flag to inform functions on treating TypeDesc::STRING has either a ustring or a ustringhash

Tests

Added testing of bitcode rendereservices for unoptimized OSL shader. array-copy tests regression: Added new test to verify copying of arrays of unequal lengths

Checklist:

• [x] I have read the contribution guidelines.
• [x] I have previously submitted a Contributor License Agreement.
• [x] I have updated the documentation, if applicable.
• [x] I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
• [x] My code follows the prevailing code style of this project.

[lgritz]

This needs a rebase and fix any merge conflicts with the several Brecht patches that just went in.

[brechtvl]

OptiX tests are now crashing with this, after applying the fix from my inline comment:

src/liboslexec/llvm_util.cpp:3032: make_function: Assertion 'llvm::isa<llvm::Function>(maybe_func)' failed: Declaration for osl_texture is wrong, LLVM had to make a cast

[AlexMWells]

@brechtvl , PR has been updated with fixes. If you could please retest, we think its read to go.

[brechtvl]

This passes the OptiX tests now for me (ignoring some tests that fail the same way in main).

[lgritz]

Yes, passes for me, too.

I'm working on some additional changes that will make this less of a stark source code break for renderers, though.

[lgritz]

Overall, I think this is good, I like the basic idea and the design, and it's definitely the way forward.

I'm concerned about a few things, and I think they are addressable (and I'm happy to do the work and amend this PR if I can). Very briefly,

1. How big a code delta this is, making review difficult (for me now) and understanding exactly what changed succinctly (later, and for others), and whether it could be much smaller, with only the essential changes.
2. Easing the work necessary for renderers to move to this. It's surprisingly substantial right now, I believe partly unnecessarily, because we removed a lot of type names and methods instead of just changing the types they represent.
3. I'm inclined to want to preserve the ability to change execution-time string representation again in the future, because, let's face it, if the PTX compilation was fast enough that we didn't live and die by the PTX cache, we would very much prefer actual ustrings. Who knows what else may push us to that or a different rep in the future.

To elaborate on a few examples of these:

As an example, in backendllvm.h you got rid of llvm_load_string, added llvm_const_hash, and then in a whole lot of places changed the name. But semantically, they do exactly the same thing -- given a symbol, or a constant string/ustring/whatever, it gives you the LLVM::Value* that represents that string in the generated IR. I wonder if it would be better to have left it as llvm_load_string but change the definition (and comment) to load the hash? Then so many of the changes elsewhere in the code base that consist of just renaming this function could go away and those sites wouldn't need to change at all. There are probably other such instances, and this merges a bit with the next comment when we look at the same kinds of changes as pertain to the renderer rather than strictly internals.

While this passes all my tests for OSL, of course our renderer doesn't build against it without considerable changes. Recall with the journaling changes, we specifically wanted things to keep working for renderers until they're ready to write new code to use the new journaling functionality (and mostly succeeded, there were just a couple minor touch-ups we needed in our renderer to absorb that change without using the journaling quite yet). In contrast to that, this patch really kind of hits the renderer in the face, requiring substantial changes in may areas all at once.

I'm also looking at how device_string.h was removed entirely, no more definition for StringParam or DeviceString, etc. I think it would be better -- at least for now, with a more thorough removal possibly down the line -- if perhaps we should still have a skeleton device_string.h that includes hashes.h, and still defines StringParam and DeviceString (both are ustringhash). And maybe some of those removed functions in oslconfig.h that converted various things to the string representation were useful after all. I think that those definitions, or something like it, also serves a long-term purpose, which is to have a named type that means "the representation of strings we're using inside shaders". By changing all those StringParam references to ustringhash, just like with the llvm_load_string issue I mentioned earlier, it creates a much larger code delta than if you had merely changed the definition of StringParam, and that would also keep flexibility that if some time in the future we wanted to change execution-time string representation again.

So, after taking quite a while to really read through this all in detail, I feel kinda bad asking for changes. And in truth, I would be very happy to do some work on my end to prototype what I mean and see if I can add a few of those oslconfig.h and device_string.h things back and see how much that reduces the internal deltas and the amount of renderer-side change it allows us to postpone or eliminate. I've already done some of this to help me on the renderer side and was considering offering it as a second PR after this merges but before the renderers all start doing their changes. But maybe I should do a little more of that work and show you how it looks before we merge this, and perhaps just amend it to this PR if you like where I'm going with it.

Also... as I went down the rabbit hole of converting our renderer to adapt to this (which I still haven't completed, it's a bigger job than I thought), I realized not only that it's a big change for the renderers to absorb and incorporates a lot of both binary and source-altering breaks, but also I'm not very confident that it's "final". By that I mean, what are the chances that after this is merged, we find a handful of other understandable, minor follow-ups that are necessary, and that also break compatibility? I would say fairly high. So I'm very nervous about my original plan to merge this, and then branch for 1.13 beta, because it may turn out to be broken in ways that require more binary-incompatible or source-incompatible fixes that screw up 1.13. So, instead, my current inclination is to branch first, making 1.13 a binary-compatible branch of what we currently have in main, from here on out only backporting things that don't break compatibility, and then this would be the first big breaking change that goes into the future 1.14. The only down-side to that is that subsequent unrelated fixes might be more difficult to backport because we will so immediately and so thoroughly diverge from the new release branch -- eliminating the grace period we usually have where for at least a few months after branching a release, fixes in main are very often trivially backportable to the release without having to alter it or reimplement the fix separately on each branch (another reason why I'm inclined to reshape this PR to just make fewer textual changes all over the code base, to narrow the amount of divergence and future merge conflicts).

[AlexMWells]

@lgritz this PR includes a lot of cleanup. Makes things explicit (as far as ustrings and hashes go). And yes we did make lots of changes to remove confusion, why have multiple ways of doing things. Although that means more changes to user code to update to it. But minimizing the delta at least in the OSL code base wasn't really a goal per say, but trying to be explicit what was passed to who was. And removing as many casts between char * and int64_t (hash code) as possible including llvm JIT.

Now, if adding back a deprecated DeviceStrings or equivalent makes Renderer transition easier, then maybe that should happen. Although I think we imagined a renderer might just define those itself vs. adding them back in OSL proper. But either way would work.

Specifics: llvm_load_string used to at one point be a customization point for CUDA so it could go off to some global variable and actually load a value, after switching to hash_codes, nothing was loaded anymore. So the cleanup was to rename the remaining variations to what they actually do which is to emit a const hash. Much more clear when reading/debugging the code (we didn't make the change by accident). This shouldn't have impact on renderers though. And yes it could be renamed back to llvm_load_string and there would be fewer diffs.

So if you still want to be able to switch back and forth between ustrings and ustringhash'es then this isn't that PR. We went out of our way to rip out the old way and what would be come unused/confusing as the interfaces.

And yes you could add back into OSL

typedef ustringhash StringParam;
typedef ustringhash DeviceString;
typedef ustringhash ustringrep;
#define STRING_PARAMS(unquoted_string) OSL_HASHIFY(unquoted_string)

and reduce the textual differences in this PR. Which might make back patching easier. Although we started with a goal ripping off the band aid and just removing ustringrep and being explicit everywhere.

[AlexMWells]

NOTE: if a renderer chooses to use STRING_PARAMS(mystring), then it is responsible for instantiating a ustring of "mystring" on the host someplace so the reverse lookup of the hashed mystring exists in the ustring table. This is partly why we recommend a rs_strdecls.h approach so the text of the string exists in one place that is #included to produce hashcodes on device and again to populate ustrings on the host.

[lgritz]

My current thinking is to

1. Branch a dev-1.13 prior to this patch and use that as the binary-compatible basis for the 1.13 release family.
2. Merge this as-is into the new main (aka future 1.14), but instruct renderer authors not to sync with it quite yet, because it's currently rough sailing to switch a renderer.
3. Submit a separate PR afterwards that contains a fairly minimal set of additional changes to make the renderer side easier and more stageable, as well as to establish a 'ustringrep' type alias that more clearly distinguishes between the things that are irrevocably ustring hashes, and those that are "the representation for strings in shader execution" which we all know today, for most of us, is the same as a ustringhash, but makes it easier for a renderer to diverge and use a different representation.