Re-enable OpReadClockKHR validation (#3013)

Re-enable OpReadClockKHR validation

Fixes #2952

* Refactor some common scope validation
* Perform correct validation for scope in OpReadClockKHR
  * Scope must be Subgroup or Device
* new tests

source/val/validate_misc.cpp

@@ -42,13 +42,18 @@ spv_result_t ValidateUndef(ValidationState_t& _, const Instruction* inst) {
 
 spv_result_t ValidateShaderClock(ValidationState_t& _,
                                  const Instruction* inst) {
-// #2952: disabled until scope discussion is resolved.
-#if 0
-  const uint32_t execution_scope = inst->word(3);
-  if (auto error = ValidateExecutionScope(_, inst, execution_scope)) {
+  const uint32_t scope = inst->GetOperandAs<uint32_t>(2);
+  if (auto error = ValidateScope(_, inst, scope)) {
     return error;
   }
-#endif
+
+  bool is_int32 = false, is_const_int32 = false;
+  uint32_t value = 0;
+  std::tie(is_int32, is_const_int32, value) = _.EvalInt32IfConst(scope);
+  if (is_const_int32 && value != SpvScopeSubgroup && value != SpvScopeDevice) {
+    return _.diag(SPV_ERROR_INVALID_DATA, inst)
+           << "Scope must be Subgroup or Device";
+  }
 
   // Result Type must be a 64 - bit unsigned integer type or
   // a vector of two - components of 32 -

source/val/validate_scopes.cpp

@@ -39,8 +39,8 @@ bool IsValidScope(uint32_t scope) {
   return false;
 }
 
-spv_result_t ValidateExecutionScope(ValidationState_t& _,
-                                    const Instruction* inst, uint32_t scope) {
+spv_result_t ValidateScope(ValidationState_t& _, const Instruction* inst,
+                           uint32_t scope) {
   SpvOp opcode = inst->opcode();
   bool is_int32 = false, is_const_int32 = false;
   uint32_t value = 0;
@@ -48,8 +48,7 @@ spv_result_t ValidateExecutionScope(ValidationState_t& _,
 
   if (!is_int32) {
     return _.diag(SPV_ERROR_INVALID_DATA, inst)
-           << spvOpcodeString(opcode)
-           << ": expected Execution Scope to be a 32-bit int";
+           << spvOpcodeString(opcode) << ": expected scope to be a 32-bit int";
   }
 
   if (!is_const_int32) {
@@ -66,7 +65,6 @@ spv_result_t ValidateExecutionScope(ValidationState_t& _,
              << "Scope ids must be constant or specialization constant when "
              << "CooperativeMatrixNV capability is present";
     }
-    return SPV_SUCCESS;
   }
 
   if (is_const_int32 && !IsValidScope(value)) {
@@ -74,6 +72,24 @@ spv_result_t ValidateExecutionScope(ValidationState_t& _,
            << "Invalid scope value:\n " << _.Disassemble(*_.FindDef(scope));
   }
 
+  return SPV_SUCCESS;
+}
+
+spv_result_t ValidateExecutionScope(ValidationState_t& _,
+                                    const Instruction* inst, uint32_t scope) {
+  SpvOp opcode = inst->opcode();
+  bool is_int32 = false, is_const_int32 = false;
+  uint32_t value = 0;
+  std::tie(is_int32, is_const_int32, value) = _.EvalInt32IfConst(scope);
+
+  if (auto error = ValidateScope(_, inst, scope)) {
+    return error;
+  }
+
+  if (!is_const_int32) {
+    return SPV_SUCCESS;
+  }
+
   // Vulkan specific rules
   if (spvIsVulkanEnv(_.context()->target_env)) {
     // Vulkan 1.1 specific rules
@@ -152,34 +168,14 @@ spv_result_t ValidateMemoryScope(ValidationState_t& _, const Instruction* inst,
   uint32_t value = 0;
   std::tie(is_int32, is_const_int32, value) = _.EvalInt32IfConst(scope);
 
-  if (!is_int32) {
-    return _.diag(SPV_ERROR_INVALID_DATA, inst)
-           << spvOpcodeString(opcode)
-           << ": expected Memory Scope to be a 32-bit int";
+  if (auto error = ValidateScope(_, inst, scope)) {
+    return error;
   }
 
   if (!is_const_int32) {
-    if (_.HasCapability(SpvCapabilityShader) &&
-        !_.HasCapability(SpvCapabilityCooperativeMatrixNV)) {
-      return _.diag(SPV_ERROR_INVALID_DATA, inst)
-             << "Scope ids must be OpConstant when Shader capability is "
-             << "present";
-    }
-    if (_.HasCapability(SpvCapabilityShader) &&
-        _.HasCapability(SpvCapabilityCooperativeMatrixNV) &&
-        !spvOpcodeIsConstant(_.GetIdOpcode(scope))) {
-      return _.diag(SPV_ERROR_INVALID_DATA, inst)
-             << "Scope ids must be constant or specialization constant when "
-             << "CooperativeMatrixNV capability is present";
-    }
     return SPV_SUCCESS;
   }
 
-  if (is_const_int32 && !IsValidScope(value)) {
-    return _.diag(SPV_ERROR_INVALID_DATA, inst)
-           << "Invalid scope value:\n " << _.Disassemble(*_.FindDef(scope));
-  }
-
   if (value == SpvScopeQueueFamilyKHR) {
     if (_.HasCapability(SpvCapabilityVulkanMemoryModelKHR)) {
       return SPV_SUCCESS;

source/val/validate_scopes.h

@@ -20,6 +20,9 @@
 namespace spvtools {
 namespace val {
 
+spv_result_t ValidateScope(ValidationState_t& _, const Instruction* inst,
+                           uint32_t scope);
+
 spv_result_t ValidateExecutionScope(ValidationState_t& _,
                                     const Instruction* inst, uint32_t scope);
 

test/val/val_atomics_test.cpp

@@ -499,9 +499,8 @@ TEST_F(ValidateAtomics, AtomicLoadWrongScopeType) {
 
   CompileSuccessfully(GenerateKernelCode(body));
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
-  EXPECT_THAT(
-      getDiagnosticString(),
-      HasSubstr("AtomicLoad: expected Memory Scope to be a 32-bit int"));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("AtomicLoad: expected scope to be a 32-bit int"));
 }
 
 TEST_F(ValidateAtomics, AtomicLoadWrongMemorySemanticsType) {
@@ -674,10 +673,9 @@ OpAtomicStore %f32_var %f32_1 %relaxed %f32_1
 
   CompileSuccessfully(GenerateKernelCode(body));
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
-  EXPECT_THAT(
-      getDiagnosticString(),
-      HasSubstr("AtomicStore: expected Memory Scope to be a 32-bit int\n  "
-                "OpAtomicStore %28 %float_1 %uint_0_1 %float_1\n"));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("AtomicStore: expected scope to be a 32-bit int\n  "
+                        "OpAtomicStore %28 %float_1 %uint_0_1 %float_1\n"));
 }
 
 TEST_F(ValidateAtomics, AtomicStoreWrongMemorySemanticsType) {
@@ -788,9 +786,8 @@ OpAtomicStore %f32_var %device %relaxed %f32_1
 
   CompileSuccessfully(GenerateKernelCode(body));
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
-  EXPECT_THAT(
-      getDiagnosticString(),
-      HasSubstr("AtomicExchange: expected Memory Scope to be a 32-bit int"));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("AtomicExchange: expected scope to be a 32-bit int"));
 }
 
 TEST_F(ValidateAtomics, AtomicExchangeWrongMemorySemanticsType) {
@@ -902,10 +899,9 @@ OpAtomicStore %f32_var %device %relaxed %f32_1
 
   CompileSuccessfully(GenerateKernelCode(body));
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
-  EXPECT_THAT(
-      getDiagnosticString(),
-      HasSubstr("AtomicCompareExchange: expected Memory Scope to be a 32-bit "
-                "int"));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("AtomicCompareExchange: expected scope to be a 32-bit "
+                        "int"));
 }
 
 TEST_F(ValidateAtomics, AtomicCompareExchangeWrongMemorySemanticsType1) {
@@ -1085,8 +1081,7 @@ TEST_F(ValidateAtomics, AtomicFlagTestAndSetWrongScopeType) {
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
   EXPECT_THAT(
       getDiagnosticString(),
-      HasSubstr(
-          "AtomicFlagTestAndSet: expected Memory Scope to be a 32-bit int"));
+      HasSubstr("AtomicFlagTestAndSet: expected scope to be a 32-bit int"));
 }
 
 TEST_F(ValidateAtomics, AtomicFlagTestAndSetWrongMemorySemanticsType) {
@@ -1159,7 +1154,7 @@ OpAtomicFlagClear %u32_var %u64_1 %relaxed
   CompileSuccessfully(GenerateKernelCode(body));
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
   EXPECT_THAT(getDiagnosticString(),
-              HasSubstr("AtomicFlagClear: expected Memory Scope to be a 32-bit "
+              HasSubstr("AtomicFlagClear: expected scope to be a 32-bit "
                         "int\n  OpAtomicFlagClear %30 %ulong_1 %uint_0_1\n"));
 }
 

test/val/val_barriers_test.cpp

@@ -341,9 +341,8 @@ OpControlBarrier %f32_1 %device %none
 
   CompileSuccessfully(GenerateShaderCode(body));
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
-  EXPECT_THAT(
-      getDiagnosticString(),
-      HasSubstr("ControlBarrier: expected Execution Scope to be a 32-bit int"));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("ControlBarrier: expected scope to be a 32-bit int"));
 }
 
 TEST_F(ValidateBarriers, OpControlBarrierU64ExecutionScope) {
@@ -353,9 +352,8 @@ OpControlBarrier %u64_1 %device %none
 
   CompileSuccessfully(GenerateShaderCode(body));
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
-  EXPECT_THAT(
-      getDiagnosticString(),
-      HasSubstr("ControlBarrier: expected Execution Scope to be a 32-bit int"));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("ControlBarrier: expected scope to be a 32-bit int"));
 }
 
 TEST_F(ValidateBarriers, OpControlBarrierFloatMemoryScope) {
@@ -365,9 +363,8 @@ OpControlBarrier %device %f32_1 %none
 
   CompileSuccessfully(GenerateShaderCode(body));
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
-  EXPECT_THAT(
-      getDiagnosticString(),
-      HasSubstr("ControlBarrier: expected Memory Scope to be a 32-bit int"));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("ControlBarrier: expected scope to be a 32-bit int"));
 }
 
 TEST_F(ValidateBarriers, OpControlBarrierU64MemoryScope) {
@@ -377,9 +374,8 @@ OpControlBarrier %device %u64_1 %none
 
   CompileSuccessfully(GenerateShaderCode(body));
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
-  EXPECT_THAT(
-      getDiagnosticString(),
-      HasSubstr("ControlBarrier: expected Memory Scope to be a 32-bit int"));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("ControlBarrier: expected scope to be a 32-bit int"));
 }
 
 TEST_F(ValidateBarriers, OpControlBarrierFloatMemorySemantics) {
@@ -797,9 +793,8 @@ OpMemoryBarrier %f32_1 %acquire_release_uniform_workgroup
 
   CompileSuccessfully(GenerateShaderCode(body));
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
-  EXPECT_THAT(
-      getDiagnosticString(),
-      HasSubstr("MemoryBarrier: expected Memory Scope to be a 32-bit int"));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("MemoryBarrier: expected scope to be a 32-bit int"));
 }
 
 TEST_F(ValidateBarriers, OpMemoryBarrierU64MemoryScope) {
@@ -809,9 +804,8 @@ OpMemoryBarrier %u64_1 %acquire_release_uniform_workgroup
 
   CompileSuccessfully(GenerateShaderCode(body));
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
-  EXPECT_THAT(
-      getDiagnosticString(),
-      HasSubstr("MemoryBarrier: expected Memory Scope to be a 32-bit int"));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("MemoryBarrier: expected scope to be a 32-bit int"));
 }
 
 TEST_F(ValidateBarriers, OpMemoryBarrierFloatMemorySemantics) {
@@ -993,8 +987,7 @@ OpMemoryNamedBarrier %barrier %f32_1 %acquire_release_workgroup
             ValidateInstructions(SPV_ENV_UNIVERSAL_1_1));
   EXPECT_THAT(
       getDiagnosticString(),
-      HasSubstr(
-          "MemoryNamedBarrier: expected Memory Scope to be a 32-bit int"));
+      HasSubstr("MemoryNamedBarrier: expected scope to be a 32-bit int"));
 }
 
 TEST_F(ValidateBarriers, OpMemoryNamedBarrierFloatMemorySemantics) {

test/val/val_decoration_test.cpp

@@ -4861,9 +4861,8 @@ TEST_F(ValidateDecorations,
   CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_4);
   EXPECT_EQ(SPV_ERROR_INVALID_DATA,
             ValidateInstructions(SPV_ENV_UNIVERSAL_1_4));
-  EXPECT_THAT(
-      getDiagnosticString(),
-      HasSubstr("ConstantNull: expected Execution Scope to be a 32-bit int"));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("ConstantNull: expected scope to be a 32-bit int"));
 }
 
 TEST_F(ValidateDecorations,

test/val/val_misc_test.cpp

@@ -144,19 +144,18 @@ OpFunctionEnd)";
   EXPECT_THAT(getDiagnosticString(), HasSubstr("vector of two components"));
 }
 
-// #2952: disabled until scope discussion is resolved.
-TEST_F(ValidateMisc, DISABLED_ShaderClockExecutionScope) {
+TEST_F(ValidateMisc, ShaderClockInvalidScopeValue) {
   const std::string spirv = ShaderClockSpriv + R"(
 %3 = OpTypeFunction %void
 %ulong = OpTypeInt 64 0
 %uint = OpTypeInt 32 0
 %_ptr_Function_ulong = OpTypePointer Function %ulong
-%uint_3 = OpConstant %uint 10
+%uint_10 = OpConstant %uint 10
 %uint_1 = OpConstant %uint 1
 %main = OpFunction %void None %3
 %5 = OpLabel
 %time1 = OpVariable %_ptr_Function_ulong Function
-%11 = OpReadClockKHR %ulong %uint_3
+%11 = OpReadClockKHR %ulong %uint_10
 OpStore %time1 %11
 OpReturn
 OpFunctionEnd)";
@@ -165,6 +164,68 @@ OpFunctionEnd)";
   EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
   EXPECT_THAT(getDiagnosticString(), HasSubstr("Invalid scope value"));
 }
