fix: interpolated int attributes wrong for batch shading (#1949)

AlexMWells · lgritz · commit 4e0b03496bda · 2025-03-04T18:27:03.000-08:00
Fix issue #1929 with batched execution forcing interpolated integer arguments to be boolean when their default value is 0 or 1. In actuality the renderer could assign any integer value, we update BatchedAnalysis to track the interpolated value as a write event which will disqualify forcing of boolean as well as ensure future writes are masked properly. Added new lockgeom test that reproduces the issue and verifies its fixed. --------- Signed-off-by: Alex M. Wells <alex.m.wells@intel.com>
diff --git a/src/cmake/testing.cmake b/src/cmake/testing.cmake
@@ -301,6 +301,7 @@ macro (osl_add_all_tests)
                 layers-nonlazycopy layers-repeatedoutputs
                 lazytrace
                 length-reg linearstep
+                lockgeom
                 logic loop luminance-reg
                 matrix matrix-reg matrix-arithmetic-reg
                 matrix-compref-reg max-reg message message-no-closure message-reg
diff --git a/src/liboslexec/batched_analysis.cpp b/src/liboslexec/batched_analysis.cpp
@@ -1119,6 +1119,7 @@ class WriteEvent {
 public:
     static constexpr int InitialAssignmentOp() { return -1; }
     static constexpr int UserDataPreplacementCopyOp() { return -2; }
+    static constexpr int UserDataInterpolatedOp() { return -3; }
 
     WriteEvent(DependencyTreeTracker::Position pos_in_tree_, int op_num_,
                int loop_op_index)
@@ -1134,7 +1135,8 @@ class WriteEvent {
         , m_op_num(op_num_)
         , m_loop_op_index(NoLoopIndex())
     {
-        OSL_ASSERT(is_initial_assignment() || is_user_data_preplacement_copy());
+        OSL_ASSERT(is_initial_assignment() || is_user_data_preplacement_copy()
+                   || is_user_data_interpolated());
     }
 
     OSL_FORCEINLINE DependencyTreeTracker::Position pos_in_tree() const
@@ -1152,11 +1154,25 @@ class WriteEvent {
         return m_op_num == UserDataPreplacementCopyOp();
     }
 
+    OSL_FORCEINLINE bool is_user_data_interpolated() const
+    {
+        return m_op_num == UserDataInterpolatedOp();
+    }
+
 
     OSL_FORCEINLINE int op_num() const
     {
-        OSL_ASSERT(!is_initial_assignment()
-                   && !is_user_data_preplacement_copy());
+#if 0  // For getting callstack in debugger
+        if(is_initial_assignment()
+          || is_user_data_preplacement_copy()
+          || is_user_data_interpolated()) {
+            // Caller's logic should of special cased these
+            // so logic bug exists
+            __builtin_trap();
+        }
+#endif
+        OSL_ASSERT(!is_initial_assignment() && !is_user_data_preplacement_copy()
+                   && !is_user_data_interpolated());
         return m_op_num;
     }
 
@@ -1397,7 +1413,8 @@ struct Analyzer {
                      write_iter != write_end; ++write_iter) {
                     // We don't bother masking initial assignment
                     if (write_iter->is_initial_assignment()
-                        || write_iter->is_user_data_preplacement_copy()) {
+                        || write_iter->is_user_data_preplacement_copy()
+                        || write_iter->is_user_data_interpolated()) {
                         continue;
                     }
 
@@ -1568,7 +1585,9 @@ struct Analyzer {
                     // Then the current write needs to be masked
                     for (auto write_iter = write_chronology.begin();
                          write_iter != write_end; ++write_iter) {
-                        if (write_iter->is_initial_assignment()) {
+                        if (write_iter->is_initial_assignment()
+                            || write_iter->is_user_data_preplacement_copy()
+                            || write_iter->is_user_data_interpolated()) {
                             // The initial assignment of all parameters happens before any instructions are generated?
                             // perhaps there is an ordering issue here for init_ops which could have early outs
                             // although not sure what that would do to execution, certainly returns in init_ops would
@@ -2204,6 +2223,19 @@ struct Analyzer {
                 OSL_DEV_ONLY(std::cout
                              << " bind_interpolated_param called for symbol: "
                              << s.name() << std::endl);
+
+                // NOTE: as this is the initial assignment to a parameter
+                // there could be no other reads/write to deal with to the symbol
+                OSL_ASSERT(m_write_chronology_by_symbol.find(&s)
+                           == m_write_chronology_by_symbol.end());
+
+                // bind_interpolated_param will have written to s
+                // so we need a write event to model that and prevent
+                // s from being forced to boolean
+                m_write_chronology_by_symbol[&s].push_back(
+                    WriteEvent(m_conditional_symbol_stack.top_pos(),
+                               WriteEvent::UserDataInterpolatedOp()));
+
                 // Interpolated params are handled by calling batched version of
                 // osl_bind_interpolated_param.  It will return a mask indicating which
                 // lanes had such userdata was available.
@@ -2241,12 +2273,17 @@ struct Analyzer {
 
                 // NOTE: as this is the initial assignment to a parameter
                 // there could be no other reads/write to deal with to the symbol
-                if (m_write_chronology_by_symbol.find(&s)
-                    != m_write_chronology_by_symbol.end()) {
-                    __builtin_trap();
+                // unless it was interpolated
+#if 0  // For getting callstack in debugger
+                if (!interpolate_param
+                    && (m_write_chronology_by_symbol.find(&s)
+                        != m_write_chronology_by_symbol.end())) {
+                        __builtin_trap();
                 }
-                OSL_ASSERT(m_write_chronology_by_symbol.find(&s)
-                           == m_write_chronology_by_symbol.end());
+#endif
+                OSL_ASSERT(interpolate_param
+                           || (m_write_chronology_by_symbol.find(&s)
+                               == m_write_chronology_by_symbol.end()));
 
                 m_write_chronology_by_symbol[&s].push_back(
                     WriteEvent(m_conditional_symbol_stack.top_pos(),
@@ -2415,6 +2452,18 @@ struct Analyzer {
                                 // no need to continue checking additional writes
                                 break;
                             }
+                        } else if (write_iter->is_user_data_interpolated()
+                                   || write_iter
+                                          ->is_user_data_preplacement_copy()) {
+                            // the interpolated assignment should be the 1st write entry,
+                            // so bool status should be unknown
+                            OSL_ASSERT(b_status == BoolStatus::Unknown);
+                            // The renderer could write any integer value, or
+                            // the preplacement data could be any integer value
+                            // so it can't be boolean!
+                            b_status = BoolStatus::No;
+                            // no need to continue checking additional writes
+                            break;
                         } else {
                             int op_index   = write_iter->op_num();
                             Opcode& opcode = m_opcodes[op_index];
diff --git a/src/testshade/batched_simplerend.cpp b/src/testshade/batched_simplerend.cpp
@@ -21,6 +21,7 @@ struct UniqueStringCache {
         , red("red")
         , green("green")
         , blue("blue")
+        , face_idx("face_idx")
         , lookupTable("lookupTable")
         , blahblah("blahblah")
         , options("options")
@@ -51,6 +52,7 @@ struct UniqueStringCache {
     ustring red;
     ustring green;
     ustring blue;
+    ustring face_idx;
     ustring lookupTable;
     ustring blahblah;
     ustring options;
@@ -663,7 +665,15 @@ BatchedSimpleRenderer<WidthT>::get_userdata(ustringhash name,
 
     // For testing of interactions with default values
     // may not provide data for all lanes
-    if (name == ucache().s && Masked<float>::is(val)) {
+    if (name == ucache().face_idx && Masked<int>::is(val)) {
+        Masked<int> out(val);
+        for (int i = 0; i < WidthT; ++i) {
+            if (out[i].is_on()) {
+                out[i] = int(4 * bsg->varying.u[i]);
+            }
+        }
+        return out.mask();
+    } else if (name == ucache().s && Masked<float>::is(val)) {
         Masked<float> out(val);
         for (int i = 0; i < WidthT; ++i) {
             // NOTE: assigning to out[i] will mask by itself
diff --git a/src/testshade/rs_simplerend.cpp b/src/testshade/rs_simplerend.cpp
@@ -366,6 +366,15 @@ rs_get_attribute(OSL::OpaqueExecContextPtr oec, OSL::ustringhash_pod object_,
     return false;
 }
 
+
+OSL_RSOP bool
+rs_get_interpolated_face_idx(OSL::OpaqueExecContextPtr ec, void* val)
+{
+    ((int*)val)[0] = int(4 * OSL::get_u(ec));
+    return true;
+}
+
+
 OSL_RSOP bool
 rs_get_interpolated_s(OSL::OpaqueExecContextPtr ec, bool derivatives, void* val)
 {
diff --git a/src/testshade/rs_strdecls.h b/src/testshade/rs_strdecls.h
@@ -37,4 +37,5 @@ RS_STRDECL("t", t)
 RS_STRDECL("red", red)
 RS_STRDECL("green", green)
 RS_STRDECL("blue", blue)
+RS_STRDECL("face_idx", face_idx)
 RS_STRDECL("test", test)
diff --git a/src/testshade/simplerend.cpp b/src/testshade/simplerend.cpp
@@ -508,6 +508,10 @@ SimpleRenderer::get_userdata(bool derivatives, ustringhash name, TypeDesc type,
     // look up something specific to the primitive, rather than have hard-
     // coded names.
 
+    if (name == RS::Hashes::face_idx && type == TypeInt) {
+        ((int*)val)[0] = int(4 * sg->u);
+        return true;
+    }
     if (name == RS::Hashes::s && type == TypeFloat) {
         ((float*)val)[0] = sg->u;
         if (derivatives) {
@@ -682,6 +686,8 @@ SimpleRenderer::build_interpolated_getter(const ShaderGroup& group,
                                           TypeDesc type, bool derivatives,
                                           InterpolatedGetterSpec& spec)
 {
+    static const OIIO::ustring rs_get_interpolated_face_idx(
+        "rs_get_interpolated_face_idx");
     static const OIIO::ustring rs_get_interpolated_s("rs_get_interpolated_s");
     static const OIIO::ustring rs_get_interpolated_t("rs_get_interpolated_t");
     static const OIIO::ustring rs_get_interpolated_red(
@@ -711,7 +717,10 @@ SimpleRenderer::build_interpolated_getter(const ShaderGroup& group,
     static const OIIO::ustring rs_get_attribute_constant_float4(
         "rs_get_attribute_constant_float4");
 
-    if (param_name == RS::Hashes::s && type == OIIO::TypeFloat) {
+    if (param_name == RS::Hashes::face_idx && type == OIIO::TypeInt) {
+        spec.set(rs_get_interpolated_face_idx,
+                 InterpolatedSpecBuiltinArg::OpaqueExecutionContext);
+    } else if (param_name == RS::Hashes::s && type == OIIO::TypeFloat) {
         spec.set(rs_get_interpolated_s,
                  InterpolatedSpecBuiltinArg::OpaqueExecutionContext,
                  InterpolatedSpecBuiltinArg::Derivatives);
diff --git a/testsuite/lockgeom/BATCHED b/testsuite/lockgeom/BATCHED
diff --git a/testsuite/lockgeom/ref/out.txt b/testsuite/lockgeom/ref/out.txt
@@ -0,0 +1,12 @@
+Compiled test0.osl -> test0.oso
+Compiled test1.osl -> test1.oso
+Compiled test2.osl -> test2.oso
+Compiled test3.osl -> test3.oso
+
+Output dst to out0.tif
+
+Output dst to out1.tif
+
+Output dst to out2.tif
+
+Output dst to out3.tif
diff --git a/testsuite/lockgeom/ref/out0.tif b/testsuite/lockgeom/ref/out0.tif
diff --git a/testsuite/lockgeom/ref/out1.tif b/testsuite/lockgeom/ref/out1.tif
diff --git a/testsuite/lockgeom/ref/out2.tif b/testsuite/lockgeom/ref/out2.tif
diff --git a/testsuite/lockgeom/ref/out3.tif b/testsuite/lockgeom/ref/out3.tif
diff --git a/testsuite/lockgeom/run.py b/testsuite/lockgeom/run.py
@@ -0,0 +1,17 @@
+#!/usr/bin/env python
+
+# Copyright Contributors to the Open Shading Language project.
+# SPDX-License-Identifier: BSD-3-Clause
+# https://github.com/AcademySoftwareFoundation/OpenShadingLanguage
+
+# The tests are similar but have different default values of face_idx 
+# to make sure batched analysis doesn't force interpolated integer 
+# argements to be boolean.
+command = testshade("-t 1 --res 256 256 -od uint8 -o dst out0.tif test0")
+command += testshade("-t 1 --res 256 256 -od uint8 -o dst out1.tif test1")
+command += testshade("-t 1 --res 256 256 -od uint8 -o dst out2.tif test2")
+command += testshade("-t 1 --res 256 256 -od uint8 -o dst out3.tif test3")
+outputs.append ("out0.tif")
+outputs.append ("out1.tif")
+outputs.append ("out2.tif")
+outputs.append ("out3.tif")
diff --git a/testsuite/lockgeom/test0.osl b/testsuite/lockgeom/test0.osl
@@ -0,0 +1,9 @@
+// Copyright Contributors to the Open Shading Language project.
+// SPDX-License-Identifier: BSD-3-Clause
+// https://github.com/AcademySoftwareFoundation/OpenShadingLanguage
+
+shader test0(int face_idx = 0 [[int lockgeom = 0]],
+               output color dst = 0)
+{
+    dst = color(face_idx * 0.25, 0, 0);
+}
diff --git a/testsuite/lockgeom/test1.osl b/testsuite/lockgeom/test1.osl
@@ -0,0 +1,9 @@
+// Copyright Contributors to the Open Shading Language project.
+// SPDX-License-Identifier: BSD-3-Clause
+// https://github.com/AcademySoftwareFoundation/OpenShadingLanguage
+
+shader test1(int face_idx = 1 [[int lockgeom = 0]],
+               output color dst = 0)
+{
+    dst = color(face_idx * 0.25, 0, 0);
+}
diff --git a/testsuite/lockgeom/test2.osl b/testsuite/lockgeom/test2.osl
@@ -0,0 +1,9 @@
+// Copyright Contributors to the Open Shading Language project.
+// SPDX-License-Identifier: BSD-3-Clause
+// https://github.com/AcademySoftwareFoundation/OpenShadingLanguage
+
+shader test2(int face_idx = 2 [[int lockgeom = 0]],
+               output color dst = 0)
+{
+    dst = color(face_idx * 0.25, 0, 0);
+}
diff --git a/testsuite/lockgeom/test3.osl b/testsuite/lockgeom/test3.osl
@@ -0,0 +1,9 @@
+// Copyright Contributors to the Open Shading Language project.
+// SPDX-License-Identifier: BSD-3-Clause
+// https://github.com/AcademySoftwareFoundation/OpenShadingLanguage
+
+shader test3(int face_idx = 3 [[int lockgeom = 0]],
+               output color dst = 0)
+{
+    dst = color(face_idx * 0.25, 0, 0);
+}
