Refactor the InstructionPass (#2924)

* move checks to more appropriate locations
  * remove some duplicated checks
* New function to check valid storage classes
* updated tests
This commit is contained in:
alan-baker 2019-09-27 00:06:36 -04:00 committed by GitHub
parent 84b1976061
commit 10951a7c9a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 152 additions and 157 deletions

View File

@ -55,18 +55,6 @@ std::string ToString(const CapabilitySet& capabilities,
return ss.str();
}
bool IsValidWebGPUStorageClass(SpvStorageClass storage_class) {
return storage_class == SpvStorageClassUniformConstant ||
storage_class == SpvStorageClassUniform ||
storage_class == SpvStorageClassStorageBuffer ||
storage_class == SpvStorageClassInput ||
storage_class == SpvStorageClassOutput ||
storage_class == SpvStorageClassImage ||
storage_class == SpvStorageClassWorkgroup ||
storage_class == SpvStorageClassPrivate ||
storage_class == SpvStorageClassFunction;
}
// Returns capabilities that enable an opcode. An empty result is interpreted
// as no prohibition of use of the opcode. If the result is non-empty, then
// the opcode may only be used if at least one of the capabilities is specified
@ -249,23 +237,6 @@ spv_result_t ReservedCheck(ValidationState_t& _, const Instruction* inst) {
return SPV_SUCCESS;
}
// Returns SPV_ERROR_INVALID_BINARY and emits a diagnostic if the instruction
// is invalid because of an execution environment constraint.
spv_result_t EnvironmentCheck(ValidationState_t& _, const Instruction* inst) {
const SpvOp opcode = inst->opcode();
switch (opcode) {
case SpvOpUndef:
if (_.features().bans_op_undef) {
return _.diag(SPV_ERROR_INVALID_BINARY, inst)
<< "OpUndef is disallowed";
}
break;
default:
break;
}
return SPV_SUCCESS;
}
// Returns SPV_ERROR_INVALID_CAPABILITY and emits a diagnostic if the
// instruction is invalid because the required capability isn't declared
// in the module.
@ -499,38 +470,6 @@ spv_result_t InstructionPass(ValidationState_t& _, const Instruction* inst) {
}
_.set_addressing_model(inst->GetOperandAs<SpvAddressingModel>(0));
_.set_memory_model(inst->GetOperandAs<SpvMemoryModel>(1));
if (_.memory_model() != SpvMemoryModelVulkanKHR &&
_.HasCapability(SpvCapabilityVulkanMemoryModelKHR)) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< "VulkanMemoryModelKHR capability must only be specified if "
"the "
"VulkanKHR memory model is used.";
}
if (spvIsWebGPUEnv(_.context()->target_env)) {
if (_.addressing_model() != SpvAddressingModelLogical) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< "Addressing model must be Logical for WebGPU environment.";
}
if (_.memory_model() != SpvMemoryModelVulkanKHR) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< "Memory model must be VulkanKHR for WebGPU environment.";
}
}
if (spvIsOpenCLEnv(_.context()->target_env)) {
if ((_.addressing_model() != SpvAddressingModelPhysical32) &&
(_.addressing_model() != SpvAddressingModelPhysical64)) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< "Addressing model must be Physical32 or Physical64 "
<< "in the OpenCL environment.";
}
if (_.memory_model() != SpvMemoryModelOpenCL) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< "Memory model must be OpenCL in the OpenCL environment.";
}
}
} else if (opcode == SpvOpExecutionMode) {
const uint32_t entry_point = inst->word(1);
_.RegisterExecutionModeForEntryPoint(entry_point,
@ -540,61 +479,9 @@ spv_result_t InstructionPass(ValidationState_t& _, const Instruction* inst) {
if (auto error = LimitCheckNumVars(_, inst->id(), storage_class)) {
return error;
}
if (spvIsWebGPUEnv(_.context()->target_env) &&
!IsValidWebGPUStorageClass(storage_class)) {
return _.diag(SPV_ERROR_INVALID_BINARY, inst)
<< "For WebGPU, OpVariable storage class must be one of "
"UniformConstant, Uniform, StorageBuffer, Input, Output, "
"Image, Workgroup, Private, Function for WebGPU";
}
if (storage_class == SpvStorageClassGeneric)
return _.diag(SPV_ERROR_INVALID_BINARY, inst)
<< "OpVariable storage class cannot be Generic";
if (_.current_layout_section() == kLayoutFunctionDefinitions) {
if (storage_class != SpvStorageClassFunction) {
return _.diag(SPV_ERROR_INVALID_LAYOUT, inst)
<< "Variables must have a function[7] storage class inside"
" of a function";
}
if (_.current_function().IsFirstBlock(
_.current_function().current_block()->id()) == false) {
return _.diag(SPV_ERROR_INVALID_CFG, inst)
<< "Variables can only be defined "
"in the first block of a "
"function";
}
} else {
if (storage_class == SpvStorageClassFunction) {
return _.diag(SPV_ERROR_INVALID_LAYOUT, inst)
<< "Variables can not have a function[7] storage class "
"outside of a function";
}
}
} else if (opcode == SpvOpTypePointer) {
const auto storage_class = inst->GetOperandAs<SpvStorageClass>(1);
if (spvIsWebGPUEnv(_.context()->target_env) &&
!IsValidWebGPUStorageClass(storage_class)) {
return _.diag(SPV_ERROR_INVALID_BINARY, inst)
<< "For WebGPU, OpTypePointer storage class must be one of "
"UniformConstant, Uniform, StorageBuffer, Input, Output, "
"Image, Workgroup, Private, Function";
}
}
// SPIR-V Spec 2.16.3: Validation Rules for Kernel Capabilities: The
// Signedness in OpTypeInt must always be 0.
if (SpvOpTypeInt == inst->opcode() && _.HasCapability(SpvCapabilityKernel) &&
inst->GetOperandAs<uint32_t>(2) != 0u) {
return _.diag(SPV_ERROR_INVALID_BINARY, inst)
<< "The Signedness in OpTypeInt "
"must always be 0 when Kernel "
"capability is used.";
}
if (auto error = ReservedCheck(_, inst)) return error;
if (auto error = EnvironmentCheck(_, inst)) return error;
if (auto error = CapabilityCheck(_, inst)) return error;
if (auto error = LimitCheckIdBound(_, inst)) return error;
if (auto error = LimitCheckStruct(_, inst)) return error;

View File

@ -461,6 +461,28 @@ spv_result_t ValidateVariable(ValidationState_t& _, const Instruction* inst) {
}
}
if (!_.IsValidStorageClass(storage_class)) {
return _.diag(SPV_ERROR_INVALID_BINARY, inst)
<< "Invalid storage class for target environment";
}
if (storage_class == SpvStorageClassGeneric) {
return _.diag(SPV_ERROR_INVALID_BINARY, inst)
<< "OpVariable storage class cannot be Generic";
}
if (inst->function() && storage_class != SpvStorageClassFunction) {
return _.diag(SPV_ERROR_INVALID_LAYOUT, inst)
<< "Variables must have a function[7] storage class inside"
" of a function";
}
if (!inst->function() && storage_class == SpvStorageClassFunction) {
return _.diag(SPV_ERROR_INVALID_LAYOUT, inst)
<< "Variables can not have a function[7] storage class "
"outside of a function";
}
// SPIR-V 3.32.8: Check that pointer type and variable type have the same
// storage class.
const auto result_storage_class_index = 1;

View File

@ -32,6 +32,10 @@ spv_result_t ValidateUndef(ValidationState_t& _, const Instruction* inst) {
<< "Cannot create undefined values with 8- or 16-bit types";
}
if (spvIsWebGPUEnv(_.context()->target_env)) {
return _.diag(SPV_ERROR_INVALID_BINARY, inst) << "OpUndef is disallowed";
}
return SPV_SUCCESS;
}