+
+TEST_F(ValidateMisc, ShaderClockSubgroupScope) {
+  const std::string spirv = ShaderClockSpriv + R"(
+%3 = OpTypeFunction %void
+%ulong = OpTypeInt 64 0
+%uint = OpTypeInt 32 0
+%_ptr_Function_ulong = OpTypePointer Function %ulong
+%subgroup = OpConstant %uint 3
+%uint_1 = OpConstant %uint 1
+%main = OpFunction %void None %3
+%5 = OpLabel
+%time1 = OpVariable %_ptr_Function_ulong Function
+%11 = OpReadClockKHR %ulong %subgroup
+OpStore %time1 %11
+OpReturn
+OpFunctionEnd)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
+TEST_F(ValidateMisc, ShaderClockDeviceScope) {
+  const std::string spirv = ShaderClockSpriv + R"(
+%3 = OpTypeFunction %void
+%ulong = OpTypeInt 64 0
+%uint = OpTypeInt 32 0
+%_ptr_Function_ulong = OpTypePointer Function %ulong
+%device = OpConstant %uint 1
+%uint_1 = OpConstant %uint 1
+%main = OpFunction %void None %3
+%5 = OpLabel
+%time1 = OpVariable %_ptr_Function_ulong Function
+%11 = OpReadClockKHR %ulong %device
+OpStore %time1 %11
+OpReturn
+OpFunctionEnd)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
+TEST_F(ValidateMisc, ShaderClockWorkgroupScope) {
+  const std::string spirv = ShaderClockSpriv + R"(
+%3 = OpTypeFunction %void
+%ulong = OpTypeInt 64 0
+%uint = OpTypeInt 32 0
+%_ptr_Function_ulong = OpTypePointer Function %ulong
+%workgroup = OpConstant %uint 2
+%uint_1 = OpConstant %uint 1
+%main = OpFunction %void None %3
+%5 = OpLabel
+%time1 = OpVariable %_ptr_Function_ulong Function
+%11 = OpReadClockKHR %ulong %workgroup
+OpStore %time1 %11
+OpReturn
+OpFunctionEnd)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Scope must be Subgroup or Device"));
+}
 }  // namespace
 }  // namespace val
 }  // namespace spvtools