Validate NonWritable decoration (#2263)
Also permit NonWritable on members of structs used for UBO and SSBO.
(That seems inadvertently removed in recent revisions of the spec.)
diff --git a/source/val/validate_decorations.cpp b/source/val/validate_decorations.cpp
index 164a5dd..f0ad177 100644
--- a/source/val/validate_decorations.cpp
+++ b/source/val/validate_decorations.cpp
@@ -950,6 +950,14 @@
const bool bufferRules =
(uniform && bufferDeco) || (push_constant && blockDeco) ||
((storage_buffer || phys_storage_buffer) && blockDeco);
+ if (uniform && blockDeco) {
+ vstate.RegisterPointerToUniformBlock(ptrInst->id());
+ vstate.RegisterStructForUniformBlock(id);
+ }
+ if ((uniform && bufferDeco) || (storage_buffer && blockDeco)) {
+ vstate.RegisterPointerToStorageBuffer(ptrInst->id());
+ vstate.RegisterStructForStorageBuffer(id);
+ }
if (blockRules || bufferRules) {
const char* deco_str = blockDeco ? "Block" : "BufferBlock";
spv_result_t recursive_status = SPV_SUCCESS;
@@ -1219,6 +1227,49 @@
return SPV_SUCCESS;
}
+// Returns SPV_SUCCESS if validation rules are satisfied for the NonWritable
+// decoration. Otherwise emits a diagnostic and returns something other than
+// SPV_SUCCESS. The |inst| parameter is the object being decorated. This must
+// be called after TypePass and AnnotateCheckDecorationsOfBuffers are called.
+spv_result_t CheckNonWritableDecoration(ValidationState_t& vstate,
+ const Instruction& inst,
+ const Decoration& decoration) {
+ assert(inst.id() && "Parser ensures the target of the decoration has an ID");
+
+ if (decoration.struct_member_index() == Decoration::kInvalidMember) {
+ // The target must be a memory object declaration.
+ // First, it must be a variable or function parameter.
+ if (inst.opcode() != SpvOpVariable &&
+ inst.opcode() != SpvOpFunctionParameter) {
+ return vstate.diag(SPV_ERROR_INVALID_ID, &inst)
+ << "Target of NonWritable decoration must be a memory object "
+ "declaration (a variable or a function parameter)";
+ }
+ // Second, it must point to a UBO, SSBO, or storage image.
+ const auto type_id = inst.type_id();
+ if (!vstate.IsPointerToUniformBlock(type_id) &&
+ !vstate.IsPointerToStorageBuffer(type_id) &&
+ !vstate.IsPointerToStorageImage(type_id)) {
+ return vstate.diag(SPV_ERROR_INVALID_ID, &inst)
+ << "Target of NonWritable decoration is invalid: must point to a "
+ "storage image, uniform block, or storage buffer";
+ }
+ } else {
+ // The target is a struct member. The annotations pass already checks that
+ // it is a structure. So now fall through to ensure that the structure is
+ // used as a UBO or SSBO.
+ const auto type_id = inst.id();
+ if (!vstate.IsStructForUniformBlock(type_id) &&
+ !vstate.IsStructForStorageBuffer(type_id)) {
+ return vstate.diag(SPV_ERROR_INVALID_ID, &inst)
+ << "Target of NonWritable member decoration is invalid: must be "
+ "the struct type of a uniform block or storage buffer";
+ }
+ }
+
+ return SPV_SUCCESS;
+}
+
// Returns SPV_SUCCESS if validation rules are satisfied for Uniform
// decorations. Otherwise emits a diagnostic and returns something other than
// SPV_SUCCESS. Assumes each decoration on a group has been propagated down to
@@ -1314,6 +1365,9 @@
if (is_shader)
PASS_OR_BAIL(CheckFPRoundingModeForShaders(vstate, *inst));
break;
+ case SpvDecorationNonWritable:
+ PASS_OR_BAIL(CheckNonWritableDecoration(vstate, *inst, decoration));
+ break;
case SpvDecorationUniform:
PASS_OR_BAIL(CheckUniformDecoration(vstate, *inst, decoration));
break;
diff --git a/source/val/validate_type.cpp b/source/val/validate_type.cpp
index 7669f72..30fc2fb 100644
--- a/source/val/validate_type.cpp
+++ b/source/val/validate_type.cpp
@@ -269,6 +269,16 @@
<< "OpTypePointer Type <id> '" << _.getIdName(type_id)
<< "' is not a type.";
}
+ // See if this points to a storage image.
+ if (type->opcode() == SpvOpTypeImage) {
+ const auto storage_class = inst->GetOperandAs<uint32_t>(1);
+ if (storage_class == SpvStorageClassUniformConstant) {
+ const auto sampled = type->GetOperandAs<uint32_t>(6);
+ // 2 indicates this image is known to be be used without a sampler, i.e.
+ // a storage image.
+ if (sampled == 2) _.RegisterPointerToStorageImage(inst->id());
+ }
+ }
return SPV_SUCCESS;
}
diff --git a/source/val/validation_state.h b/source/val/validation_state.h
index 2c94906..676b423 100644
--- a/source/val/validation_state.h
+++ b/source/val/validation_state.h
@@ -562,6 +562,63 @@
bool GetPointerTypeInfo(uint32_t id, uint32_t* data_type,
uint32_t* storage_class) const;
+ // Is the ID the type of a pointer to a uniform block: Block-decorated struct
+ // in uniform storage class? The result is only valid after internal method
+ // CheckDecorationsOfBuffers has been called.
+ bool IsPointerToUniformBlock(uint32_t type_id) const {
+ return pointer_to_uniform_block_.find(type_id) !=
+ pointer_to_uniform_block_.cend();
+ }
+ // Save the ID of a pointer to uniform block.
+ void RegisterPointerToUniformBlock(uint32_t type_id) {
+ pointer_to_uniform_block_.insert(type_id);
+ }
+ // Is the ID the type of a struct used as a uniform block?
+ // The result is only valid after internal method CheckDecorationsOfBuffers
+ // has been called.
+ bool IsStructForUniformBlock(uint32_t type_id) const {
+ return struct_for_uniform_block_.find(type_id) !=
+ struct_for_uniform_block_.cend();
+ }
+ // Save the ID of a struct of a uniform block.
+ void RegisterStructForUniformBlock(uint32_t type_id) {
+ struct_for_uniform_block_.insert(type_id);
+ }
+ // Is the ID the type of a pointer to a storage buffer: BufferBlock-decorated
+ // struct in uniform storage class, or Block-decorated struct in StorageBuffer
+ // storage class? The result is only valid after internal method
+ // CheckDecorationsOfBuffers has been called.
+ bool IsPointerToStorageBuffer(uint32_t type_id) const {
+ return pointer_to_storage_buffer_.find(type_id) !=
+ pointer_to_storage_buffer_.cend();
+ }
+ // Save the ID of a pointer to a storage buffer.
+ void RegisterPointerToStorageBuffer(uint32_t type_id) {
+ pointer_to_storage_buffer_.insert(type_id);
+ }
+ // Is the ID the type of a struct for storage buffer?
+ // The result is only valid after internal method CheckDecorationsOfBuffers
+ // has been called.
+ bool IsStructForStorageBuffer(uint32_t type_id) const {
+ return struct_for_storage_buffer_.find(type_id) !=
+ struct_for_storage_buffer_.cend();
+ }
+ // Save the ID of a struct of a storage buffer.
+ void RegisterStructForStorageBuffer(uint32_t type_id) {
+ struct_for_storage_buffer_.insert(type_id);
+ }
+
+ // Is the ID the type of a pointer to a storage image? That is, the pointee
+ // type is an image type which is known to not use a sampler.
+ bool IsPointerToStorageImage(uint32_t type_id) const {
+ return pointer_to_storage_image_.find(type_id) !=
+ pointer_to_storage_image_.cend();
+ }
+ // Save the ID of a pointer to a storage image.
+ void RegisterPointerToStorageImage(uint32_t type_id) {
+ pointer_to_storage_image_.insert(type_id);
+ }
+
// Tries to evaluate a 32-bit signed or unsigned scalar integer constant.
// Returns tuple <is_int32, is_const_int32, value>.
// OpSpecConstant* return |is_const_int32| as false since their values cannot
@@ -701,6 +758,23 @@
std::unordered_map<uint32_t, std::vector<uint32_t>> function_to_entry_points_;
const std::vector<uint32_t> empty_ids_;
+ // The IDs of types of pointers to Block-decorated structs in Uniform storage
+ // class. This is populated at the start of ValidateDecorations.
+ std::unordered_set<uint32_t> pointer_to_uniform_block_;
+ // The IDs of struct types for uniform blocks.
+ // This is populated at the start of ValidateDecorations.
+ std::unordered_set<uint32_t> struct_for_uniform_block_;
+ // The IDs of types of pointers to BufferBlock-decorated structs in Uniform
+ // storage class, or Block-decorated structs in StorageBuffer storage class.
+ // This is populated at the start of ValidateDecorations.
+ std::unordered_set<uint32_t> pointer_to_storage_buffer_;
+ // The IDs of struct types for storage buffers.
+ // This is populated at the start of ValidateDecorations.
+ std::unordered_set<uint32_t> struct_for_storage_buffer_;
+ // The IDs of types of pointers to storage images. This is populated in the
+ // TypePass.
+ std::unordered_set<uint32_t> pointer_to_storage_image_;
+
/// Maps ids to friendly names.
std::unique_ptr<spvtools::FriendlyNameMapper> friendly_mapper_;
spvtools::NameMapper name_mapper_;
diff --git a/test/val/val_capability_test.cpp b/test/val/val_capability_test.cpp
index 488e957..37da4ea 100644
--- a/test/val/val_capability_test.cpp
+++ b/test/val/val_capability_test.cpp
@@ -1129,9 +1129,14 @@
"%intt = OpTypeInt 32 0\n" + std::string(kVoidFVoid),
AllCapabilities()),
std::make_pair(std::string(kOpenCLMemoryModel) +
+ // NonWritable must target something valid, such as a storage image.
"OpEntryPoint Kernel %func \"compute\" \n"
- "OpDecorate %intt NonWritable\n"
- "%intt = OpTypeInt 32 0\n" + std::string(kVoidFVoid),
+ "OpDecorate %var NonWritable "
+ "%float = OpTypeFloat 32 "
+ "%imstor = OpTypeImage %float 2D 0 0 0 2 Unknown "
+ "%ptr = OpTypePointer UniformConstant %imstor "
+ "%var = OpVariable %ptr UniformConstant "
+ + std::string(kVoidFVoid),
AllCapabilities()),
std::make_pair(std::string(kOpenCLMemoryModel) +
"OpEntryPoint Kernel %func \"compute\" \n"
diff --git a/test/val/val_decoration_test.cpp b/test/val/val_decoration_test.cpp
index 1506a29..8f32ee0 100644
--- a/test/val/val_decoration_test.cpp
+++ b/test/val/val_decoration_test.cpp
@@ -115,7 +115,7 @@
OpCapability Linkage
OpMemoryModel Logical GLSL450
OpDecorate %1 DescriptorSet 0
- OpDecorate %1 NonWritable
+ OpDecorate %1 RelaxedPrecision
OpDecorate %1 Restrict
%1 = OpDecorationGroup
OpGroupDecorate %1 %2 %3
@@ -136,9 +136,10 @@
EXPECT_EQ(SPV_SUCCESS, ValidateAndRetrieveValidationState());
// Decoration group has 3 decorations.
- auto expected_decorations = std::vector<Decoration>{
- Decoration(SpvDecorationDescriptorSet, {0}),
- Decoration(SpvDecorationNonWritable), Decoration(SpvDecorationRestrict)};
+ auto expected_decorations =
+ std::vector<Decoration>{Decoration(SpvDecorationDescriptorSet, {0}),
+ Decoration(SpvDecorationRelaxedPrecision),
+ Decoration(SpvDecorationRestrict)};
// Decoration group is applied to id 1, 2, 3, and 4. Note that id 1 (which is
// the decoration group id) also has all the decorations.
@@ -5491,6 +5492,255 @@
"improperly straddling vector at offset 28"));
}
+// NonWritable
+
+// Returns a SPIR-V shader module with variables in various storage classes,
+// parameterizable by which ID should be decorated as NonWritable.
+std::string ShaderWithNonWritableTarget(const std::string& target,
+ bool member_decorate = false) {
+ const std::string decoration_inst =
+ std::string(member_decorate ? "OpMemberDecorate " : "OpDecorate ") +
+ target + (member_decorate ? " 0" : "");
+
+ return std::string(R"(
+ OpCapability Shader
+ OpExtension "SPV_KHR_storage_buffer_storage_class"
+ OpMemoryModel Logical GLSL450
+ OpEntryPoint Vertex %main "main"
+ OpName %label "label"
+ OpName %param_f "param_f"
+ OpName %param_p "param_p"
+ OpName %_ptr_imstor "_ptr_imstor"
+ OpName %_ptr_imsam "_ptr_imsam"
+ OpName %var_wg "var_wg"
+ OpName %var_imsam "var_imsam"
+ OpName %var_priv "var_priv"
+ OpName %var_func "var_func"
+ OpName %struct_bad "struct_bad"
+
+ OpDecorate %struct_b Block
+ OpDecorate %struct_bb BufferBlock
+ OpDecorate %struct_b_rtarr Block
+ OpMemberDecorate %struct_b 0 Offset 0
+ OpMemberDecorate %struct_bb 0 Offset 0
+ OpMemberDecorate %struct_b_rtarr 0 Offset 0
+ OpDecorate %rtarr ArrayStride 4
+)") + decoration_inst +
+
+ R"( NonWritable
+
+ %void = OpTypeVoid
+ %void_fn = OpTypeFunction %void
+ %float = OpTypeFloat 32
+ %float_0 = OpConstant %float 0
+ %struct_b = OpTypeStruct %float
+ %struct_bb = OpTypeStruct %float
+ %rtarr = OpTypeRuntimeArray %float
+%struct_b_rtarr = OpTypeStruct %rtarr
+%struct_bad = OpTypeStruct %float
+ ; storage image
+ %imstor = OpTypeImage %float 2D 0 0 0 2 R32f
+ ; sampled image
+ %imsam = OpTypeImage %float 2D 0 0 0 1 R32f
+
+%_ptr_Uniform_stb = OpTypePointer Uniform %struct_b
+%_ptr_Uniform_stbb = OpTypePointer Uniform %struct_bb
+%_ptr_StorageBuffer_stb = OpTypePointer StorageBuffer %struct_b
+%_ptr_StorageBuffer_stb_rtarr = OpTypePointer StorageBuffer %struct_b_rtarr
+%_ptr_Workgroup = OpTypePointer Workgroup %float
+%_ptr_Private = OpTypePointer Private %float
+%_ptr_Function = OpTypePointer Function %float
+%_ptr_imstor = OpTypePointer UniformConstant %imstor
+%_ptr_imsam = OpTypePointer UniformConstant %imsam
+
+%extra_fn = OpTypeFunction %void %float %_ptr_Private %_ptr_imstor
+
+%var_ubo = OpVariable %_ptr_Uniform_stb Uniform
+%var_ssbo_u = OpVariable %_ptr_Uniform_stbb Uniform
+%var_ssbo_sb = OpVariable %_ptr_StorageBuffer_stb StorageBuffer
+%var_ssbo_sb_rtarr = OpVariable %_ptr_StorageBuffer_stb_rtarr StorageBuffer
+%var_wg = OpVariable %_ptr_Workgroup Workgroup
+%var_priv = OpVariable %_ptr_Private Private
+%var_imstor = OpVariable %_ptr_imstor UniformConstant
+%var_imsam = OpVariable %_ptr_imsam UniformConstant
+
+ %helper = OpFunction %void None %extra_fn
+ %param_f = OpFunctionParameter %float
+ %param_p = OpFunctionParameter %_ptr_Private
+ %param_pimstor = OpFunctionParameter %_ptr_imstor
+%helper_label = OpLabel
+%helper_func_var = OpVariable %_ptr_Function Function
+ OpReturn
+ OpFunctionEnd
+
+ %main = OpFunction %void None %void_fn
+ %label = OpLabel
+%var_func = OpVariable %_ptr_Function Function
+ OpReturn
+ OpFunctionEnd
+)";
+}
+
+TEST_F(ValidateDecorations, NonWritableLabelTargetBad) {
+ std::string spirv = ShaderWithNonWritableTarget("%label");
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("Target of NonWritable decoration must be a "
+ "memory object declaration (a variable or a function "
+ "parameter)\n %label = OpLabel"));
+}
+
+TEST_F(ValidateDecorations, NonWritableTypeTargetBad) {
+ std::string spirv = ShaderWithNonWritableTarget("%void");
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("Target of NonWritable decoration must be a "
+ "memory object declaration (a variable or a function "
+ "parameter)\n %void = OpTypeVoid"));
+}
+
+TEST_F(ValidateDecorations, NonWritableValueTargetBad) {
+ std::string spirv = ShaderWithNonWritableTarget("%float_0");
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("Target of NonWritable decoration must be a "
+ "memory object declaration (a variable or a function "
+ "parameter)\n %float_0 = OpConstant %float 0"));
+}
+
+TEST_F(ValidateDecorations, NonWritableValueParamBad) {
+ std::string spirv = ShaderWithNonWritableTarget("%param_f");
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("Target of NonWritable decoration is invalid: must "
+ "point to a storage image, uniform block, or storage "
+ "buffer\n %param_f = OpFunctionParameter %float"));
+}
+
+TEST_F(ValidateDecorations, NonWritablePointerParamButWrongTypeBad) {
+ std::string spirv = ShaderWithNonWritableTarget("%param_p");
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr(
+ "Target of NonWritable decoration is invalid: must "
+ "point to a storage image, uniform block, or storage "
+ "buffer\n %param_p = OpFunctionParameter %_ptr_Private_float"));
+}
+
+TEST_F(ValidateDecorations, NonWritablePointerParamStorageImageGood) {
+ std::string spirv = ShaderWithNonWritableTarget("%param_pimstor");
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(), Eq(""));
+}
+
+TEST_F(ValidateDecorations, NonWritableVarStorageImageGood) {
+ std::string spirv = ShaderWithNonWritableTarget("%var_imstor");
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(), Eq(""));
+}
+
+TEST_F(ValidateDecorations, NonWritableVarSampledImageBad) {
+ std::string spirv = ShaderWithNonWritableTarget("%var_imsam");
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("Target of NonWritable decoration is invalid: must "
+ "point to a storage image, uniform block, or storage "
+ "buffer\n %var_imsam"));
+}
+
+TEST_F(ValidateDecorations, NonWritableVarUboGood) {
+ std::string spirv = ShaderWithNonWritableTarget("%var_ubo");
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(), Eq(""));
+}
+
+TEST_F(ValidateDecorations, NonWritableVarSsboInUniformGood) {
+ std::string spirv = ShaderWithNonWritableTarget("%var_ssbo_u");
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(), Eq(""));
+}
+
+TEST_F(ValidateDecorations, NonWritableVarSsboInStorageBufferGood) {
+ std::string spirv = ShaderWithNonWritableTarget("%var_ssbo_sb");
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(), Eq(""));
+}
+
+TEST_F(ValidateDecorations, NonWritableMemberOfSsboInStorageBufferGood) {
+ std::string spirv = ShaderWithNonWritableTarget("%struct_b_rtarr", true);
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(), Eq(""));
+}
+
+TEST_F(ValidateDecorations, NonWritableMemberOfNonBlockStructBad) {
+ std::string spirv = ShaderWithNonWritableTarget("%struct_bad", true);
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr("Target of NonWritable member decoration is invalid: must be "
+ "the struct type of a uniform block or storage buffer"));
+}
+
+TEST_F(ValidateDecorations, NonWritableVarWorkgroupBad) {
+ std::string spirv = ShaderWithNonWritableTarget("%var_wg");
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("Target of NonWritable decoration is invalid: must "
+ "point to a storage image, uniform block, or storage "
+ "buffer\n %var_wg"));
+}
+
+TEST_F(ValidateDecorations, NonWritableVarPrivateBad) {
+ std::string spirv = ShaderWithNonWritableTarget("%var_priv");
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("Target of NonWritable decoration is invalid: must "
+ "point to a storage image, uniform block, or storage "
+ "buffer\n %var_priv"));
+}
+
+TEST_F(ValidateDecorations, NonWritableVarFunctionBad) {
+ std::string spirv = ShaderWithNonWritableTarget("%var_func");
+
+ CompileSuccessfully(spirv);
+ EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("Target of NonWritable decoration is invalid: must "
+ "point to a storage image, uniform block, or storage "
+ "buffer\n %var_func"));
+}
+
} // namespace
} // namespace val
} // namespace spvtools
