From 363bfca2ed888b501268e3e2bb748bbbbf5a32b5 Mon Sep 17 00:00:00 2001 From: David Neto Date: Tue, 5 Jun 2018 23:07:51 -0700 Subject: [PATCH] Operand lookup succeeds if it's enabled by a capability - Fix tests for basic group operations (e.g. Reduce) to allow for new capabilities in SPIR-V 1.3 that enable them. - Refactor operand capability check to avoid code duplication and to put all checks that don't need table lookup before any table lookup. - Test round trip assembly/disassembly support for extension SPV_NV_viewport_array2 - Test assembly and validation of decoration ViewportRelativeNV Fixes #1596 --- source/operand.cpp | 14 ++--- source/validate_instruction.cpp | 73 ++++++++++++++------------ test/binary_parse_test.cpp | 4 +- test/operand_capabilities_test.cpp | 16 ++++-- test/text_to_binary.extension_test.cpp | 38 ++++++++++++-- test/val/val_capability_test.cpp | 52 ++++++++++++++++++ 6 files changed, 147 insertions(+), 50 deletions(-) diff --git a/source/operand.cpp b/source/operand.cpp index 07377d5a9..62ac3f471 100644 --- a/source/operand.cpp +++ b/source/operand.cpp @@ -55,16 +55,17 @@ spv_result_t spvOperandTableNameLookup(spv_target_env env, if (type != group.type) continue; for (uint64_t index = 0; index < group.count; ++index) { const auto& entry = group.entries[index]; - // We considers the current operand as available as long as + // We consider the current operand as available as long as // 1. The target environment satisfies the minimal requirement of the // operand; or - // 2. There is at least one extension enabling this operand. + // 2. There is at least one extension enabling this operand; or + // 3. There is at least one capability enabling this operand. // // Note that the second rule assumes the extension enabling this operand // is indeed requested in the SPIR-V code; checking that should be // validator's work. if ((spvVersionForTargetEnv(env) >= entry.minVersion || - entry.numExtensions > 0u) && + entry.numExtensions > 0u || entry.numCapabilities > 0u) && nameLength == strlen(entry.name) && !strncmp(entry.name, name, nameLength)) { *pEntry = &entry; @@ -109,16 +110,17 @@ spv_result_t spvOperandTableValueLookup(spv_target_env env, // opcode value. for (auto it = std::lower_bound(beg, end, needle, comp); it != end && it->value == value; ++it) { - // We considers the current operand as available as long as + // We consider the current operand as available as long as // 1. The target environment satisfies the minimal requirement of the // operand; or - // 2. There is at least one extension enabling this operand. + // 2. There is at least one extension enabling this operand; or + // 3. There is at least one capability enabling this operand. // // Note that the second rule assumes the extension enabling this operand // is indeed requested in the SPIR-V code; checking that should be // validator's work. if (spvVersionForTargetEnv(env) >= it->minVersion || - it->numExtensions > 0u) { + it->numExtensions > 0u || it->numCapabilities > 0u) { *pEntry = it; return SPV_SUCCESS; } diff --git a/source/validate_instruction.cpp b/source/validate_instruction.cpp index dac0f57ca..92cb8cc15 100644 --- a/source/validate_instruction.cpp +++ b/source/validate_instruction.cpp @@ -61,7 +61,7 @@ std::string ToString(const CapabilitySet& capabilities, // Reports a missing-capability error to _'s diagnostic stream and returns // SPV_ERROR_INVALID_CAPABILITY. -spv_result_t CapabilityError(ValidationState_t& _, int which_operand, +spv_result_t CapabilityError(const ValidationState_t& _, int which_operand, SpvOp opcode, const std::string& required_capabilities) { return _.diag(SPV_ERROR_INVALID_CAPABILITY) @@ -101,9 +101,13 @@ CapabilitySet EnablingCapabilitiesForOp(const ValidationState_t& state, return CapabilitySet(); } -// Returns an operand's required capabilities. -CapabilitySet RequiredCapabilities(const ValidationState_t& state, - spv_operand_type_t type, uint32_t operand) { +// Returns SPV_SUCCESS if the given operand is enabled by capabilities declared +// in the module. Otherwise issues an error message and returns +// SPV_ERROR_INVALID_CAPABILITY. +spv_result_t CheckRequiredCapabilities(const ValidationState_t& state, + SpvOp opcode, int which_operand, + spv_operand_type_t type, + uint32_t operand) { // Mere mention of PointSize, ClipDistance, or CullDistance in a Builtin // decoration does not require the associated capability. The use of such // a variable value should trigger the capability requirement, but that's @@ -114,47 +118,51 @@ CapabilitySet RequiredCapabilities(const ValidationState_t& state, case SpvBuiltInPointSize: case SpvBuiltInClipDistance: case SpvBuiltInCullDistance: - return CapabilitySet(); + return SPV_SUCCESS; default: break; } } else if (type == SPV_OPERAND_TYPE_FP_ROUNDING_MODE) { // Allow all FP rounding modes if requested if (state.features().free_fp_rounding_mode) { - return CapabilitySet(); + return SPV_SUCCESS; } + } else if (type == SPV_OPERAND_TYPE_GROUP_OPERATION && + state.features().group_ops_reduce_and_scans && + (operand <= uint32_t(SpvGroupOperationExclusiveScan))) { + // Allow certain group operations if requested. + return SPV_SUCCESS; } - spv_operand_desc operand_desc; - const auto ret = state.grammar().lookupOperand(type, operand, &operand_desc); - if (ret == SPV_SUCCESS) { + CapabilitySet enabling_capabilities; + spv_operand_desc operand_desc = nullptr; + const auto lookup_result = + state.grammar().lookupOperand(type, operand, &operand_desc); + if (lookup_result == SPV_SUCCESS) { // Allow FPRoundingMode decoration if requested. if (type == SPV_OPERAND_TYPE_DECORATION && operand_desc->value == SpvDecorationFPRoundingMode) { - if (state.features().free_fp_rounding_mode) return CapabilitySet(); + if (state.features().free_fp_rounding_mode) return SPV_SUCCESS; // Vulkan API requires more capabilities on rounding mode. if (spvIsVulkanEnv(state.context()->target_env)) { - CapabilitySet cap_set; - cap_set.Add(SpvCapabilityStorageUniformBufferBlock16); - cap_set.Add(SpvCapabilityStorageUniform16); - cap_set.Add(SpvCapabilityStoragePushConstant16); - cap_set.Add(SpvCapabilityStorageInputOutput16); - return cap_set; + enabling_capabilities.Add(SpvCapabilityStorageUniformBufferBlock16); + enabling_capabilities.Add(SpvCapabilityStorageUniform16); + enabling_capabilities.Add(SpvCapabilityStoragePushConstant16); + enabling_capabilities.Add(SpvCapabilityStorageInputOutput16); } - } - // Allow certain group operations if requested. - if (state.features().group_ops_reduce_and_scans && - type == SPV_OPERAND_TYPE_GROUP_OPERATION && - (operand <= uint32_t(SpvGroupOperationExclusiveScan))) { - return CapabilitySet(); + } else { + enabling_capabilities = state.grammar().filterCapsAgainstTargetEnv( + operand_desc->capabilities, operand_desc->numCapabilities); } - return state.grammar().filterCapsAgainstTargetEnv( - operand_desc->capabilities, operand_desc->numCapabilities); + if (!state.HasAnyOfCapabilities(enabling_capabilities)) { + return CapabilityError(state, which_operand, opcode, + ToString(enabling_capabilities, state.grammar())); + } } - return CapabilitySet(); + return SPV_SUCCESS; } // Returns operand's required extensions. @@ -196,11 +204,9 @@ spv_result_t CapabilityCheck(ValidationState_t& _, // Check for required capabilities for each bit position of the mask. for (uint32_t mask_bit = 0x80000000; mask_bit; mask_bit >>= 1) { if (word & mask_bit) { - const auto caps = RequiredCapabilities(_, operand.type, mask_bit); - if (!_.HasAnyOfCapabilities(caps)) { - return CapabilityError(_, i + 1, opcode, - ToString(caps, _.grammar())); - } + spv_result_t status = CheckRequiredCapabilities( + _, opcode, i + 1, operand.type, mask_bit); + if (status != SPV_SUCCESS) return status; } } } else if (spvIsIdType(operand.type)) { @@ -209,10 +215,9 @@ spv_result_t CapabilityCheck(ValidationState_t& _, // https://github.com/KhronosGroup/SPIRV-Tools/issues/248 } else { // Check the operand word as a whole. - const auto caps = RequiredCapabilities(_, operand.type, word); - if (!_.HasAnyOfCapabilities(caps)) { - return CapabilityError(_, i + 1, opcode, ToString(caps, _.grammar())); - } + spv_result_t status = + CheckRequiredCapabilities(_, opcode, i + 1, operand.type, word); + if (status != SPV_SUCCESS) return status; } } return SPV_SUCCESS; diff --git a/test/binary_parse_test.cpp b/test/binary_parse_test.cpp index bb1d6ffd1..f3b9d80fc 100644 --- a/test/binary_parse_test.cpp +++ b/test/binary_parse_test.cpp @@ -887,8 +887,8 @@ INSTANTIATE_TEST_CASE_P( "component 32"}, {"%2 = OpFunction %2 !31", "Invalid function control operand: 31 has invalid mask component 16"}, - {"OpLoopMerge %1 %2 !7", - "Invalid loop control operand: 7 has invalid mask component 4"}, + {"OpLoopMerge %1 %2 !1027", + "Invalid loop control operand: 1027 has invalid mask component 1024"}, {"%2 = OpImageFetch %1 %image %coord !511", "Invalid image operand: 511 has invalid mask component 256"}, {"OpSelectionMerge %1 !7", diff --git a/test/operand_capabilities_test.cpp b/test/operand_capabilities_test.cpp index a980621d0..bb1bac35c 100644 --- a/test/operand_capabilities_test.cpp +++ b/test/operand_capabilities_test.cpp @@ -59,6 +59,7 @@ TEST_P(EnumCapabilityTest, Sample) { EXPECT_THAT(ElementsIn(cap_set), Eq(ElementsIn(get<1>(GetParam()).expected_capabilities))) << " capability value " << get<1>(GetParam()).value; + spvContextDestroy(context); } #define CASE0(TYPE, VALUE) \ @@ -77,6 +78,12 @@ TEST_P(EnumCapabilityTest, Sample) { SpvCapability##CAP1, SpvCapability##CAP2 \ } \ } +#define CASE3(TYPE, VALUE, CAP1, CAP2, CAP3) \ + { \ + SPV_OPERAND_TYPE_##TYPE, uint32_t(Spv##VALUE), CapabilitySet { \ + SpvCapability##CAP1, SpvCapability##CAP2, SpvCapability##CAP3 \ + } \ + } #define CASE5(TYPE, VALUE, CAP1, CAP2, CAP3, CAP4, CAP5) \ { \ SPV_OPERAND_TYPE_##TYPE, uint32_t(Spv##VALUE), CapabilitySet { \ @@ -615,9 +622,12 @@ INSTANTIATE_TEST_CASE_P( GroupOperation, EnumCapabilityTest, Combine(Values(SPV_ENV_UNIVERSAL_1_0, SPV_ENV_UNIVERSAL_1_1), ValuesIn(std::vector{ - CASE1(GROUP_OPERATION, GroupOperationReduce, Kernel), - CASE1(GROUP_OPERATION, GroupOperationInclusiveScan, Kernel), - CASE1(GROUP_OPERATION, GroupOperationExclusiveScan, Kernel), + CASE3(GROUP_OPERATION, GroupOperationReduce, Kernel, + GroupNonUniformArithmetic, GroupNonUniformBallot), + CASE3(GROUP_OPERATION, GroupOperationInclusiveScan, Kernel, + GroupNonUniformArithmetic, GroupNonUniformBallot), + CASE3(GROUP_OPERATION, GroupOperationExclusiveScan, Kernel, + GroupNonUniformArithmetic, GroupNonUniformBallot), })), ); // See SPIR-V Section 3.29 Kernel Enqueue Flags diff --git a/test/text_to_binary.extension_test.cpp b/test/text_to_binary.extension_test.cpp index 690075f2b..92ceb4a0c 100644 --- a/test/text_to_binary.extension_test.cpp +++ b/test/text_to_binary.extension_test.cpp @@ -145,19 +145,19 @@ INSTANTIATE_TEST_CASE_P( MakeInstruction(SpvOpSubgroupBallotKHR, {1, 2, 3})}, {"%2 = OpSubgroupFirstInvocationKHR %1 %3\n", MakeInstruction(SpvOpSubgroupFirstInvocationKHR, {1, 2, 3})}, - {"OpDecorate %1 BuiltIn SubgroupEqMaskKHR\n", + {"OpDecorate %1 BuiltIn SubgroupEqMask\n", MakeInstruction(SpvOpDecorate, {1, SpvDecorationBuiltIn, SpvBuiltInSubgroupEqMaskKHR})}, - {"OpDecorate %1 BuiltIn SubgroupGeMaskKHR\n", + {"OpDecorate %1 BuiltIn SubgroupGeMask\n", MakeInstruction(SpvOpDecorate, {1, SpvDecorationBuiltIn, SpvBuiltInSubgroupGeMaskKHR})}, - {"OpDecorate %1 BuiltIn SubgroupGtMaskKHR\n", + {"OpDecorate %1 BuiltIn SubgroupGtMask\n", MakeInstruction(SpvOpDecorate, {1, SpvDecorationBuiltIn, SpvBuiltInSubgroupGtMaskKHR})}, - {"OpDecorate %1 BuiltIn SubgroupLeMaskKHR\n", + {"OpDecorate %1 BuiltIn SubgroupLeMask\n", MakeInstruction(SpvOpDecorate, {1, SpvDecorationBuiltIn, SpvBuiltInSubgroupLeMaskKHR})}, - {"OpDecorate %1 BuiltIn SubgroupLtMaskKHR\n", + {"OpDecorate %1 BuiltIn SubgroupLtMask\n", MakeInstruction(SpvOpDecorate, {1, SpvDecorationBuiltIn, SpvBuiltInSubgroupLtMaskKHR})}, })), ); @@ -523,6 +523,34 @@ INSTANTIATE_TEST_CASE_P( {1, SpvDecorationHlslCounterBufferGOOGLE, 2})}, })), ); +// SPV_NV_viewport_array2 + +INSTANTIATE_TEST_CASE_P( + SPV_NV_viewport_array2, ExtensionRoundTripTest, + Combine(Values(SPV_ENV_UNIVERSAL_1_0, SPV_ENV_UNIVERSAL_1_1, + SPV_ENV_UNIVERSAL_1_2, SPV_ENV_UNIVERSAL_1_3, + SPV_ENV_VULKAN_1_0, SPV_ENV_VULKAN_1_1), + ValuesIn(std::vector{ + {"OpExtension \"SPV_NV_viewport_array2\"\n", + MakeInstruction(SpvOpExtension, + MakeVector("SPV_NV_viewport_array2"))}, + // The EXT and NV extensions have the same token number for this + // capability. + {"OpCapability ShaderViewportIndexLayerEXT\n", + MakeInstruction(SpvOpCapability, + {SpvCapabilityShaderViewportIndexLayerNV})}, + // Check the new capability's token number + {"OpCapability ShaderViewportIndexLayerEXT\n", + MakeInstruction(SpvOpCapability, {5254})}, + // Decorations + {"OpDecorate %1 ViewportRelativeNV\n", + MakeInstruction(SpvOpDecorate, + {1, SpvDecorationViewportRelativeNV})}, + {"OpDecorate %1 BuiltIn ViewportMaskNV\n", + MakeInstruction(SpvOpDecorate, {1, SpvDecorationBuiltIn, + SpvBuiltInViewportMaskNV})}, + })), ); + // SPV_NV_shader_subgroup_partitioned INSTANTIATE_TEST_CASE_P( diff --git a/test/val/val_capability_test.cpp b/test/val/val_capability_test.cpp index c088237c1..17a79c54a 100644 --- a/test/val/val_capability_test.cpp +++ b/test/val/val_capability_test.cpp @@ -1965,4 +1965,56 @@ OpMemoryModel Physical64 OpenCL "Embedded Profile")); } +TEST_F(ValidateCapability, DecorationFromExtensionMissingEnabledByCapability) { + // Decoration ViewportRelativeNV is enabled by ShaderViewportMaskNV, which in + // turn is enabled by SPV_NV_viewport_array2. + const std::string spirv = R"( +OpCapability Shader +OpMemoryModel Logical Simple +OpDecorate %void ViewportRelativeNV +)" + string(kVoidFVoid); + + CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_0); + EXPECT_EQ(SPV_ERROR_INVALID_CAPABILITY, + ValidateInstructions(SPV_ENV_UNIVERSAL_1_0)); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Operand 2 of Decorate requires one of these " + "capabilities: ShaderViewportMaskNV")); +} + +TEST_F(ValidateCapability, CapabilityEnabledByMissingExtension) { + // Capability ShaderViewportMaskNV is enabled by SPV_NV_viewport_array2. + const std::string spirv = R"( +OpCapability Shader +OpCapability ShaderViewportMaskNV +OpMemoryModel Logical Simple +)" + string(kVoidFVoid); + + CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_0); + EXPECT_EQ(SPV_ERROR_MISSING_EXTENSION, + ValidateInstructions(SPV_ENV_UNIVERSAL_1_0)); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("operand 5255 requires one of these extensions: " + "SPV_NV_viewport_array2")); +} + +TEST_F(ValidateCapability, + DecorationEnabledByCapabilityEnabledByPresentExtension) { + // Decoration ViewportRelativeNV is enabled by ShaderViewportMaskNV, which in + // turn is enabled by SPV_NV_viewport_array2. + const std::string spirv = R"( +OpCapability Shader +OpCapability Linkage +OpCapability ShaderViewportMaskNV +OpExtension "SPV_NV_viewport_array2" +OpMemoryModel Logical Simple +OpDecorate %void ViewportRelativeNV +%void = OpTypeVoid +)"; + + CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_0); + EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_UNIVERSAL_1_0)) + << getDiagnosticString(); +} + } // namespace