From 2cd040b0d34223fa95a397ec630a369f2c4ab33e Mon Sep 17 00:00:00 2001 From: Ryan Harrison Date: Thu, 29 Nov 2018 14:51:17 -0500 Subject: [PATCH] Merging two ValidateMemoryScope implementations (#2132) Fixes #2125 --- source/val/validate_atomics.cpp | 36 +--------------------- source/val/validate_barriers.cpp | 40 ------------------------- source/val/validate_scopes.cpp | 51 ++++++++++++++++++++++++++++++++ source/val/validate_scopes.h | 3 ++ test/val/val_atomics_test.cpp | 34 ++++++++++++++------- 5 files changed, 78 insertions(+), 86 deletions(-) diff --git a/source/val/validate_atomics.cpp b/source/val/validate_atomics.cpp index a36771ab6..b2f2edc32 100644 --- a/source/val/validate_atomics.cpp +++ b/source/val/validate_atomics.cpp @@ -21,46 +21,12 @@ #include "source/spirv_target_env.h" #include "source/util/bitutils.h" #include "source/val/instruction.h" +#include "source/val/validate_scopes.h" #include "source/val/validation_state.h" namespace spvtools { namespace val { -// Validates Memory Scope operand. -spv_result_t ValidateMemoryScope(ValidationState_t& _, const Instruction* inst, - uint32_t id) { - const 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(id); - - if (!is_int32) { - return _.diag(SPV_ERROR_INVALID_DATA, inst) - << spvOpcodeString(opcode) << ": expected Scope to be 32-bit int"; - } - - if (!is_const_int32) { - return SPV_SUCCESS; - } - -#if 0 - // TODO(atgoo@github.com): this check fails Vulkan CTS, reenable once fixed. - if (spvIsVulkanEnv(_.context()->target_env)) { - if (value != SpvScopeDevice && value != SpvScopeWorkgroup && - value != SpvScopeInvocation) { - return _.diag(SPV_ERROR_INVALID_DATA, inst) - << spvOpcodeString(opcode) - << ": in Vulkan environment memory scope is limited to Device, " - "Workgroup and Invocation"; - } - } -#endif - - // TODO(atgoo@github.com) Add checks for OpenCL and OpenGL environments. - - return SPV_SUCCESS; -} - // Validates a Memory Semantics operand. spv_result_t ValidateMemorySemantics(ValidationState_t& _, const Instruction* inst, diff --git a/source/val/validate_barriers.cpp b/source/val/validate_barriers.cpp index 3ed0f5781..86e178abe 100644 --- a/source/val/validate_barriers.cpp +++ b/source/val/validate_barriers.cpp @@ -31,46 +31,6 @@ namespace spvtools { namespace val { namespace { -// Validates Memory Scope operand. -spv_result_t ValidateMemoryScope(ValidationState_t& _, const Instruction* inst, - uint32_t id) { - const 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(id); - - if (!is_int32) { - return _.diag(SPV_ERROR_INVALID_DATA, inst) - << spvOpcodeString(opcode) - << ": expected Memory Scope to be a 32-bit int"; - } - - if (!is_const_int32) { - return SPV_SUCCESS; - } - - if (spvIsVulkanEnv(_.context()->target_env)) { - if (value == SpvScopeCrossDevice) { - return _.diag(SPV_ERROR_INVALID_DATA, inst) - << spvOpcodeString(opcode) - << ": in Vulkan environment, Memory Scope cannot be CrossDevice"; - } - if (_.context()->target_env == SPV_ENV_VULKAN_1_0 && - value != SpvScopeDevice && value != SpvScopeWorkgroup && - value != SpvScopeInvocation) { - return _.diag(SPV_ERROR_INVALID_DATA, inst) - << spvOpcodeString(opcode) - << ": in Vulkan 1.0 environment Memory Scope is limited to " - "Device, " - "Workgroup and Invocation"; - } - } - - // TODO(atgoo@github.com) Add checks for OpenCL and OpenGL environments. - - return SPV_SUCCESS; -} - // Validates Memory Semantics operand. spv_result_t ValidateMemorySemantics(ValidationState_t& _, const Instruction* inst, uint32_t id) { diff --git a/source/val/validate_scopes.cpp b/source/val/validate_scopes.cpp index a5ac7ae06..13383d223 100644 --- a/source/val/validate_scopes.cpp +++ b/source/val/validate_scopes.cpp @@ -100,5 +100,56 @@ spv_result_t ValidateExecutionScope(ValidationState_t& _, return SPV_SUCCESS; } +spv_result_t ValidateMemoryScope(ValidationState_t& _, const Instruction* inst, + uint32_t scope) { + const 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 (!is_int32) { + return _.diag(SPV_ERROR_INVALID_DATA, inst) + << spvOpcodeString(opcode) + << ": expected Memory Scope to be a 32-bit int"; + } + + if (!is_const_int32) { + return SPV_SUCCESS; + } + + // Vulkan Specific rules + if (spvIsVulkanEnv(_.context()->target_env)) { + if (value == SpvScopeCrossDevice) { + return _.diag(SPV_ERROR_INVALID_DATA, inst) + << spvOpcodeString(opcode) + << ": in Vulkan environment, Memory Scope cannot be CrossDevice"; + } + // Vulkan 1.0 specifc rules + if (_.context()->target_env == SPV_ENV_VULKAN_1_0 && + value != SpvScopeDevice && value != SpvScopeWorkgroup && + value != SpvScopeInvocation) { + return _.diag(SPV_ERROR_INVALID_DATA, inst) + << spvOpcodeString(opcode) + << ": in Vulkan 1.0 environment Memory Scope is limited to " + "Device, " + "Workgroup and Invocation"; + } + // Vulkan 1.1 specifc rules + if (_.context()->target_env == SPV_ENV_VULKAN_1_0 && + value != SpvScopeDevice && value != SpvScopeWorkgroup && + value != SpvScopeSubgroup && value != SpvScopeInvocation) { + return _.diag(SPV_ERROR_INVALID_DATA, inst) + << spvOpcodeString(opcode) + << ": in Vulkan 1.1 environment Memory Scope is limited to " + "Device, " + "Workgroup and Invocation"; + } + } + + // TODO(atgoo@github.com) Add checks for OpenCL and OpenGL environments. + + return SPV_SUCCESS; +} + } // namespace val } // namespace spvtools diff --git a/source/val/validate_scopes.h b/source/val/validate_scopes.h index 40e533e71..311ca7ff2 100644 --- a/source/val/validate_scopes.h +++ b/source/val/validate_scopes.h @@ -23,5 +23,8 @@ namespace val { spv_result_t ValidateExecutionScope(ValidationState_t& _, const Instruction* inst, uint32_t scope); +spv_result_t ValidateMemoryScope(ValidationState_t& _, const Instruction* inst, + uint32_t scope); + } // namespace val } // namespace spvtools diff --git a/test/val/val_atomics_test.cpp b/test/val/val_atomics_test.cpp index 5250d4773..6cf2c9b07 100644 --- a/test/val/val_atomics_test.cpp +++ b/test/val/val_atomics_test.cpp @@ -404,8 +404,10 @@ TEST_F(ValidateAtomics, AtomicLoadWrongScopeType) { CompileSuccessfully(GenerateKernelCode(body)); ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); - EXPECT_THAT(getDiagnosticString(), - HasSubstr("AtomicLoad: expected Scope to be 32-bit int")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("AtomicLoad: expected Memory Scope to be a 32-bit int\n %40 = " + "OpAtomicLoad %float %28 %float_1 %uint_0_1\n")); } TEST_F(ValidateAtomics, AtomicLoadWrongMemorySemanticsType) { @@ -535,8 +537,10 @@ OpAtomicStore %f32_var %f32_1 %relaxed %f32_1 CompileSuccessfully(GenerateKernelCode(body)); ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); - EXPECT_THAT(getDiagnosticString(), - HasSubstr("AtomicStore: expected Scope to be 32-bit int")); + 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")); } TEST_F(ValidateAtomics, AtomicStoreWrongMemorySemanticsType) { @@ -648,8 +652,11 @@ OpAtomicStore %f32_var %device %relaxed %f32_1 CompileSuccessfully(GenerateKernelCode(body)); ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); - EXPECT_THAT(getDiagnosticString(), - HasSubstr("AtomicExchange: expected Scope to be 32-bit int")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr( + "AtomicExchange: expected Memory Scope to be a 32-bit int\n %40 = " + "OpAtomicExchange %float %28 %float_1 %uint_0_1 %float_0\n")); } TEST_F(ValidateAtomics, AtomicExchangeWrongMemorySemanticsType) { @@ -762,7 +769,9 @@ OpAtomicStore %f32_var %device %relaxed %f32_1 ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); EXPECT_THAT( getDiagnosticString(), - HasSubstr("AtomicCompareExchange: expected Scope to be 32-bit int")); + HasSubstr("AtomicCompareExchange: expected Memory Scope to be a 32-bit " + "int\n %40 = OpAtomicCompareExchange %float %28 %float_1 " + "%uint_0_1 %uint_0_1 %float_0 %float_0\n")); } TEST_F(ValidateAtomics, AtomicCompareExchangeWrongMemorySemanticsType1) { @@ -942,9 +951,11 @@ TEST_F(ValidateAtomics, AtomicFlagTestAndSetWrongScopeType) { CompileSuccessfully(GenerateKernelCode(body)); ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); - EXPECT_THAT(getDiagnosticString(), - HasSubstr("AtomicFlagTestAndSet: " - "expected Scope to be 32-bit int")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr( + "AtomicFlagTestAndSet: expected Memory Scope to be a 32-bit int\n " + "%40 = OpAtomicFlagTestAndSet %bool %30 %ulong_1 %uint_0_1\n")); } TEST_F(ValidateAtomics, AtomicFlagTestAndSetWrongMemorySemanticsType) { @@ -1017,7 +1028,8 @@ OpAtomicFlagClear %u32_var %u64_1 %relaxed CompileSuccessfully(GenerateKernelCode(body)); ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); EXPECT_THAT(getDiagnosticString(), - HasSubstr("AtomicFlagClear: expected Scope to be 32-bit int")); + HasSubstr("AtomicFlagClear: expected Memory Scope to be a 32-bit " + "int\n OpAtomicFlagClear %30 %ulong_1 %uint_0_1\n")); } TEST_F(ValidateAtomics, AtomicFlagClearWrongMemorySemanticsType) {