From 2de0c629d5e782099df790153cb4949d354f9bb0 Mon Sep 17 00:00:00 2001 From: Stephen Friedman Date: Tue, 12 May 2020 09:24:50 -0700 Subject: [PATCH 1/2] Add support for connecting individual scalar outputs to a dynamic array input. This requires you to first declare a setting for the dynamic array input that will be used to size and provide values for any unconnected elements of the dynamically sized array. After that, individual scalar->arrayelem connections can be specified from any number of different upstream shading nodes. This facilitates gathering inputs from across your shading network, for example several procedurally generated color signals and using them to drive inputs to shading nodes that are best left dynamically sized, such as the inputs to spline operations. This includes a new test in vararray-scalar-connect to demonstrate and test the new connection method. : Signed-off-by: Stephen Friedman --- src/cmake/testing.cmake | 1 + src/liboslexec/backendllvm.cpp | 71 +++++--- src/liboslexec/backendllvm.h | 8 +- src/liboslexec/batched_backendllvm.cpp | 84 +++++---- src/liboslexec/batched_backendllvm.h | 6 +- src/liboslexec/batched_llvm_instance.cpp | 169 ++++++++++++++++-- src/liboslexec/llvm_instance.cpp | 165 +++++++++++++++-- src/liboslexec/shadingsys.cpp | 21 ++- testsuite/vararray-scalar-connect/BATCHED | 0 testsuite/vararray-scalar-connect/ref/out.txt | 20 +++ testsuite/vararray-scalar-connect/run.py | 14 ++ testsuite/vararray-scalar-connect/test.osl | 17 ++ .../vararray-scalar-connect/upstream.osl | 30 ++++ 13 files changed, 515 insertions(+), 91 deletions(-) create mode 100644 testsuite/vararray-scalar-connect/BATCHED create mode 100644 testsuite/vararray-scalar-connect/ref/out.txt create mode 100755 testsuite/vararray-scalar-connect/run.py create mode 100644 testsuite/vararray-scalar-connect/test.osl create mode 100644 testsuite/vararray-scalar-connect/upstream.osl diff --git a/src/cmake/testing.cmake b/src/cmake/testing.cmake index fc5e956b0..b69c6cf1f 100644 --- a/src/cmake/testing.cmake +++ b/src/cmake/testing.cmake @@ -398,6 +398,7 @@ macro (osl_add_all_tests) userdata userdata-defaults userdata-partial userdata-custom userdata-passthrough vararray-connect vararray-default vararray-deserialize vararray-param + vararray-scalar-connect vecctr vector vector-reg wavelength_color wavelength_color-reg Werror xml xml-reg ) diff --git a/src/liboslexec/backendllvm.cpp b/src/liboslexec/backendllvm.cpp index a72854152..89882fcfa 100644 --- a/src/liboslexec/backendllvm.cpp +++ b/src/liboslexec/backendllvm.cpp @@ -742,8 +742,8 @@ BackendLLVM::llvm_test_nonzero(Symbol& val, bool test_derivs) bool -BackendLLVM::llvm_assign_impl(Symbol& Result, Symbol& Src, int arrayindex, - int srccomp, int dstcomp) +BackendLLVM::llvm_assign_impl(Symbol& Result, Symbol& Src, int srcarrayindex, + int dstarrayindex, int srccomp, int dstcomp) { OSL_DASSERT(!Result.typespec().is_structure()); OSL_DASSERT(!Src.typespec().is_structure()); @@ -751,28 +751,31 @@ BackendLLVM::llvm_assign_impl(Symbol& Result, Symbol& Src, int arrayindex, const TypeSpec& result_t(Result.typespec()); const TypeSpec& src_t(Src.typespec()); - llvm::Value* arrind = arrayindex >= 0 ? ll.constant(arrayindex) : NULL; + llvm::Value* sarrind = srcarrayindex >= 0 ? ll.constant(srcarrayindex) + : NULL; + llvm::Value* darrind = dstarrayindex >= 0 ? ll.constant(dstarrayindex) + : NULL; if (Result.typespec().is_closure() || Src.typespec().is_closure()) { if (Src.typespec().is_closure()) { - llvm::Value* srcval = llvm_load_value(Src, 0, arrind, 0); - llvm_store_value(srcval, Result, 0, arrind, 0); + llvm::Value* srcval = llvm_load_value(Src, 0, sarrind, 0); + llvm_store_value(srcval, Result, 0, darrind, 0); } else { llvm::Value* null = ll.constant_ptr(NULL, ll.type_void_ptr()); - llvm_store_value(null, Result, 0, arrind, 0); + llvm_store_value(null, Result, 0, darrind, 0); } return true; } if (Result.typespec().is_matrix() && Src.typespec().is_int_or_float()) { // Handle m=f, m=i separately - llvm::Value* src = llvm_load_value(Src, 0, arrind, 0, + llvm::Value* src = llvm_load_value(Src, 0, sarrind, 0, TypeDesc::FLOAT /*cast*/); // m=f sets the diagonal components to f, the others to zero llvm::Value* zero = ll.constant(0.0f); for (int i = 0; i < 4; ++i) for (int j = 0; j < 4; ++j) - llvm_store_value(i == j ? src : zero, Result, 0, arrind, + llvm_store_value(i == j ? src : zero, Result, 0, darrind, i * 4 + j); llvm_zero_derivs(Result); // matrices don't have derivs currently return true; @@ -786,7 +789,7 @@ BackendLLVM::llvm_assign_impl(Symbol& Result, Symbol& Src, int arrayindex, // will ensure they are the same size, except for certain cases where // the size difference is intended (by the optimizer). if (result_t.is_array() && !Src.is_constant() && src_t.is_array() - && arrayindex == -1) { + && srcarrayindex == -1 && dstarrayindex == -1) { OSL_DASSERT(assignable(result_t.elementtype(), src_t.elementtype())); llvm::Value* resultptr = llvm_get_pointer(Result); llvm::Value* srcptr = llvm_get_pointer(Src); @@ -806,12 +809,14 @@ BackendLLVM::llvm_assign_impl(Symbol& Result, Symbol& Src, int arrayindex, // The following code handles f=f, f=i, v=v, v=f, v=i, m=m, s=s. // Remember that llvm_load_value will automatically convert scalar->triple. - TypeDesc rt = Result.typespec().simpletype(); - TypeDesc basetype = TypeDesc::BASETYPE(rt.basetype); - const int num_components = rt.aggregate; - const bool singlechan = (srccomp != -1) || (dstcomp != -1); - if (!singlechan) { - if (rt.is_array() && arrayindex == -1) { + TypeDesc rt = Result.typespec().simpletype(); + TypeDesc basetype = TypeDesc::BASETYPE(rt.basetype); + const int num_components = rt.aggregate; + const bool singlechan = (srccomp != -1) || (dstcomp != -1); + const bool scalar_to_array_elem = (Src.arraylen() == 0 + && dstarrayindex != -1); + if (!singlechan && !scalar_to_array_elem) { + if (rt.is_array() && srcarrayindex == -1 && dstarrayindex == -1) { // Initialize entire array const int num_elements = std::min(rt.numelements(), src_t.simpletype().numelements()); @@ -836,14 +841,24 @@ BackendLLVM::llvm_assign_impl(Symbol& Result, Symbol& Src, int arrayindex, for (int i = 0; i < num_components; ++i) { llvm::Value* src_val = Src.is_constant() - ? llvm_load_constant_value(Src, arrayindex, i, + ? llvm_load_constant_value(Src, srcarrayindex, i, basetype) - : llvm_load_value(Src, 0, arrind, i, basetype); + : llvm_load_value(Src, 0, sarrind, i, basetype); if (!src_val) return false; - llvm_store_value(src_val, Result, 0, arrind, i); + llvm_store_value(src_val, Result, 0, darrind, i); } } + } else if (scalar_to_array_elem) { + for (int i = 0; i < num_components; ++i) { + llvm::Value* src_val + = Src.is_constant() + ? llvm_load_constant_value(Src, -1, i, basetype) + : llvm_load_value(Src, 0, NULL, i, basetype); + if (!src_val) + return false; + llvm_store_value(src_val, Result, 0, darrind, i); + } } else { // connect individual component of an aggregate type // set srccomp to 0 for case when src is actually a float @@ -851,17 +866,18 @@ BackendLLVM::llvm_assign_impl(Symbol& Result, Symbol& Src, int arrayindex, srccomp = 0; llvm::Value* src_val = Src.is_constant() - ? llvm_load_constant_value(Src, arrayindex, srccomp, basetype) - : llvm_load_value(Src, 0, arrind, srccomp, basetype); + ? llvm_load_constant_value(Src, srcarrayindex, srccomp, + basetype) + : llvm_load_value(Src, 0, sarrind, srccomp, basetype); if (!src_val) return false; // write source float into all components when dstcomp == -1, otherwise // the single element requested. if (dstcomp == -1) { for (int i = 0; i < num_components; ++i) - llvm_store_value(src_val, Result, 0, arrind, i); + llvm_store_value(src_val, Result, 0, darrind, i); } else - llvm_store_value(src_val, Result, 0, arrind, dstcomp); + llvm_store_value(src_val, Result, 0, darrind, dstcomp); } // Handle derivatives @@ -871,18 +887,19 @@ BackendLLVM::llvm_assign_impl(Symbol& Result, Symbol& Src, int arrayindex, if (!singlechan) { for (int d = 1; d <= 2; ++d) { for (int i = 0; i < num_components; ++i) { - llvm::Value* val = llvm_load_value(Src, d, arrind, i); - llvm_store_value(val, Result, d, arrind, i); + llvm::Value* val = llvm_load_value(Src, d, sarrind, i); + llvm_store_value(val, Result, d, darrind, i); } } } else { for (int d = 1; d <= 2; ++d) { - llvm::Value* val = llvm_load_value(Src, d, arrind, srccomp); + llvm::Value* val = llvm_load_value(Src, d, sarrind, + srccomp); if (dstcomp == -1) { for (int i = 0; i < num_components; ++i) - llvm_store_value(val, Result, d, arrind, i); + llvm_store_value(val, Result, d, darrind, i); } else - llvm_store_value(val, Result, d, arrind, dstcomp); + llvm_store_value(val, Result, d, darrind, dstcomp); } } } else { diff --git a/src/liboslexec/backendllvm.h b/src/liboslexec/backendllvm.h index 9c0dbaf9c..7cc64df89 100644 --- a/src/liboslexec/backendllvm.h +++ b/src/liboslexec/backendllvm.h @@ -75,7 +75,8 @@ class BackendLLVM final : public OSOProcessorBase { void llvm_create_constant(const Symbol& sym); - void llvm_assign_initial_value(const Symbol& sym, bool force = false); + void llvm_assign_initial_value(const Symbol& sym, bool force = false, + int arrayindex = -1); llvm::LLVMContext& llvm_context() const { return ll.context(); } AllocationMap& named_values() { return m_named_values; } @@ -256,8 +257,9 @@ class BackendLLVM final : public OSOProcessorBase { /// Implementation of Simple assignment. If arrayindex >= 0, in /// designates a particular array index to assign. - bool llvm_assign_impl(Symbol& Result, Symbol& Src, int arrayindex = -1, - int srccomp = -1, int dstcomp = -1); + bool llvm_assign_impl(Symbol& Result, Symbol& Src, int srcarrayindex = -1, + int dstarrayindex = -1, int srccomp = -1, + int dstcomp = -1); /// Convert the name of a global (and its derivative index) into the diff --git a/src/liboslexec/batched_backendllvm.cpp b/src/liboslexec/batched_backendllvm.cpp index e94122ef4..70a72fec3 100644 --- a/src/liboslexec/batched_backendllvm.cpp +++ b/src/liboslexec/batched_backendllvm.cpp @@ -1894,7 +1894,8 @@ BatchedBackendLLVM::llvm_test_nonzero(const Symbol& val, bool test_derivs) bool BatchedBackendLLVM::llvm_assign_impl(const Symbol& Result, const Symbol& Src, - int arrayindex, int srccomp, int dstcomp) + int srcarrayindex, int dstarrayindex, + int srccomp, int dstcomp) { OSL_DEV_ONLY(std::cout << "llvm_assign_impl arrayindex=" << arrayindex << " Result(" << Result.name() @@ -1914,25 +1915,28 @@ BatchedBackendLLVM::llvm_assign_impl(const Symbol& Result, const Symbol& Src, const TypeSpec& result_t(Result.typespec()); const TypeSpec& src_t(Src.typespec()); - llvm::Value* arrind = arrayindex >= 0 ? ll.constant(arrayindex) : NULL; + llvm::Value* sarrind = srcarrayindex >= 0 ? ll.constant(srcarrayindex) + : NULL; + llvm::Value* darrind = dstarrayindex >= 0 ? ll.constant(dstarrayindex) + : NULL; if (Result.typespec().is_closure() || Src.typespec().is_closure()) { if (Src.typespec().is_closure()) { - llvm::Value* srcval = llvm_load_value(Src, 0, arrind, 0, + llvm::Value* srcval = llvm_load_value(Src, 0, sarrind, 0, TypeUnknown, op_is_uniform); - llvm_store_value(srcval, Result, 0, arrind, 0); + llvm_store_value(srcval, Result, 0, darrind, 0); } else { llvm::Value* null_value = ll.constant_ptr(NULL, ll.type_void_ptr()); if (!op_is_uniform) null_value = ll.widen_value(null_value); - llvm_store_value(null_value, Result, 0, arrind, 0); + llvm_store_value(null_value, Result, 0, darrind, 0); } return true; } if (Result.typespec().is_matrix() && Src.typespec().is_int_or_float()) { // Handle m=f, m=i separately - llvm::Value* src = llvm_load_value(Src, 0, arrind, 0, + llvm::Value* src = llvm_load_value(Src, 0, sarrind, 0, TypeDesc::FLOAT /*cast*/, op_is_uniform); // m=f sets the diagonal components to f, the others to zero @@ -1944,7 +1948,7 @@ BatchedBackendLLVM::llvm_assign_impl(const Symbol& Result, const Symbol& Src, for (int i = 0; i < 4; ++i) for (int j = 0; j < 4; ++j) - llvm_store_value(i == j ? src : zero, Result, 0, arrind, + llvm_store_value(i == j ? src : zero, Result, 0, darrind, i * 4 + j); llvm_zero_derivs(Result); // matrices don't have derivs currently return true; @@ -1954,34 +1958,36 @@ BatchedBackendLLVM::llvm_assign_impl(const Symbol& Result, const Symbol& Src, // The following code handles f=f, f=i, v=v, v=f, v=i, m=m, s=s. // Remember that llvm_load_value will automatically convert scalar->triple. - TypeDesc rt = Result.typespec().simpletype(); - TypeDesc basetype = TypeDesc::BASETYPE(rt.basetype); - const int num_components = rt.aggregate; - const bool singlechan = (srccomp != -1) || (dstcomp != -1); + TypeDesc rt = Result.typespec().simpletype(); + TypeDesc basetype = TypeDesc::BASETYPE(rt.basetype); + const int num_components = rt.aggregate; + const bool singlechan = (srccomp != -1) || (dstcomp != -1); + const bool scalar_to_array_elem = (Src.arraylen() == 0 + && dstarrayindex != -1); // Because we are not mem-copying arrays wholesale, // We will add an outer array index loop to copy 1 element or entire array - int start_array_index = arrayindex; + int start_array_index = srcarrayindex; int end_array_index = start_array_index + 1; - if (start_array_index == -1) { + if (srcarrayindex == -1 && dstarrayindex == -1) { if (result_t.is_array() && src_t.is_array()) { - start_array_index = 0; - end_array_index = std::min(result_t.arraylength(), - src_t.arraylength()); + start_array_index = dstarrayindex = srcarrayindex = 0; + end_array_index = std::min(result_t.arraylength(), + src_t.arraylength()); } } - for (arrayindex = start_array_index; arrayindex < end_array_index; - ++arrayindex) { - arrind = arrayindex >= 0 ? ll.constant(arrayindex) : NULL; - - if (!singlechan) { + for (srcarrayindex = start_array_index; srcarrayindex < end_array_index; + ++srcarrayindex, ++dstarrayindex) { + sarrind = srcarrayindex >= 0 ? ll.constant(srcarrayindex) : NULL; + darrind = dstarrayindex >= 0 ? ll.constant(dstarrayindex) : NULL; + if (!singlechan && !scalar_to_array_elem) { for (int i = 0; i < num_components; ++i) { // Automatically handle widening the source value to match the destination's llvm::Value* src_val = Src.is_constant() - ? llvm_load_constant_value(Src, arrayindex, i, + ? llvm_load_constant_value(Src, srcarrayindex, i, basetype, op_is_uniform) - : llvm_load_value(Src, 0, arrind, i, basetype, + : llvm_load_value(Src, 0, sarrind, i, basetype, op_is_uniform); if (!src_val) return false; @@ -1991,7 +1997,19 @@ BatchedBackendLLVM::llvm_assign_impl(const Symbol& Result, const Symbol& Src, << ll.llvm_typenameof(src_val) << std::endl); // The llvm_load_value above should have handled bool to int conversions // when the basetype == Typedesc::INT - llvm_store_value(src_val, Result, 0, arrind, i); + llvm_store_value(src_val, Result, 0, darrind, i); + } + } else if (scalar_to_array_elem) { + for (int i = 0; i < num_components; ++i) { + llvm::Value* src_val + = Src.is_constant() + ? llvm_load_constant_value(Src, -1, i, basetype, + op_is_uniform) + : llvm_load_value(Src, 0, NULL, i, basetype, + op_is_uniform); + if (!src_val) + return false; + llvm_store_value(src_val, Result, 0, darrind, i); } } else { // connect individual component of an aggregate type @@ -2001,9 +2019,9 @@ BatchedBackendLLVM::llvm_assign_impl(const Symbol& Result, const Symbol& Src, // Automatically handle widening the source value to match the destination's llvm::Value* src_val = Src.is_constant() - ? llvm_load_constant_value(Src, arrayindex, srccomp, + ? llvm_load_constant_value(Src, srcarrayindex, srccomp, basetype, op_is_uniform) - : llvm_load_value(Src, 0, arrind, srccomp, basetype, + : llvm_load_value(Src, 0, sarrind, srccomp, basetype, op_is_uniform); if (!src_val) return false; @@ -2015,9 +2033,9 @@ BatchedBackendLLVM::llvm_assign_impl(const Symbol& Result, const Symbol& Src, // the single element requested. if (dstcomp == -1) { for (int i = 0; i < num_components; ++i) - llvm_store_value(src_val, Result, 0, arrind, i); + llvm_store_value(src_val, Result, 0, darrind, i); } else - llvm_store_value(src_val, Result, 0, arrind, dstcomp); + llvm_store_value(src_val, Result, 0, darrind, dstcomp); } // Handle derivatives @@ -2036,25 +2054,25 @@ BatchedBackendLLVM::llvm_assign_impl(const Symbol& Result, const Symbol& Src, if (Src.has_derivs()) { // src and result both have derivs -- copy them // allow a uniform Src to store to a varying Result, - val = llvm_load_value(Src, d, arrind, i, + val = llvm_load_value(Src, d, sarrind, i, TypeUnknown, op_is_uniform); } - llvm_store_value(val, Result, d, arrind, i); + llvm_store_value(val, Result, d, darrind, i); } } else { llvm::Value* val = zero; if (Src.has_derivs()) { // src and result both have derivs -- copy them // allow a uniform Src to store to a varying Result, - val = llvm_load_value(Src, d, arrind, srccomp, + val = llvm_load_value(Src, d, sarrind, srccomp, TypeUnknown, op_is_uniform); } if (dstcomp == -1) { for (int i = 0; i < num_components; ++i) - llvm_store_value(val, Result, d, arrind, i); + llvm_store_value(val, Result, d, darrind, i); } else - llvm_store_value(val, Result, d, arrind, dstcomp); + llvm_store_value(val, Result, d, darrind, dstcomp); } } } diff --git a/src/liboslexec/batched_backendllvm.h b/src/liboslexec/batched_backendllvm.h index 9bafa7767..fce47a577 100644 --- a/src/liboslexec/batched_backendllvm.h +++ b/src/liboslexec/batched_backendllvm.h @@ -74,7 +74,7 @@ class BatchedBackendLLVM : public OSOProcessorBase { void llvm_assign_initial_value(const Symbol& sym, llvm::Value* llvm_initial_shader_mask_value, - bool force = false); + bool force = false, int arrayindex = -1); llvm::LLVMContext& llvm_context() const { return ll.context(); } AllocationMap& named_values() { return m_named_values; } @@ -335,8 +335,8 @@ class BatchedBackendLLVM : public OSOProcessorBase { /// Implementation of Simple assignment. If arrayindex >= 0, in /// designates a particular array index to assign. bool llvm_assign_impl(const Symbol& Result, const Symbol& Src, - int arrayindex = -1, int srccomp = -1, - int dstcomp = -1); + int srcarrayindex = -1, int dstarrayindex = -1, + int srccomp = -1, int dstcomp = -1); /// Convert the name of a global (and its derivative index) into the diff --git a/src/liboslexec/batched_llvm_instance.cpp b/src/liboslexec/batched_llvm_instance.cpp index 8e6ff0a76..f92aa7743 100644 --- a/src/liboslexec/batched_llvm_instance.cpp +++ b/src/liboslexec/batched_llvm_instance.cpp @@ -994,8 +994,14 @@ BatchedBackendLLVM::llvm_type_closure_component_wide_ptr() void BatchedBackendLLVM::llvm_assign_initial_value( - const Symbol& sym, llvm::Value* llvm_initial_shader_mask_value, bool force) + const Symbol& sym, llvm::Value* llvm_initial_shader_mask_value, bool force, + int arrayindex) { + // the array-index is intended for sparse param connections and we use + // force to explicitly initialize them, so we expect that anything with a + // valid array index will be a param and is forced. + OSL_DASSERT((sym.symtype() == SymTypeParam && force) || arrayindex == -1); + // Don't write over connections! Connection values are written into // our layer when the earlier layer is run, as part of its code. So // we just don't need to initialize it here at all. @@ -1263,8 +1269,15 @@ BatchedBackendLLVM::llvm_assign_initial_value( OSL_ASSERT((!sym.is_uniform() || !sym.renderer_output()) && "All render outputs should be varying"); - - for (int a = 0, c = 0; a < arraylen; ++a) { + int a = 0, c = 0; + if (arrayindex != -1) { + // If we have an array index, setup the loop variables to only + // execute that index + a = arrayindex; + c = num_components * arrayindex; + arraylen = arrayindex + 1; + } + for (; a < arraylen; ++a) { llvm::Value* arrind = sym.typespec().is_array() ? ll.constant(a) : NULL; if (sym.typespec().is_closure_based()) @@ -2132,6 +2145,78 @@ BatchedBackendLLVM::build_llvm_instance(bool groupentry) llvm_assign_initial_value(s, llvm_initial_shader_mask_value); } + // Track all symbols who needed 'partial' initialization + tsl::robin_set initedsyms; + + // Make a third pass for sparsely connected array parameters. + // Only init the unconnected elements, upstream shaders are responsible for + // initing and overwritting any connected elements. + for (int c = 0, Nc = inst()->nconnections(); c < Nc; ++c) { + const Connection& con(inst()->connection(c)); + + + Symbol* dstsym(inst()->symbol(con.dst.param)); + // Find any sparse connections, and then find any remaining + // unconnected entries to initialize their values here, upstream + // will have handled all the connected ones. + if (con.dst.arrayindex != -1 && con.src.arrayindex == -1 + && initedsyms.count(dstsym) == 0) { + initedsyms.insert(dstsym); + int arraylen = 0; + TypeSpec dstType = dstsym->typespec(); + if (dstType.is_unsized_array()) { + const ShaderInstance::SymOverrideInfo* sym_override + = inst()->instoverride(con.dst.param); + + OSL_DASSERT(sym_override); + + if (sym_override + && (sym_override->valuesource() == Symbol::InstanceVal + || sym_override->valuesource() + == Symbol::ConnectedVal)) { + arraylen = sym_override->arraylen(); + } else { + arraylen = 0; + } + + } else { + arraylen = dstType.numelements(); + } + + std::vector inited(arraylen, false); + unsigned ninit = arraylen; + + if (con.dst.arrayindex < arraylen) { + inited[con.dst.arrayindex] = true; + ninit--; + } + for (int rc = c + 1; rc < Nc && ninit; ++rc) { + const Connection& next(inst()->connection(rc)); + // Allow redundant/overwriting connections, i.e: + // 1. connect layer.value[i] connect layer.value[j] + // 2. connect layer.value connect layer.value + if (inst()->symbol(next.dst.param) == dstsym) { + if (next.dst.arrayindex != -1) { + if (next.dst.arrayindex < arraylen + && !inited[next.dst.arrayindex]) { + inited[next.dst.arrayindex] = true; + --ninit; + } + } else + ninit = 0; + } + } + if (ninit) { + for (int e = 0; e < arraylen; ++e) { + if (!inited[e]) { + llvm_assign_initial_value(*dstsym, initial_shader_mask, + true, e); + } + } + } + } + } + // All the symbols are stack allocated now. if (groupentry) { @@ -2157,9 +2242,6 @@ BatchedBackendLLVM::build_llvm_instance(bool groupentry) ll.op_branch(m_exit_instance_block); // also sets insert point } - // Track all symbols who needed 'partial' initialization - tsl::robin_set initedsyms; - { // The current mask could be altered by early returns or exit // But for copying output parameters to connected shaders, @@ -2183,9 +2265,6 @@ BatchedBackendLLVM::build_llvm_instance(bool groupentry) for (int c = 0, Nc = child->nconnections(); c < Nc; ++c) { const Connection& con(child->connection(c)); if (con.srclayer == this->layer()) { - OSL_ASSERT( - con.src.arrayindex == -1 && con.dst.arrayindex == -1 - && "no support for individual array element connections"); // Validate unsupported connection vecSrc -> vecDst[j] OSL_ASSERT( (con.dst.channel == -1 @@ -2234,12 +2313,80 @@ BatchedBackendLLVM::build_llvm_instance(bool groupentry) } } + if (con.dst.arrayindex != -1 && con.src.arrayindex == -1 + && initedsyms.count(dstsym) == 0) { + initedsyms.insert(dstsym); + // This may just be a sparse set of connections, so we go + // ahead and run the initializers if any would otherwise + // be left out + int initArrayIndex = con.dst.arrayindex; + + int arraylen = 0; + TypeSpec dstType = dstsym->typespec(); + if (dstType.is_unsized_array()) { + const ShaderInstance::SymOverrideInfo* + sym_override + = child->instoverride(con.dst.param); + + OSL_DASSERT(sym_override); + + if (sym_override + && (sym_override->valuesource() + == Symbol::InstanceVal + || sym_override->valuesource() + == Symbol::ConnectedVal)) { + arraylen = sym_override->arraylen(); + } else { + arraylen = 0; + } + + } else { + arraylen = dstType.numelements(); + } + + std::vector inited(arraylen, false); + inited[con.dst.arrayindex] = true; + unsigned ninit = arraylen - 1; + llvm_assign_initial_value(*dstsym, + initial_shader_mask, true, + initArrayIndex); + for (int rc = c + 1; rc < Nc && ninit; ++rc) { + const Connection& next(child->connection(rc)); + if (next.srclayer == this->layer()) { + // Allow redundant/overwriting connections, i.e: + // 1. connect layer.value[i] connect layer.value[j] + // 2. connect layer.value connect layer.value + if (child->symbol(next.dst.param) + == dstsym) { + if (next.dst.arrayindex != -1) { + initArrayIndex = next.dst.arrayindex; + if (!inited[initArrayIndex]) { + inited[initArrayIndex] = true; + llvm_assign_initial_value( + *dstsym, + initial_shader_mask, true, + initArrayIndex); + } + } else { + llvm_assign_initial_value( + *dstsym, initial_shader_mask, + true); + break; + } + } + } + } + } + // llvm_run_connected_layers tracks layers that have been run, // so no need to do it here as well llvm_run_connected_layers(*srcsym, con.src.param); - // Perform actual copy assignment from src to dest sym - llvm_assign_impl(*dstsym, *srcsym, -1, con.src.channel, + // The upstream run will write the value to it's local output + // parameter location, now we just have to copy it to the + // destination layer's input location. + llvm_assign_impl(*dstsym, *srcsym, con.src.arrayindex, + con.dst.arrayindex, con.src.channel, con.dst.channel); } } diff --git a/src/liboslexec/llvm_instance.cpp b/src/liboslexec/llvm_instance.cpp index 368ed4c38..c80426870 100644 --- a/src/liboslexec/llvm_instance.cpp +++ b/src/liboslexec/llvm_instance.cpp @@ -537,13 +537,19 @@ BackendLLVM::llvm_create_constant(const Symbol& sym) } void -BackendLLVM::llvm_assign_initial_value(const Symbol& sym, bool force) +BackendLLVM::llvm_assign_initial_value(const Symbol& sym, bool force, + int arrayindex) { + // the array-index is intended for sparse param connections and we use + // force to explicitly initialize them, so we expect that anything with a + // valid array index will be a param and is forced. + OSL_DASSERT((sym.symtype() == SymTypeParam && force) || arrayindex == -1); + // Don't write over connections! Connection values are written into // our layer when the earlier layer is run, as part of its code. So // we just don't need to initialize it here at all. if (!force && sym.valuesource() == Symbol::ConnectedVal - && !sym.typespec().is_closure_based()) + && !sym.typespec().is_closure_based() && arrayindex == -1) return; // For "globals" that are closures, there is nothing to initialize. if (sym.typespec().is_closure_based() && sym.symtype() == SymTypeGlobal) @@ -553,7 +559,7 @@ BackendLLVM::llvm_assign_initial_value(const Symbol& sym, bool force) // assigned to them. Unless they are params, in which case we took // care of it in the group entry point. if (sym.typespec().is_closure_based() && sym.symtype() != SymTypeParam - && sym.symtype() != SymTypeOutputParam) { + && sym.symtype() != SymTypeOutputParam && arrayindex == -1) { llvm_assign_zero(sym); return; } @@ -800,7 +806,15 @@ BackendLLVM::llvm_assign_initial_value(const Symbol& sym, bool force) int num_components = sym.typespec().simpletype().aggregate; TypeSpec elemtype = sym.typespec().elementtype(); int arraylen = std::max(1, sym.typespec().arraylength()); - for (int a = 0, c = 0; a < arraylen; ++a) { + int a = 0, c = 0; + if (arrayindex != -1) { + // If we have an array index, setup the loop variables to only + // execute that index + a = arrayindex; + c = num_components * arrayindex; + arraylen = arrayindex + 1; + } + for (; a < arraylen; ++a) { llvm::Value* arrind = sym.typespec().is_array() ? ll.constant(a) : NULL; if (sym.typespec().is_closure_based()) @@ -1485,6 +1499,77 @@ BackendLLVM::build_llvm_instance(bool groupentry) llvm_assign_initial_value(s); } + // Track all symbols who needed 'partial' initialization + std::unordered_set initedsyms; + + // Make a third pass for sparsely connected array parameters. + // Only init the unconnected elements, upstream shaders are responsible for + // initing and overwritting any connected elements. + for (int c = 0, Nc = inst()->nconnections(); c < Nc; ++c) { + const Connection& con(inst()->connection(c)); + + + Symbol* dstsym(inst()->symbol(con.dst.param)); + // Find any sparse connections, and then find any remaining + // unconnected entries to initialize their values here, upstream + // will have handled all the connected ones. + if (con.dst.arrayindex != -1 && con.src.arrayindex == -1 + && initedsyms.count(dstsym) == 0) { + initedsyms.insert(dstsym); + int arraylen = 0; + TypeSpec dstType = dstsym->typespec(); + if (dstType.is_unsized_array()) { + const ShaderInstance::SymOverrideInfo* sym_override + = inst()->instoverride(con.dst.param); + + OSL_DASSERT(sym_override); + + if (sym_override + && (sym_override->valuesource() == Symbol::InstanceVal + || sym_override->valuesource() + == Symbol::ConnectedVal)) { + arraylen = sym_override->arraylen(); + } else { + arraylen = 0; + } + + } else { + arraylen = dstType.numelements(); + } + + std::vector inited(arraylen, false); + unsigned ninit = arraylen; + + if (con.dst.arrayindex < arraylen) { + inited[con.dst.arrayindex] = true; + ninit--; + } + for (int rc = c + 1; rc < Nc && ninit; ++rc) { + const Connection& next(inst()->connection(rc)); + // Allow redundant/overwriting connections, i.e: + // 1. connect layer.value[i] connect layer.value[j] + // 2. connect layer.value connect layer.value + if (inst()->symbol(next.dst.param) == dstsym) { + if (next.dst.arrayindex != -1) { + if (next.dst.arrayindex < arraylen + && !inited[next.dst.arrayindex]) { + inited[next.dst.arrayindex] = true; + --ninit; + } + } else + ninit = 0; + } + } + if (ninit) { + for (int e = 0; e < arraylen; ++e) { + if (!inited[e]) { + llvm_assign_initial_value(*dstsym, true, e); + } + } + } + } + } + // All the symbols are stack allocated now. if (groupentry) { @@ -1509,9 +1594,6 @@ BackendLLVM::build_llvm_instance(bool groupentry) if (llvm_has_exit_instance_block()) ll.op_branch(m_exit_instance_block); // also sets insert point - // Track all symbols who needed 'partial' initialization - std::unordered_set initedsyms; - // Transfer all of this layer's outputs into the downstream shader's // inputs. for (int layer = this->layer() + 1; layer < group().nlayers(); ++layer) { @@ -1523,9 +1605,6 @@ BackendLLVM::build_llvm_instance(bool groupentry) for (int c = 0, Nc = child->nconnections(); c < Nc; ++c) { const Connection& con(child->connection(c)); if (con.srclayer == this->layer()) { - OSL_ASSERT( - con.src.arrayindex == -1 && con.dst.arrayindex == -1 - && "no support for individual array element connections"); // Validate unsupported connection vecSrc -> vecDst[j] OSL_ASSERT((con.dst.channel == -1 || con.src.type.aggregate() == TypeDesc::SCALAR @@ -1540,6 +1619,7 @@ BackendLLVM::build_llvm_instance(bool groupentry) if (con.dst.channel != -1 && initedsyms.count(dstsym) == 0) { initedsyms.insert(dstsym); std::bitset<32> inited(0); // Only need to be 16 (matrix4) + inited[con.dst.channel] = true; OSL_DASSERT(dstsym->typespec().aggregate() <= inited.size()); unsigned ninit = dstsym->typespec().aggregate() - 1; @@ -1568,13 +1648,72 @@ BackendLLVM::build_llvm_instance(bool groupentry) } } + if (con.dst.arrayindex != -1 && con.src.arrayindex == -1 + && initedsyms.count(dstsym) == 0) { + initedsyms.insert(dstsym); + // This may just be a sparse set of connections, so we go + // ahead and run the initializers if any would otherwise + // be left out + int initArrayIndex = con.dst.arrayindex; + + int arraylen = 0; + TypeSpec dstType = dstsym->typespec(); + if (dstType.is_unsized_array()) { + const ShaderInstance::SymOverrideInfo* sym_override + = child->instoverride(con.dst.param); + + OSL_DASSERT(sym_override); + + if (sym_override + && (sym_override->valuesource() + == Symbol::InstanceVal + || sym_override->valuesource() + == Symbol::ConnectedVal)) { + arraylen = sym_override->arraylen(); + } else { + arraylen = 0; + } + + } else { + arraylen = dstType.numelements(); + } + + std::vector inited(arraylen, false); + inited[con.dst.arrayindex] = true; + unsigned ninit = arraylen - 1; + llvm_assign_initial_value(*dstsym, true, initArrayIndex); + for (int rc = c + 1; rc < Nc && ninit; ++rc) { + const Connection& next(child->connection(rc)); + if (next.srclayer == this->layer()) { + // Allow redundant/overwriting connections, i.e: + // 1. connect layer.value[i] connect layer.value[j] + // 2. connect layer.value connect layer.value + if (child->symbol(next.dst.param) == dstsym) { + if (next.dst.arrayindex != -1) { + initArrayIndex = next.dst.arrayindex; + if (!inited[initArrayIndex]) { + inited[initArrayIndex] = true; + llvm_assign_initial_value( + *dstsym, true, initArrayIndex); + } + } else { + llvm_assign_initial_value(*dstsym, true); + break; + } + } + } + } + } + // llvm_run_connected_layers tracks layers that have been run, // so no need to do it here as well llvm_run_connected_layers(*srcsym, con.src.param); - // FIXME -- I'm not sure I understand this. Isn't this - // unnecessary if we wrote to the parameter ourself? - llvm_assign_impl(*dstsym, *srcsym, -1, con.src.channel, + // The upstream run will write the value to it's local output + // parameter location, now we just have to copy it to the + // destination layer's input location. + llvm_assign_impl(*dstsym, *srcsym, con.src.arrayindex, + con.dst.arrayindex, con.src.channel, con.dst.channel); } } diff --git a/src/liboslexec/shadingsys.cpp b/src/liboslexec/shadingsys.cpp index 307d57355..ce6c64211 100644 --- a/src/liboslexec/shadingsys.cpp +++ b/src/liboslexec/shadingsys.cpp @@ -3557,7 +3557,26 @@ ShadingSystemImpl::decode_connected_param(string_view connectionname, return c; } c.arrayindex = index; - if (c.arrayindex >= c.type.arraylength()) { + int arraylen = 0; + if (c.type.is_unsized_array()) { + const ShaderInstance::SymOverrideInfo* sym_override + = inst->instoverride(c.param); + OSL_DASSERT(sym_override); + + if (sym_override->valuesource() != Symbol::InstanceVal + && sym_override->valuesource() != Symbol::ConnectedVal) { + errorfmt( + "ConnectShaders: unable to connect to element of dynamic array " + "parameter \"{}\" without an override to indicate size.", + connectionname); + c.param = -1; + return c; + } + arraylen = sym_override->arraylen(); + } else { + arraylen = c.type.arraylength(); + } + if (c.arrayindex >= arraylen) { errorfmt("ConnectShaders: cannot request array element {} from a {}", connectionname, c.type); c.arrayindex = c.type.arraylength() - 1; // clamp it diff --git a/testsuite/vararray-scalar-connect/BATCHED b/testsuite/vararray-scalar-connect/BATCHED new file mode 100644 index 000000000..e69de29bb diff --git a/testsuite/vararray-scalar-connect/ref/out.txt b/testsuite/vararray-scalar-connect/ref/out.txt new file mode 100644 index 000000000..a5310dadc --- /dev/null +++ b/testsuite/vararray-scalar-connect/ref/out.txt @@ -0,0 +1,20 @@ +Compiled test.osl -> test.oso +Compiled upstream.osl -> upstream.oso +Connect u1.fout4 to t.a[0] +Connect u2.fout3 to t.a[2] +Connect u1.fout[1] to t.a[3] +Connect u2.fout[0] to t.a[4] +a array length 6 + [0] = 24 + [1] = 42 + [2] = 7 + [3] = 22 + [4] = 9 + [5] = 46 +b array length 5 + [0] = 3 + [1] = 4 + [2] = 5 + [3] = 6 + [4] = 7 + diff --git a/testsuite/vararray-scalar-connect/run.py b/testsuite/vararray-scalar-connect/run.py new file mode 100755 index 000000000..7f92dbf30 --- /dev/null +++ b/testsuite/vararray-scalar-connect/run.py @@ -0,0 +1,14 @@ +# Copyright Contributors to the Open Shading Language project. +# SPDX-License-Identifier: BSD-3-Clause +# https://github.com/AcademySoftwareFoundation/OpenShadingLanguage +#!/usr/bin/env python + + +command += testshade("-param:type=float[4] fin 21,22,23,24 -layer u1 upstream \ + -layer u2 upstream \ + -param:type=float[6] a 41,42,43,44,45,46 \ + -layer t test \ + -connect u1 fout4 t a[0] \ + -connect u2 fout3 t a[2] \ + -connect u1 fout[1] t a[3] \ + -connect u2 fout[0] t a[4]") diff --git a/testsuite/vararray-scalar-connect/test.osl b/testsuite/vararray-scalar-connect/test.osl new file mode 100644 index 000000000..f0a17f1ee --- /dev/null +++ b/testsuite/vararray-scalar-connect/test.osl @@ -0,0 +1,17 @@ +// Copyright Contributors to the Open Shading Language project. +// SPDX-License-Identifier: BSD-3-Clause +// https://github.com/AcademySoftwareFoundation/OpenShadingLanguage + +void print_array_contents (string name, float f[]) +{ + printf ("%s array length %d\n", name, arraylength(f)); + for (int i = 0; i < arraylength(f); ++i) + printf (" [%d] = %g\n", i, f[i]); +} + + +shader test (float a[] = {0}, float b[] = {3,4,5,6,7}) +{ + print_array_contents ("a", a); + print_array_contents ("b", b); +} diff --git a/testsuite/vararray-scalar-connect/upstream.osl b/testsuite/vararray-scalar-connect/upstream.osl new file mode 100644 index 000000000..c0ea0fb9d --- /dev/null +++ b/testsuite/vararray-scalar-connect/upstream.osl @@ -0,0 +1,30 @@ +// Copyright Contributors to the Open Shading Language project. +// SPDX-License-Identifier: BSD-3-Clause +// https://github.com/AcademySoftwareFoundation/OpenShadingLanguage + +shader upstream (float fin[4] = {9, 8, 7, 6}, + output float fout[4] = { 0, 0, 0, 0 }, + output float fout1 = 0, + output float fout2 = 0, + output float fout3 = 0, + output float fout4 = 0) +{ + if (1) + { + fout[0] = fin[0]; + fout[1] = fin[1]; + fout[2] = fin[2]; + fout[3] = fin[3]; + } + else + { + fout[0] = 2.1; + fout[1] = 2.2; + fout[2] = 2.3; + fout[3] = 2.4; + } + fout1 = fout[0]; + fout2 = fout[1]; + fout3 = fout[2]; + fout4 = fout[3]; +} From c80f5e0f17983c4c0e3d2152c444ff649c84ec69 Mon Sep 17 00:00:00 2001 From: Stephen Friedman Date: Fri, 7 Jun 2024 16:08:25 -0700 Subject: [PATCH 2/2] When connecting individual scalar elements to an array, we don't need to init the values that upstream nodes will overwrite, so remove that to simplify. Signed-off-by: Stephen Friedman --- src/liboslexec/batched_llvm_instance.cpp | 65 ------------------------ src/liboslexec/llvm_instance.cpp | 57 --------------------- 2 files changed, 122 deletions(-) diff --git a/src/liboslexec/batched_llvm_instance.cpp b/src/liboslexec/batched_llvm_instance.cpp index f92aa7743..8bda1d7ee 100644 --- a/src/liboslexec/batched_llvm_instance.cpp +++ b/src/liboslexec/batched_llvm_instance.cpp @@ -2313,71 +2313,6 @@ BatchedBackendLLVM::build_llvm_instance(bool groupentry) } } - if (con.dst.arrayindex != -1 && con.src.arrayindex == -1 - && initedsyms.count(dstsym) == 0) { - initedsyms.insert(dstsym); - // This may just be a sparse set of connections, so we go - // ahead and run the initializers if any would otherwise - // be left out - int initArrayIndex = con.dst.arrayindex; - - int arraylen = 0; - TypeSpec dstType = dstsym->typespec(); - if (dstType.is_unsized_array()) { - const ShaderInstance::SymOverrideInfo* - sym_override - = child->instoverride(con.dst.param); - - OSL_DASSERT(sym_override); - - if (sym_override - && (sym_override->valuesource() - == Symbol::InstanceVal - || sym_override->valuesource() - == Symbol::ConnectedVal)) { - arraylen = sym_override->arraylen(); - } else { - arraylen = 0; - } - - } else { - arraylen = dstType.numelements(); - } - - std::vector inited(arraylen, false); - inited[con.dst.arrayindex] = true; - unsigned ninit = arraylen - 1; - llvm_assign_initial_value(*dstsym, - initial_shader_mask, true, - initArrayIndex); - for (int rc = c + 1; rc < Nc && ninit; ++rc) { - const Connection& next(child->connection(rc)); - if (next.srclayer == this->layer()) { - // Allow redundant/overwriting connections, i.e: - // 1. connect layer.value[i] connect layer.value[j] - // 2. connect layer.value connect layer.value - if (child->symbol(next.dst.param) - == dstsym) { - if (next.dst.arrayindex != -1) { - initArrayIndex = next.dst.arrayindex; - if (!inited[initArrayIndex]) { - inited[initArrayIndex] = true; - llvm_assign_initial_value( - *dstsym, - initial_shader_mask, true, - initArrayIndex); - } - } else { - llvm_assign_initial_value( - *dstsym, initial_shader_mask, - true); - break; - } - } - } - } - } - // llvm_run_connected_layers tracks layers that have been run, // so no need to do it here as well llvm_run_connected_layers(*srcsym, con.src.param); diff --git a/src/liboslexec/llvm_instance.cpp b/src/liboslexec/llvm_instance.cpp index c80426870..a61eecebd 100644 --- a/src/liboslexec/llvm_instance.cpp +++ b/src/liboslexec/llvm_instance.cpp @@ -1648,63 +1648,6 @@ BackendLLVM::build_llvm_instance(bool groupentry) } } - if (con.dst.arrayindex != -1 && con.src.arrayindex == -1 - && initedsyms.count(dstsym) == 0) { - initedsyms.insert(dstsym); - // This may just be a sparse set of connections, so we go - // ahead and run the initializers if any would otherwise - // be left out - int initArrayIndex = con.dst.arrayindex; - - int arraylen = 0; - TypeSpec dstType = dstsym->typespec(); - if (dstType.is_unsized_array()) { - const ShaderInstance::SymOverrideInfo* sym_override - = child->instoverride(con.dst.param); - - OSL_DASSERT(sym_override); - - if (sym_override - && (sym_override->valuesource() - == Symbol::InstanceVal - || sym_override->valuesource() - == Symbol::ConnectedVal)) { - arraylen = sym_override->arraylen(); - } else { - arraylen = 0; - } - - } else { - arraylen = dstType.numelements(); - } - - std::vector inited(arraylen, false); - inited[con.dst.arrayindex] = true; - unsigned ninit = arraylen - 1; - llvm_assign_initial_value(*dstsym, true, initArrayIndex); - for (int rc = c + 1; rc < Nc && ninit; ++rc) { - const Connection& next(child->connection(rc)); - if (next.srclayer == this->layer()) { - // Allow redundant/overwriting connections, i.e: - // 1. connect layer.value[i] connect layer.value[j] - // 2. connect layer.value connect layer.value - if (child->symbol(next.dst.param) == dstsym) { - if (next.dst.arrayindex != -1) { - initArrayIndex = next.dst.arrayindex; - if (!inited[initArrayIndex]) { - inited[initArrayIndex] = true; - llvm_assign_initial_value( - *dstsym, true, initArrayIndex); - } - } else { - llvm_assign_initial_value(*dstsym, true); - break; - } - } - } - } - } - // llvm_run_connected_layers tracks layers that have been run, // so no need to do it here as well llvm_run_connected_layers(*srcsym, con.src.param);