Moved validation to builtins section

KhronosGroup · Jun 30, 2022 · 14c9841 · 14c9841
1 parent 0a2a688
commit 14c9841
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 48 deletions.
diff --git a/source/val/validate_annotation.cpp b/source/val/validate_annotation.cpp
@@ -260,30 +260,15 @@ spv_result_t ValidateDecorationTarget(ValidationState_t& _, SpvDecoration dec,
       }
       break;
     case SpvDecorationBuiltIn:
-      if (target->opcode() != SpvOpVariable &&
-          !spvOpcodeIsConstant(target->opcode())) {
-        return _.diag(SPV_ERROR_INVALID_DATA, inst)
-               << "BuiltIns can only target variables, structure members or "
-                  "constants";
-      }
-      if (inst->GetOperandAs<SpvBuiltIn>(2) == SpvBuiltInWorkgroupSize) {
-        if (spvOpcodeIsConstant(target->opcode())) {
-          // can only validate product if static
-          uint64_t x_size, y_size, z_size;
-          bool static_x = _.GetConstantValUint64(target->word(3), &x_size);
-          bool static_y = _.GetConstantValUint64(target->word(4), &y_size);
-          bool static_z = _.GetConstantValUint64(target->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.";
-          }
-        } else if (_.HasCapability(SpvCapabilityShader)) {
-          return fail(0) << "must be a constant for WorkgroupSize";
+      if (target->opcode() != SpvOpVariable) {
+        if (!spvOpcodeIsConstant(target->opcode())) {
+          return fail(0)
+                 << "BuiltIns can only target variables, structure members or "
+                    "constants";
+        } else if (inst->GetOperandAs<SpvBuiltIn>(2) !=
+                   SpvBuiltInWorkgroupSize) {
+          return fail(0) << "must be a variable";
         }
-      } else if (target->opcode() != SpvOpVariable) {
-        return fail(0) << "must be a variable";
       }
       break;
     case SpvDecorationNoPerspective:

diff --git a/source/val/validate_builtins.cpp b/source/val/validate_builtins.cpp
@@ -3123,16 +3123,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 {
@@ -3145,7 +3135,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.";
+        }
+    }
 
   // Seed at reference checks with this built-in.
   return ValidateWorkgroupSizeAtReference(decoration, inst, inst, inst);
@@ -4081,6 +4093,11 @@ spv_result_t BuiltInsValidator::ValidateSingleBuiltInAtDefinition(
     const Decoration& decoration, const Instruction& inst) {
   const SpvBuiltIn label = SpvBuiltIn(decoration.params()[0]);
 
+  // TODO - universal check needed before early return
+  if (label == SpvBuiltInWorkgroupSize) {
+      return ValidateWorkgroupSizeAtDefinition(decoration, inst);
+  }
+
   if (!spvIsVulkanEnv(_.context()->target_env)) {
     // Early return. All currently implemented rules are based on Vulkan spec.
     //
@@ -4183,9 +4200,6 @@ spv_result_t BuiltInsValidator::ValidateSingleBuiltInAtDefinition(
     case SpvBuiltInVertexIndex: {
       return ValidateVertexIndexAtDefinition(decoration, inst);
     }
-    case SpvBuiltInWorkgroupSize: {
-      return ValidateWorkgroupSizeAtDefinition(decoration, inst);
-    }
     case SpvBuiltInVertexId: {
       return ValidateVertexIdAtDefinition(decoration, inst);
     }
@@ -4247,6 +4261,7 @@ spv_result_t BuiltInsValidator::ValidateSingleBuiltInAtDefinition(
     case SpvBuiltInCullMaskKHR: {
       return ValidateRayTracingBuiltinsAtDefinition(decoration, inst);
     }
+    case SpvBuiltInWorkgroupSize:  // validated above
     case SpvBuiltInWorkDim:
     case SpvBuiltInGlobalSize:
     case SpvBuiltInEnqueuedWorkgroupSize:

diff --git a/test/val/val_annotation_test.cpp b/test/val/val_annotation_test.cpp
@@ -402,14 +402,9 @@ OpDecorate %var BuiltIn )" +
 %var = OpVariable %ptr Input
 )";
 
+  // Workgroup are valid unless used with Vulkan env (VUID 04427)
   CompileSuccessfully(text);
-  if (deco != "WorkgroupSize") {
-    EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
-  } else {
-    EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
-    EXPECT_THAT(getDiagnosticString(),
-                HasSubstr("must be a constant for WorkgroupSize"));
-  }
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
 }
 
 TEST_P(BuiltInDecorations, IntegerType) {
@@ -424,7 +419,7 @@ OpDecorate %int BuiltIn )" +
 )";
 
   CompileSuccessfully(text);
-  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
   EXPECT_THAT(getDiagnosticString(),
               HasSubstr("BuiltIns can only target variables, structure members "
                         "or constants"));
@@ -448,7 +443,7 @@ OpFunctionEnd
 )";
 
   CompileSuccessfully(text);
-  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
   EXPECT_THAT(getDiagnosticString(),
               HasSubstr("BuiltIns can only target variables, structure members "
                         "or constants"));

diff --git a/test/val/val_builtins_test.cpp b/test/val/val_builtins_test.cpp
@@ -2923,7 +2923,7 @@ OpDecorate %copy BuiltIn WorkgroupSize
   generator.entry_points_.push_back(std::move(entry_point));
 
   CompileSuccessfully(generator.Build(), SPV_ENV_VULKAN_1_0);
-  ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  ASSERT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
   EXPECT_THAT(getDiagnosticString(),
               HasSubstr("BuiltIns can only target variables, structure "
                         "members or constants"));
@@ -3825,7 +3825,7 @@ OpDecorate %void BuiltIn Position
 )";
 
   CompileSuccessfully(text);
-  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
   EXPECT_THAT(getDiagnosticString(),
               HasSubstr("BuiltIns can only target variables, structure members "
                         "or constants"));