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>
diff --git a/source/val/validate_atomics.cpp b/source/val/validate_atomics.cpp
index e9e4960..b8867dd 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 @@
<< ": 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 a005614..15887eb 100644
--- a/test/val/val_atomics_test.cpp
+++ b/test/val/val_atomics_test.cpp
@@ -190,6 +190,9 @@
%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 @@
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 @@
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 @@
"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 @@
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 @@
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 @@
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 @@
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 @@
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) {