View File

@ -485,6 +485,44 @@ spv_result_t ValidateExecutionMode(ValidationState_t& _,
return SPV_SUCCESS;
}
spv_result_t ValidateMemoryModel(ValidationState_t& _,
const Instruction* inst) {
// Already produced an error if multiple memory model instructions are
// present.
if (_.memory_model() != SpvMemoryModelVulkanKHR &&
_.HasCapability(SpvCapabilityVulkanMemoryModelKHR)) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< "VulkanMemoryModelKHR capability must only be specified if "
"the VulkanKHR memory model is used.";
}
if (spvIsWebGPUEnv(_.context()->target_env)) {
if (_.addressing_model() != SpvAddressingModelLogical) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< "Addressing model must be Logical for WebGPU environment.";
}
if (_.memory_model() != SpvMemoryModelVulkanKHR) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< "Memory model must be VulkanKHR for WebGPU environment.";
}
}
if (spvIsOpenCLEnv(_.context()->target_env)) {
if ((_.addressing_model() != SpvAddressingModelPhysical32) &&
(_.addressing_model() != SpvAddressingModelPhysical64)) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< "Addressing model must be Physical32 or Physical64 "
<< "in the OpenCL environment.";
}
if (_.memory_model() != SpvMemoryModelOpenCL) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< "Memory model must be OpenCL in the OpenCL environment.";
}
}
return SPV_SUCCESS;
}
} // namespace
spv_result_t ModeSettingPass(ValidationState_t& _, const Instruction* inst) {
@ -496,6 +534,9 @@ spv_result_t ModeSettingPass(ValidationState_t& _, const Instruction* inst) {
case SpvOpExecutionModeId:
if (auto error = ValidateExecutionMode(_, inst)) return error;
break;
case SpvOpMemoryModel:
if (auto error = ValidateMemoryModel(_, inst)) return error;
break;
default:
break;
}

View File

@ -106,6 +106,17 @@ spv_result_t ValidateTypeInt(ValidationState_t& _, const Instruction* inst) {
return _.diag(SPV_ERROR_INVALID_VALUE, inst)
<< "OpTypeInt has invalid signedness:";
}
// SPIR-V Spec 2.16.3: Validation Rules for Kernel Capabilities: The
// Signedness in OpTypeInt must always be 0.
if (SpvOpTypeInt == inst->opcode() && _.HasCapability(SpvCapabilityKernel) &&
inst->GetOperandAs<uint32_t>(2) != 0u) {
return _.diag(SPV_ERROR_INVALID_BINARY, inst)
<< "The Signedness in OpTypeInt "
"must always be 0 when Kernel "
"capability is used.";
}
return SPV_SUCCESS;
}
@ -445,6 +456,12 @@ spv_result_t ValidateTypePointer(ValidationState_t& _,
if (sampled == 2) _.RegisterPointerToStorageImage(inst->id());
}
}
if (!_.IsValidStorageClass(storage_class)) {
return _.diag(SPV_ERROR_INVALID_BINARY, inst)
<< "Invalid storage class for target environment";
}
return SPV_SUCCESS;
}

