spirv-val: Validate zero product workgroup size

source/val/validate_builtins.cpp

@@ -269,6 +269,9 @@ 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|.
@@ -3215,27 +3218,44 @@ 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())) {
+  if (spv_result_t error = ValidateI32Vec(
+          decoration, inst, 3,
+          [this, &inst](const std::string& message) -> spv_result_t {
+            return _.diag(SPV_ERROR_INVALID_DATA, &inst)
+                   << _.VkErrorID(4427) << "According to the "
+                   << spvLogStringForEnv(_.context()->target_env)
+                   << " spec BuiltIn WorkgroupSize variable needs to be a "
+                      "3-component 32-bit int vector. "
+                   << message;
+          })) {
+    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.";
     }
-
-    if (spv_result_t error = ValidateI32Vec(
-            decoration, inst, 3,
-            [this, &inst](const std::string& message) -> spv_result_t {
-              return _.diag(SPV_ERROR_INVALID_DATA, &inst)
-                     << _.VkErrorID(4427) << "According to the "
-                     << spvLogStringForEnv(_.context()->target_env)
-                     << " spec BuiltIn WorkgroupSize variable needs to be a "
-                        "3-component 32-bit int vector. "
-                     << message;
-            })) {
-      return error;
+  } else if (inst.opcode() == spv::Op::OpConstantComposite) {
+    // can only validate product if static and not spec constant
+    if (_.FindDef(inst.word(3))->opcode() == spv::Op::OpConstant &&
+        _.FindDef(inst.word(4))->opcode() == spv::Op::OpConstant &&
+        _.FindDef(inst.word(5))->opcode() == spv::Op::OpConstant) {
+      uint64_t x_size, y_size, z_size;
+      // ValidateI32Vec above confirms there will be 3 words to read
+      bool static_x = _.EvalConstantValUint64(inst.word(3), &x_size);
+      bool static_y = _.EvalConstantValUint64(inst.word(4), &y_size);
+      bool static_z = _.EvalConstantValUint64(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 << ").";
+      }
     }
   }
 
@@ -4304,18 +4324,20 @@ spv_result_t BuiltInsValidator::ValidateMeshShadingEXTBuiltinsAtReference(
 spv_result_t BuiltInsValidator::ValidateSingleBuiltInAtDefinition(
     const Decoration& decoration, const Instruction& inst) {
   const spv::BuiltIn label = decoration.builtin();
+  // Universial checks
+  if (label == spv::BuiltIn::WorkgroupSize) {
+    return ValidateWorkgroupSizeAtDefinition(decoration, inst);
+  }
 
-  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;
+  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
   // consider leaving a TODO and/or creating an issue.
@@ -4407,9 +4429,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_mode_setting.cpp

@@ -323,6 +323,42 @@ spv_result_t ValidateEntryPoint(ValidationState_t& _, const Instruction* inst) {
     }
   }
 
+  if (_.EntryPointHasLocalSizeOrId(entry_point_id)) {
+    const Instruction* local_size_inst =
+        _.EntryPointLocalSizeOrId(entry_point_id);
+    if (local_size_inst) {
+      const auto mode = local_size_inst->GetOperandAs<spv::ExecutionMode>(1);
+      const uint32_t operand_x = local_size_inst->GetOperandAs<uint32_t>(2);
+      const uint32_t operand_y = local_size_inst->GetOperandAs<uint32_t>(3);
+      const uint32_t operand_z = local_size_inst->GetOperandAs<uint32_t>(4);
+      if (mode == spv::ExecutionMode::LocalSize) {
+        if ((operand_x * operand_y * operand_z) == 0) {
+          return _.diag(SPV_ERROR_INVALID_DATA, local_size_inst)
+                 << "Local Size execution mode must not have a product of zero "
+                    "(X "
+                    "= "
+                 << operand_x << ", Y = " << operand_y << ", Z = " << operand_z
+                 << ").";
+        }
+      } else if (mode == spv::ExecutionMode::LocalSizeId) {
+        // can only validate product if static and not spec constant
+        // (This is done for us in EvalConstantValUint64)
+        uint64_t x_size, y_size, z_size;
+        bool static_x = _.EvalConstantValUint64(operand_x, &x_size);
+        bool static_y = _.EvalConstantValUint64(operand_y, &y_size);
+        bool static_z = _.EvalConstantValUint64(operand_z, &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;
 }
 

test/val/val_modes_test.cpp

@@ -88,6 +88,99 @@ OpDecorate %int3_1 BuiltIn WorkgroupSize
   EXPECT_THAT(SPV_SUCCESS, ValidateInstructions(env));
 }
 
+TEST_F(ValidateMode, GLComputeZeroWorkgroupSize) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpDecorate %int3_1 BuiltIn WorkgroupSize
+%int = OpTypeInt 32 0
+%int3 = OpTypeVector %int 3
+%int_0 = OpConstant %int 0
+%int_1 = OpConstant %int 1
+%int3_1 = OpConstantComposite %int3 %int_1 %int_0 %int_0
+)" + kVoidFunction;
+
+  CompileSuccessfully(spirv);
+  EXPECT_THAT(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr(
+          "WorkgroupSize decorations must not have a static product of zero"));
+}
+
+TEST_F(ValidateMode, GLComputeZeroSpecWorkgroupSize) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpDecorate %int3_1 BuiltIn WorkgroupSize
+%int = OpTypeInt 32 0
+%int3 = OpTypeVector %int 3
+%int_0 = OpSpecConstant %int 0
+%int_1 = OpConstant %int 1
+%int3_1 = OpConstantComposite %int3 %int_1 %int_0 %int_0
+)" + kVoidFunction;
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
+TEST_F(ValidateMode, GLComputeZeroSpecCompositeWorkgroupSize) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpDecorate %int3_1 BuiltIn WorkgroupSize
+%int = OpTypeInt 32 0
+%int3 = OpTypeVector %int 3
+%int_0 = OpSpecConstant %int 0
+%int_1 = OpSpecConstant %int 1
+%int3_1 = OpSpecConstantComposite %int3 %int_1 %int_0 %int_0
+)" + kVoidFunction;
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
+TEST_F(ValidateMode, KernelZeroWorkgroupSizeConstant) {
+  const std::string spirv = R"(
+OpCapability Addresses
+OpCapability Linkage
+OpCapability Kernel
+OpMemoryModel Physical32 OpenCL
+OpEntryPoint Kernel %main "main"
+OpDecorate %int3_1 BuiltIn WorkgroupSize
+%int = OpTypeInt 32 0
+%int3 = OpTypeVector %int 3
+%int_0 = OpConstant %int 0
+%int_1 = OpConstant %int 1
+%int3_1 = OpConstantComposite %int3 %int_1 %int_0 %int_0
+)" + kVoidFunction;
+
+  CompileSuccessfully(spirv);
+  EXPECT_THAT(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(), HasSubstr("must be a variable"));
+}
+
+TEST_F(ValidateMode, KernelZeroWorkgroupSizeVariable) {
+  const std::string spirv = R"(
+OpCapability Addresses
+OpCapability Linkage
+OpCapability Kernel
+OpMemoryModel Physical32 OpenCL
+OpEntryPoint Kernel %main "main"
+OpDecorate %var BuiltIn WorkgroupSize
+%int = OpTypeInt 32 0
+%int3 = OpTypeVector %int 3
+%ptr = OpTypePointer Input %int3
+%var = OpVariable %ptr Input
+)" + kVoidFunction;
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
 TEST_F(ValidateMode, GLComputeVulkanLocalSize) {
   const std::string spirv = R"(
 OpCapability Shader
@@ -101,6 +194,38 @@ OpExecutionMode %main LocalSize 1 1 1
   EXPECT_THAT(SPV_SUCCESS, ValidateInstructions(env));
 }
 
