Validate PushConstants annotation and type (#2140)

* Validate PushConstants have Block annotation and are struct or array of structs
* Add passing test and split into universal/vulkan environment tests
This commit is contained in:
Alejandro Lopez 2018-11-30 13:12:05 -05:00 committed by alan-baker
parent 625db3890d
commit b8e2a9f258
4 changed files with 166 additions and 6 deletions

View File

@ -795,6 +795,19 @@ spv_result_t CheckDecorationsOfBuffers(ValidationState_t& vstate) {
MemberConstraints constraints;
ComputeMemberConstraintsForStruct(&constraints, id, LayoutConstraints(),
vstate);
// Vulkan 14.5.1: Check Block annotation for PushConstant variables.
if (spvIsVulkanEnv(vstate.context()->target_env)) {
if (push_constant && !hasDecoration(id, SpvDecorationBlock, vstate)) {
return vstate.diag(SPV_ERROR_INVALID_ID, vstate.FindDef(id))
<< "PushConstant id '" << id
<< "' is missing Block decoration.\n"
<< "From Vulkan spec, section 14.5.1:\n"
<< "Such variables must be identified with a Block "
"decoration";
}
}
// Prepare for messages
const char* sc_str =
uniform ? "Uniform"

View File

@ -73,9 +73,9 @@ bool AreLayoutCompatibleStructs(ValidationState_t& _, const Instruction* type1,
bool HaveLayoutCompatibleMembers(ValidationState_t& _, const Instruction* type1,
const Instruction* type2) {
assert(type1->opcode() == SpvOpTypeStruct &&
"type1 must be and OpTypeStruct instruction.");
"type1 must be an OpTypeStruct instruction.");
assert(type2->opcode() == SpvOpTypeStruct &&
"type2 must be and OpTypeStruct instruction.");
"type2 must be an OpTypeStruct instruction.");
const auto& type1_operands = type1->operands();
const auto& type2_operands = type2->operands();
if (type1_operands.size() != type2_operands.size()) {
@ -100,9 +100,9 @@ bool HaveLayoutCompatibleMembers(ValidationState_t& _, const Instruction* type1,
bool HaveSameLayoutDecorations(ValidationState_t& _, const Instruction* type1,
const Instruction* type2) {
assert(type1->opcode() == SpvOpTypeStruct &&
"type1 must be and OpTypeStruct instruction.");
"type1 must be an OpTypeStruct instruction.");
assert(type2->opcode() == SpvOpTypeStruct &&
"type2 must be and OpTypeStruct instruction.");
"type2 must be an OpTypeStruct instruction.");
const std::vector<Decoration>& type1_decorations =
_.id_decorations(type1->id());
const std::vector<Decoration>& type2_decorations =
@ -373,8 +373,20 @@ spv_result_t ValidateVariable(ValidationState_t& _, const Instruction* inst) {
}
}
// Vulkan 14.5.1: Check type of PushConstant variables.
// Vulkan 14.5.2: Check type of UniformConstant and Uniform variables.
if (spvIsVulkanEnv(_.context()->target_env)) {
if (storage_class == SpvStorageClassPushConstant) {
if (!IsAllowedTypeOrArrayOfSame(_, pointee, {SpvOpTypeStruct})) {
return _.diag(SPV_ERROR_INVALID_ID, inst)
<< "PushConstant OpVariable <id> '" << _.getIdName(inst->id())
<< "' has illegal type.\n"
<< "From Vulkan spec, section 14.5.1:\n"
<< "Such variables must be typed as OpTypeStruct, "
<< "or an array of this type";
}
}
if (storage_class == SpvStorageClassUniformConstant) {
if (!IsAllowedTypeOrArrayOfSame(
_, pointee,
@ -382,7 +394,7 @@ spv_result_t ValidateVariable(ValidationState_t& _, const Instruction* inst) {
SpvOpTypeAccelerationStructureNV})) {
return _.diag(SPV_ERROR_INVALID_ID, inst)
<< "UniformConstant OpVariable <id> '" << _.getIdName(inst->id())
<< " 'has illegal type.\n"
<< "' has illegal type.\n"
<< "From Vulkan spec, section 14.5.2:\n"
<< "Variables identified with the UniformConstant storage class "
<< "are used only as handles to refer to opaque resources. Such "
@ -396,7 +408,7 @@ spv_result_t ValidateVariable(ValidationState_t& _, const Instruction* inst) {
if (!IsAllowedTypeOrArrayOfSame(_, pointee, {SpvOpTypeStruct})) {
return _.diag(SPV_ERROR_INVALID_ID, inst)
<< "Uniform OpVariable <id> '" << _.getIdName(inst->id())
<< " 'has illegal type.\n"
<< "' has illegal type.\n"
<< "From Vulkan spec, section 14.5.2:\n"
<< "Variables identified with the Uniform storage class are "
"used "

View File

@ -2219,6 +2219,65 @@ TEST_F(ValidateDecorations,
"rules: member 1 at offset 4 is not aligned to 16"));
}
TEST_F(ValidateDecorations, PushConstantMissingBlockGood) {
std::string spirv = R"(
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %1 "main"
OpExecutionMode %1 OriginUpperLeft
OpMemberDecorate %struct 0 Offset 0
%void = OpTypeVoid
%voidfn = OpTypeFunction %void
%float = OpTypeFloat 32
%struct = OpTypeStruct %float
%ptr = OpTypePointer PushConstant %struct
%pc = OpVariable %ptr PushConstant
%1 = OpFunction %void None %voidfn
%label = OpLabel
OpReturn
OpFunctionEnd
)";
CompileSuccessfully(spirv);
EXPECT_EQ(SPV_SUCCESS, ValidateAndRetrieveValidationState())
<< getDiagnosticString();
}
TEST_F(ValidateDecorations, VulkanPushConstantMissingBlockBad) {
std::string spirv = R"(
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %1 "main"
OpExecutionMode %1 OriginUpperLeft
OpMemberDecorate %struct 0 Offset 0
%void = OpTypeVoid
%voidfn = OpTypeFunction %void
%float = OpTypeFloat 32
%struct = OpTypeStruct %float
%ptr = OpTypePointer PushConstant %struct
%pc = OpVariable %ptr PushConstant
%1 = OpFunction %void None %voidfn
%label = OpLabel
OpReturn
OpFunctionEnd
)";
CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_1);
EXPECT_EQ(SPV_ERROR_INVALID_ID,
ValidateAndRetrieveValidationState(SPV_ENV_VULKAN_1_1));
EXPECT_THAT(getDiagnosticString(),
HasSubstr("PushConstant id '2' is missing Block decoration.\n"
"From Vulkan spec, section 14.5.1:\n"
"Such variables must be identified with a Block "
"decoration"));
}
TEST_F(ValidateDecorations, StorageBufferStorageClassArrayBaseAlignmentGood) {
// Spot check buffer rules when using StorageBuffer storage class with Block
// decoration.

View File

@ -838,6 +838,82 @@ TEST_F(ValidateMemory, ArrayLenIndexNotPointerToStruct) {
"The Struture's type in OpArrayLength <id> '12' must be a pointer to "
"an OpTypeStruct.\n %12 = OpArrayLength %uint %11 0\n"));
}
TEST_F(ValidateMemory, PushConstantNotStructGood) {
std::string spirv = R"(
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %1 "main"
OpExecutionMode %1 OriginUpperLeft
%void = OpTypeVoid
%voidfn = OpTypeFunction %void
%float = OpTypeFloat 32
%ptr = OpTypePointer PushConstant %float
%pc = OpVariable %ptr PushConstant
%1 = OpFunction %void None %voidfn
%label = OpLabel
OpReturn
OpFunctionEnd
)";
CompileSuccessfully(spirv);
EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
}
TEST_F(ValidateMemory, VulkanPushConstantNotStructBad) {
std::string spirv = R"(
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %1 "main"
OpExecutionMode %1 OriginUpperLeft
%void = OpTypeVoid
%voidfn = OpTypeFunction %void
%float = OpTypeFloat 32
%ptr = OpTypePointer PushConstant %float
%pc = OpVariable %ptr PushConstant
%1 = OpFunction %void None %voidfn
%label = OpLabel
OpReturn
OpFunctionEnd
)";
CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_1);
EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_1));
EXPECT_THAT(getDiagnosticString(),
HasSubstr("PushConstant OpVariable <id> '6' has illegal type.\n"
"From Vulkan spec, section 14.5.1:\n"
"Such variables must be typed as OpTypeStruct, "
"or an array of this type"));
}
TEST_F(ValidateMemory, VulkanPushConstant) {
std::string spirv = R"(
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %1 "main"
OpExecutionMode %1 OriginUpperLeft
OpDecorate %struct Block
OpMemberDecorate %struct 0 Offset 0
%void = OpTypeVoid
%voidfn = OpTypeFunction %void
%float = OpTypeFloat 32
%struct = OpTypeStruct %float
%ptr = OpTypePointer PushConstant %struct
%pc = OpVariable %ptr PushConstant
%1 = OpFunction %void None %voidfn
%label = OpLabel
OpReturn
OpFunctionEnd
)";
CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_1);
EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_1));
}
} // namespace
} // namespace val
} // namespace spvtools