From 11516c0b9ae2972713f3461035f930c77a4d4d01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Petit?= Date: Mon, 22 Jul 2019 13:38:42 +0100 Subject: [PATCH] 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 --- source/val/validate_atomics.cpp | 80 +++++++++++++++++++++++---------- test/val/val_atomics_test.cpp | 48 +++++++++++--------- 2 files changed, 83 insertions(+), 45 deletions(-) diff --git a/source/val/validate_atomics.cpp b/source/val/validate_atomics.cpp index e9e496056..b8867ddcc 100644 --- a/source/val/validate_atomics.cpp +++ b/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) { - case SpvStorageClassUniform: - case SpvStorageClassWorkgroup: - case SpvStorageClassCrossWorkgroup: - case SpvStorageClassGeneric: - case SpvStorageClassAtomicCounter: - case SpvStorageClassImage: - case SpvStorageClassStorageBuffer: - case SpvStorageClassPhysicalStorageBufferEXT: - break; - default: - if (spvIsOpenCLEnv(_.context()->target_env)) { - if (storage_class != SpvStorageClassFunction) { - return _.diag(SPV_ERROR_INVALID_DATA, inst) - << spvOpcodeString(opcode) - << ": expected Pointer Storage Class to be Uniform, " - "Workgroup, CrossWorkgroup, Generic, AtomicCounter, " - "Image, StorageBuffer or Function"; - } - } else { + // Validate storage class against universal rules + if (!IsStorageClassAllowedByUniversalRules(storage_class)) { + return _.diag(SPV_ERROR_INVALID_DATA, inst) + << spvOpcodeString(opcode) + << ": storage class forbidden by universal validation rules."; + } + + // Then Shader rules + if (_.HasCapability(SpvCapabilityShader)) { + if (storage_class == SpvStorageClassFunction) { + return _.diag(SPV_ERROR_INVALID_DATA, inst) + << spvOpcodeString(opcode) + << ": Function storage class forbidden when the Shader " + "capability is declared."; + } + } + + // And finally OpenCL environment rules + if (spvIsOpenCLEnv(_.context()->target_env)) { + 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) - << ": expected Pointer Storage Class to be Uniform, " - "Workgroup, CrossWorkgroup, Generic, AtomicCounter, " - "Image or StorageBuffer"; + << "Storage class cannot be Generic in OpenCL 1.2 " + "environment"; } + } } if (opcode == SpvOpAtomicFlagTestAndSet || diff --git a/test/val/val_atomics_test.cpp b/test/val/val_atomics_test.cpp index a0056141e..15887eb36 100644 --- a/test/val/val_atomics_test.cpp +++ b/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( - getDiagnosticString(), - HasSubstr("AtomicStore: expected Pointer Storage Class to be Uniform, " - "Workgroup, CrossWorkgroup, Generic, AtomicCounter, Image or " - "StorageBuffer")); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("AtomicStore: Function storage class forbidden when " + "the Shader capability is declared.")); } // 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 = " - "OpAtomicLoad %float %28 %float_1 %uint_0_1\n")); + HasSubstr("AtomicLoad: expected Memory Scope to be a 32-bit int")); } 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( - getDiagnosticString(), - HasSubstr("AtomicStore: expected Pointer Storage Class to be Uniform, " - "Workgroup, CrossWorkgroup, Generic, AtomicCounter, Image or " - "StorageBuffer")); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("AtomicStore: storage class forbidden by universal " + "validation rules.")); } 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( - "AtomicExchange: expected Memory Scope to be a 32-bit int\n %40 = " - "OpAtomicExchange %float %28 %float_1 %uint_0_1 %float_0\n")); + HasSubstr("AtomicExchange: expected Memory Scope to be a 32-bit int")); } 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 " - "%uint_0_1 %uint_0_1 %float_0 %float_0\n")); + "int")); } 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 " - "%40 = OpAtomicFlagTestAndSet %bool %30 %ulong_1 %uint_0_1\n")); + "AtomicFlagTestAndSet: expected Memory Scope to be a 32-bit int")); } 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 = " - "OpAtomicIIncrement %uint %30 %uint_1_0 %uint_6\n")); + "AcquireRelease or SequentiallyConsistent")); } TEST_F(ValidateAtomics, AtomicUniformMemorySemanticsShader) {