diff --git a/CMakeLists.txt b/CMakeLists.txt index 1af9e14055..a770ac8f3a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -241,7 +241,9 @@ TESTSUITE ( and-or-not-synonyms aastep arithmetic array array-derivs array-range bug-locallifetime bug-outputinit bug-param-duplicate bug-peep cellnoise closure closure-array color comparison compile-buffer - component-range const-array-params const-array-fill + component-range + connect-components + const-array-params const-array-fill debugnan debug-uninit derivs derivs-muldiv-clobber draw_string diff --git a/src/liboslexec/backendllvm.cpp b/src/liboslexec/backendllvm.cpp index 3a9254f8fd..3af8b779ed 100644 --- a/src/liboslexec/backendllvm.cpp +++ b/src/liboslexec/backendllvm.cpp @@ -767,7 +767,7 @@ BackendLLVM::llvm_test_nonzero (Symbol &val, bool test_derivs) bool BackendLLVM::llvm_assign_impl (Symbol &Result, Symbol &Src, - int arrayindex) + int arrayindex, int srccomp, int dstcomp) { ASSERT (! Result.typespec().is_structure()); ASSERT (! Src.typespec().is_structure()); @@ -825,29 +825,69 @@ BackendLLVM::llvm_assign_impl (Symbol &Result, Symbol &Src, // Remember that llvm_load_value will automatically convert scalar->triple. TypeDesc rt = Result.typespec().simpletype(); TypeDesc basetype = TypeDesc::BASETYPE(rt.basetype); - int num_components = rt.aggregate; - for (int i = 0; i < num_components; ++i) { + const int num_components = rt.aggregate; + const bool singlechan = (srccomp != -1) || (dstcomp != -1); + if (!singlechan) { + for (int i = 0; i < num_components; ++i) { + llvm::Value* src_val = Src.is_constant() + ? llvm_load_constant_value (Src, arrayindex, i, basetype) + : llvm_load_value (Src, 0, arrind, i, basetype); + if (!src_val) + return false; + llvm_store_value (src_val, Result, 0, arrind, i); + } + } else { + // connect individual component of an aggregate type + // set srccomp to 0 for case when src is actually a float + if (srccomp == -1) srccomp = 0; llvm::Value* src_val = Src.is_constant() - ? llvm_load_constant_value (Src, arrayindex, i, basetype) - : llvm_load_value (Src, 0, arrind, i, basetype); + ? llvm_load_constant_value (Src, arrayindex, srccomp, basetype) + : llvm_load_value (Src, 0, arrind, srccomp, basetype); if (!src_val) return false; - llvm_store_value (src_val, Result, 0, arrind, i); + // write source float into all compnents 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); + } else + llvm_store_value (src_val, Result, 0, arrind, dstcomp); } // Handle derivatives if (Result.has_derivs()) { if (Src.has_derivs()) { // src and result both have derivs -- copy them - 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); + 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); + } + } + } else { + for (int d = 1; d <= 2; ++d) { + llvm::Value* val = llvm_load_value (Src, d, arrind, srccomp); + if (dstcomp == -1) { + for (int i = 0; i < num_components; ++i) + llvm_store_value (val, Result, d, arrind, i); + } + else + llvm_store_value (val, Result, d, arrind, dstcomp); } } } else { // Result wants derivs but src didn't have them -- zero them - llvm_zero_derivs (Result); + if (dstcomp != -1) { + // memset the single deriv component's to zero + if (Result.has_derivs() && Result.typespec().elementtype().is_floatbased()) { + // dx + ll.op_memset (ll.GEP(llvm_void_ptr(Result,1), dstcomp), 0, 1, rt.basesize()); + // dy + ll.op_memset (ll.GEP(llvm_void_ptr(Result,2), dstcomp), 0, 1, rt.basesize()); + } + } else + llvm_zero_derivs (Result); } } return true; diff --git a/src/liboslexec/backendllvm.h b/src/liboslexec/backendllvm.h index d7d52081fa..e39389fbab 100644 --- a/src/liboslexec/backendllvm.h +++ b/src/liboslexec/backendllvm.h @@ -85,7 +85,7 @@ class BackendLLVM : public OSOProcessorBase { typedef std::map AllocationMap; - void llvm_assign_initial_value (const Symbol& sym); + void llvm_assign_initial_value (const Symbol& sym, bool force = false); llvm::LLVMContext &llvm_context () const { return ll.context(); } AllocationMap &named_values () { return m_named_values; } @@ -229,7 +229,8 @@ class BackendLLVM : public OSOProcessorBase { /// Implementaiton 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); + bool llvm_assign_impl (Symbol &Result, Symbol &Src, int arrayindex = -1, + int srcomp = -1, int dstcomp = -1); /// Convert the name of a global (and its derivative index) into the diff --git a/src/liboslexec/instance.cpp b/src/liboslexec/instance.cpp index d0bf866db4..56a0aea22e 100644 --- a/src/liboslexec/instance.cpp +++ b/src/liboslexec/instance.cpp @@ -544,6 +544,27 @@ ShaderInstance::copy_code_from_master (ShaderGroup &group) +std::string +ConnectedParam::str (const ShaderInstance *inst) +{ + const Symbol *s = inst->symbol(param); + return Strutil::format ("%s%s%s (%s)", s->name(), + arrayindex >= 0 ? Strutil::format("[%d]", arrayindex) : std::string(), + channel >= 0 ? Strutil::format("[%d]", channel) : std::string(), + type); +} + + + +std::string +Connection::str (const ShaderGroup &group, const ShaderInstance *dstinst) +{ + return Strutil::format ("%s -> %s", src.str (group[srclayer]), + dst.str (dstinst)); +} + + + // Are the two vectors equivalent(a[i],b[i]) in each of their members? template inline bool diff --git a/src/liboslexec/llvm_instance.cpp b/src/liboslexec/llvm_instance.cpp index 2e0d42b8ac..628a4c8e5b 100644 --- a/src/liboslexec/llvm_instance.cpp +++ b/src/liboslexec/llvm_instance.cpp @@ -29,6 +29,8 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include #include #include +#include +#include #include #include @@ -365,12 +367,12 @@ BackendLLVM::llvm_type_closure_component_ptr () void -BackendLLVM::llvm_assign_initial_value (const Symbol& sym) +BackendLLVM::llvm_assign_initial_value (const Symbol& sym, bool force) { // 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 (sym.valuesource() == Symbol::ConnectedVal && + if (!force && sym.valuesource() == Symbol::ConnectedVal && !sym.typespec().is_closure_based()) return; if (sym.typespec().is_closure_based() && sym.symtype() == SymTypeGlobal) @@ -896,22 +898,66 @@ 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) { ShaderInstance *child = group()[layer]; - for (int c = 0; c < child->nconnections(); ++c) { + for (int c = 0, Nc = child->nconnections(); c < Nc; ++c) { const Connection &con (child->connection (c)); if (con.srclayer == this->layer()) { - ASSERT (con.src.arrayindex == -1 && con.src.channel == -1 && - con.dst.arrayindex == -1 && con.dst.channel == -1 && - "no support for individual element/channel connection"); + ASSERT (con.src.arrayindex == -1 && con.dst.arrayindex == -1 && + "no support for individual array element connections"); + // Validate unsupported connection vecSrc -> vecDst[j] + ASSERT ((con.dst.channel == -1 || + con.src.type.aggregate() == TypeDesc::SCALAR || + con.src.channel != -1) && + "no support for vector -> vector[i] connections"); + Symbol *srcsym (inst()->symbol (con.src.param)); Symbol *dstsym (child->symbol (con.dst.param)); + + // Check remining connections to see if any channels of this + // aggregate need to be initialize. + if (con.dst.channel != -1 && initedsyms.count(dstsym) == 0) { + initedsyms.insert(dstsym); + std::bitset<32> inited(0); // Only need to be 16 (matrix4) + assert(dstsym->typespec().aggregate() <= inited.size()); + unsigned ninit = dstsym->typespec().aggregate() - 1; + 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.channel != -1) { + assert(next.dst.channel < (int)inited.size()); + if (!inited[next.dst.channel]) { + inited[next.dst.channel] = true; + --ninit; + } + } else + ninit = 0; + } + } + } + if (ninit) { + // FIXME: Init only components that are not connected + llvm_assign_initial_value (*dstsym, true); + } + } + + // 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); + llvm_assign_impl (*dstsym, *srcsym, -1, + con.src.channel, con.dst.channel); } } } diff --git a/src/liboslexec/oslexec_pvt.h b/src/liboslexec/oslexec_pvt.h index 3a0118f44f..aa96124a21 100644 --- a/src/liboslexec/oslexec_pvt.h +++ b/src/liboslexec/oslexec_pvt.h @@ -950,6 +950,9 @@ struct ConnectedParam { bool is_complete () const { return arrayindex == -1 && channel == -1; } + + // Debug output of ConnectedParam + std::string str (const ShaderInstance *inst); }; @@ -971,6 +974,12 @@ struct Connection { bool operator!= (const Connection &c) const { return srclayer != c.srclayer || src != c.src || dst != c.dst; } + + // Does the connection fully join the source and destination. + bool is_complete () const { return src.is_complete() && dst.is_complete(); } + + // Debug output of ConnectedParam + std::string str (const ShaderGroup &group, const ShaderInstance *dstinst); }; diff --git a/src/liboslexec/runtimeoptimize.cpp b/src/liboslexec/runtimeoptimize.cpp index 3c1aaae4d6..c2cc0eec52 100644 --- a/src/liboslexec/runtimeoptimize.cpp +++ b/src/liboslexec/runtimeoptimize.cpp @@ -921,12 +921,19 @@ RuntimeOptimizer::simplify_params () // It's connected to an earlier layer. If the output var of // the upstream shader is effectively constant or a global, // then so is this variable. - turn_into_nop (s->initbegin(), s->initend(), - "connected value doesn't need init ops"); for (auto&& c : inst()->connections()) { - if (c.dst.param == i) { + if (c.dst.param != i) + continue; + if (c.dst.is_complete()) { + /// All components are being set through either + /// float->triple or triple->triple + /// Get rid of the un-needed init ops. + turn_into_nop (s->initbegin(), s->initend(), + "connected value doesn't need init ops"); + } + if (c.is_complete()) { // srcsym is the earlier group's output param, which - // is connected as the input to the param we're + // is fully connected as the input to the param we're // examining. ShaderInstance *uplayer = group()[c.srclayer]; Symbol *srcsym = uplayer->symbol(c.src.param); @@ -938,8 +945,7 @@ RuntimeOptimizer::simplify_params () // If so, make sure the global is in this instance's // symbol table, and alias the parameter to it. ustringmap_t &g (m_params_holding_globals[c.srclayer]); - ustringmap_t::const_iterator f; - f = g.find (srcsym->name()); + auto f = g.find (srcsym->name()); if (f != g.end()) { if (debug() > 1) debug_opt ("Remapping %s.%s because it's connected to " @@ -972,6 +978,20 @@ RuntimeOptimizer::simplify_params () } } } + // FIXME / N.B.: We only optimize "fully complete" connections, + // not those involving individual components or array elements + // of the connected parameters, because we sure don't track the + // constness or aliasing of individual components/element, only + // whole variables. But there are two cases where the logic + // above fails to fully exploit the connection propagating a + // constant value. (a) Partial-to-whole connections, for example + // connecting one component of an upstream triple output to a + // downstream float input, should propagate the constant, but we + // currently neglect this case. (b) If *multiple* conections + // combine to fully propagate values, for example if someone was + // foolish enough to connect R, G, and B components of color + // parameters *separately*, we sure don't notice that and treat + // it as a full connection of the color. } } } diff --git a/testsuite/connect-components/downstream.osl b/testsuite/connect-components/downstream.osl new file mode 100644 index 0000000000..a9122d3819 --- /dev/null +++ b/testsuite/connect-components/downstream.osl @@ -0,0 +1,22 @@ +shader downstream ( + float f_in = -1, + float f_comp_in = -1, + color C_in = -1, + color C_justr_in = -1, + color C_justg_in = -1, + color C_justb_in = -1, + color C_rgb_in = -1, + color C_rgb_separate_in = -1, + color C_f_to_rgb_in = -1 + ) +{ + printf ("f_in = %g\n", f_in); + printf ("f_comp_in = %g\n", f_comp_in); + printf ("C_in = %g\n", C_in); + printf ("C_justr_in = %g\n", C_justr_in); + printf ("C_justg_in = %g\n", C_justg_in); + printf ("C_justb_in = %g\n", C_justb_in); + printf ("C_rgb_in = %g\n", C_rgb_in); + printf ("C_rgb_separate_in = %g\n", C_rgb_separate_in); + printf ("C_f_to_rgb_in = %g\n", C_f_to_rgb_in); +} diff --git a/testsuite/connect-components/ref/out.txt b/testsuite/connect-components/ref/out.txt new file mode 100644 index 0000000000..45036a9d3b --- /dev/null +++ b/testsuite/connect-components/ref/out.txt @@ -0,0 +1,12 @@ +Compiled downstream.osl -> downstream.oso +Compiled upstream.osl -> upstream.oso +f_in = 1 +f_comp_in = 11 +C_in = 10 11 12 +C_justr_in = 2 -1 -1 +C_justg_in = -1 3 -1 +C_justb_in = -1 -1 4 +C_rgb_in = 2 3 4 +C_rgb_separate_in = 12 11 10 +C_f_to_rgb_in = 2 2 2 + diff --git a/testsuite/connect-components/run.py b/testsuite/connect-components/run.py new file mode 100755 index 0000000000..02e2c498be --- /dev/null +++ b/testsuite/connect-components/run.py @@ -0,0 +1,3 @@ +#!/usr/bin/env python + +command = testshade("-group test.oslgroup") diff --git a/testsuite/connect-components/test.oslgroup b/testsuite/connect-components/test.oslgroup new file mode 100644 index 0000000000..d2acbb7231 --- /dev/null +++ b/testsuite/connect-components/test.oslgroup @@ -0,0 +1,15 @@ +shader upstream upstream; +shader downstream downstream; +connect upstream.f_out downstream.f_in; +connect upstream.C_out downstream.C_in; +connect upstream.C_out[1] downstream.f_comp_in; +connect upstream.r_out downstream.C_rgb_in[0]; +connect upstream.g_out downstream.C_rgb_in[1]; +connect upstream.b_out downstream.C_rgb_in[2]; +connect upstream.C_out[0] downstream.C_rgb_separate_in[2]; +connect upstream.C_out[1] downstream.C_rgb_separate_in[1]; +connect upstream.C_out[2] downstream.C_rgb_separate_in[0]; +connect upstream.r_out downstream.C_justr_in[0]; +connect upstream.g_out downstream.C_justg_in[1]; +connect upstream.b_out downstream.C_justb_in[2]; +connect upstream.r_out downstream.C_f_to_rgb_in diff --git a/testsuite/connect-components/upstream.osl b/testsuite/connect-components/upstream.osl new file mode 100644 index 0000000000..ed228668a3 --- /dev/null +++ b/testsuite/connect-components/upstream.osl @@ -0,0 +1,16 @@ +shader upstream ( + output float f_out = -1, + output float r_out = -1, + output float g_out = -1, + output float b_out = -1, + output color C_out = -1, + output color C2_out = -1 + ) +{ + f_out = 1; + r_out = 2; + g_out = 3; + b_out = 4; + C_out = color(10,11,12); + C2_out = color(20,21,22); +} diff --git a/testsuite/runtest.py b/testsuite/runtest.py index a4ad60faed..cb919796b0 100755 --- a/testsuite/runtest.py +++ b/testsuite/runtest.py @@ -313,12 +313,9 @@ def runtest (command, outputs, failureok=0, failthresh=0, failpercent=0) : # Force any local shaders to compile automatically, prepending the # compilation onto whatever else the individual run.py file requested. -for testfile in glob.glob (os.path.join (test_source_dir, "*.osl")) : - shutil.copyfile (testfile, os.path.basename(testfile)) -for testfile in glob.glob (os.path.join (test_source_dir, "*.h")) : - shutil.copyfile (testfile, os.path.basename(testfile)) -for testfile in glob.glob (os.path.join (test_source_dir, "*.xml")) : - shutil.copyfile (testfile, os.path.basename(testfile)) +for filetype in [ "*.osl", "*.h", "*.oslgroup", "*.xml" ] : + for testfile in glob.glob (os.path.join (test_source_dir, filetype)) : + shutil.copyfile (testfile, os.path.basename(testfile)) if compile_osl_files : compiles = "" oslfiles = glob.glob ("*.osl")