View File

@ -212,14 +212,6 @@ ValidationState_t::ValidationState_t(const spv_const_context ctx,
}
}
switch (env) {
case SPV_ENV_WEBGPU_0:
features_.bans_op_undef = true;
break;
default:
break;
}
// Only attempt to count if we have words, otherwise let the other validation
// fail and generate an error.
if (num_words > 0) {
@ -1277,5 +1269,52 @@ bool ValidationState_t::ContainsLimitedUseIntOrFloatType(uint32_t id) const {
return false;
}
bool ValidationState_t::IsValidStorageClass(
SpvStorageClass storage_class) const {
if (spvIsWebGPUEnv(context()->target_env)) {
switch (storage_class) {
case SpvStorageClassUniformConstant:
case SpvStorageClassUniform:
case SpvStorageClassStorageBuffer:
case SpvStorageClassInput:
case SpvStorageClassOutput:
case SpvStorageClassImage:
case SpvStorageClassWorkgroup:
case SpvStorageClassPrivate:
case SpvStorageClassFunction:
return true;
default:
return false;
}
}
if (spvIsVulkanEnv(context()->target_env)) {
switch (storage_class) {
case SpvStorageClassUniformConstant:
case SpvStorageClassUniform:
case SpvStorageClassStorageBuffer:
case SpvStorageClassInput:
case SpvStorageClassOutput:
case SpvStorageClassImage:
case SpvStorageClassWorkgroup:
case SpvStorageClassPrivate:
case SpvStorageClassFunction:
case SpvStorageClassPushConstant:
case SpvStorageClassPhysicalStorageBuffer:
case SpvStorageClassRayPayloadNV:
case SpvStorageClassIncomingRayPayloadNV:
case SpvStorageClassHitAttributeNV:
case SpvStorageClassCallableDataNV:
case SpvStorageClassIncomingCallableDataNV:
case SpvStorageClassShaderRecordBufferNV:
return true;
default:
return false;
}
}
return true;
}
} // namespace val
} // namespace spvtools

View File

