spirv-val: Add Vulkan Invocation Sematics check (#4182)

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

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;

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<const uint32_t>(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) {

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

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:

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

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