From da8d2bc175e88c61fe22b3fbc4e57cb187907b56 Mon Sep 17 00:00:00 2001 From: alan-baker Date: Thu, 7 Nov 2019 09:51:38 -0500 Subject: [PATCH] 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 | 15 ++++--- source/val/validate_scopes.cpp | 50 +++++++++++------------ source/val/validate_scopes.h | 3 ++ test/val/val_atomics_test.cpp | 29 ++++++-------- test/val/val_barriers_test.cpp | 33 ++++++--------- test/val/val_decoration_test.cpp | 5 +-- test/val/val_misc_test.cpp | 69 ++++++++++++++++++++++++++++++-- 7 files changed, 128 insertions(+), 76 deletions(-) diff --git a/source/val/validate_misc.cpp b/source/val/validate_misc.cpp index 883893d5..f0deedfa 100644 --- a/source/val/validate_misc.cpp +++ b/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(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 70a65f75..1b987863 100644 --- a/source/val/validate_scopes.cpp +++ b/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; diff --git a/source/val/validate_scopes.h b/source/val/validate_scopes.h index 311ca7ff..ba8b301a 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 129dd7f5..cd723b22 100644 --- a/test/val/val_atomics_test.cpp +++ b/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")); } diff --git a/test/val/val_barriers_test.cpp b/test/val/val_barriers_test.cpp index 18f57f80..ae166d95 100644 --- a/test/val/val_barriers_test.cpp +++ b/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) { diff --git a/test/val/val_decoration_test.cpp b/test/val/val_decoration_test.cpp index 431a4b77..3840d7dd 100644 --- a/test/val/val_decoration_test.cpp +++ b/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, diff --git a/test/val/val_misc_test.cpp b/test/val/val_misc_test.cpp index dd9fff96..93954844 100644 --- a/test/val/val_misc_test.cpp +++ b/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