Validate storage class OpenCL environment rules for atomics (#2750)

This change refactors all storage class validation for atomics
to reflect the similar refactoring in the specification.

It is currently not possible to write a test for the check
rejecting Generic in an OpenCL 1.2 environment as the required
GenericPointer capability isn't allowed there. I've decided
to keep the check nonetheless to guard against the capability
becoming available without the rules for atomics being updated.

The ID changes in existing tests aren't ideal but introducing
names drags in a substantial refactoring of this file.

Contributes to #2595.

Signed-off-by: Kevin Petit <kevin.petit@arm.com>

source/val/validate_atomics.cpp

@@ -25,6 +25,28 @@
 #include "source/val/validate_scopes.h"
 #include "source/val/validation_state.h"
 
+namespace {
+
+bool IsStorageClassAllowedByUniversalRules(uint32_t storage_class) {
+  switch (storage_class) {
+    case SpvStorageClassUniform:
+    case SpvStorageClassStorageBuffer:
+    case SpvStorageClassWorkgroup:
+    case SpvStorageClassCrossWorkgroup:
+    case SpvStorageClassGeneric:
+    case SpvStorageClassAtomicCounter:
+    case SpvStorageClassImage:
+    case SpvStorageClassFunction:
+    case SpvStorageClassPhysicalStorageBufferEXT:
+      return true;
+      break;
+    default:
+      return false;
+  }
+}
+
+}  // namespace
+
 namespace spvtools {
 namespace val {
 
@@ -119,32 +141,42 @@ spv_result_t AtomicsPass(ValidationState_t& _, const Instruction* inst) {
                << ": expected Pointer to be of type OpTypePointer";
       }
 
-      switch (storage_class) {
+      // Validate storage class against universal rules
-        case SpvStorageClassUniform:
+      if (!IsStorageClassAllowedByUniversalRules(storage_class)) {
-        case SpvStorageClassWorkgroup:
+        return _.diag(SPV_ERROR_INVALID_DATA, inst)
-        case SpvStorageClassCrossWorkgroup:
+               << spvOpcodeString(opcode)
-        case SpvStorageClassGeneric:
+               << ": storage class forbidden by universal validation rules.";
-        case SpvStorageClassAtomicCounter:
+      }
-        case SpvStorageClassImage:
+
-        case SpvStorageClassStorageBuffer:
+      // Then Shader rules
-        case SpvStorageClassPhysicalStorageBufferEXT:
+      if (_.HasCapability(SpvCapabilityShader)) {
-          break;
+        if (storage_class == SpvStorageClassFunction) {
-        default:
+          return _.diag(SPV_ERROR_INVALID_DATA, inst)
-          if (spvIsOpenCLEnv(_.context()->target_env)) {
+                 << spvOpcodeString(opcode)
-            if (storage_class != SpvStorageClassFunction) {
+                 << ": Function storage class forbidden when the Shader "
-              return _.diag(SPV_ERROR_INVALID_DATA, inst)
+                    "capability is declared.";
-                     << spvOpcodeString(opcode)
+        }
-                     << ": expected Pointer Storage Class to be Uniform, "
+      }
-                        "Workgroup, CrossWorkgroup, Generic, AtomicCounter, "
+
-                        "Image, StorageBuffer or Function";
+      // And finally OpenCL environment rules
-            }
+      if (spvIsOpenCLEnv(_.context()->target_env)) {
-          } else {
+        if ((storage_class != SpvStorageClassFunction) &&
+            (storage_class != SpvStorageClassWorkgroup) &&
+            (storage_class != SpvStorageClassCrossWorkgroup) &&
+            (storage_class != SpvStorageClassGeneric)) {
+          return _.diag(SPV_ERROR_INVALID_DATA, inst)
+                 << spvOpcodeString(opcode)
+                 << ": storage class must be Function, Workgroup, "
+                    "CrossWorkGroup or Generic in the OpenCL environment.";
+        }
+
+        if (_.context()->target_env == SPV_ENV_OPENCL_1_2) {
+          if (storage_class == SpvStorageClassGeneric) {
             return _.diag(SPV_ERROR_INVALID_DATA, inst)
-                   << spvOpcodeString(opcode)
+                   << "Storage class cannot be Generic in OpenCL 1.2 "
-                   << ": expected Pointer Storage Class to be Uniform, "
+                      "environment";
-                      "Workgroup, CrossWorkgroup, Generic, AtomicCounter, "
-                      "Image or StorageBuffer";
           }
+        }
       }
 
       if (opcode == SpvOpAtomicFlagTestAndSet ||

test/val/val_atomics_test.cpp

@@ -190,6 +190,9 @@ OpMemoryModel Physical32 OpenCL
 %f32_ptr_uniformconstant = OpTypePointer UniformConstant %f32
 %f32_uc_var = OpVariable %f32_ptr_uniformconstant UniformConstant
 
+%f32_ptr_image = OpTypePointer Image %f32
+%f32_im_var = OpVariable %f32_ptr_image Image
+
 %main = OpFunction %void None %func
 %main_entry = OpLabel
 )";
@@ -288,11 +291,9 @@ OpAtomicStore %f32_var_function %device %relaxed %f32_1
 
   CompileSuccessfully(GenerateShaderCode(body), SPV_ENV_VULKAN_1_0);
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
-  EXPECT_THAT(
+  EXPECT_THAT(getDiagnosticString(),
-      getDiagnosticString(),
+              HasSubstr("AtomicStore: Function storage class forbidden when "
-      HasSubstr("AtomicStore: expected Pointer Storage Class to be Uniform, "
+                        "the Shader capability is declared."));
-                "Workgroup, CrossWorkgroup, Generic, AtomicCounter, Image or "
-                "StorageBuffer"));
 }
 
 // TODO(atgoo@github.com): the corresponding check fails Vulkan CTS,
@@ -500,8 +501,7 @@ TEST_F(ValidateAtomics, AtomicLoadWrongScopeType) {
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
   EXPECT_THAT(
       getDiagnosticString(),
-      HasSubstr("AtomicLoad: expected Memory Scope to be a 32-bit int\n  %40 = "
+      HasSubstr("AtomicLoad: expected Memory Scope to be a 32-bit int"));
-                "OpAtomicLoad %float %28 %float_1 %uint_0_1\n"));
 }
 
 TEST_F(ValidateAtomics, AtomicLoadWrongMemorySemanticsType) {
@@ -641,6 +641,19 @@ OpAtomicStore %f32vec4_var %device %relaxed %f32_1
                 "type"));
 }
 
+TEST_F(ValidateAtomics, AtomicStoreWrongPointerStorageTypeForOpenCL) {
+  const std::string body = R"(
+OpAtomicStore %f32_im_var %device %relaxed %f32_1
+)";
+
+  CompileSuccessfully(GenerateKernelCode(body));
+  ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_OPENCL_1_2));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("AtomicStore: storage class must be Function, Workgroup, "
+                "CrossWorkGroup or Generic in the OpenCL environment."));
+}
+
 TEST_F(ValidateAtomics, AtomicStoreWrongPointerStorageType) {
   const std::string body = R"(
 OpAtomicStore %f32_uc_var %device %relaxed %f32_1
@@ -648,11 +661,9 @@ OpAtomicStore %f32_uc_var %device %relaxed %f32_1
 
   CompileSuccessfully(GenerateKernelCode(body));
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
-  EXPECT_THAT(
+  EXPECT_THAT(getDiagnosticString(),
-      getDiagnosticString(),
+              HasSubstr("AtomicStore: storage class forbidden by universal "
-      HasSubstr("AtomicStore: expected Pointer Storage Class to be Uniform, "
+                        "validation rules."));
-                "Workgroup, CrossWorkgroup, Generic, AtomicCounter, Image or "
-                "StorageBuffer"));
 }
 
 TEST_F(ValidateAtomics, AtomicStoreWrongScopeType) {
@@ -778,9 +789,7 @@ OpAtomicStore %f32_var %device %relaxed %f32_1
   ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
   EXPECT_THAT(
       getDiagnosticString(),
-      HasSubstr(
+      HasSubstr("AtomicExchange: expected Memory Scope to be a 32-bit int"));
-          "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) {
@@ -895,8 +904,7 @@ OpAtomicStore %f32_var %device %relaxed %f32_1
   EXPECT_THAT(
       getDiagnosticString(),
       HasSubstr("AtomicCompareExchange: expected Memory Scope to be a 32-bit "
-                "int\n  %40 = OpAtomicCompareExchange %float %28 %float_1 "
+                "int"));
-                "%uint_0_1 %uint_0_1 %float_0 %float_0\n"));
 }
 
 TEST_F(ValidateAtomics, AtomicCompareExchangeWrongMemorySemanticsType1) {
@@ -1077,8 +1085,7 @@ TEST_F(ValidateAtomics, AtomicFlagTestAndSetWrongScopeType) {
   EXPECT_THAT(
       getDiagnosticString(),
       HasSubstr(
-          "AtomicFlagTestAndSet: expected Memory Scope to be a 32-bit int\n  "
+          "AtomicFlagTestAndSet: expected Memory Scope to be a 32-bit int"));
-          "%40 = OpAtomicFlagTestAndSet %bool %30 %ulong_1 %uint_0_1\n"));
 }
 
 TEST_F(ValidateAtomics, AtomicFlagTestAndSetWrongMemorySemanticsType) {
@@ -1179,8 +1186,7 @@ OpAtomicStore %u32_var %device %relaxed %u32_1
   EXPECT_THAT(getDiagnosticString(),
               HasSubstr("AtomicIIncrement: Memory Semantics can have at most "
                         "one of the following bits set: Acquire, Release, "
-                        "AcquireRelease or SequentiallyConsistent\n  %40 = "
+                        "AcquireRelease or SequentiallyConsistent"));
-                        "OpAtomicIIncrement %uint %30 %uint_1_0 %uint_6\n"));
 }
 
 TEST_F(ValidateAtomics, AtomicUniformMemorySemanticsShader) {