Validate Volatile memory semantics bit (#2672)

* Can only be used with Vulkan memory model * Can only be used with atomics * Bit setting must match for compare exchange opcodes * Updated memory semantics checks to allow constant instructions generally with CooperativeMatrixNV
2025-01-13 09:50:06 +00:00 · 2019-06-17 13:35:40 -04:00 · 2019-06-17 13:35:40 -04:00 · 3d5fb7b908
commit 3d5fb7b908
parent 400dbde0ba
4 changed files with 307 additions and 3 deletions
--- a/source/val/validate_atomics.cpp
+++ b/source/val/validate_atomics.cpp
@ -175,13 +175,37 @@ spv_result_t AtomicsPass(ValidationState_t& _, const Instruction* inst) {
        return error;
      }

-      if (auto error = ValidateMemorySemantics(_, inst, operand_index++))
+      const auto equal_semantics_index = operand_index++;
+      if (auto error = ValidateMemorySemantics(_, inst, equal_semantics_index))
        return error;

      if (opcode == SpvOpAtomicCompareExchange ||
          opcode == SpvOpAtomicCompareExchangeWeak) {
-        if (auto error = ValidateMemorySemantics(_, inst, operand_index++))
+        const auto unequal_semantics_index = operand_index++;
+        if (auto error =
+                ValidateMemorySemantics(_, inst, unequal_semantics_index))
          return error;
+
+        // Volatile bits must match for equal and unequal semantics. Previous
+        // checks guarantee they are 32-bit constants, but we need to recheck
+        // whether they are evaluatable constants.
+        bool is_int32 = false;
+        bool is_equal_const = false;
+        bool is_unequal_const = false;
+        uint32_t equal_value = 0;
+        uint32_t unequal_value = 0;
+        std::tie(is_int32, is_equal_const, equal_value) = _.EvalInt32IfConst(
+            inst->GetOperandAs<uint32_t>(equal_semantics_index));
+        std::tie(is_int32, is_unequal_const, unequal_value) =
+            _.EvalInt32IfConst(
+                inst->GetOperandAs<uint32_t>(unequal_semantics_index));
+        if (is_equal_const && is_unequal_const &&
+            ((equal_value & SpvMemorySemanticsVolatileMask) ^
+             (unequal_value & SpvMemorySemanticsVolatileMask))) {
+          return _.diag(SPV_ERROR_INVALID_ID, inst)
+                 << "Volatile mask setting must match for Equal and Unequal "
+                    "memory semantics";
+        }
      }

      if (opcode == SpvOpAtomicStore) {
--- a/source/val/validate_memory_semantics.cpp
+++ b/source/val/validate_memory_semantics.cpp
@ -39,11 +39,20 @@ spv_result_t ValidateMemorySemantics(ValidationState_t& _,
  }

  if (!is_const_int32) {
-    if (_.HasCapability(SpvCapabilityShader)) {
+    if (_.HasCapability(SpvCapabilityShader) &&
+        !_.HasCapability(SpvCapabilityCooperativeMatrixNV)) {
      return _.diag(SPV_ERROR_INVALID_DATA, inst)
             << "Memory Semantics ids must be OpConstant when Shader "
                "capability is present";
    }
+
+    if (_.HasCapability(SpvCapabilityShader) &&
+        _.HasCapability(SpvCapabilityCooperativeMatrixNV) &&
+        !spvOpcodeIsConstant(_.GetIdOpcode(id))) {
+      return _.diag(SPV_ERROR_INVALID_DATA, inst)
+             << "Memory Semantics must be a constant instruction when "
+                "CooperativeMatrixNV capability is present";
+    }
    return SPV_SUCCESS;
  }

@ -127,6 +136,21 @@ spv_result_t ValidateMemorySemantics(ValidationState_t& _,
           << "VulkanMemoryModelKHR";
  }

+  if (value & SpvMemorySemanticsVolatileMask) {
+    if (!_.HasCapability(SpvCapabilityVulkanMemoryModelKHR)) {
+      return _.diag(SPV_ERROR_INVALID_DATA, inst)
+             << spvOpcodeString(opcode)
+             << ": Memory Semantics Volatile requires capability "
+                "VulkanMemoryModelKHR";
+    }
+
+    if (!spvOpcodeIsAtomicOp(inst->opcode())) {
+      return _.diag(SPV_ERROR_INVALID_DATA, inst)
+             << "Memory Semantics Volatile can only be used with atomic "
+                "instructions";
+    }
+  }
+
  if (value & SpvMemorySemanticsUniformMemoryMask &&
      !_.HasCapability(SpvCapabilityShader)) {
    return _.diag(SPV_ERROR_INVALID_DATA, inst)
--- a/test/val/val_atomics_test.cpp
+++ b/test/val/val_atomics_test.cpp
@ -1998,6 +1998,153 @@ TEST_F(ValidateAtomics, CompareExchangeWeakV14Bad) {
          "AtomicCompareExchangeWeak requires SPIR-V version 1.3 or earlier"));
 }

+TEST_F(ValidateAtomics, CompareExchangeVolatileMatch) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability VulkanMemoryModelKHR
+OpCapability Linkage
+OpExtension "SPV_KHR_vulkan_memory_model"
+OpMemoryModel Logical VulkanKHR
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%int_0 = OpConstant %int 0
+%int_1 = OpConstant %int 1
+%workgroup = OpConstant %int 2
+%volatile = OpConstant %int 32768
+%ptr_wg_int = OpTypePointer Workgroup %int
+%wg_var = OpVariable %ptr_wg_int Workgroup
+%void_fn = OpTypeFunction %void
+%func = OpFunction %void None %void_fn
+%entry = OpLabel
+%cmp_ex = OpAtomicCompareExchange %int %wg_var %workgroup %volatile %volatile %int_0 %int_1
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
+TEST_F(ValidateAtomics, CompareExchangeVolatileMismatch) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability VulkanMemoryModelKHR
+OpCapability Linkage
+OpExtension "SPV_KHR_vulkan_memory_model"
+OpMemoryModel Logical VulkanKHR
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%int_0 = OpConstant %int 0
+%int_1 = OpConstant %int 1
+%workgroup = OpConstant %int 2
+%volatile = OpConstant %int 32768
+%non_volatile = OpConstant %int 0
+%ptr_wg_int = OpTypePointer Workgroup %int
+%wg_var = OpVariable %ptr_wg_int Workgroup
+%void_fn = OpTypeFunction %void
+%func = OpFunction %void None %void_fn
+%entry = OpLabel
+%cmp_ex = OpAtomicCompareExchange %int %wg_var %workgroup %non_volatile %volatile %int_0 %int_1
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Volatile mask setting must match for Equal and "
+                        "Unequal memory semantics"));
+}
+
+TEST_F(ValidateAtomics, CompareExchangeVolatileMismatchCooperativeMatrix) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability VulkanMemoryModelKHR
+OpCapability Linkage
+OpCapability CooperativeMatrixNV
+OpExtension "SPV_KHR_vulkan_memory_model"
+OpExtension "SPV_NV_cooperative_matrix"
+OpMemoryModel Logical VulkanKHR
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%int_0 = OpConstant %int 0
+%int_1 = OpConstant %int 1
+%workgroup = OpConstant %int 2
+%volatile = OpSpecConstant %int 32768
+%non_volatile = OpSpecConstant %int 32768
+%ptr_wg_int = OpTypePointer Workgroup %int
+%wg_var = OpVariable %ptr_wg_int Workgroup
+%void_fn = OpTypeFunction %void
+%func = OpFunction %void None %void_fn
+%entry = OpLabel
+%cmp_ex = OpAtomicCompareExchange %int %wg_var %workgroup %volatile %non_volatile %int_0 %int_1
+OpReturn
+OpFunctionEnd
+)";
+
+  // This is ok because we cannot evaluate the spec constant defaults.
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
+TEST_F(ValidateAtomics, VolatileRequiresVulkanMemoryModel) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpMemoryModel Logical GLSL450
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%int_0 = OpConstant %int 0
+%int_1 = OpConstant %int 1
+%workgroup = OpConstant %int 2
+%volatile = OpConstant %int 32768
+%ptr_wg_int = OpTypePointer Workgroup %int
+%wg_var = OpVariable %ptr_wg_int Workgroup
+%void_fn = OpTypeFunction %void
+%func = OpFunction %void None %void_fn
+%entry = OpLabel
+%ld = OpAtomicLoad %int %wg_var %workgroup %volatile
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Memory Semantics Volatile requires capability "
+                        "VulkanMemoryModelKHR"));
+}
+
+TEST_F(ValidateAtomics, CooperativeMatrixSemanticsMustBeConstant) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Linkage
+OpCapability CooperativeMatrixNV
+OpExtension "SPV_NV_cooperative_matrix"
+OpMemoryModel Logical GLSL450
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%int_0 = OpConstant %int 0
+%int_1 = OpConstant %int 1
+%workgroup = OpConstant %int 2
+%undef = OpUndef %int
+%ptr_wg_int = OpTypePointer Workgroup %int
+%wg_var = OpVariable %ptr_wg_int Workgroup
+%void_fn = OpTypeFunction %void
+%func = OpFunction %void None %void_fn
+%entry = OpLabel
+%ld = OpAtomicLoad %int %wg_var %workgroup %undef
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Memory Semantics must be a constant instruction when "
+                        "CooperativeMatrixNV capability is present"));
+}
+
 }  // namespace
 }  // namespace val
 }  // namespace spvtools