@ -79,9 +79,6 @@ class ValidationState_t {
// Permit group oerations Reduce, InclusiveScan, ExclusiveScan
bool group_ops_reduce_and_scans = false;
// Disallows the use of OpUndef
bool bans_op_undef = false;
// Allow OpTypeInt with 8 bit width?
bool declare_int8_type = false;
@ -707,6 +704,9 @@ class ValidationState_t {
// * OpCopyObject
const Instruction* TracePointer(const Instruction* inst) const;
// Validates the storage class for the target environment.
bool IsValidStorageClass(SpvStorageClass storage_class) const;
private:
ValidationState_t(const ValidationState_t&);

View File

@ -342,11 +342,10 @@ TEST_P(ValidateCFG, VariableNotInFirstBlockBad) {
str += "OpFunctionEnd\n";
CompileSuccessfully(str);
ASSERT_EQ(SPV_ERROR_INVALID_CFG, ValidateInstructions());
EXPECT_THAT(
getDiagnosticString(),
HasSubstr(
"Variables can only be defined in the first block of a function"));
ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
EXPECT_THAT(getDiagnosticString(),
HasSubstr("All OpVariable instructions in a function must be the "
"first instructions in the first block"));
}
TEST_P(ValidateCFG, BlockSelfLoopIsOk) {

View File

@ -45,15 +45,18 @@ TEST_F(ValidateOpenCL, NonPhysicalAddressingModelBad) {
TEST_F(ValidateOpenCL, NonOpenCLMemoryModelBad) {
std::string spirv = R"(
OpCapability Kernel
OpMemoryModel Physical32 GLSL450
OpCapability Addresses
OpCapability VulkanMemoryModelKHR
OpExtension "SPV_KHR_vulkan_memory_model"
OpMemoryModel Physical32 VulkanKHR
)";
CompileSuccessfully(spirv);
EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_OPENCL_1_2));
EXPECT_THAT(getDiagnosticString(),
HasSubstr("Memory model must be OpenCL in the OpenCL environment."
"\n OpMemoryModel Physical32 GLSL450\n"));
EXPECT_THAT(
getDiagnosticString(),
HasSubstr("Memory model must be OpenCL in the OpenCL environment."));
}
TEST_F(ValidateOpenCL, NonVoidSampledTypeImageBad) {

View File

@ -157,6 +157,7 @@ TEST_F(ValidateStorage, GenericVariableOutsideFunction) {
const auto str = R"(
OpCapability Kernel
OpCapability Linkage
OpCapability GenericPointer
OpMemoryModel Logical OpenCL
%intt = OpTypeInt 32 0
%ptrt = OpTypePointer Function %intt
@ -172,6 +173,7 @@ TEST_F(ValidateStorage, GenericVariableInsideFunction) {
const auto str = R"(
OpCapability Shader
OpCapability Linkage
OpCapability GenericPointer
OpMemoryModel Logical GLSL450
%intt = OpTypeInt 32 1
%voidt = OpTypeVoid
@ -184,7 +186,7 @@ TEST_F(ValidateStorage, GenericVariableInsideFunction) {
OpFunctionEnd
)";
CompileSuccessfully(str);
ASSERT_EQ(SPV_ERROR_INVALID_BINARY, ValidateInstructions());
EXPECT_EQ(SPV_ERROR_INVALID_BINARY, ValidateInstructions());
EXPECT_THAT(getDiagnosticString(),
HasSubstr("OpVariable storage class cannot be Generic"));
}
@ -307,12 +309,10 @@ INSTANTIATE_TEST_SUITE_P(
std::make_tuple("Workgroup", false, true, ""),
std::make_tuple("Private", false, true, ""),
std::make_tuple("Function", true, true, ""),
std::make_tuple(
"CrossWorkgroup", false, false,
"For WebGPU, OpTypePointer storage class must be one of"),
std::make_tuple(
"PushConstant", false, false,
"For WebGPU, OpTypePointer storage class must be one of")));
std::make_tuple("CrossWorkgroup", false, false,
"Invalid storage class for target environment"),
std::make_tuple("PushConstant", false, false,
"Invalid storage class for target environment")));
} // namespace
} // namespace val

View File

@ -187,23 +187,6 @@ TEST_F(ValidateWebGPU, LogicalAddressingVulkanKHRMemoryGood) {
EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_WEBGPU_0));
}
TEST_F(ValidateWebGPU, NonLogicalAddressingModelBad) {
std::string spirv = R"(
OpCapability Shader
OpCapability VulkanMemoryModelKHR
OpExtension "SPV_KHR_vulkan_memory_model"
OpMemoryModel Physical32 VulkanKHR
)";
CompileSuccessfully(spirv);
EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_WEBGPU_0));
EXPECT_THAT(getDiagnosticString(),
HasSubstr("Addressing model must be Logical for WebGPU "
"environment.\n OpMemoryModel Physical32 "
"Vulkan\n"));
}
TEST_F(ValidateWebGPU, NonVulkanKHRMemoryModelBad) {
std::string spirv = R"(
OpCapability Shader