spirv-val: Validate zero product workgroup size

source/val/validate_builtins.cpp

@@ -260,6 +260,8 @@ class BuiltInsValidator {
   // specified. Seeds id_to_at_reference_checks_ with decorated ids if needed.
   spv_result_t ValidateSingleBuiltInAtDefinition(const Decoration& decoration,
                                                  const Instruction& inst);
+  spv_result_t ValidateSingleBuiltInAtDefinitionVulkan(const Decoration& decoration,
+                                                 const Instruction& inst, const spv::BuiltIn label);
 
   // The following section contains functions which are called when id defined
   // by |inst| is decorated with BuiltIn |decoration|.
@@ -3147,16 +3149,6 @@ spv_result_t BuiltInsValidator::ValidateI32Vec4InputAtDefinition(
 
 spv_result_t BuiltInsValidator::ValidateWorkgroupSizeAtDefinition(
     const Decoration& decoration, const Instruction& inst) {
-  if (spvIsVulkanEnv(_.context()->target_env)) {
-    if (spvIsVulkanEnv(_.context()->target_env) &&
-        !spvOpcodeIsConstant(inst.opcode())) {
-      return _.diag(SPV_ERROR_INVALID_DATA, &inst)
-             << _.VkErrorID(4426)
-             << "Vulkan spec requires BuiltIn WorkgroupSize to be a "
-                "constant. "
-             << GetIdDesc(inst) << " is not a constant.";
-    }
-
     if (spv_result_t error = ValidateI32Vec(
             decoration, inst, 3,
             [this, &inst](const std::string& message) -> spv_result_t {
@@ -3169,7 +3161,29 @@ spv_result_t BuiltInsValidator::ValidateWorkgroupSizeAtDefinition(
             })) {
       return error;
     }
-  }
+
+    if (!spvOpcodeIsConstant(inst.opcode())) {
+      if (spvIsVulkanEnv(_.context()->target_env)) {
+      return _.diag(SPV_ERROR_INVALID_DATA, &inst)
+             << _.VkErrorID(4426)
+             << "Vulkan spec requires BuiltIn WorkgroupSize to be a "
+                "constant. "
+             << GetIdDesc(inst) << " is not a constant.";
+      }
+    } else {
+        // can only validate product if static
+        uint64_t x_size, y_size, z_size;
+        // ValidateI32Vec above confirms there will be 3 words to read
+        bool static_x = _.GetConstantValUint64(inst.word(3), &x_size);
+        bool static_y = _.GetConstantValUint64(inst.word(4), &y_size);
+        bool static_z = _.GetConstantValUint64(inst.word(5), &z_size);
+        if (static_x && static_y && static_z &&
+            ((x_size * y_size * z_size) == 0)) {
+        return _.diag(SPV_ERROR_INVALID_DATA, &inst)
+                << "WorkgroupSize decorations must not have a static "
+                    "product of zero (X = " << x_size << ", Y = " << y_size << ", Z = " << z_size << ").";
+        }
+    }
 
   // Seed at reference checks with this built-in.
   return ValidateWorkgroupSizeAtReference(decoration, inst, inst, inst);
@@ -4112,16 +4126,19 @@ spv_result_t BuiltInsValidator::ValidateSingleBuiltInAtDefinition(
     const Decoration& decoration, const Instruction& inst) {
   const spv::BuiltIn label = spv::BuiltIn(decoration.params()[0]);
 
-  if (!spvIsVulkanEnv(_.context()->target_env)) {
-    // Early return. All currently implemented rules are based on Vulkan spec.
-    //
-    // TODO: If you are adding validation rules for environments other than
-    // Vulkan (or general rules which are not environment independent), then
-    // you need to modify or remove this condition. Consider also adding early
-    // returns into BuiltIn-specific rules, so that the system doesn't spawn new
-    // rules which don't do anything.
-    return SPV_SUCCESS;
+  // Universial checks
+  if (label == spv::BuiltIn::WorkgroupSize) {
+      return ValidateWorkgroupSizeAtDefinition(decoration, inst);
+  }
+
+  if (spvIsVulkanEnv(_.context()->target_env)) {
+    return ValidateSingleBuiltInAtDefinitionVulkan(decoration, inst, label);
   }
+  return SPV_SUCCESS;
+}
+
+spv_result_t BuiltInsValidator::ValidateSingleBuiltInAtDefinitionVulkan(
+    const Decoration& decoration, const Instruction& inst, const spv::BuiltIn label) {
 
   // If you are adding a new BuiltIn enum, please register it here.
   // If the newly added enum has validation rules associated with it
@@ -4214,9 +4231,6 @@ spv_result_t BuiltInsValidator::ValidateSingleBuiltInAtDefinition(
     case spv::BuiltIn::VertexIndex: {
       return ValidateVertexIndexAtDefinition(decoration, inst);
     }
-    case spv::BuiltIn::WorkgroupSize: {
-      return ValidateWorkgroupSizeAtDefinition(decoration, inst);
-    }
     case spv::BuiltIn::VertexId: {
       return ValidateVertexIdAtDefinition(decoration, inst);
     }

source/val/validate_instruction.cpp

@@ -470,10 +470,9 @@ spv_result_t InstructionPass(ValidationState_t& _, const Instruction* inst) {
     }
     _.set_addressing_model(inst->GetOperandAs<spv::AddressingModel>(0));
     _.set_memory_model(inst->GetOperandAs<spv::MemoryModel>(1));
-  } else if (opcode == spv::Op::OpExecutionMode) {
-    const uint32_t entry_point = inst->word(1);
-    _.RegisterExecutionModeForEntryPoint(entry_point,
-                                         spv::ExecutionMode(inst->word(2)));
+  } else if (opcode == spv::Op::OpExecutionMode ||
+             opcode == spv::Op::OpExecutionModeId) {
+    _.RegisterExecutionModeForEntryPoint(inst);
   } else if (opcode == spv::Op::OpVariable) {
     const auto storage_class = inst->GetOperandAs<spv::StorageClass>(2);
     if (auto error = LimitCheckNumVars(_, inst->id(), storage_class)) {

source/val/validate_mode_setting.cpp

@@ -323,6 +323,38 @@ spv_result_t ValidateEntryPoint(ValidationState_t& _, const Instruction* inst) {
     }
   }
 
+  const Instruction* local_size_inst =
+      _.GetLocalSizeInstruction(entry_point_id);
+  if (local_size_inst) {
+    const uint32_t x = local_size_inst->word(3);
+    const uint32_t y = local_size_inst->word(4);
+    const uint32_t z = local_size_inst->word(5);
+    if ((x * y * z) == 0) {
+      return _.diag(SPV_ERROR_INVALID_DATA, local_size_inst)
+             << "Local Size execution mode must not have a product of zero (X "
+                "= "
+             << x << ", Y = " << y << ", Z = " << z << ").";
+    }
+  }
+
+  const Instruction* local_size_id_inst =
+      _.GetLocalSizeIdInstruction(entry_point_id);
+  if (local_size_id_inst) {
+    uint64_t x_size, y_size, z_size;
+    bool static_x =
+        _.GetConstantValUint64(local_size_id_inst->word(3), &x_size);
+    bool static_y =
+        _.GetConstantValUint64(local_size_id_inst->word(4), &y_size);
+    bool static_z =
+        _.GetConstantValUint64(local_size_id_inst->word(5), &z_size);
+    if (static_x && static_y && static_z && ((x_size * y_size * z_size) == 0)) {
+      return _.diag(SPV_ERROR_INVALID_DATA, local_size_inst)
+             << "Local Size Id execution mode must not have a product of zero "
+                "(X = "
+             << x_size << ", Y = " << y_size << ", Z = " << z_size << ").";
+    }
+  }
+
   return SPV_SUCCESS;
 }
 

source/val/validation_state.h

@@ -211,18 +211,30 @@ class ValidationState_t {
   /// instruction
   bool in_block() const;
 
+  /// Description is unique and not shared by all entry points with same id
   struct EntryPointDescription {
     std::string name;
     std::vector<uint32_t> interfaces;
   };
 
+  /// All the meta data about a single entry point id
+  struct EntryPointData {
+    std::vector<EntryPointDescription> descriptions;
+    /// It is presumed that the same function could theoretically be used as
+    /// 'main' by multiple OpEntryPoint instructions.
+    std::set<spv::ExecutionModel> execution_models;
+    std::set<spv::ExecutionMode> execution_modes;
+    const Instruction* local_size = nullptr;
+    const Instruction* local_size_id = nullptr;
+  };
+
   /// Registers |id| as an entry point with |execution_model| and |interfaces|.
   void RegisterEntryPoint(const uint32_t id,
                           spv::ExecutionModel execution_model,
                           EntryPointDescription&& desc) {
     entry_points_.push_back(id);
-    entry_point_to_execution_models_[id].insert(execution_model);
-    entry_point_descriptions_[id].emplace_back(desc);
+    entry_point_data_[id].execution_models.insert(execution_model);
+    entry_point_data_[id].descriptions.emplace_back(desc);
   }
 
   /// Returns a list of entry point function ids
@@ -235,38 +247,66 @@ class ValidationState_t {
   }
 
   /// Registers execution mode for the given entry point.
-  void RegisterExecutionModeForEntryPoint(uint32_t entry_point,
-                                          spv::ExecutionMode execution_mode) {
-    entry_point_to_execution_modes_[entry_point].insert(execution_mode);
+  void RegisterExecutionModeForEntryPoint(const Instruction* inst) {
+    const uint32_t entry_point = inst->word(1);
+    const spv::ExecutionMode mode = spv::ExecutionMode(inst->word(2));
+    entry_point_data_[entry_point].execution_modes.insert(mode);
+
+    // Save for now since the IDs might have not been parsed yet
+    if (mode == spv::ExecutionMode::LocalSize) {
+      entry_point_data_[entry_point].local_size = inst;
+    } else if (mode == spv::ExecutionMode::LocalSizeId) {
+      entry_point_data_[entry_point].local_size_id = inst;
+    }
   }
 
   /// Returns the interface descriptions of a given entry point.
   const std::vector<EntryPointDescription>& entry_point_descriptions(
       uint32_t entry_point) {
-    return entry_point_descriptions_.at(entry_point);
+    return entry_point_data_[entry_point].descriptions;
   }
 
   /// Returns Execution Models for the given Entry Point.
   /// Returns nullptr if none found (would trigger assertion).
   const std::set<spv::ExecutionModel>* GetExecutionModels(
       uint32_t entry_point) const {
-    const auto it = entry_point_to_execution_models_.find(entry_point);
-    if (it == entry_point_to_execution_models_.end()) {
+    const auto it = entry_point_data_.find(entry_point);
+    if (it == entry_point_data_.end()) {
       assert(0);
       return nullptr;
     }
-    return &it->second;
+    return &it->second.execution_models;
   }
 
   /// Returns Execution Modes for the given Entry Point.
   /// Returns nullptr if none found.
   const std::set<spv::ExecutionMode>* GetExecutionModes(
       uint32_t entry_point) const {
-    const auto it = entry_point_to_execution_modes_.find(entry_point);
-    if (it == entry_point_to_execution_modes_.end()) {
+    const auto it = entry_point_data_.find(entry_point);
+    if (it == entry_point_data_.end()) {
+      return nullptr;
+    }
+    return &it->second.execution_modes;
+  }
+
+  /// Returns the Local Size Execution Modes for the given Entry Point.
+  /// Returns nullptr if none found.
+  const Instruction* GetLocalSizeInstruction(uint32_t entry_point) const {
+    const auto it = entry_point_data_.find(entry_point);
+    if (it == entry_point_data_.end()) {
       return nullptr;
     }
-    return &it->second;
+    return it->second.local_size;
+  }
+
+  /// Returns the Local Size Id Execution Modes for the given Entry Point.
+  /// Returns nullptr if none found.
+  const Instruction* GetLocalSizeIdInstruction(uint32_t entry_point) const {
+    const auto it = entry_point_data_.find(entry_point);
+    if (it == entry_point_data_.end()) {
+      return nullptr;
+    }
+    return it->second.local_size_id;
   }
 
   /// Traverses call tree and computes function_to_entry_points_.
@@ -870,9 +910,8 @@ class ValidationState_t {
   /// IDs that are entry points, ie, arguments to OpEntryPoint.
   std::vector<uint32_t> entry_points_;
 
-  /// Maps an entry point id to its descriptions.
-  std::unordered_map<uint32_t, std::vector<EntryPointDescription>>
-      entry_point_descriptions_;
+  /// Maps an entry point id to all the information about it.
+  std::unordered_map<uint32_t, EntryPointData> entry_point_data_;
 
   /// IDs that are entry points, ie, arguments to OpEntryPoint, and root a call
   /// graph that recurses.
@@ -930,16 +969,6 @@ class ValidationState_t {
   /// Maps function ids to function stat objects.
   std::unordered_map<uint32_t, Function*> id_to_function_;
 
-  /// Mapping entry point -> execution models. It is presumed that the same
-  /// function could theoretically be used as 'main' by multiple OpEntryPoint
-  /// instructions.
-  std::unordered_map<uint32_t, std::set<spv::ExecutionModel>>
-      entry_point_to_execution_models_;
-
-  /// Mapping entry point -> execution modes.
-  std::unordered_map<uint32_t, std::set<spv::ExecutionMode>>
-      entry_point_to_execution_modes_;
-
   /// Mapping function -> array of entry points inside this
   /// module which can (indirectly) call the function.
   std::unordered_map<uint32_t, std::vector<uint32_t>> function_to_entry_points_;