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
diff --git a/source/val/validate_misc.cpp b/source/val/validate_misc.cpp
index 883893d..f0deedf 100644
--- a/source/val/validate_misc.cpp
+++ b/source/val/validate_misc.cpp
@@ -42,13 +42,18 @@
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 -
diff --git a/source/val/validate_scopes.cpp b/source/val/validate_scopes.cpp
index 70a65f7..1b98786 100644
--- a/source/val/validate_scopes.cpp
+++ b/source/val/validate_scopes.cpp
@@ -39,8 +39,8 @@
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 @@
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 @@
<< "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 @@
<< "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 @@
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;
diff --git a/source/val/validate_scopes.h b/source/val/validate_scopes.h
index 311ca7f..ba8b301 100644
--- a/source/val/validate_scopes.h
+++ b/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);
diff --git a/test/val/val_atomics_test.cpp b/test/val/val_atomics_test.cpp
index 129dd7f..cd723b2 100644
--- a/test/val/val_atomics_test.cpp
+++ b/test/val/val_atomics_test.cpp
@@ -499,9 +499,8 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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"));
}
diff --git a/test/val/val_barriers_test.cpp b/test/val/val_barriers_test.cpp
index 18f57f8..ae166d9 100644
--- a/test/val/val_barriers_test.cpp
+++ b/test/val/val_barriers_test.cpp
@@ -341,9 +341,8 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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) {
diff --git a/test/val/val_decoration_test.cpp b/test/val/val_decoration_test.cpp
index 431a4b7..3840d7d 100644
--- a/test/val/val_decoration_test.cpp
+++ b/test/val/val_decoration_test.cpp
@@ -4861,9 +4861,8 @@
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,
diff --git a/test/val/val_misc_test.cpp b/test/val/val_misc_test.cpp
index dd9fff9..9395484 100644
--- a/test/val/val_misc_test.cpp
+++ b/test/val/val_misc_test.cpp
@@ -144,19 +144,18 @@
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 @@
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
