Fix GPU interpolated param initialization

[chellmuth]

Description

Currently, the GPU will crash when initializing non-string interpolated parameters with default values.

It looks like this is a regression from the ustringhash conversion in 12530f115dfc759b00ee4de6dc27bf5fa3f32fe0. I believe that before the phase 3 commit, we had global variables on GPU that we memcpy'd the defaults values from. Now after their removal, we're memcpy'ing from an illegal address.

As a proposed fix I removed the entire memcpy branch of the code and let interpolated parameters fall back to the default code path that stores the values directly. I'm open to instead guarding the removed block with "&& !gpu" making the patch have no effect on cpu, but leaned towards having unified code paths. Still, it's not an area of the code I'm very familiar with, any feedback would be appreciated.

Tests

Added userdata-defaults that tests against the regression.

Checklist:

• [x] I have read the contribution guidelines.
• [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. If I haven't already run clang-format v17 before submitting, I definitely will look at the CI test that runs clang-format and fix anything that it highlights as being nonconforming.

[lgritz]

This looks correct to me.

I think there is a possibility that setting the values one by one could be a slight speed penalty compared to the memcpy, especially if there are many values comprising the parameter. But it would be very difficult to know for sure if there were any situations in which this would make a noticeable difference.

My intuition is that it probably doesn't matter -- I mean, there are not typically a lot of interpolated parameters for a shader, and the ones that exist are usually short, like a Pref point, interpolated shading normal, or texture coordinates, but very rarely would an interpolated parameter be a long array, say.

So I think Chris is right, the tiny chance that maybe this is a barely measurably small speed hit is outweighed by simplifying the code.

I would love to get another pair of eyes on this for a sanity check. Maybe @AlexMWells or @steenax86 can say if they see any downside that we've missed?

[lgritz]

BTW, don't sweat the icc and icx CI failures, that's unrelated and failing on all branches. I think Intel broke their compiler downloads again.

[sfriedmapixar]

@lgritz, won't this break interactive ReParameter edits? I thought that memcpy is what pulls values from the single place where the parameters are stored so they could be edited into their uses...if those are baked down into the code, I'd expect edits to stop working ... or am I missing something?

[lgritz]

@sfriedmapixar My heart just skipped a beat when I read yournote, but looking at it again, it's interpolated params that are being changed, not interactive.

I think it's ok? But @chellmuth can you double check that this doesn't inadvertently break interactive param adjustment?

[chellmuth]

Yeah, reparameter is still working fine after this change.

Like Larry pointed out, this change is only affecting interpolated parameters, so in theory if interactive parameters are on this codepath all the normal non-interpolated ones would have already been skipping this removed else-if clause.

In practice, I don't believe interactive parameters are initialized with this function llvm_assign_initial_value. We guard against it in the caller, build_llvm_instance:

        // Skip if it's an interpolated (userdata) parameter and we're
        // initializing them lazily, or if it's an interactively-adjusted
        // parameter.
        if ((s.symtype() == SymTypeParam || s.symtype() == SymTypeOutputParam)
            && !s.typespec().is_closure() && !s.connected()
            && !s.connected_down()
            && (s.interactive()
                || (s.interpolated() && shadingsys().lazy_userdata())))
            continue;
        // Set initial value for params (may contain init ops)
        llvm_assign_initial_value(s);

[sfriedmapixar]

Interesting -- so there's that block you quoted, that skips it for interactive() and there's a similar but not-quite-the-same that eventually calls llvm_assign_initial_values in llvm_gen.cpp#llvm_gen_useparam

    // If it's an interpolated (userdata) parameter and we're
    // initializing them lazily, now we have to do it.
    if ((sym.symtype() == SymTypeParam
         || sym.symtype() == SymTypeOutputParam)
        && sym.interpolated() && !sym.typespec().is_closure()
        && !sym.connected() && !sym.connected_down()
        && rop.shadingsys().lazy_userdata()) {
        rop.llvm_assign_initial_value(sym);
    }

so my question is, where do params that are marked interactive get initialized?

[chellmuth]

My understanding is that for an interactive parameter there is no explicit initialization. The value of the parameter is always retrieved directly from the interactive data block passed in because we assume the renderer initialized it correctly before shader execution.

From getLLVMSymbolBase in backendllvm.cpp:

    if (sym.symtype() == SymTypeParam && sym.interactive()) {
        // Special case for interactively-edited parameters -- they live in
        // the interactive data block for the group.
        // Generate the pointer to this symbol by offsetting into the
        // interactive data block.
        int offset = group().interactive_param_offset(layer(), sym.name());
        return ll.offset_ptr(m_llvm_interactive_params_ptr, offset,
                             llvm_ptr_type(sym.typespec().elementtype()));
    }

I'll let Larry correct me if I'm wrong though :)

[lgritz]

I think that's right -- for "interactive" parameters, there's some managed memory, but the renderer puts the values there as the sliders change or whatever.

[sfriedmapixar]

I see, thanks for the explaination. In that case this seems OK to me as well.