shaderpipeline: Remove unused function parameters

panda/src/dxgsg9/dxShaderContext9.cxx

@@ -17,6 +17,7 @@
 #include "shaderModuleSpirV.h"
 #include "spirVTransformer.h"
 #include "spirVHoistStructResourcesPass.h"
+#include "spirVRemoveUnusedVariablesPass.h"
 
 #include <io.h>
 #include <stdio.h>
@@ -108,6 +109,7 @@ compile_module(const ShaderModule *module, DWORD *&data) {
   if (hoist_necessary) {
     SpirVTransformer transformer(stream);
     transformer.run(SpirVHoistStructResourcesPass());
+    transformer.run(SpirVRemoveUnusedVariablesPass());
     stream = transformer.get_result();
 
 #ifndef NDEBUG

panda/src/shaderpipeline/spirVHoistStructResourcesPass.cxx

@@ -116,6 +116,9 @@ transform_definition_op(Instruction op) {
 
   case spv::OpTypePointer:
     if (op.nargs >= 3) {
+      if (_affected_types.count(op.args[2])) {
+        _affected_pointer_types.insert(op.args[0]);
+      }
       if (is_deleted(op.args[2])) {
         delete_id(op.args[0]);
         return false;
@@ -138,8 +141,10 @@ transform_definition_op(Instruction op) {
         // Structs with non-opaque types must be passed through pointers.
         nassertd(!_affected_types.count(arg)) continue;
 
-        auto ait = _affected_types.find(get_type_id(arg));
-        if (ait != _affected_types.end()) {
+        if (_affected_pointer_types.count(arg)) {
+          auto ait = _affected_types.find(unwrap_pointer_type(arg));
+          nassertd(ait != _affected_types.end()) continue;
+
           // Passing a struct with non-opaque types to a function.  That
           // means adding additional parameters for the hoisted variables.
           for (auto &pair : ait->second) {
@@ -149,6 +154,9 @@ transform_definition_op(Instruction op) {
         }
       }
 
+      // Let's go ahead and modify the definition now.
+      _db.modify_definition(op.args[0])._parameters = pvector<uint32_t>(new_args.data() + 2, new_args.data() + new_args.size());
+
       add_definition(spv::OpTypeFunction, new_args.data(), new_args.size());
       return false;
     }
@@ -156,8 +164,8 @@ transform_definition_op(Instruction op) {
 
   case spv::OpVariable:
     if (op.nargs >= 3) {
-      uint32_t pointer_type_id = unwrap_pointer_type(op.args[0]);
-      auto ait = _affected_types.find(pointer_type_id);
+      uint32_t type_id = unwrap_pointer_type(op.args[0]);
+      auto ait = _affected_types.find(type_id);
       if (ait != _affected_types.end()) {
         uint32_t var_id = op.args[1];
         spv::StorageClass storage_class = (spv::StorageClass)op.args[2];
@@ -177,7 +185,7 @@ transform_definition_op(Instruction op) {
       }
 
       // If the struct contained only samplers, delete the old variable.
-      if (is_deleted(pointer_type_id)) {
+      if (is_deleted(type_id)) {
         delete_id(op.args[1]);
         return false;
       }
@@ -188,6 +196,16 @@ transform_definition_op(Instruction op) {
   return true;
 }
 
+/**
+ *
+ */
+bool SpirVHoistStructResourcesPass::
+begin_function(Instruction op) {
+  // We will be re-recording the parameters.
+  _db.modify_definition(op.args[1])._parameters.clear();
+  return true;
+}
+
 /**
  *
  */
@@ -202,13 +220,16 @@ transform_function_op(Instruction op, uint32_t function_id) {
         delete_id(param_id);
       } else {
         add_instruction(op.opcode, op.args, op.nargs);
+        _db.modify_definition(function_id)._parameters.push_back(op.args[1]);
       }
 
       // Structs with non-opaque types must be passed through pointers.
       nassertr(!_affected_types.count(op.args[0]), false);
 
-      auto ait = _affected_types.find(unwrap_pointer_type(op.args[0]));
-      if (ait != _affected_types.end()) {
+      if (_affected_pointer_types.count(op.args[0])) {
+        auto ait = _affected_types.find(unwrap_pointer_type(op.args[0]));
+        nassertr(ait != _affected_types.end(), false);
+
         // Passing a struct with non-opaque types to a function.  That means
         // adding additional parameters for the hoisted variables.
         for (auto &pair : ait->second) {
@@ -329,7 +350,6 @@ transform_function_op(Instruction op, uint32_t function_id) {
             auto hit = _hoisted_vars.find(full);
             nassertr(hit != _hoisted_vars.end(), false);
             uint32_t hoisted_var_id = hit->second;
-            mark_used(hoisted_var_id);
 
             uint32_t hoisted_type_ptr_id = define_pointer_type(pair.first, spv::StorageClassUniformConstant);
             nassertr(hoisted_type_ptr_id != 0, false);
@@ -360,12 +380,16 @@ transform_function_op(Instruction op, uint32_t function_id) {
         uint32_t arg = op.args[i];
         uint32_t arg_type_id = get_type_id(arg);
         if (!is_deleted(arg_type_id)) {
-          // Type is deleted, skip this arg.
           new_args.push_back(arg);
         }
 
-        auto ait = _affected_types.find(unwrap_pointer_type(arg_type_id));
-        if (ait != _affected_types.end()) {
+        // Structs with samplers must be passed through a pointer.
+        nassertd(!_affected_types.count(arg_type_id)) continue;
+
+        if (_affected_pointer_types.count(arg_type_id)) {
+          auto ait = _affected_types.find(unwrap_pointer_type(arg_type_id));
+          nassertd(ait != _affected_types.end()) continue;
+
           // Passing a struct with non-opaque types to a function.  That means
           // adding additional parameters for the hoisted variables.
           size_t j = i;
@@ -413,19 +437,33 @@ transform_function_op(Instruction op, uint32_t function_id) {
     // people actually do this, we can add support.
     nassertr(!_affected_types.count(op.args[0]), false);
     nassertr(!is_deleted(op.args[2]), false);
+    mark_used(op.args[2]);
     break;
 
   case spv::OpCopyObject:
   case spv::OpCopyLogical:
+    // Not allowed to copy structs containing resources.
+    nassertr(!_affected_types.count(op.args[0]), false);
+    nassertr(!_affected_pointer_types.count(op.args[0]), false);
+
     // Copying an empty struct means deleting the copy.
     if (is_deleted(op.args[2])) {
       delete_id(op.args[1]);
       return false;
     }
+    break;
 
-    // Not allowed to copy structs containing resources.
-    nassertr(!_affected_types.count(op.args[0]), false);
-    nassertr(!_affected_types.count(get_type_id(op.args[0])), false);
+  case spv::OpReturnValue:
+    // Cannot return a struct with an opaque type from a function.
+    if (op.nargs >= 1) {
+      uint32_t value_id = op.args[0];
+      uint32_t type_id = get_type_id(op.args[0]);
+      mark_used(value_id);
+      nassertr(!is_deleted(value_id), true);
+      nassertr(!is_deleted(type_id), true);
+      nassertr(!_affected_types.count(type_id), true);
+      nassertr(!_affected_pointer_types.count(type_id), true);
+    }
     break;
 
   default:

panda/src/shaderpipeline/spirVHoistStructResourcesPass.h

@@ -23,6 +23,7 @@
 class EXPCL_PANDA_SHADERPIPELINE SpirVHoistStructResourcesPass final : public SpirVTransformPass {
 public:
   virtual bool transform_definition_op(Instruction op);
+  virtual bool begin_function(Instruction op);
   virtual bool transform_function_op(Instruction op, uint32_t function_id);
 
   virtual void postprocess();
@@ -36,6 +37,7 @@ private:
   // members only) leading to the hoisted type in question, as well as the
   // type that the wrapped additional variables should have.
   pmap<uint32_t, pvector<std::pair<const ShaderType *, AccessChain> > > _affected_types;
+  pset<uint32_t> _affected_pointer_types;
 
   // For each access chain consisting only of struct members
   // (prefixed by a variable id), map to the variable that has been hoisted

panda/src/shaderpipeline/spirVRemoveUnusedVariablesPass.cxx

@@ -18,17 +18,44 @@
  */
 void SpirVRemoveUnusedVariablesPass::
 preprocess() {
+  pmap<uint32_t, BitArray> ftype_params_used;
+
   for (uint32_t id = 0; id < get_id_bound(); ++id) {
     Definition &def = _db.modify_definition(id);
 
     if (def.is_variable() && !def.is_used()) {
-      if (shader_cat.is_debug() && !def._name.empty()) {
+      if (shader_cat.is_debug()) {
         shader_cat.debug()
           << "Removing unused variable " << def._name << " (" << id << ")\n";
       }
       def.clear();
       delete_id(id);
     }
+    if (def.is_function()) {
+      for (size_t i = 0; i < def._parameters.size(); ++i) {
+        uint32_t param_id = def._parameters[i];
+        const Definition &param_def = _db.get_definition(param_id);
+        if (param_def.is_used() || !_db.get_definition(param_def._type_id).is_pointer_type()) {
+          ftype_params_used[def._type_id].set_bit(i);
+        }
+      }
+    }
+  }
+
+  for (auto it = ftype_params_used.begin(); it != ftype_params_used.end(); ++it) {
+    uint32_t func_type_id = it->first;
+    const Definition &def = _db.get_definition(func_type_id);
+
+    for (size_t i = 0; i < def._parameters.size(); ++i) {
+      if (!it->second.get_bit(i)) {
+        if (shader_cat.is_debug()) {
+          shader_cat.debug()
+            << "Removing unused function parameter " << i
+            << " from function type " << func_type_id << "\n";
+        }
+        delete_function_parameter(func_type_id, i);
+      }
+    }
   }
 
   // This is really all we need to do; the base class takes care of deletions.

panda/src/shaderpipeline/spirVRemoveUnusedVariablesPass.h

@@ -17,7 +17,7 @@
 #include "spirVTransformPass.h"
 
 /**
- * Removes unused variables.
+ * Removes unused variables and function parameters.
  */
 class EXPCL_PANDA_SHADERPIPELINE SpirVRemoveUnusedVariablesPass final : public SpirVTransformPass {
 public:

panda/src/shaderpipeline/spirVResultDatabase.I

@@ -35,6 +35,14 @@ is_variable() const {
   return _dtype == DT_variable;
 }
 
+/**
+ * Returns true if this is a function parameter.
+ */
+INLINE bool SpirVResultDatabase::Definition::
+is_function_parameter() const {
+  return _dtype == DT_function_parameter;
+}
+
 /**
  * Returns true if this is a constant.
  */

panda/src/shaderpipeline/spirVResultDatabase.cxx

@@ -217,6 +217,17 @@ parse_instruction(spv::Op opcode, uint32_t *args, uint32_t nargs, uint32_t &curr
     record_pointer_type(args[0], (spv::StorageClass)args[1], args[2]);
     break;
 
+  case spv::OpTypeFunction:
+    {
+      Definition &def = modify_definition(args[0]);
+      def._dtype = DT_type;
+      def._type_id = args[1];
+      for (size_t i = 2; i < nargs; ++i) {
+        def._parameters.push_back(args[i]);
+      }
+    }
+    break;
+
   case spv::OpTypeImage:
     {
       const ShaderType::Scalar *sampled_type;
@@ -435,16 +446,15 @@ parse_instruction(spv::Op opcode, uint32_t *args, uint32_t nargs, uint32_t &curr
       return;
     }
     {
-      const Definition &func_def = modify_definition(args[1]);
-      if (func_def.is_function() && func_def._type_id != args[0]) {
+      const Definition &ftype_def = get_definition(args[3]);
+      if (ftype_def._type_id != args[0]) {
         shader_cat.error()
-          << "OpFunctionCall has mismatched return type ("
-          << args[0] << " != " << func_def._type_id << ")\n";
-        return;
+          << "OpFunction has mismatched return type ("
+          << args[0] << " != " << ftype_def._type_id << ")\n";
       }
     }
     current_function_id = args[1];
-    record_function(args[1], args[0]);
+    record_function(args[1], args[3]);
     break;
 
   case spv::OpFunctionParameter:
@@ -484,10 +494,11 @@ parse_instruction(spv::Op opcode, uint32_t *args, uint32_t nargs, uint32_t &curr
       // Error checking.  Note that it's valid for the function to not yet have
       // been defined.
       if (func_def.is_function()) {
-        if (func_def._type_id != 0 && func_def._type_id != args[0]) {
+        const Definition &ftype_def = get_definition(func_def._type_id);
+        if (ftype_def._type_id != 0 && ftype_def._type_id != args[0]) {
           shader_cat.error()
             << "OpFunctionCall has mismatched return type ("
-            << func_def._type_id << " != " << args[0] << ")\n";
+            << ftype_def._type_id << " != " << args[0] << ")\n";
           return;
         }
       }
@@ -503,7 +514,6 @@ parse_instruction(spv::Op opcode, uint32_t *args, uint32_t nargs, uint32_t &curr
       // to not yet have been declared.
       func_def._dtype = DT_function;
       func_def._flags |= DF_used;
-      func_def._type_id = args[0];
       record_temporary(args[1], args[0], args[2], current_function_id);
     }
     break;
@@ -578,6 +588,7 @@ parse_instruction(spv::Op opcode, uint32_t *args, uint32_t nargs, uint32_t &curr
   case spv::OpArrayLength:
   case spv::OpConvertPtrToU:
     mark_used(args[2]);
+    _defs[args[1]]._type_id = args[0];
     break;
 
   case spv::OpDecorate:
@@ -658,6 +669,7 @@ parse_instruction(spv::Op opcode, uint32_t *args, uint32_t nargs, uint32_t &curr
     for (size_t i = 2; i < nargs; ++i) {
       mark_used(args[i]);
     }
+    _defs[args[1]]._type_id = args[0];
     break;
 
   case spv::OpCopyObject:
@@ -669,6 +681,7 @@ parse_instruction(spv::Op opcode, uint32_t *args, uint32_t nargs, uint32_t &curr
     if (_defs[args[2]]._flags & DF_constant_expression) {
       _defs[args[1]]._flags |= DF_constant_expression;
     }
+    _defs[args[1]]._type_id = args[0];
     break;
 
   case spv::OpImageSampleImplicitLod:
@@ -689,6 +702,7 @@ parse_instruction(spv::Op opcode, uint32_t *args, uint32_t nargs, uint32_t &curr
       if (var_id != 0) {
         _defs[var_id]._flags |= DF_non_dref_sampled;
       }
+      _defs[args[1]]._type_id = args[0];
     }
     break;
 
@@ -708,6 +722,7 @@ parse_instruction(spv::Op opcode, uint32_t *args, uint32_t nargs, uint32_t &curr
       if (var_id != 0) {
         _defs[var_id]._flags |= DF_dref_sampled;
       }
+      _defs[args[1]]._type_id = args[0];
     }
     break;
 
@@ -740,6 +755,7 @@ parse_instruction(spv::Op opcode, uint32_t *args, uint32_t nargs, uint32_t &curr
     if ((_defs[args[2]]._flags & DF_constant_expression) != 0) {
       _defs[args[1]]._flags |= DF_constant_expression;
     }
+    _defs[args[1]]._type_id = args[0];
     break;
 
   // Binary arithmetic operators
@@ -778,6 +794,7 @@ parse_instruction(spv::Op opcode, uint32_t *args, uint32_t nargs, uint32_t &curr
         (_defs[args[3]]._flags & DF_constant_expression) != 0) {
       _defs[args[1]]._flags |= DF_constant_expression;
     }
+    _defs[args[1]]._type_id = args[0];
     break;
 
   case spv::OpSelect:
@@ -791,6 +808,7 @@ parse_instruction(spv::Op opcode, uint32_t *args, uint32_t nargs, uint32_t &curr
         (_defs[args[4]]._flags & DF_constant_expression) != 0) {
       _defs[args[1]]._flags |= DF_constant_expression;
     }
+    _defs[args[1]]._type_id = args[0];
     break;
 
   case spv::OpReturnValue:
@@ -805,9 +823,18 @@ parse_instruction(spv::Op opcode, uint32_t *args, uint32_t nargs, uint32_t &curr
     // on the safe side.
     mark_used(args[2]);
     mark_used(args[3]);
+    _defs[args[1]]._type_id = args[0];
     break;
 
   default:
+    {
+      bool has_result, has_type;
+      HasResultAndType(opcode, &has_result, &has_type);
+      if (has_result && has_type) {
+        // Record the result type of this operation.
+        _defs[args[1]]._type_id = args[0];
+      }
+    }
     break;
   }
 }
@@ -1010,6 +1037,9 @@ record_function_parameter(uint32_t id, uint32_t type_id, uint32_t function_id) {
   def._function_id = function_id;
 
   nassertv(function_id != 0);
+
+  Definition &func_def = modify_definition(function_id);
+  func_def._parameters.push_back(id);
 }
 
 /**

panda/src/shaderpipeline/spirVResultDatabase.h

@@ -80,6 +80,7 @@ public:
     uint32_t _function_id = 0;
     uint32_t _spec_id = 0;
     MemberDefinitions _members;
+    pvector<uint32_t> _parameters;
     int _flags = 0;
 
     // Only defined for DT_variable and DT_pointer_type.
@@ -88,6 +89,7 @@ public:
     INLINE bool is_type() const;
     INLINE bool is_pointer_type() const;
     INLINE bool is_variable() const;
+    INLINE bool is_function_parameter() const;
     INLINE bool is_constant() const;
     INLINE bool is_spec_constant() const;
     INLINE bool is_function() const;

panda/src/shaderpipeline/spirVTransformPass.I

@@ -18,7 +18,7 @@ INLINE uint32_t SpirVTransformPass::
 get_type_id(uint32_t id) const {
   const Definition &def = _db.get_definition(id);
   nassertr(def._type_id != 0, 0);
-  nassertr(is_defined(id), def._type_id);
+  nassertr(is_defined(id) || def.is_function(), def._type_id);
   return def._type_id;
 }
 

panda/src/shaderpipeline/spirVTransformPass.cxx

@@ -232,6 +232,27 @@ transform_definition_op(Instruction op) {
       }
     }
     break;
+
+  case spv::OpTypeFunction:
+    if (op.nargs >= 3) {
+      auto it = _deleted_function_parameters.find(op.args[0]);
+      if (it != _deleted_function_parameters.end()) {
+        pvector<uint32_t> new_args({op.args[0], op.args[1]});
+        for (size_t i = 2; i < op.nargs; ++i) {
+          if (!it->second.count(i - 2)) {
+            if (is_deleted(op.args[i])) {
+              delete_function_parameter(op.args[0], i);
+            } else {
+              new_args.push_back(op.args[i]);
+            }
+          }
+        }
+        add_definition(spv::OpTypeFunction, new_args.data(), new_args.size());
+        mark_defined(op.args[0]);
+        return false;
+      }
+    }
+    break;
   }
   return true;
 }
@@ -308,6 +329,29 @@ transform_function_op(Instruction op, uint32_t function_id) {
     nassertr(!is_deleted(op.args[3]), true);
     break;
 
+  case spv::OpFunctionCall:
+    if (op.nargs >= 4) {
+      uint32_t func_type_id = get_type_id(op.args[2]);
+
+      auto it = _deleted_function_parameters.find(func_type_id);
+      if (it != _deleted_function_parameters.end()) {
+        pvector<uint32_t> new_args({op.args[0], op.args[1], op.args[2]});
+        for (size_t i = 3; i < op.nargs; ++i) {
+          if (!it->second.count(i - 3)) {
+            new_args.push_back(op.args[i]);
+          }
+        }
+        add_instruction(spv::OpFunctionCall, new_args.data(), new_args.size());
+        mark_defined(new_args[1]);
+        return false;
+      }
+    }
+    break;
+
+  case spv::OpReturnValue:
+    nassertr(!is_deleted(op.args[0]), true);
+    break;
+
   default:
     break;
   };
@@ -469,6 +513,25 @@ delete_struct_member(uint32_t id, uint32_t member_index) {
   }
 }
 
+/**
+ * Deletes the given parameter of the given function type.
+ */
+void SpirVTransformPass::
+delete_function_parameter(uint32_t type_id, uint32_t param_index) {
+  if (!_deleted_function_parameters[type_id].insert(param_index).second) {
+    // Was already deleted.
+    return;
+  }
+
+  for (size_t id = 0; id < get_id_bound(); ++id) {
+    const Definition &def = _db.get_definition(id);
+    if (def._type_id == type_id) {
+      uint32_t param_id = def._parameters[param_index];
+      delete_id(param_id);
+    }
+  }
+}
+
 /**
  *
  */

panda/src/shaderpipeline/spirVTransformPass.h

@@ -60,8 +60,10 @@ public:
 
   void add_name(uint32_t id, const std::string &name);
 
-  void delete_struct_member(uint32_t id, uint32_t member_index);
   void delete_id(uint32_t id);
+  void delete_struct_member(uint32_t id, uint32_t member_index);
+  void delete_function_parameter(uint32_t type_id, uint32_t param_index);
+
   INLINE bool is_deleted(uint32_t id) const;
   INLINE bool is_member_deleted(uint32_t id, uint32_t member) const;
 
@@ -126,6 +128,7 @@ protected:
   BitArray _defined;
   pset<uint32_t> _deleted_ids;
   pmap<uint32_t, pset<uint32_t> > _deleted_members;
+  pmap<uint32_t, pset<uint32_t> > _deleted_function_parameters;
 
   SpirVResultDatabase _db;
 };