--- a/test/val/val_barriers_test.cpp
+++ b/test/val/val_barriers_test.cpp
@ -1361,6 +1361,115 @@ OpFunctionEnd
  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_UNIVERSAL_1_3));
 }

+TEST_F(ValidateBarriers, VolatileMemoryBarrier) {
+  const std::string text = R"(
+OpCapability Shader
+OpCapability VulkanMemoryModelKHR
+OpCapability VulkanMemoryModelDeviceScopeKHR
+OpCapability Linkage
+OpExtension "SPV_KHR_vulkan_memory_model"
+OpMemoryModel Logical VulkanKHR
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%device = OpConstant %int 1
+%semantics = OpConstant %int 32768
+%functy = OpTypeFunction %void
+%func = OpFunction %void None %functy
+%1 = OpLabel
+OpMemoryBarrier %device %semantics
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(text);
+  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Memory Semantics Volatile can only be used with "
+                        "atomic instructions"));
+}
+
+TEST_F(ValidateBarriers, VolatileControlBarrier) {
+  const std::string text = R"(
+OpCapability Shader
+OpCapability VulkanMemoryModelKHR
+OpCapability VulkanMemoryModelDeviceScopeKHR
+OpCapability Linkage
+OpExtension "SPV_KHR_vulkan_memory_model"
+OpMemoryModel Logical VulkanKHR
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%device = OpConstant %int 1
+%semantics = OpConstant %int 32768
+%functy = OpTypeFunction %void
+%func = OpFunction %void None %functy
+%1 = OpLabel
+OpControlBarrier %device %device %semantics
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(text);
+  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Memory Semantics Volatile can only be used with "
+                        "atomic instructions"));
+}
+
+TEST_F(ValidateBarriers, CooperativeMatrixSpecConstantVolatile) {
+  const std::string text = R"(
+OpCapability Shader
+OpCapability VulkanMemoryModelKHR
+OpCapability VulkanMemoryModelDeviceScopeKHR
+OpCapability CooperativeMatrixNV
+OpCapability Linkage
+OpExtension "SPV_KHR_vulkan_memory_model"
+OpExtension "SPV_NV_cooperative_matrix"
+OpMemoryModel Logical VulkanKHR
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%device = OpConstant %int 1
+%semantics = OpSpecConstant %int 32768
+%functy = OpTypeFunction %void
+%func = OpFunction %void None %functy
+%1 = OpLabel
+OpControlBarrier %device %device %semantics
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(text);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
+TEST_F(ValidateBarriers, CooperativeMatrixNonConstantSemantics) {
+  const std::string text = R"(
+OpCapability Shader
+OpCapability VulkanMemoryModelKHR
+OpCapability VulkanMemoryModelDeviceScopeKHR
+OpCapability CooperativeMatrixNV
+OpCapability Linkage
+OpExtension "SPV_KHR_vulkan_memory_model"
+OpExtension "SPV_NV_cooperative_matrix"
+OpMemoryModel Logical VulkanKHR
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%device = OpConstant %int 1
+%semantics = OpUndef %int
+%functy = OpTypeFunction %void
+%func = OpFunction %void None %functy
+%1 = OpLabel
+OpControlBarrier %device %device %semantics
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(text);
+  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Memory Semantics must be a constant instruction when "
+                        "CooperativeMatrixNV capability is present"));
+}
+
 }  // namespace
 }  // namespace val
 }  // namespace spvtools