From 042eff73fe0d965d0e3f5987b25eb142f5fe1e38 Mon Sep 17 00:00:00 2001 From: sfricke-samsung <46493288+sfricke-samsung@users.noreply.github.com> Date: Tue, 16 Mar 2021 07:53:37 -0700 Subject: [PATCH] spirv-val: Add Vulkan Invocation Sematics check (#4182) --- source/val/validate_atomics.cpp | 7 +- source/val/validate_barriers.cpp | 6 +- source/val/validate_memory_semantics.cpp | 15 ++++- source/val/validate_memory_semantics.h | 3 +- source/val/validation_state.cpp | 2 + test/val/val_atomics_test.cpp | 82 ++++++++++++++++++++++++ test/val/val_barriers_test.cpp | 24 +++++++ 7 files changed, 131 insertions(+), 8 deletions(-) diff --git a/source/val/validate_atomics.cpp b/source/val/validate_atomics.cpp index 8c7bc02f2..eba3e50e6 100644 --- a/source/val/validate_atomics.cpp +++ b/source/val/validate_atomics.cpp @@ -293,14 +293,15 @@ spv_result_t AtomicsPass(ValidationState_t& _, const Instruction* inst) { } const auto equal_semantics_index = operand_index++; - if (auto error = ValidateMemorySemantics(_, inst, equal_semantics_index)) + if (auto error = ValidateMemorySemantics(_, inst, equal_semantics_index, + memory_scope)) return error; if (opcode == SpvOpAtomicCompareExchange || opcode == SpvOpAtomicCompareExchangeWeak) { const auto unequal_semantics_index = operand_index++; - if (auto error = - ValidateMemorySemantics(_, inst, unequal_semantics_index)) + if (auto error = ValidateMemorySemantics( + _, inst, unequal_semantics_index, memory_scope)) return error; // Volatile bits must match for equal and unequal semantics. Previous diff --git a/source/val/validate_barriers.cpp b/source/val/validate_barriers.cpp index b499c8c08..3a9e3e7c5 100644 --- a/source/val/validate_barriers.cpp +++ b/source/val/validate_barriers.cpp @@ -69,7 +69,7 @@ spv_result_t BarriersPass(ValidationState_t& _, const Instruction* inst) { return error; } - if (auto error = ValidateMemorySemantics(_, inst, 2)) { + if (auto error = ValidateMemorySemantics(_, inst, 2, memory_scope)) { return error; } break; @@ -82,7 +82,7 @@ spv_result_t BarriersPass(ValidationState_t& _, const Instruction* inst) { return error; } - if (auto error = ValidateMemorySemantics(_, inst, 1)) { + if (auto error = ValidateMemorySemantics(_, inst, 1, memory_scope)) { return error; } break; @@ -119,7 +119,7 @@ spv_result_t BarriersPass(ValidationState_t& _, const Instruction* inst) { return error; } - if (auto error = ValidateMemorySemantics(_, inst, 2)) { + if (auto error = ValidateMemorySemantics(_, inst, 2, memory_scope)) { return error; } break; diff --git a/source/val/validate_memory_semantics.cpp b/source/val/validate_memory_semantics.cpp index 6c3e1d44b..d9189313a 100644 --- a/source/val/validate_memory_semantics.cpp +++ b/source/val/validate_memory_semantics.cpp @@ -25,7 +25,8 @@ namespace val { spv_result_t ValidateMemorySemantics(ValidationState_t& _, const Instruction* inst, - uint32_t operand_index) { + uint32_t operand_index, + uint32_t memory_scope) { const SpvOp opcode = inst->opcode(); const auto id = inst->GetOperandAs(operand_index); bool is_int32 = false, is_const_int32 = false; @@ -178,6 +179,18 @@ spv_result_t ValidateMemorySemantics(ValidationState_t& _, "of the following bits set: Acquire, Release, " "AcquireRelease " "or SequentiallyConsistent"; + } else if (opcode != SpvOpMemoryBarrier && num_memory_order_set_bits) { + // should leave only atomics and control barriers for Vulkan env + bool memory_is_int32 = false, memory_is_const_int32 = false; + uint32_t memory_value = 0; + std::tie(memory_is_int32, memory_is_const_int32, memory_value) = + _.EvalInt32IfConst(memory_scope); + if (memory_is_int32 && memory_value == SpvScopeInvocation) { + return _.diag(SPV_ERROR_INVALID_DATA, inst) + << _.VkErrorID(4641) << spvOpcodeString(opcode) + << ": Vulkan specification requires Memory Semantics to be None " + "if used with Invocation Memory Scope"; + } } if (opcode == SpvOpMemoryBarrier && !includes_storage_class) { diff --git a/source/val/validate_memory_semantics.h b/source/val/validate_memory_semantics.h index 72a3e1004..9e6f93a36 100644 --- a/source/val/validate_memory_semantics.h +++ b/source/val/validate_memory_semantics.h @@ -22,7 +22,8 @@ namespace val { spv_result_t ValidateMemorySemantics(ValidationState_t& _, const Instruction* inst, - uint32_t operand_index); + uint32_t operand_index, + uint32_t memory_scope); } // namespace val } // namespace spvtools diff --git a/source/val/validation_state.cpp b/source/val/validation_state.cpp index 514fc051e..2386a2b3b 100644 --- a/source/val/validation_state.cpp +++ b/source/val/validation_state.cpp @@ -1686,6 +1686,8 @@ std::string ValidationState_t::VkErrorID(uint32_t id, return VUID_WRAP(VUID-StandaloneSpirv-None-04639); case 4640: return VUID_WRAP(VUID-StandaloneSpirv-None-04640); + case 4641: + return VUID_WRAP(VUID-StandaloneSpirv-None-04641); case 4642: return VUID_WRAP(VUID-StandaloneSpirv-None-04642); case 4651: diff --git a/test/val/val_atomics_test.cpp b/test/val/val_atomics_test.cpp index ea37b6052..90d0b3d1f 100644 --- a/test/val/val_atomics_test.cpp +++ b/test/val/val_atomics_test.cpp @@ -244,6 +244,7 @@ TEST_F(ValidateAtomics, AtomicLoadInt32VulkanSuccess) { const std::string body = R"( %val1 = OpAtomicLoad %u32 %u32_var %device %relaxed %val2 = OpAtomicLoad %u32 %u32_var %workgroup %acquire +%val3 = OpAtomicLoad %u32 %u32_var %invocation %relaxed )"; CompileSuccessfully(GenerateShaderComputeCode(body), SPV_ENV_VULKAN_1_0); @@ -354,6 +355,7 @@ OpExtension "SPV_EXT_shader_atomic_float_add" TEST_F(ValidateAtomics, AtomicAddFloatVulkanSuccess) { const std::string body = R"( %val1 = OpAtomicFAddEXT %f32 %f32_var %device %relaxed %f32_1 +%val2 = OpAtomicFAddEXT %f32 %f32_var %invocation %relaxed %f32_1 )"; const std::string extra = R"( OpCapability AtomicFloat32AddEXT @@ -396,6 +398,7 @@ TEST_F(ValidateAtomics, AtomicLoadInt64WithCapabilityVulkanSuccess) { const std::string body = R"( %val1 = OpAtomicLoad %u64 %u64_var %device %relaxed %val2 = OpAtomicLoad %u64 %u64_var %workgroup %acquire + %val3 = OpAtomicLoad %u64 %u64_var %invocation %relaxed )"; CompileSuccessfully( @@ -515,6 +518,21 @@ TEST_F(ValidateAtomics, AtomicLoadVulkanSequentiallyConsistent) { "Release, AcquireRelease and SequentiallyConsistent")); } +TEST_F(ValidateAtomics, AtomicLoadVulkanInvocationSemantics) { + const std::string body = R"( +%val1 = OpAtomicLoad %u32 %u32_var %invocation %acquire +)"; + + CompileSuccessfully(GenerateShaderCode(body), SPV_ENV_VULKAN_1_0); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0)); + EXPECT_THAT(getDiagnosticString(), + AnyVUID("VUID-StandaloneSpirv-None-04641")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("AtomicLoad: Vulkan specification requires Memory Semantics to " + "be None if used with Invocation Memory Scope")); +} + TEST_F(ValidateAtomics, AtomicLoadShaderFloat) { const std::string body = R"( %val1 = OpAtomicLoad %f32 %f32_var %device %relaxed @@ -681,6 +699,7 @@ OpAtomicStore %u32_var %subgroup %sequentially_consistent %u32_1 TEST_F(ValidateAtomics, AtomicStoreVulkanSuccess) { const std::string body = R"( OpAtomicStore %u32_var %device %release %u32_1 +OpAtomicStore %u32_var %invocation %relaxed %u32_1 )"; CompileSuccessfully(GenerateShaderCode(body), SPV_ENV_VULKAN_1_0); @@ -732,6 +751,21 @@ OpAtomicStore %u32_var %device %sequentially_consistent %u32_1 "Acquire, AcquireRelease and SequentiallyConsistent")); } +TEST_F(ValidateAtomics, AtomicStoreVulkanInvocationSemantics) { + const std::string body = R"( +OpAtomicStore %u32_var %invocation %acquire %u32_1 +)"; + + CompileSuccessfully(GenerateShaderCode(body), SPV_ENV_VULKAN_1_0); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0)); + EXPECT_THAT(getDiagnosticString(), + AnyVUID("VUID-StandaloneSpirv-None-04641")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("AtomicStore: Vulkan specification requires Memory Semantics " + "to be None if used with Invocation Memory Scope")); +} + TEST_F(ValidateAtomics, AtomicStoreWrongPointerType) { const std::string body = R"( OpAtomicStore %f32_1 %device %relaxed %f32_1 @@ -931,6 +965,22 @@ OpAtomicStore %f32_var %device %relaxed %f32_1 "expected Value to be of type Result Type")); } +TEST_F(ValidateAtomics, AtomicExchangeVulkanInvocationSemantics) { + const std::string body = R"( +OpAtomicStore %u32_var %invocation %relaxed %u32_1 +%val2 = OpAtomicExchange %u32 %u32_var %invocation %acquire %u32_0 +)"; + + CompileSuccessfully(GenerateShaderCode(body), SPV_ENV_VULKAN_1_0); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0)); + EXPECT_THAT(getDiagnosticString(), + AnyVUID("VUID-StandaloneSpirv-None-04641")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("AtomicExchange: Vulkan specification requires Memory " + "Semantics to be None if used with Invocation Memory Scope")); +} + TEST_F(ValidateAtomics, AtomicCompareExchangeShaderSuccess) { const std::string body = R"( OpAtomicStore %u32_var %device %relaxed %u32_1 @@ -1106,6 +1156,38 @@ OpAtomicStore %f32_var %device %relaxed %f32_1 "expected Result Type to be int scalar type")); } +TEST_F(ValidateAtomics, AtomicCompareExchangeVulkanInvocationSemanticsEqual) { + const std::string body = R"( +OpAtomicStore %u32_var %device %relaxed %u32_1 +%val2 = OpAtomicCompareExchange %u32 %u32_var %invocation %release %relaxed %u32_0 %u32_0 +)"; + + CompileSuccessfully(GenerateShaderCode(body), SPV_ENV_VULKAN_1_0); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0)); + EXPECT_THAT(getDiagnosticString(), + AnyVUID("VUID-StandaloneSpirv-None-04641")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("AtomicCompareExchange: Vulkan specification requires Memory " + "Semantics to be None if used with Invocation Memory Scope")); +} + +TEST_F(ValidateAtomics, AtomicCompareExchangeVulkanInvocationSemanticsUnequal) { + const std::string body = R"( +OpAtomicStore %u32_var %device %relaxed %u32_1 +%val2 = OpAtomicCompareExchange %u32 %u32_var %invocation %relaxed %acquire %u32_0 %u32_0 +)"; + + CompileSuccessfully(GenerateShaderCode(body), SPV_ENV_VULKAN_1_0); + ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0)); + EXPECT_THAT(getDiagnosticString(), + AnyVUID("VUID-StandaloneSpirv-None-04641")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("AtomicCompareExchange: Vulkan specification requires Memory " + "Semantics to be None if used with Invocation Memory Scope")); +} + TEST_F(ValidateAtomics, AtomicArithmeticsSuccess) { const std::string body = R"( OpAtomicStore %u32_var %device %relaxed %u32_1 diff --git a/test/val/val_barriers_test.cpp b/test/val/val_barriers_test.cpp index 46f5b5a1c..c36bd1952 100644 --- a/test/val/val_barriers_test.cpp +++ b/test/val/val_barriers_test.cpp @@ -411,6 +411,30 @@ OpControlBarrier %workgroup %workgroup %acquire ASSERT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_1)); } +TEST_F(ValidateBarriers, OpControlBarrierVulkanInvocationSuccess) { + const std::string body = R"( +OpControlBarrier %workgroup %invocation %none +)"; + + CompileSuccessfully(GenerateShaderCode(body), SPV_ENV_VULKAN_1_0); + ASSERT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_0)); +} + +TEST_F(ValidateBarriers, OpControlBarrierVulkanInvocationFailure) { + const std::string body = R"( +OpControlBarrier %workgroup %invocation %acquire +)"; + + CompileSuccessfully(GenerateShaderCode(body), SPV_ENV_VULKAN_1_0); + EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0)); + EXPECT_THAT(getDiagnosticString(), + AnyVUID("VUID-StandaloneSpirv-None-04641")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("ControlBarrier: Vulkan specification requires Memory " + "Semantics to be None if used with Invocation Memory Scope")); +} + TEST_F(ValidateBarriers, OpControlBarrierAcquireAndRelease) { const std::string body = R"( OpControlBarrier %device %device %acquire_and_release_uniform