+TEST_F(ValidateMode, GLComputeZeroLocalSize) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 0
+)" + kVoidFunction;
+
+  CompileSuccessfully(spirv);
+  EXPECT_THAT(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("Local Size execution mode must not have a product of zero"));
+}
+
+TEST_F(ValidateMode, KernelZeroLocalSize) {
+  const std::string spirv = R"(
+OpCapability Addresses
+OpCapability Linkage
+OpCapability Kernel
+OpMemoryModel Physical32 OpenCL
+OpEntryPoint Kernel %main "main"
+OpExecutionMode %main LocalSize 1 1 0
+)" + kVoidFunction;
+
+  CompileSuccessfully(spirv);
+  EXPECT_THAT(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("Local Size execution mode must not have a product of zero"));
+}
+
 TEST_F(ValidateMode, GLComputeVulkanLocalSizeIdBad) {
   const std::string spirv = R"(
 OpCapability Shader
@@ -135,6 +260,64 @@ OpExecutionModeId %main LocalSizeId %int_1 %int_1 %int_1
   EXPECT_THAT(SPV_SUCCESS, ValidateInstructions(env));
 }
 
+TEST_F(ValidateMode, GLComputeZeroLocalSizeId) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionModeId %main LocalSizeId %int_1 %int_0 %int_1
+%int = OpTypeInt 32 0
+%int_1 = OpConstant %int 1
+%int_0 = OpConstant %int 0
+)" + kVoidFunction;
+
+  spv_target_env env = SPV_ENV_UNIVERSAL_1_3;
+  CompileSuccessfully(spirv, env);
+  EXPECT_THAT(SPV_ERROR_INVALID_DATA, ValidateInstructions(env));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr(
+          "Local Size Id execution mode must not have a product of zero"));
+}
+
+TEST_F(ValidateMode, GLComputeZeroSpecLocalSizeId) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionModeId %main LocalSizeId %int_1 %int_0 %int_1
+%int = OpTypeInt 32 0
+%int_1 = OpConstant %int 1
+%int_0 = OpSpecConstant %int 0
+)" + kVoidFunction;
+
+  spv_target_env env = SPV_ENV_UNIVERSAL_1_3;
+  CompileSuccessfully(spirv, env);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(env));
+}
+
+TEST_F(ValidateMode, KernelZeroLocalSizeId) {
+  const std::string spirv = R"(
+OpCapability Addresses
+OpCapability Linkage
+OpCapability Kernel
+OpMemoryModel Physical32 OpenCL
+OpEntryPoint Kernel %main "main"
+OpExecutionModeId %main LocalSizeId %int_1 %int_0 %int_1
+%int = OpTypeInt 32 0
+%int_1 = OpConstant %int 1
+%int_0 = OpConstant %int 0
+)" + kVoidFunction;
+
+  spv_target_env env = SPV_ENV_UNIVERSAL_1_3;
+  CompileSuccessfully(spirv, env);
+  EXPECT_THAT(SPV_ERROR_INVALID_DATA, ValidateInstructions(env));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr(
+          "Local Size Id execution mode must not have a product of zero"));
+}
+
 TEST_F(ValidateMode, FragmentOriginLowerLeftVulkan) {
   const std::string spirv = R"(
 OpCapability Shader