Allow individual components of aggregate types to be connected.

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

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;

src/liboslexec/backendllvm.h

@@ -85,7 +85,7 @@ class BackendLLVM : public OSOProcessorBase {
 
     typedef std::map<std::string, llvm::Value*> 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

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<class T>
 inline bool

src/liboslexec/llvm_instance.cpp

@@ -29,6 +29,8 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #include <cmath>
 #include <iostream>
 #include <unordered_map>
+#include <unordered_set>
+#include <bitset>
 
 #include <OpenImageIO/timer.h>
 #include <OpenImageIO/sysutil.h>
@@ -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<Symbol*> 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);
             }
         }
     }

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);
 };
 
 

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.
         }
     }
 }

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);
+}

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
+

testsuite/connect-components/run.py

@@ -0,0 +1,3 @@
+#!/usr/bin/env python
+
+command = testshade("-group test.oslgroup")

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

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);
+}